Fixed pkcs11-register defaults on macOS and Windows #3053

frankmorgner · 2024-02-27T13:17:23Z

pkcs11-register command line defaults are based on the platform. On macOS/Windows, we don't want to register pkcs#11 modules to avoid conflict with the native API. For this to be effective, we need to build cmdline explicitly on those platforms.

Checklist

Documentation is added or updated
New files have a LGPL 2.1 license statement
PKCS#11 module is tested
Windows minidriver is tested
macOS tokend is tested

frankmorgner · 2024-03-05T11:22:34Z

The generated commandline files are causing problems with -Wshorten-64-to-32, which is why e59586d detects and disables this warning. The strict build, however, explicitly enables this warning - overwriting the local setting for the command line file:

clang -DHAVE_CONFIG_H -I. -I../..  -I../../src -D'DEFAULT_PKCS11_PROVIDER="/usr/local/lib/opensc-pkcs11.so"' -D'DEFAULT_ONEPIN_PKCS11_PROVIDER="/usr/local/lib/onepin-opensc-pkcs11.so"'   -Wno-unknown-warning-option -Wno-shorten-64-to-32 -Wall -Wextra -Wno-unused-parameter -Werror -Wstrict-aliasing=2 -DDEBUG_PROFILE=1 -pedantic -Werror -Wall -Wno-strict-prototypes -Wno-empty-translation-unit -Wno-incompatible-library-redeclaration -Wno-variadic-macros -Wno-unused-function -Wno-incompatible-pointer-types-discards-qualifiers -Wno-gnu-zero-variadic-macro-arguments -Wno-format-pedantic -Wno-pedantic -Wno-incompatible-function-pointer-types -Wshorten-64-to-32 -c -o goid_tool-goid-tool-cmdline.o `test -f 'goid-tool-cmdline.c' || echo './'`goid-tool-cmdline.c

@Jakuje , do you know how to circumvent this?

Jakuje · 2024-03-05T11:53:55Z

@Jakuje , do you know how to circumvent this?

I was fixing this manually in b21d01e#diff-b9804e986581c6f7b6926feede5d5abddf4af651138eca115894fef96632e309

Together with the typo that I managed to push also to upstream:

https://savannah.gnu.org/bugs/?64743
https://git.savannah.gnu.org/cgit/gengetopt.git/commit/?id=bef6597d1d077fe751a89307ea4f7cc5701653ef

I can try to fix also the cast in gengetopt, but given that this commit was first in 4 years since last release, I am not sure how active development is and when we can expect this to get pulled by distributions. I filled this here:

https://savannah.gnu.org/bugs/index.php?65417

For the time being, I believe we should not regenerate the cmdline files as part of the build process (at least not for the goid that is having this issue).

Side note: Once you do the detection of the -Wshorten-64-to-32 support in configure, I think we can remove it from the list of forced CFLAGS in strict build as it becomes and it should be pulled automatically.

Other option to avoid this issue might be not to use the int argument in the goid tool. The short/long already have the casts or do not need it so they should not demonstrate this issue and code change should be quite minimal.

frankmorgner · 2024-03-06T23:40:32Z

Thanks for tracking this to the original source.

In CI, we're currently not using --enable-strict to set default CFLAGS so I think the detection in configure.ac will not help.

I pushed a commit with the suggested workaround of using a short.

Jakuje · 2024-03-07T07:34:09Z

One more thing. Once more regenerate the cmdline files during the build it is a question, whether the resulting files should be in git at all. Indeed, it helps to keep track of changes in the gengetopt tool (if there are some), but otherwise it will just cause these issues. If we add gengetopt as a dependency, we can generate them during the build (and during the make dist).

If we want to keep them in git, I think we should not codespell and code style check them as there is really nothing the PR contributor could do about typos in them and formatting issues. Unfortunately, I do not see an option in https://github.com/yshui/git-clang-format-lint to exclude files (but that should not be an issue), but codespell has an exclude_file, which can contain the files to exclude so thats what

https://github.com/codespell-project/actions-codespell?tab=readme-ov-file#parameter-exclude_file

frankmorgner · 2024-03-08T06:59:34Z

There is no easy way to get gengetopt on Windows - we would have to compile this ourselves. Yes, we are already doing this for OpenSSL and zlib, but those libraries are popular enough to have compiled binaries available somewhere. Basically, developer on-boarding would be a little more complicated on Windows. #2577 would fix that to some extent, but l currently don't have time to take a deeper look into this.

