-
Notifications
You must be signed in to change notification settings - Fork 979
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 PKCS11 support #1153
add PKCS11 support #1153
Conversation
Thanks for the contribution! Before we can merge this, we need @JackDoanRivian to sign the Salesforce Inc. Contributor License Agreement. |
Related: #328 |
How tricky would it be to add support for |
@numinit not that bad for For |
I think that's basically all I'd expect; just symmetry between the sign, CA and keygen commands :-) I noticed the keygen command doesn't actually generate a key anyway, which was well within my expectation for PKCS#11. Fully agreed that using the HSM's native tooling is better there. |
@JackDoanRivian this ought to be a good start :-) |
@numinit this looks great! Is it okay with you if I cherrypick this? You'll have to sign the Salesforce CLA for this to land once I do. |
Updated so the test suite passes. https://github.com/numinit/nebula/tree/pkcs11-v1.9.3 |
@JackDoanRivian Go for it, CLA signed. I've been integration testing it and it works great: https://github.com/numinit/nixpkcs |
I made minor fixes to the existing tests, but probably need to write some more CLI tests. Regardless, a smoketest of the whole process with multiple Nebula nodes using libsoftokn works great. https://github.com/numinit/nixpkcs/blob/master/test.nix#L221 |
Running into some trouble with tpm2-pkcs11 using softhsm under QEMU. I'll debug a bit.
|
pub, _, pubCurve, err = cert.UnmarshalPublicKey(rawPub) | ||
if err != nil { | ||
return fmt.Errorf("error while parsing in-pub: %s", err) | ||
} | ||
if pubCurve != curve { | ||
return fmt.Errorf("curve of in-pub does not match ca") | ||
} | ||
} else if isP11 { | ||
pub, err = p11Client.GetPubKey() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be a good spot to check that out pkcs11 public key matches the one in the CA-cert we're using
One more cherry-pick: numinit@98dbced |
@numinit do you have setup instructions for tpm2? I'd love to try that out. |
… forcing CGO onto people
@JackDoanRivian Ack, I missed that comment. Good example that runs in the NixOS test suite is here: https://github.com/numinit/nixpkcs/blob/master/nixos/tests/nebula.nix#L41 Config specializations (I test this with both nssdb and TPM2) are here: https://github.com/numinit/nixpkcs/blob/master/flake.nix#L51 Ultimately nixpkcs creates all those declaratively, the imperative commands and environment are buried in that source, though. :-) |
Finally got some time to test this on macOS with an old school YubiKey 4 (USB A!) and I am able to get it working. A couple notes:
And here's how I configured it:
pki:
ca: /Users/jmaguire/src/nebula/ca.crt
cert: /Users/jmaguire/src/nebula/pkcs11.crt
key: pkcs11:model=YubiKey%20YK4;manufacturer=Yubico%20%28www.yubico.com%29;serial=5599240;token=YubiKey%20PIV%20%235599240;id=%01?module-path=/opt/homebrew/lib/libykcs11.dylib&pin-value=123456 Here's how I patched the Makefile: diff --git a/Makefile b/Makefile
index 1b75dd5..bba5a4c 100644
--- a/Makefile
+++ b/Makefile
@@ -117,8 +117,9 @@ bin-freebsd-arm64: build/freebsd-arm64/nebula build/freebsd-arm64/nebula-cert
bin-boringcrypto: build/linux-$(shell go env GOARCH)-boringcrypto/nebula build/linux-$(shell go env GOARCH)-boringcrypto/nebula-cert
mv $? .
-bin-pkcs11: build/linux-$(shell go env GOARCH)-pkcs11/nebula build/linux-$(shell go env GOARCH)-pkcs11/nebula-cert
- mv $? .
+bin-pkcs11: BUILD_ARGS += -tags pkcs11
+bin-pkcs11: CGO_ENABLED = 1
+bin-pkcs11: bin
bin:
go build $(BUILD_ARGS) -ldflags "$(LDFLAGS)" -o ./nebula${NEBULA_CMD_SUFFIX} ${NEBULA_CMD_PATH} |
Adds PKCS11 support for the P256 curve. It could probably be generalized for curve 25519 as well, but this is what our HSMs support, and therefore what I could easily test.
I've tested very thoroughly against the difficult-to-obtain Microchip TA100, and did a quick verification with a Yubikey 5C.
Here's some quick examples of how this change can be exercised on a TA100. I'll follow up with Yubikey instructions when time permits:
This change requires building with CGO, so here's the exact invocation we used (adjust as needed for your target architecture of course):
Unfortunately static linking with musl doesn't work, because musl doesn't implement
dlopen
for statically-linked binaries. 😞A config file's
pki
section would look like this:Please let me know what you think! I'm very open to making adjustments to make this fit in better with the existing code.