Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to specify initialize flags for pkcs11 provider #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lo1ol
Copy link

@lo1ol lo1ol commented Sep 30, 2021

New pkcs11-helper interface allows to setup pkcs11 provider via
properties: alonbl/pkcs11-helper@b78d21c

Also pkcs11-helper added ability to setup init args for pkcs11 provider:
alonbl/pkcs11-helper@133f893

Thank you for your contribution

You are welcome to open PR, but they are used for discussion only. All
patches must eventually go to the openvpn-devel mailing list for review:

Please send your patch using git-send-email. For example to send your latest commit to the list:

$ git send-email --to=openvpn-devel@lists.sourceforge.net HEAD~1

For details, see these Wiki articles:

New pkcs11-helper interface allows to setup pkcs11 provider via
properties: alonbl/pkcs11-helper@b78d21c

Also pkcs11-helper added ability to setup init args for pkcs11 provider:
alonbl/pkcs11-helper@133f893

Signed-off-by: Petr Mikhalicin <mkh199740@mail.ru>
@ordex
Copy link
Member

ordex commented Sep 17, 2022

@dsommers what's our take on this? it's a new pkcs11-helper interface (I am not sure how we'd deal with backwards compatibility though). Maybe we don't want to expand pkcs11 in this direction?

@dsommers
Copy link
Member

I wonder which PKCS#11 providers this has been tested against. I'm also lacking some pointers to where CKF_LIBRARY_CANT_CREATE_OS_THREADS and CKF_OS_LOCKING_OK are documented and what these settings can do, which providers supports them. It is needed to document far better what these --pkcs11-init-flags flags are and what they do; preferably with pointers to other external sources.

Generally speaking, I think this patch makes somewhat sense. But the pkcs11-helper code isn't exactly too verbose in the documentation, and the way to little documentation in this patch doesn't make it too easy to understand which use cases this new option are needed. It might also help to split it up in a few smaller chunks doing individual changes on its own. This patch re-implements the "add provider" call and adds a new option; this should at least be split up into separate commits.

More documentation is definitely needed (there are no man page changes) and the changes in src/openvpn/pkcs11.c, in particular what the purpose of each of the pkcs11h_setProviderProperty() calls are also missing (what are they setting and why? How does it change the behaviour). Improving the code comments here would help a lot.

I know the prior code isn't documented well either, but since the PKCS#11 stuff is quite magical for many people - lets try to help the understanding when we change these code paths. That will also ease the code review next round. And it will help other contributors digging into these code paths in 1-2 or 10 years from now.

@dsommers
Copy link
Member

Another detail here is striking me - and this is not something this patch should fix(!)

There seems to be a strict relation between the --pkcs11-providers option and the other --pkcs11-* settings which can be set. If you only use a single provider, that's probably not an issue; but the code supports setting up more providers - then all the --pkcs11-* options used must be in sync.

It would probably be a good idea to rethink the whole PKCS#11 options, and move it towards a similar direction to what we're doing with --dns.

So instead of:

pkcs11-providers provider1 provider2 provider3
pkcs11-private-mode 1 3 4
pkcs11-protected-authentication 0 1 1

you could have:

pkcs11 1 provider provider1
pkcs11 1 private-mode 1
pkcs11 1 protected-auth 0

pkcs11 2 provider provider2
pkcs11 2 private-mode 3
pkcs11 2 protected-auth 1

pkcs11 2 provider provider3
pkcs11 2 private-mode 4
pkcs11 2 protected-auth 1

It's a more verbose approach, but it ties each setting to each provider more clearly. For example, if the first example you set pkcs11-protected-authentication 0 1 that setting would only be set for the two first providers.

This should also move all the various pkcs11 variables and struct members to be re-implemented using a "pkcs11 config struct" with one element per configured pkcs11 provider. This would also make the overall PKCS#11 code easier, and the #ifdef ENABLE_PKCS11 block in context_init1() could be done a lot simpler. Basically everything inside the if() section belongs into pkcs11.[ch]; just pass a pointer to the array of "pkcs11 config struct", and let the whole pkcs11_initialize() and array iteration happen in pkcs11.c.

It seems the whole PKCS#11 implementation is getting ready for a solid clean-up as well ...

@becm
Copy link
Contributor

becm commented Dec 15, 2022

The middle part of this change (use calls to register/set-provider-property/initialize) is now part of master/2.6 in a way that does not break when compiled with with pkcs11-helper releases before v1.28 (introduction of new interface).

@becm
Copy link
Contributor

becm commented Dec 15, 2022

@dsommers if the code for the config restructure is not too complex, it might be nice use named entities, like pkcs11-helper already does internally (can be ignored in an alternative PKCS11 implementation).

pkcs11 myprovider path 'whatever.dll'
pkcs11 myprovider private-mode 1

pkcs11 myotherprovider path 'C:\to\other\provider.dll'
pkcs11 myotherprovider protected-auth 1

You could still opt to name your providers 1, 2 and 3 tho'. 😁

Limiting identifiers to numeric values might also lead to confusion what to expect when commenting out (disable) all entries for provider 1 and there are still settings defined for a 2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants