[WIP] Add support for longer RSA keys longer than 2048, init ECDSA support #21

tomaszkrzyzanowski · 2024-03-19T21:22:20Z

Closes #20

as @cipherboy suggested I have created the code to support longer RSA and added support for ECDSA keys

Looking forward to suggestions as I'm not very proficient in Go yet :)

RSA part was tested (load key, sign) with 4096 bit keys.
ECDSA was tested with ecdsa-p521 curve (load only yet)

cipherboy

Some feedback :-) I think there's some good parts here, but it could be simplified.

Thanks @tomaszkrzyzanowski for getting this started!

cipherboy · 2024-03-20T23:55:58Z

internal/keyvault/keyvault.go

@@ -52,14 +53,15 @@ func (vw *VaultClientWrapper) GetCertificateChain(ctx context.Context) ([]*x509.
 	return crypto.ParseCertificates(certBytes)
 }

-func (vw *VaultClientWrapper) SignWithTransit(ctx context.Context, encodedData string, signAlgorithm string) ([]byte, error) {
+func (vw *VaultClientWrapper) SignWithTransit(ctx context.Context, encodedData string, signAlgorithm string, hashAlgorithm string) ([]byte, error) {


This I think is a good catch; right now we do not correctly identify the hash algorithm which means our signatures likely do not comply with Notary's specifications: https://github.com/notaryproject/specifications/blob/main/specs/signature-specification.md#signature-algorithm-requirements

Yes, that was one of an issue while using 4096-bit RSA keys while signing :)

cipherboy · 2024-03-21T00:00:32Z

cmd/key-helper/importKey.go

@@ -55,7 +56,11 @@ func importKeyCommand() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			if err := importKeyToTransit(ctx, vaultClient, ciphertext, keyName); err != nil {
+			keyType, vaultRSAOAEPHashFunction, err := checkKeyType(keyPath)


IIUC this variable naming (vaultRSAOAEPHashFunction) is incorrect. While you do pass it to import, for the BYOK stage, it is determined by a mapping of key algorithm->hash which aligns with notary's spec and isn't set during the above wrapPrivateKey (and thus, isn't used for the BYOK's RSA-OAEP encryption stage).

I don't think this flag is necessary either (due to it not being used in the earlier call), and further, I don't think it'll work (it'd break the parsing on the server, preventing the import, IMO, because it wasn't used and instead artificially determined). I'd remove it and the associated code.

See old line 129, new line 134 for where the RSA-OAEP stage is hard-coded to use SHA-256. There is no benefit in making this configurable and I'd avoid doing so. :-)

ohh that was a leftover :D I wasn't sure why transit was protesting against sent signature, and later I have discovered that was a signer.go issue :)

cipherboy · 2024-03-21T00:04:12Z

cmd/key-helper/importKey.go

@@ -140,7 +145,89 @@ func wrapPrivateKey(wrappingKey string, privateKeyPath string) (string, error) {
 	return base64Ciphertext, nil
 }

-func importKeyToTransit(ctx context.Context, client *vault.Client, ciphertext string, keyName string) error {
+// Checks PrivateKey type and bitlength in EC, PKCS1 and PKCS8 containers


Personally, I expected changes above in wrapPrivateKey, and this code can largely be merged in there, IMO, conditioned off the type of the read key:

switch typedKey := privateKey.(type) { case *rsa.PrivateKey: ... &c case ... &c }

Then the returned key type can be merged alongside the base64Ciphertext value and not need to re-read the source key. My 2c.

cipherboy · 2024-03-21T00:09:29Z

cmd/key-helper/importKey.go

+	pemDecoded, _ := pem.Decode([]byte(keyString))
+
+	switch pemDecoded.Type {
+	case "EC PRIVATE KEY":


I would not, in general, suggest this. PEM blobs can have a variety of markers and usually it is better to opportunistically read various blocks than it is to rely on the typed header.

I guess this is a difference in philosophies between OpenBao and Notary though:

https://github.com/notaryproject/notation-core-go/blob/93218d92c134a02dfa325c074374cdfb4d108fab/x509/key.go#L34-L49

vs

https://github.com/openbao/openbao/blob/6736b400f6a5efdbf7247462d3b21fa49432f634/sdk/helper/certutil/helpers.go#L244-L251

into

https://github.com/openbao/openbao/blob/6736b400f6a5efdbf7247462d3b21fa49432f634/sdk/helper/certutil/helpers.go#L210-L242

which ignores the PEM header entirely, but gives its expected canonical header.

@FeynmanZhou do you know who to ask about the project philosophies around parsing keys here?

By reusing the already parsed version in wrapPrivateKey, you can avoid all the below parsing-of-DER code, but obviously will leave the length determination code. My 2c., but this should be a stronger design and less prone to breaking (one source to update to add new key parsing, rather than once here and once in the shared library). Obviously new key_type correlation will need to be handled manually here, but we can default to err out.

cipherboy · 2024-03-21T00:09:48Z

cmd/key-helper/importKey.go

+		"P-384": "ecdsa-p384",
+		"P-521": "ecdsa-p521",
+	}
+	digestAlgorithm := map[string]string{


Just to be clear, this is the mapping I think should be removed; the above two should stay.

cipherboy · 2024-03-21T00:12:30Z

internal/signature/signer.go

@@ -62,6 +63,14 @@ func Sign(ctx context.Context, req *proto.GenerateSignatureRequest) (*proto.Gene
 		}
 	}

+	// Map of Notation RSA-OAEP hash algorithms to Vault's transit engine /sign endpoint equivalent


This isn't correct. RSA-OAEP is an encryption algorithm; RSA-PSS is used for signing.

More correctly, I think this would be commented as Notary to OpenBao/Vault hash algorithm naming conversion map. My 2c.

… type name

tomaszkrzyzanowski · 2024-03-21T16:42:49Z

@cipherboy thanks for all the suggestions, the idea behind merging checkKeyType and wrapPrivateKey looks much more obvious right now 😅

I'm going to test out the EC keys during the weekend and leave the feedback if they are working fine or require some more work :)

tomaszkrzyzanowski · 2024-04-03T11:02:38Z

@cipherboy I have tested yesterday the EC key-based signatures, and it doesn't work.

It fails to verify the Vault Transit's signature on the notation cli side right after signature creation - the plugin output is failing with

Error: generated signature failed verification: signature is invalid. Error: crypto/ecdsa: verification error

But as for now I have no clue, why Vault would respond with a broken signature - probably sth in invocation of "SignWithTransit()" - I assumed that "signature_algorithm" should be empty for ECDSA (as Vault transit api doc say nothing about EC for that field)

So seems like I'm stuck right now with this, and I'm not sure how I should approach this to solve this, or maybe remove the EC parts and deliver longer RSA only for now

Add support for longer RSA keys longer than 2048, init ECDSA support

1de7d65

tomaszkrzyzanowski requested review from cipherboy, OliverShang, Two-Hearts and shizhMSFT as code owners March 19, 2024 21:22

tomaszkrzyzanowski mentioned this pull request Mar 19, 2024

Support for longer RSA keys #20

Open

cipherboy reviewed Mar 21, 2024

View reviewed changes

tomaszkrzyzanowski added 3 commits March 21, 2024 17:19

Merge checkKeyType function into wrapPrivateKey

46f71a2

Notary to Vault hash algorithm map comment improvement

b46e574

Better descriptions for maps of key length/curve to vault/openbao key…

a8e639e

… type name

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add support for longer RSA keys longer than 2048, init ECDSA support #21

[WIP] Add support for longer RSA keys longer than 2048, init ECDSA support #21

tomaszkrzyzanowski commented Mar 19, 2024

cipherboy left a comment

cipherboy Mar 20, 2024

tomaszkrzyzanowski Mar 21, 2024

cipherboy Mar 21, 2024

cipherboy Mar 21, 2024

tomaszkrzyzanowski Mar 21, 2024

cipherboy Mar 21, 2024

cipherboy Mar 21, 2024

cipherboy Mar 21, 2024

cipherboy Mar 21, 2024

cipherboy Mar 21, 2024

tomaszkrzyzanowski commented Mar 21, 2024

tomaszkrzyzanowski commented Apr 3, 2024

[WIP] Add support for longer RSA keys longer than 2048, init ECDSA support #21

Are you sure you want to change the base?

[WIP] Add support for longer RSA keys longer than 2048, init ECDSA support #21

Conversation

tomaszkrzyzanowski commented Mar 19, 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

tomaszkrzyzanowski commented Mar 21, 2024

tomaszkrzyzanowski commented Apr 3, 2024