PKCS11 Wrapper #14

glatigny · 2024-11-13T10:20:43Z

Continuation of #8

Split the wrapper and the PKCS11 Client
Remove the Encapsulation in Encrypt/Decrypt
Use "ReadBaoVariable" and update Env variables
Update parameters (according to developer.hashicorp.com/vault/docs/configuration/seal/pkcs11)
Support of Token Label parameter
Add tests for options
Apply @cipherboy comments from PR Add PKCS#11 Wrapper #8

Tested with SoftHSM2 (AES_CBC_PAD & RSA_PKCS keys)

Signed-off-by: jsmoon <[email protected]>

Some parts of the code are commented for future improvements. In case of algorithm which would requires more inputs to know the IV length, the IsIvNeeded can return "0". Clearly I'm not very fan of it, but I don't know if giving access to the PKCS#11 session is better. Signed-off-by: Jérôme GLATIGNY <[email protected]>

Signed-off-by: Jérôme GLATIGNY <[email protected]>

* Moving PKCS11 client into the struct * Creating the client during configuration loading * Support of Finalize to destroy the client * Adding tools functions Signed-off-by: Jérôme GLATIGNY <[email protected]>

* Move client into a specific file * Support of setting: token label * Encrypt do not wrap anymore * Decrypt do not unwrap anymore * Encrypt store the key identifier in the KeyInfo * Decrypt and read the key identifier from the KeyInfo * Client Encrypt and Decrypt return and take a key identifier * Pkcs11Key struct to store the key identifier (label and id) Signed-off-by: Jérôme GLATIGNY <[email protected]>

Signed-off-by: Jérôme GLATIGNY <[email protected]>

wrappers/pkcs11/options.go

Signed-off-by: Jérôme GLATIGNY <[email protected]>

cipherboy

Sorry about the delay! This is looking fairly good, thank you! I've left a few comments here and there.

wrappers/pkcs11/pkcs11.go

cipherboy · 2024-11-14T10:46:14Z

wrappers/pkcs11/pkcs11.go

