Add PKCS#11 Wrapper #8
Conversation
Signed-off-by: jsmoon <[email protected]>
Signed-off-by: jsmoon <[email protected]>
|
Hi and thanks for that PR ! I'm working on a project which requires the usage of an HSM (so PKCS#11) ; thus I want to help and contribute. For the usage of RSA keys, the initialization is working good. First, I made some tests using an AES256 key and I've got Maybe it can be interesting to make the PKCS#11 wrapper settings compatible with the Vault wrapper (module => lib, etc)? |
|
\o hey @jsmoon-openerd! Do you mind adding the DCO ( |
| // These constants contain the accepted env vars; the Vault one is for backwards compat | ||
| const ( | ||
| EnvPkcs11WrapperKeyId = "PKCS11_WRAPPER_KEY_ID" | ||
| EnvVaultPkcs11SealKeyId = "VAULT_PKCS11_SEAL_KEY_ID" |
There was a problem hiding this comment.
Do you want to name this BAO_ and use the helper to read the value?
Also, I think per docs this should be called VAULT_HSM_KEY_ID and other environment variables shouldn't be PKCS11_... but instead BAO_HSM_... (which will then be VAULT_ with the helper). :-)
|
|
||
| // Map that holds non-sensitive configuration info | ||
| wrapConfig := new(wrapping.WrapperConfig) | ||
| wrapConfig.Metadata = make(map[string]string) |
| // Order of precedence Pkcs11 values: | ||
| // * Environment variable | ||
| // * Value from Vault configuration file | ||
| // * Instance metadata role (access key and secret key) |
There was a problem hiding this comment.
I don't think this is true, right?
Under Azure KMS, we see:
credentialChain := []providers.Provider{
providers.NewEnvCredentialProvider(),
providers.NewConfigurationCredentialProvider(credConfig),
providers.NewInstanceMetadataProvider(),
}
but we don't see that here, because we don't put PKCS#11 pins in the metadata for the running VM (like Azure puts credentials into the VM's metadata that it can access to communicate with other services).
|
|
||
| // EncryptDEK uses the PKCS11 encrypt operation to encrypt the DEK. | ||
| func (kms *pkcs11KMS) EncryptDEK(ctx context.Context, plainDEK []byte) ([]byte, error) { | ||
| p := pkcs11.New(kms.module) |
There was a problem hiding this comment.
I'd expect this to be done up in SetConfig(...) and then cached for subsequent calls. I'd also prefer to refactor login out into a separate helper that can be called in multiple places.
| } | ||
|
|
||
| defer p.Destroy() | ||
| defer p.Finalize() |
There was a problem hiding this comment.
p.Destroy() and p.Finalize() should probably be called from Finalize(...) on the Wrapper, not per-call, otherwise there will be lots and lots of overhead.
I'm not sure that this is thread safe to do this way anyways (loading the module per-call); Wrappers try to be concurrency friendly.
| } | ||
| defer p.Logout(session) | ||
|
|
||
| keyIdBytes, err := hex.DecodeString(kms.keyId) |
There was a problem hiding this comment.
Per docs I think we need to trim the 0x prefix?
| } | ||
|
|
||
| // EncryptDEK uses the PKCS11 encrypt operation to encrypt the DEK. | ||
| func (kms *pkcs11KMS) EncryptDEK(ctx context.Context, plainDEK []byte) ([]byte, error) { |
There was a problem hiding this comment.
I'd prefer we don't call this EncryptDEK -- while perhaps strictly true on some level, I think we want this to be just strictly an Encrypt(...) operation and not something that implies it is a Wrap(...) operation.
| wrapConfig.Metadata["kms_key_id"] = k.keyId | ||
| wrapConfig.Metadata["slot"] = strconv.Itoa(int(k.client.slot)) | ||
| if k.client.label != "" { | ||
| wrapConfig.Metadata["label"] = k.client.label |
There was a problem hiding this comment.
Per the docs, I think this actually backwards -- I think label is required, key_id is optional.
| // Map that holds non-sensitive configuration info | ||
| wrapConfig := new(wrapping.WrapperConfig) | ||
| wrapConfig.Metadata = make(map[string]string) | ||
| wrapConfig.Metadata["kms_key_id"] = k.keyId |
There was a problem hiding this comment.
| wrapConfig.Metadata["kms_key_id"] = k.keyId | |
| wrapConfig.Metadata["key_id"] = k.keyId |
For consistency :-)
| return nil, fmt.Errorf("given plaintext for encryption is nil") | ||
| } | ||
|
|
||
| env, err := wrapping.EnvelopeEncrypt(plaintext, opt...) |
There was a problem hiding this comment.
I don't think we want to do this by default; I'd prefer direct encryption. This will help in the future if we add managed keys to the Transit engine.
For seal purposes, it doesn't matter as the root key is small enough to be directly encrypted.
| // This returns the ciphertext, and/or any errors from this | ||
| // call. This should be called after the KMS client has been instantiated. | ||
| func (k *Wrapper) Encrypt(_ context.Context, plaintext []byte, opt ...wrapping.Option) (*wrapping.BlobInfo, error) { | ||
| if plaintext == nil { |
There was a problem hiding this comment.
I guess this is the check used elsewhere, but I'd probably prefer:
| if plaintext == nil { | |
| if len(plaintext) == 0 { |
| return k.currentKeyId.Load().(string), nil | ||
| } | ||
|
|
||
| // Encrypt is used to encrypt the master key using the the PKCS11. |
There was a problem hiding this comment.
| // Encrypt is used to encrypt the master key using the the PKCS11. | |
| // Encrypt is used to encrypt data using the the PKCS11 key. |
| return nil, fmt.Errorf("error decrypting data encryption key: %w", err) | ||
| } | ||
|
|
||
| envInfo := &wrapping.EnvelopeInfo{ |
| } | ||
| } | ||
|
|
||
| func MechanisString(mech uint) string { |
There was a problem hiding this comment.
| func MechanisString(mech uint) string { | |
| func MechanismString(mech uint) string { |
| } | ||
| } | ||
|
|
||
| type pkcs11KMS struct { |
There was a problem hiding this comment.
My structuring comment would be to do what Transit does, and split the low-level PKCS#11 client out from the higher level Wrapper implementation into two separate files. My 2c. :-)
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode key id: %w", err) | ||
| } | ||
| template := []*pkcs11.Attribute{ |
There was a problem hiding this comment.
Keyfinding should probably also be refactored into a common method.
| if kms.mechanism != 0 { | ||
| mechanism = kms.mechanism | ||
| } else { | ||
| mechanism = pkcs11.CKM_AES_CBC_PAD |
There was a problem hiding this comment.
I know this is what Vault prefers, but I'd like to see CKM_AES_GCM used instead.
There was a problem hiding this comment.
I think we also need more advanced auto-detection of mechanisms, based on the list of supported mechanisms, but that's an issue for another time and I'm happy to take that one on.
| if kms.mechanism != 0 { | ||
| mechanism = kms.mechanism | ||
| } else { | ||
| mechanism = pkcs11.CKM_RSA_PKCS |
There was a problem hiding this comment.
| mechanism = pkcs11.CKM_RSA_PKCS | |
| mechanism = pkcs11.CKM_RSA_PKCS_OAEP |
Vault prefers this.
| if kms.mechanism != 0 { | ||
| mechanism = kms.mechanism | ||
| } else { | ||
| mechanism = pkcs11.CKM_AES_CBC_PAD |
There was a problem hiding this comment.
I think we also need more advanced auto-detection of mechanisms, based on the list of supported mechanisms, but that's an issue for another time and I'm happy to take that one on.
|
Hey @jsmoon-openerd! Sorry, I meant to batch comments but I guess somehow they got submitted in groups rather than all at once (I'd rather have that problem than them disappearing, though!). The change set is looking fairly good for a first pass, but I've made some commentary about code structuring, compatibility with Vault, and potential issues with PKCS#11 modules. Let me know if you want me to pick this up for you -- I'll need you to push the DCO sign-off first, but happy to do so if you want. :-) |
|
\o hey @jsmoon-openerd -- just checking in if you're able to do the DCO sign-off. :-) Thanks! |
|
any news on this one guys? |
|
@SorinS Since we don't have signed-off commits, we'll be unable to merge this unless @jsmoon-openerd is able to update them. :-) So, a clean re-implementation would be ideal as a path forward if that doesn't happen sometime here in the near future. |
Thank you for answering, I'll wait a bit for @jsmoon-openerd then I might take this up. This is an enterprise feature and I am certain it will be needed for unsealing and PKI in large enterprises... |
|
@SorinS I concur, though currently this would just implement auto-unseal with HSMs. Managed keys (as Vault Enterprise calls them) is an entire codebase we don't have, and I'm not sure I'd implement it the same way either tbh. That said, have you seen our draft roadmap -- if you're interested in doing g-k-w and core work, there's an item in there about using g-k-w plugins from OpenBao which would be cool :-) |
|
@SorinS If you're stil interested in contributing a new, clean-room implementation, I'm happy to review it. :-) |
|
@cipherboy thank you, I will try to develop a new implementation. I am aiming to test on SoftHSM and Luna 7... |
|
@SorinS I just looked at the commits again, and only the last commit (fixing CBC Pad support) is not signed off. If you want to base on the previous commit and apply my suggestions above, I think we can use this PR as-is, minus the commit with the CBC Pad fixes... I think most of that will need to be redone anyways based on my feedback above so I think you might end up recreating something different... (But please, preserve the original attribution on the first two commits!) |
|
@cipherboy thank you, working on it... |
PKCS#11 currently support RSA, AES encryption algorithm.
splitted from #6 .