vault
vault copied to clipboard
Vault-16367: add support RSA padding scheme pkcs1v15 for encryption
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.
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!
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 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 :-)
@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 thanks for the review and comments; I'll have a look this evening on them and make it mergeable.
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!
@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 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. :-)
\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!
\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.
📄 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!
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 :)
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.