+	k.keyId = client.GetCurrentKey().String()
+
+	// Send a value to test the wrapper and to set the current key id
+	if _, err := k.Encrypt(context.Background(), []byte("a")); err != nil {


I wonder if we think this will always work? For CKM_AES_GCM and CKM_RSA_PKCS_OAEP I think it will, but not for CKM_AES_CBC, which Vault Enterprise supports.

Since this is a fresh start, I suppose we could forbid insecure mechanisms and require AEAD from our mechanism... It is 2024, I don't really see the need to encourage use of CBC any more. Perhaps a note why we think this is fine would be good?

Do you want to take a stab at denying insecure mechanisms? I don't know how we'd do that generally, e.g., if someone wanted to substitute Korean or Japanese algorithms here for instance... I don't really want to maintain the encrypt-then-MAC construction either, tbh.

I think I'll remove the encryption test here. The PKCS11 client already test the session.
Regarding the mechanisms, of course AEAD are better.

Something currently wrong in the implementation is the parameter hmac_mechanism is not handled and should be required for CBC and CBC PAD.
At first, I was thinking that SoftHSM2 did not support AES GCM, but it's just the documentation which is not up-to-date.
There is still the issue with YubiHSM2 but yes, it might be interesting to review the list of supported mechanisms !

I'm not inclined to support hmac parameter and just require an AEAD mech.

Yes, of course.
It just like you wrote 3 days ago: to support encrypt-then-mac it requires to add the HMAC mechanism parameter.

cipherboy · 2024-11-25T15:49:24Z

wrappers/pkcs11/pkcs11_client.go

+	wrapConfig := new(wrapping.WrapperConfig)
+	wrapConfig.Metadata = make(map[string]string)
+	wrapConfig.Metadata["lib"] = lib
+	wrapConfig.Metadata["key_label"] = keyLabel


You probably know better than I, but I was reading the PKCS#11v3.0 spec and I noticed it very carefully doesn't say CKA_LABEL is guaranteed unique. OpenSC/OpenSC#1613 seems to reference this as a place where it has happened in the wild...

Do we want to allow CKA_UNIQUE_ID as an identifier to compensate?

Indeed the Key Label (CKA_LABEL) and Key ID (CKA_ID) are not unique ; the user is free to choose them (and edit them).
The Unique ID is unique by definition and generated by the Token/HSM itself ; which means that if you import your secret the Unique ID will change.
That's why, I'm not a fan of using the Unique ID in the configuration.

That's why, I'm not a fan of using the Unique ID in the configuration.

Hmm, but if we error like in https://github.com/openbao/go-kms-wrapping/pull/14/files#r1863149975, we're essentially then saying there's no way to use HSMs configured with multiple keys with the same CKA_LABEL and/or CKA_ID values?

I think we're fine with that, but just making sure I understand the implications of this. It seems like we'd prefer a way to disambiguate the keys, given the FindObjects(...) return order may not be deterministic.

Label + ID + KeyType muse be unique yes.
Now, if the ID is not provide during encryption and we found one key for that label, we can retrieve the ID to store it in the metadata, so during the decryption, we will be able to find the right key even if another one has been created with the same label.
If there is more than one key, the fonction FindKey will return an error.

wrappers/pkcs11/pkcs11_client.go

cipherboy · 2024-11-25T16:05:24Z

wrappers/pkcs11/pkcs11_client.go

+	case api.ReadBaoVariable(EnvHsmWrapperKeyId) != "" && !opts.Options.WithDisallowEnvVars:
+		keyId = api.ReadBaoVariable(EnvHsmWrapperKeyId)
+	case opts.withKeyId != "":
+		keyId = opts.withKeyId


I think this comment might still apply? https://github.com/openbao/go-kms-wrapping/pull/8/files/45ad5ab66ef39ae498a8a736b1a190d34ca1c397#r1718338953

wrappers/pkcs11/pkcs11_client.go

cipherboy · 2024-11-25T16:07:10Z

wrappers/pkcs11/pkcs11_client.go

+	if err := c.client.FindObjectsInit(session, template); err != nil {
+		return nil, fmt.Errorf("failed to pkcs11 FindObjectsInit: %s", err)
+	}
+	obj, _, err := c.client.FindObjects(session, 2)


How do we want to handle multiple results? I think right now, given the above comment about uniqueness, we're at the mercy of the library as to what order it returns results in, which might be random.

Thoughts?

There is a part in Encrypt and Decrypt that I'll move into FindKey: (and I will change the text too).

if len(obj) != 1 { return nil, nil, fmt.Errorf("expected 1 object, got %d", len(obj)) }

Since we're asking for 2 objects max, we can detect if there is more than 1 and then return an error.

wrappers/pkcs11/pkcs11_client.go

cipherboy · 2024-11-25T18:25:32Z

wrappers/pkcs11/pkcs11_client.go

+		return nil, nil, err
+	}
+
+	var iv []byte


@glatigny I think you need to correctly handle mechanism parameters here. See https://pkg.go.dev/github.com/miekg/pkcs11#NewGCMParams or https://pkg.go.dev/github.com/miekg/pkcs11#NewOAEPParams.

Otherwise, this fails for me:

pkcs11_acc_test.go:32: err : failed to pkcs11 EncryptInit: pkcs11: 0x7: CKR_ARGUMENTS_BAD

I ended up with a Dockerfile:

FROM registry.fedoraproject.org/fedora:latest COPY . /go-kms-wrapping # pkcs11-tool --module "$BAO_HSM_LIB" --token-label "$BAO_HSM_TOKEN_LABEL" --so-pin 1234 --pin 4321 --keygen --key-type aes:32 --label "$BAO_HSM_KEY_LABEL" && \ # Set up SoftHSM, generate a key, and run tests. RUN true && \ dnf update --refresh -y && \ dnf install -y softhsm opensc golang && \ mkdir /softhsm && \ export SOFTHSM2_CONF=/go-kms-wrapping/wrappers/pkcs11/softhsm2.conf && \ softhsm2-util --init-token --slot 0 --label "OpenBao g-k-w" --so-pin 1234 --pin 4321 && \ softhsm2-util --show-slots && \ export BAO_HSM_TOKEN_LABEL="OpenBao g-k-w" && \ export BAO_HSM_PIN=4321 && \ export BAO_HSM_LIB=/usr/lib64/softhsm/libsofthsm.so && \ export BAO_HSM_KEY_LABEL=bao-root-key && \ pkcs11-tool --module "$BAO_HSM_LIB" --token-label "$BAO_HSM_TOKEN_LABEL" --so-pin 1234 --pin 4321 --keypairgen --key-type rsa:4096 --label "$BAO_HSM_KEY_LABEL" && \ pkcs11-tool --module "$BAO_HSM_LIB" --token-label "$BAO_HSM_TOKEN_LABEL" --so-pin 1234 --pin 4321 --show-info && \ cd /go-kms-wrapping/wrappers/pkcs11 && \ export VAULT_ACC=true && \ go test -v ./... && \ true