The source code files for command line interface of pkcs11-register are switched to macOS/Windows defaults in case of adapting the configuration of the browsers. Setting up gengetopt as requirement in Linux is much easier. Also regenerates command line files for all other tools.

Jakuje · 2024-03-11T14:50:00Z

packaging/opensc.spec

@@ -17,6 +17,7 @@ BuildRequires:  docbook-style-xsl
 BuildRequires:  autoconf automake libtool gcc
 BuildRequires:  bash-completion
 BuildRequires:  zlib-devel
+BuildRequires:  gengetopt


Is this needed for the build from the dist tarball? If so, it might be a problem as we do not have the gengetopt in RHEL (only in EPEL).

You can build without gengetopt, but obviously OpenSC would not include egk-tool goid-tool opensc-asn1 pkcs11-register, opensc-notify and npa-tool.

dependant tools will only be created if gengetopt is available during build. On Windows, this requires generating the files via `make cmdline -C src/tools` in cygwin with gengetopt installed fixes missing distribution of pkcs11-register.ggo.in fixes CVCDIR and X509DIR defaults on Windows

frankmorgner · 2024-03-12T18:07:00Z

Finally... after many hickups and corner cases, gengetopt is now an optional build dependency and we don't need to handle cmdline files in the source code distribution.

Jakuje · 2024-03-13T08:52:12Z

configure.ac

+AX_CHECK_COMPILE_FLAG([-Wshorten-64-to-32], [have_shorten_warning_option="yes"], [have_shorten_warning_option="no"])
+AM_CONDITIONAL([HAVE_SHORTEN_WARNING_OPTION], [test "${have_shorten_warning_option}" = "yes"])


I think I mentioned this in some previous comments, given that we have this part of configure, can you remove it from the linux-strict build?

https://github.com/OpenSC/OpenSC/blob/master/.github/workflows/linux-strict.yml#L37

I think you can also remove the line following line altogether:

https://github.com/OpenSC/OpenSC/blob/master/.github/workflows/linux-strict.yml#L72

Jakuje · 2024-03-13T09:01:42Z

src/tools/Makefile.am

+if ENABLE_GENGETOPT
+bin_PROGRAMS += egk-tool goid-tool opensc-asn1 pkcs11-register
+
+if ENABLE_NOTIFY
+bin_PROGRAMS += opensc-notify
+endif


Can you move these two blocks below the if ENABLE_OPENSSL to avoid closing and opening the ENABLE_OPENSSL block before and after this?

Jakuje · 2024-03-13T09:06:06Z

src/tools/Makefile.am

@@ -101,51 +117,76 @@ dnie_tool_LDADD = $(OPTIONAL_OPENSSL_LIBS)
 gids_tool_SOURCES = gids-tool.c util.c
 gids_tool_LDADD = $(OPTIONAL_OPENSSL_LIBS)

-npa_tool_SOURCES = npa-tool.c fread_to_eof.c util.c npa-tool-cmdline.c
+npa_tool_SOURCES = npa-tool.c fread_to_eof.c util.c
+nodist_npa_tool_SOURCES = npa-tool-cmdline.c


should this be nodist? I would say that we should generate these files during dist and include them in the release tarball, similarly as we include the configure.

Jakuje · 2024-03-13T09:10:09Z

.appveyor.yml

+    #- curl -sSf -o C:\cygwin64\setup-x86_64.exe https://cygwin.com/setup-x86_64.exe
+    #- C:\cygwin64\setup-x86_64.exe -q -P gengetopt


I see this was likely an attempt to install it through cygwin. Is it still needed or is is just an artifact of failed attempt to avoid people trying? I would propose to remove the commented out lines or added some explanation why it did not work.

Jakuje · 2024-03-13T09:12:06Z

.appveyor.yml

+  - wget https://ftp.gnu.org/gnu/gengetopt/gengetopt-2.23.tar.xz
+  - tar xJf gengetopt-2.23.tar.xz
+  - cd gengetopt-2.23
+  - bash -c "exec 0</dev/null && ./configure --prefix=/ || cat config.log"
+  - bash -c "exec 0</dev/null && make"
+  - cd ..


Looking at the above block, I see installation of other dependencies (openpace, zlib) is done in c:\ directly, which makes the paths slightly nicer. Whats the reason for installing the gengeopt in different path? Can the appveyor DownloadFile be used (I assume it can do some caching)?

frankmorgner · 2024-03-13T13:02:17Z

