search

notaryproject / notation-hashicorp-vault

HashiCorp Vault provider for Notation
https://notaryproject.dev/
Apache License 2.0
7 stars 8 forks source link

Support for longer RSA keys #20

Closed tomaszkrzyzanowski closed 1 week ago

tomaszkrzyzanowski commented 6 months ago

Hello!

As for now, keyhelper is limited to supporting only RSA2048 keys, while notation CLI and Vault's transit secret engine is supporting longer keys (3072 and 4096 bit).

I would appreciate it if keyhelper could detect the key length and use the right key length during importing key to transit engine

cipherboy commented 5 months ago

This sounds like a great RFE @tomaszkrzyzanowski -- are you interested in providing a PR for this? I think wrapPrivateKey:

https://github.com/notaryproject/notation-hashicorp-vault/blob/10173d8ef5706424cfa978ab5e2c8f6002c45805/cmd/key-helper/importKey.go#L101

can be updated to return another string or int corresponding to the key type+length (either in transit format or otherwise just the raw number of bits).

If you move this cast earlier:

https://github.com/notaryproject/notation-hashicorp-vault/blob/10173d8ef5706424cfa978ab5e2c8f6002c45805/cmd/key-helper/importKey.go#L131

you can just call the Size() method. And then obviously importKeyToTransit would need to be updated accordingly:

https://github.com/notaryproject/notation-hashicorp-vault/blob/main/cmd/key-helper/importKey.go#L154

I don't know off the top of my head if Notary supports other types of keys (ECDSA?) but given the upcoming PQC standards, I'd imagine it'd want to start doing so... So perhaps returning the string would be most optimal and ECDSA/... support can be added more easily.

HTH!

tomaszkrzyzanowski commented 5 months ago

@cipherboy I was already working locally on support for this and in #21 is what I have implemented with mostly standard libraries.

There were a few differences in how Notation and Vault defines names for ie. hash algorithms, and I tried to cover this with maps.

Moreover, when I started, I haven't had bigger awareness of notation-go library, and I'm thinking if it's fine to leave it as is or maybe reimplement this with notation-go/signature parts instead?

My code was mostly written before your suggestion about contribution, so I decided to show it as it is, but I can rework it accordingly to your description in previous message :)

BR, Tomasz

tomaszkrzyzanowski commented 1 week ago

23 Has been merged, so it seems to be implemented :)