with config (wrappers/pkcs11/softhsm2.conf):

directories.tokendir = /softhsm objectstore.backend = file log.level = DEBUG

and run via:

$ podman build -f wrappers/pkcs11/Dockerfile .

cipherboy · 2024-11-26T15:06:42Z

wrappers/pkcs11/pkcs11_client.go

+	switch mech {
+	case pkcs11.CKM_AES_GCM:
+		return true, 16
+	case pkcs11.CKM_AES_CBC_PAD:


I've changed my mind, let's deny this mechanism as well if you don't mind and only do CKM_AES_GCM. Otherwise, we'd need to implement an encrypt-then-mac construct with an additional HMAC key.

However, we should probably implement CKM_AES_CCM; it looks like YubiHSM2 doesn't support GCM mode per https://downloads.yubico.com/support/YubiHSM%20Manual_1_5_0.pdf

CKM_AES_CCM requires some modifications in the current structure.
jsmoon started with mechanisms which requires an IV while CCM uses a nonce and some other parameters (like the MAC Len).
AAD can also be used with GCM or CCM ; I don't think that we want to use it but, if so, we need to store it.
For the moment, the IV is stored as a prefix ; we add it during encryption and we extract it from the ciphertext during decryption.

Bad news... It is not possible to add CCM support for the moment.

The PKCS11 lib doesn't have the function to create the CCM parameters:
https://github.com/miekg/pkcs11/blob/master/params.go
but the struct is available in the headers:
https://github.com/miekg/pkcs11/blob/master/pkcs11t.h#L1888
So it needs a small PR to add the wrapping struct and functions.

@glatigny Hmm.... Sadly it kinda looks like miekg/pkcs11 is getting less active. Things seemed to stall after attempting the PKCS#11 v3.0 port: miekg/pkcs11#148 ... Though, I'm sure he'd be welcoming of a PR for CCM support.

I'm not aware of any good alternatives to this library, so perhaps we should start trying to get involved upstream?

That said, I'm fine leaving off CCM for now.

dangerousd75 · 2024-11-28T12:07:52Z

I am prepared to test this with Thales USB HSM, NitroKey HSM and Yubico 2 HSM in December.
My Entrust Edge will only arrive in February.

* Add rsa_oaep_hash parameter. * New functions for the creation of PKCS11 encrypt/decrypt parameters. * FindKey now return only one key handle. Signed-off-by: Jérôme GLATIGNY <[email protected]>

wrappers/pkcs11/pkcs11_client.go

cipherboy · 2024-12-02T16:31:35Z

wrappers/pkcs11/pkcs11_client.go

+	var params interface{}
+	switch mechanism {
+	case pkcs11.CKM_AES_GCM:
+		params = pkcs11.NewGCMParams(prefix, nil, 128)


Hmm, check out this note: https://pkg.go.dev/github.com/miekg/pkcs11#NewGCMParams

Note that some HSMs, like CloudHSM, will ignore the IV you pass in and write their own. As a result, to support all libraries, memory is not freed automatically, so that after the EncryptInit/Encrypt operation the HSM's IV can be read back out. It is up to the caller to ensure that Free() is called on the GCMParams object at an appropriate time, which is after Encrypt/Decrypt. As an example:

.... so we likely want to make sure we save params here and update our copy of prefix from this value before doing the append in Encrypt(...).

* Do not concat the IV with the encrypted content anymore, use the BlobInfo instead. * Split AES GCM and RSA OAEP into different functions. * Handle Specific cases for GCM (some HSM don't read the IV/nonce and generate it instead). * Use the PKCS#11 random generator and propose an interface for it. * Move constants into a dedicated section. Signed-off-by: Jérôme GLATIGNY <[email protected]>

glatigny · 2024-12-04T09:50:05Z

Hey @cipherboy
The last commit introduce some refactoring.

I wanted to make the constants a bit cleaner so I moved them into a dedicated section.
At the same time, I changed the IV/Nonce length for AES GCM.

While spliting the Encrypt/Decrypt functions for AES GCM and RSA OAEP, I saw how the IV could be handle by CloudHSM (and maybe others) and also saw that BlobInfo can embed the IV.

I also remove the usage of uuid in order to use the random generator via PKCS#11.
Valut has the "Entropy augmentation seal" feature, the GenerateRandom function could be a start for it.

jsmoon-openerd and others added 7 commits June 21, 2024 19:06

Add PKCS#11 Wrapper

1b0bdef

Signed-off-by: jsmoon <[email protected]>

Remove unnecessary conditional statements

1f56ebc

Signed-off-by: jsmoon <[email protected]>

[feat] Update PKCS#11 options to be more compatible with Vault.

315b946

Signed-off-by: Jérôme GLATIGNY <[email protected]>

[refactor] first part of refactoring

c4560b3

* Moving PKCS11 client into the struct * Creating the client during configuration loading * Support of Finalize to destroy the client * Adding tools functions Signed-off-by: Jérôme GLATIGNY <[email protected]>

[refactor] validation of unit test

02488dd

Signed-off-by: Jérôme GLATIGNY <[email protected]>

cipherboy reviewed Nov 13, 2024

View reviewed changes

wrappers/pkcs11/options.go Show resolved Hide resolved

[fix] fixing copyright

66e5465

Signed-off-by: Jérôme GLATIGNY <[email protected]>

glatigny requested a review from cipherboy November 14, 2024 08:08

cipherboy mentioned this pull request Nov 25, 2024

Add PKCS#11 Wrapper #8

Closed

cipherboy reviewed Nov 25, 2024

View reviewed changes

wrappers/pkcs11/pkcs11_client.go Show resolved Hide resolved

cipherboy reviewed Nov 25, 2024

View reviewed changes

cipherboy reviewed Nov 26, 2024

View reviewed changes

[fix] fix support of AES GCM and RSA OAEP.

c2547bd

* Add rsa_oaep_hash parameter. * New functions for the creation of PKCS11 encrypt/decrypt parameters. * FindKey now return only one key handle. Signed-off-by: Jérôme GLATIGNY <[email protected]>

cipherboy reviewed Dec 2, 2024

View reviewed changes

wrappers/pkcs11/pkcs11_client.go Outdated Show resolved Hide resolved

cipherboy reviewed Dec 2, 2024

View reviewed changes

wrappers/pkcs11/pkcs11_client.go Outdated Show resolved Hide resolved

cipherboy reviewed Dec 2, 2024

View reviewed changes

glatigny requested a review from cipherboy December 10, 2024 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PKCS11 Wrapper #14

PKCS11 Wrapper #14

glatigny commented Nov 13, 2024

cipherboy left a comment

cipherboy Nov 14, 2024

glatigny Nov 28, 2024

cipherboy Nov 28, 2024

glatigny Nov 29, 2024

cipherboy Nov 25, 2024

glatigny Nov 28, 2024

cipherboy Dec 2, 2024

glatigny Dec 3, 2024

cipherboy Nov 25, 2024

cipherboy Nov 25, 2024

glatigny Nov 29, 2024

cipherboy Nov 25, 2024

cipherboy Nov 25, 2024

cipherboy Nov 26, 2024

cipherboy Nov 26, 2024

glatigny Nov 29, 2024

glatigny Nov 29, 2024

cipherboy Nov 29, 2024 •

edited

Loading

dangerousd75 commented Nov 28, 2024

cipherboy Dec 2, 2024

glatigny commented Dec 4, 2024 •

edited

Loading

PKCS11 Wrapper #14

Are you sure you want to change the base?

PKCS11 Wrapper #14

Conversation

glatigny commented Nov 13, 2024

cipherboy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cipherboy Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

dangerousd75 commented Nov 28, 2024

Choose a reason for hiding this comment

glatigny commented Dec 4, 2024 • edited Loading

cipherboy Nov 29, 2024 •

edited

Loading

glatigny commented Dec 4, 2024 •

edited

Loading