currently, this PR is designed to integrate gengetopt as mandatory build requirement. There is no other way to change default options based on the platform, I think. For example, if you generate a distribution tarball with cmdline files in it, they will contain the default values for your Linux machine. On macOS, we want different defaults when installing from the distribution tarball.

frankmorgner · 2024-03-13T22:24:22Z

We could do the following:

include the cmdline files in the distribution,
but always with macOS/Windows defaults (safe for Linux, but not ideal).
modify src/tools/pkcs11-register.desktop.in to explicitly add --skip-chrome=off --skip-firefox=off, which "restores" the Linux defaults

frankmorgner force-pushed the register-defaults branch from f5a11a2 to 68940cf Compare February 27, 2024 13:30

pkcs11-register: Chrome supports native crypto modules like Firefox

efa464c

frankmorgner force-pushed the register-defaults branch 2 times, most recently from fdb35e5 to e59586d Compare February 28, 2024 14:58

frankmorgner added 3 commits March 10, 2024 02:16

include .dirstamp in gitignore

6abfbd8

fixed shorten-64-to-32 warning for gengetopt generated files

a635b44

frankmorgner force-pushed the register-defaults branch 3 times, most recently from 09c9794 to 4ad2952 Compare March 11, 2024 14:30

Jakuje reviewed Mar 11, 2024

View reviewed changes

frankmorgner force-pushed the register-defaults branch 2 times, most recently from 4f6cdab to d2b4e45 Compare March 12, 2024 18:03

frankmorgner added 4 commits March 12, 2024 19:04

goid-tool: workaround for compiler warning of generated code

65d890d

fixed dtrust-tool build (extension)

1dd7140

fixed formatting

0e74297

frankmorgner force-pushed the register-defaults branch from d2b4e45 to 0e74297 Compare March 12, 2024 18:05

Jakuje reviewed Mar 13, 2024

View reviewed changes

frankmorgner added 2 commits March 13, 2024 23:56

CI: cache gengetopt in appveyor

82b2f47

a

e7a0e37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed pkcs11-register defaults on macOS and Windows #3053

Fixed pkcs11-register defaults on macOS and Windows #3053

frankmorgner commented Feb 27, 2024

frankmorgner commented Mar 5, 2024

Jakuje commented Mar 5, 2024

frankmorgner commented Mar 6, 2024

Jakuje commented Mar 7, 2024

frankmorgner commented Mar 8, 2024

Jakuje Mar 11, 2024

frankmorgner Mar 11, 2024

frankmorgner commented Mar 12, 2024

Jakuje Mar 13, 2024

Jakuje Mar 13, 2024

Jakuje Mar 13, 2024

Jakuje Mar 13, 2024

Jakuje Mar 13, 2024

frankmorgner commented Mar 13, 2024

frankmorgner commented Mar 13, 2024

		AX_CHECK_COMPILE_FLAG([-Wshorten-64-to-32], [have_shorten_warning_option="yes"], [have_shorten_warning_option="no"])
		AM_CONDITIONAL([HAVE_SHORTEN_WARNING_OPTION], [test "${have_shorten_warning_option}" = "yes"])

		#- curl -sSf -o C:\cygwin64\setup-x86_64.exe https://cygwin.com/setup-x86_64.exe
		#- C:\cygwin64\setup-x86_64.exe -q -P gengetopt

Fixed pkcs11-register defaults on macOS and Windows #3053

Are you sure you want to change the base?

Fixed pkcs11-register defaults on macOS and Windows #3053

Conversation

frankmorgner commented Feb 27, 2024

Checklist

frankmorgner commented Mar 5, 2024

Jakuje commented Mar 5, 2024

frankmorgner commented Mar 6, 2024

Jakuje commented Mar 7, 2024

frankmorgner commented Mar 8, 2024

Jakuje Mar 11, 2024

Choose a reason for hiding this comment

frankmorgner Mar 11, 2024

Choose a reason for hiding this comment

frankmorgner commented Mar 12, 2024

Jakuje Mar 13, 2024

Choose a reason for hiding this comment

Jakuje Mar 13, 2024

Choose a reason for hiding this comment

Jakuje Mar 13, 2024

Choose a reason for hiding this comment

Jakuje Mar 13, 2024

Choose a reason for hiding this comment

Jakuje Mar 13, 2024

Choose a reason for hiding this comment

frankmorgner commented Mar 13, 2024

frankmorgner commented Mar 13, 2024