vault icon indicating copy to clipboard operation
vault copied to clipboard

Vault-16367: add support RSA padding scheme pkcs1v15 for encryption

Open marcellanz opened this issue 1 year ago • 15 comments

This PR implements the feature requested by issue: https://github.com/hashicorp/vault/issues/16367 with support for the transit secrets engine to choose a RSA padding scheme by the padding_scheme parameter for encryption, decryption, datakey and rewrap.

This PR includes UI support to choose the padding scheme for RSA keys.

marcellanz avatar Jul 19 '22 22:07 marcellanz

CLA assistant check
All committers have signed the CLA.

hashicorp-cla avatar Jul 19 '22 22:07 hashicorp-cla

Hey there @marcellanz -- terribly sorry about the delay. Originally I think there was some discussion over the scope of this and if it fit well with our goals for Transit. The good news is, I'd like to see this land in the next (1.15) release. :-)

If you're still up for it, I'd be happy to work with you to get this reviewed and merged!

cipherboy avatar May 23 '23 15:05 cipherboy

If you're still up for it, I'd be happy to work with you to get this reviewed and merged!

Thanks @cipherboy, no worries. We'd still much like to see this merged into a release to be used with an official released version of vault. Let me know how I can help and what to do to go forward with the PR.

marcellanz avatar May 24 '23 11:05 marcellanz

@marcellanz Cool! I think a rebase would be the first step; we've added a few features since then that might be causing these conflicts: managed keys and public-key only asymmetric crypto.

I'll add some commentary in the PR :-)

cipherboy avatar May 24 '23 11:05 cipherboy

@marcellanz This is small enough I might try and sneak it in before the release if you can get the SDK change fixed up and then merge it in.

cipherboy avatar May 24 '23 11:05 cipherboy

@cipherboy thanks for the review and comments; I'll have a look this evening on them and make it mergeable.

marcellanz avatar May 24 '23 12:05 marcellanz

I should get @hellobontempo to review the Transit UI additions but otherwise this is looking good to me with the one API change noted.

Rest are tbh mostly nits.

👋 I added the ui label which should run the ui tests when you push to this branch next (otherwise they're skipped) . First pass, things look good. But I won't have time to give it a more thorough review until tomorrow!

hellobontempo avatar May 24 '23 17:05 hellobontempo

@cipherboy I've corrected most comments from you above and made it buildable with an up to date branch. I'll push that soon. Open is the naming change from padding_scheme to encryption_algorithm which I think is reasonable and consistent with other parameters.

marcellanz avatar May 31 '23 08:05 marcellanz

@marcellanz Cool, ty! I think this might not make 1.14 sadly at this point, but 1.15 branch is open so I can merge it there when you get the updates pushed. :-)

cipherboy avatar May 31 '23 11:05 cipherboy

\o hey @marcellanz -- wanted to let you know 1.14 just GA'd, so we're happy to merge this to 1.15 branch when you get the updates in btw :-) No rush!

cipherboy avatar Jun 21 '23 15:06 cipherboy

\o hey @marcellanz -- wanted to let you know 1.14 just GA'd, so we're happy to merge this to 1.15 branch when you get the updates in btw :-) No rush!

Awesome! Thanks @cipherboy and @hellobontempo – "work work" has kicked in lately, but I will soon catch up here with UI feedback covering and then push a clean state.

marcellanz avatar Jun 22 '23 09:06 marcellanz

📄 Content Checks

Updated: Thu, 02 Nov 2023 13:05:01 GMT

Found 5 error(s)

content/api-docs/secret/transit.mdx

Position Description Rule
11:9-11:55 Unexpected product-relative link: /docs/secrets/transit. Ensure that relative links are fully-qualified Developer paths: /{productSlug}/docs/secrets/transit ensure-valid-link-format
73:14-73:71 Unexpected product-relative link: /docs/concepts/duration-format. Ensure that relative links are fully-qualified Developer paths: /{productSlug}/docs/concepts/duration-format ensure-valid-link-format
196:29-196:89 Unexpected product-relative link: /docs/secret/transit. Ensure that relative links are fully-qualified Developer paths: /{productSlug}/docs/secret/transit ensure-valid-link-format
394:48-394:105 Unexpected product-relative link: /docs/concepts/duration-format. Ensure that relative links are fully-qualified Developer paths: /{productSlug}/docs/concepts/duration-format ensure-valid-link-format
586:119-586:171 Unexpected product-relative link: /docs/configuration/listener/tcp. Ensure that relative links are fully-qualified Developer paths: /{productSlug}/docs/configuration/listener/tcp ensure-valid-link-format

No rush, but once the merge conflicts are resolved I'm happy to give it a final look-over from the UI perspective!

hashishaw avatar Nov 20 '23 16:11 hashishaw

Hey everyone, is this PR still alive? This feature would be very useful to decrypt data which was encrypted with plain openssl rsautl CLI, which by default use pkcs1v15 so it's not compatible with the current transit engine. If @marcellanz is busy atm, I would be pleased to update it and shave off the pending issues to let it roll forward :)

dj-thd2 avatar Jan 03 '24 20:01 dj-thd2

Hey everyone, is this PR still alive? This feature would be very useful to decrypt data which was encrypted with plain openssl rsautl CLI, which by default use pkcs1v15 so it's not compatible with the current transit engine. If @marcellanz is busy atm, I would be pleased to update it and shave off the pending issues to let it roll forward :)

Hi @dj-thd2 – alive, planned to continue soon. Let us solve merge conflicts and then seen how it fits.

marcellanz avatar Jan 10 '24 11:01 marcellanz