Add automatic support for p11-kit #260
Conversation
This commit adds support to automatically use the p11-kit-proxy module on the PKCS#11 KMS if no other module has been specified. Fixes #259
kms/pkcs11/pkcs11_test.go
Outdated
| if findP11KitProxy(context.Background()) != "" { | ||
| wantMissingModule = k | ||
| wantErrMissingModule = false | ||
| } |
There was a problem hiding this comment.
This would be a more accurate test if the output of the shell command were mocked instead.
There was a problem hiding this comment.
I'll use var findP11KitProxy = func(...) {...}
|
Reporting In some systems without development packages installed, pkg-config may be absent. Can we try just As a follow-up, it would also be good to remove the explicit module-parh from most of the examples in the documentation, and make it clear that needing do so is a pathological case for a misconfigured system. |
I haven't.
Sure, that worked for me.
People use this on containers where p11-kit is not available. |
|
Looks good to me; thanks for the prompt solution!
Well... I might argue that those come under the heading of 'misconfigured'. p11-kit is small and it should be present. And even when you only have one provider that you want to use, its packaging should still ship it with the p11-kit .module file and it shouldn't need to be manually specified. But that's my crusade, not yours :) And I'll begrudgingly accept the counter-argument if you have only one token then p11-kit, tiny though it may be, is overkill. I'd still like to see the documentation at least mention that in a properly configured system, module-path should be unnecessary. |
kms/pkcs11/pkcs11.go
Outdated
| // the full path. | ||
| if config.Path = u.Get("module-path"); config.Path == "" { | ||
| config.Path = defaultModule | ||
| } |
There was a problem hiding this comment.
This is in the wrong place, isn't it? It'll only happen if a URI is given, but there's an alternative code flow where it's all in the config? That should benefit from the default, too.
There's already a check for config.Path == "" a few lines further down, with an error return which is no longer telling the truth because it says that 'module-path is required'.
Also... what's wrong with pkcs11:manufacturer=piv_II;id=%01 which I usually use as my examples in documentation? Why are only token/serial/slot-id accepted, and not model/manufacturer?
(And why is this something you even need to do for yourself; surely "match token against the attributes in a URI" is a generic thing that ought to be somewhere much further down the stack than your code; in ThalesIgnite/crypto11 or miekg/pkcs11. Ideally you just want to call a function like the matchSlots() I wrote here, and in your case as discussed, you want to just bail if len(slots) != 1).
There was a problem hiding this comment.
I'd still like to see the documentation at least mention that in a properly configured system, module-path should be unnecessary.
In particular, these two:
- https://smallstep.com/docs/step-ca/configuration/#pkcs-11
- https://github.com/smallstep/step-kms-plugin/blob/main/README.md
.. especially the former. I don't want to live in a world where we have to tell users to explicitly pass a module-path.
All those examples for how to use different HSMs differ mostly in the module-path, which shouldn't be needed anyway.
The packaging of the PKCS#11 provider module itself should come with a p11-kit .module file, the token should automatically appear in the output of things like p11-kit list-tokens or p11tool --list-tokens (and thus also in GUIs when you're selecting a client cert for 802.1x or VPN auth, etc.)
The common case is that user should just be able to select a token from the list, e.g. pkcs11:manufacturer=piv_II.
There was a problem hiding this comment.
This is in the wrong place, isn't it? It'll only happen if a URI is given, but there's an alternative code flow where it's all in the config? That should benefit from the default, too.
PKCS#11 cannot be configured without a URI, currently you will get the following error:
return nil, errors.New("kms uri 'module-path' are required")
We can make it more explicit and actually fail if URI == "".
Also... what's wrong with pkcs11:manufacturer=piv_II;id=%01 which I usually use as my examples in documentation? Why are only token/serial/slot-id accepted, and not model/manufacturer?
Nothing, but https://github.com/ThalesIgnite/crypto11, which is the one we use on top of github.com/miekg/pkcs11 doesn't support it, see this code.
Regarding the documentation, some of it is managed by a different team, and we haven't integrated this yet, so it will have to wait a bit. But I can change step-kms-plugin README and help once this is integrated.
There was a problem hiding this comment.
I've fixed some of this in a23378d
There was a problem hiding this comment.
Nothing, but https://github.com/ThalesIgnite/crypto11, which is the one we use on top of github.com/miekg/pkcs11 doesn't support it, see this code.
Thanks. I've added that specific reference to ThalesGroup/crypto11#104
|
FWIW (although I don't think it matters any more) the |
Yes, using pkg-config just make things more complicated, when dlopen can find it in most cases, and if it can't you can always set the right environment variables or change configuration files. |
| if err != nil { | ||
| return nil, err | ||
| return nil, errors.Wrap(err, "kms uri 'slot-id' is not valid") |
There was a problem hiding this comment.
Could include the value of slot-id
There was a problem hiding this comment.
The wrapped error contains the string.
Co-authored-by: Herman Slatman <hslatman@users.noreply.github.com>
Description
This commit adds support to automatically use the p11-kit-proxy module on the PKCS#11 KMS if no other module has been specified.
Fixes #259
cc: @dwmw2