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

marcellanz · 2022-07-19T22:04:14Z

This PR implements the feature requested by issue: #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.

…without UI tests (yet).

…ntation.

…rt' into feature/transit_pkcs1v15_support

hashicorp-cla · 2022-07-19T22:04:18Z

All committers have signed the CLA.

…sen by parameter.

cipherboy · 2023-05-23T15:39:24Z

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!

marcellanz · 2023-05-24T11:00:56Z

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.

cipherboy · 2023-05-24T11:17:26Z

@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

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.

cipherboy · 2023-05-24T11:21:10Z

sdk/helper/keysutil/policy.go

@@ -818,7 +818,7 @@ func (p *Policy) convergentVersion(ver int) int {
 	return convergentVersion
 }

-func (p *Policy) Encrypt(ver int, context, nonce []byte, value string) (string, error) {
+func (p *Policy) Encrypt(ver int, context, nonce []byte, value string, paddingScheme string) (string, error) {


I should double check I haven't violated this myself recently, but in general, we wish to avoid modifying these functions as they lie in our public sdk/ space.

I think you could change this to Encrypt->EncryptWithAlgorithm and have the former call the latter with oaep by default for consistency perhaps...?

Then if/when we introduce ECEIS based schemes, I think it might make the most sense?

Makes perfectly sense for me too. Does the new EncryptWithAlgorithm have to be documented somewhere then I assume?

No, I don't think our SDK has much documentation sadly. I think it'd be cool to get better inline godoc in the future, but we don't have much presently.

(If you want to work on this, happy to review such PRs in the future too!)

sdk/helper/keysutil/policy.go

cipherboy · 2023-05-24T11:22:59Z

builtin/logical/transit/path_datakey.go

+				Type: framework.TypeString,
+				Description: `The padding scheme to use for decrypt. Currently only applies to RSA key types.
+Options are 'oaep' or 'pkcs1v15'. Defaults to 'oaep'`,
+			},


It'd be great to include Default: "oaep" here for consistency if we could :-) When you call d.Get(...) below, it'll populate it with oaep.

This applies in a few places as well below :-)

If I were to nitpick, for consistency with sign/verify I might suggest calling this encryption_algorithm, as it'll likely be more widely used than just RSA here. padding_scheme is definitely more precise, but boxes us into a corner w.r.t. EC algorithms.

I might suggest calling this encryption_algorithm

I think too, this makes a lot of sense and is consistent with the existing API.

cipherboy · 2023-05-24T11:30:39Z

@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.

marcellanz · 2023-05-24T12:07:58Z

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

hellobontempo · 2023-05-24T17:25:01Z

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!

marcellanz · 2023-05-31T08:25:49Z

@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.

cipherboy · 2023-05-31T11:42:49Z

@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. :-)

hellobontempo

Thanks for tackling the UI part of this work as well! 🥇 The formatting looks great - thanks for following our current linting patterns! 🤩

I left some comments! Nothing blocking, though. It would be great to see a test added in transit-key-actions-test.js confirming that padding_scheme only renders for the desired key type - if that's something you have time to include

ui/app/templates/components/transit-key-action/datakey.hbs

hellobontempo · 2023-06-01T19:00:08Z

ui/app/templates/components/transit-key-action/decrypt.hbs

@@ -28,6 +30,27 @@
        </div>
      </div>
    {{/if}}
+    {{#if (or (eq @key.type "rsa-2048") (eq @key.type "rsa-3072") (eq @key.type "rsa-4096"))}}


same comment here as above ☝️

hellobontempo · 2023-06-01T19:00:21Z

ui/app/templates/components/transit-key-action/decrypt.hbs

+              onchange={{action (mut @padding_scheme) value="target.value"}}
+            >
+              {{#each (array "oaep" "pkcs1v15") as |scheme|}}
+                <option selected={{if @padding_scheme (eq @padding_scheme scheme) (eq scheme "oaep")}} value={{scheme}}>


and here! 😄

hellobontempo · 2023-06-01T19:00:35Z

ui/app/templates/components/transit-key-action/encrypt.hbs

@@ -40,6 +47,27 @@
        </div>
      </div>
    {{/if}}
+    {{#if (or (eq @key.type "rsa-2048") (eq @key.type "rsa-3072") (eq @key.type "rsa-4096"))}}


also this file and line 62

hellobontempo · 2023-06-01T19:00:46Z

ui/app/templates/components/transit-key-action/rewrap.hbs

@@ -29,6 +34,27 @@
        </div>
      </div>
    {{/if}}
+    {{#if (or (eq @key.type "rsa-2048") (eq @key.type "rsa-3072") (eq @key.type "rsa-4096"))}}


cipherboy · 2023-06-21T15:04:10Z

\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!

marcellanz · 2023-06-22T09:29:39Z

\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.

Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com>

digital-content-events · 2023-08-16T15:03:58Z

📄 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`

website/content/api-docs/secret/transit.mdx

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

schavis

Docs LGTM, feel free to merge once ENG signs off.

hashishaw · 2023-11-20T16:56:47Z

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

dj-thd2 · 2024-01-03T20:52:34Z

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 :)

marcellanz · 2024-01-10T11:06:35Z

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.

heatherezell · 2024-07-15T19:37:03Z

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.

Hi there, just wanting to check on this and see how things are going. Thanks!

stevendpclark · 2024-10-10T13:21:05Z

Closing this PR out, thanks @marcellanz for all your work on this PR!

Your work was incorporated within #25486 and will be available in the future 1.19 version of Vault.

marcellanz added 6 commits July 19, 2022 19:06

[transit-pkcs1v15] transit support for the pkcs1v15 padding scheme – …

381d091

…without UI tests (yet).

Merge branch 'hashicorp:main' into feature/transit_pkcs1v15_support

e61c72f

Merge branch 'hashicorp:main' into feature/transit_pkcs1v15_support

6d92f49

Merge branch 'hashicorp:main' into feature/transit_pkcs1v15_support

9b57e68

[transit-pkcs1v15] renamed padding_scheme parameter in transit docume…

91d11dc

…ntation.

Merge remote-tracking branch 'upstream/feature/transit_pkcs1v15_suppo…

cbc17d1

…rt' into feature/transit_pkcs1v15_support

marcellanz requested a review from taoism4504 as a code owner July 19, 2022 22:04

marcellanz added 2 commits July 20, 2022 00:06

[transit-pkcs1v15] add changelog file.

31b0c51

[transit-pkcs1v15] remove the algorithm path as padding_scheme is cho…

8a7728d

…sen by parameter.

heatherezell added secret/transit cryptosec enhancement labels Jul 25, 2022

cipherboy self-assigned this May 23, 2023

cipherboy reviewed May 24, 2023

View reviewed changes

hellobontempo added the ui label May 24, 2023

hellobontempo reviewed Jun 1, 2023

View reviewed changes

marcellanz and others added 3 commits August 16, 2023 17:03

Update ui/app/templates/components/transit-key-action/datakey.hbs

99a82d4

Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com>

Update ui/app/templates/components/transit-key-action/datakey.hbs

a19b563

Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com>

Update ui/app/templates/components/transit-key-action/datakey.hbs

efa5fb1

Co-authored-by: claire bontempo <68122737+hellobontempo@users.noreply.github.com>

heatherezell requested review from schavis and removed request for taoism4504 October 28, 2023 00:07

schavis requested changes Oct 31, 2023

View reviewed changes

website/content/api-docs/secret/transit.mdx Outdated Show resolved Hide resolved

website/content/api-docs/secret/transit.mdx Outdated Show resolved Hide resolved

website/content/api-docs/secret/transit.mdx Outdated Show resolved Hide resolved

marcellanz and others added 3 commits November 2, 2023 14:03

Update website/content/api-docs/secret/transit.mdx

9b56e02

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

Update website/content/api-docs/secret/transit.mdx

8f79fc0

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

Update website/content/api-docs/secret/transit.mdx

ee4a3c3

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

schavis reviewed Nov 13, 2023

View reviewed changes

schavis self-requested a review November 13, 2023 22:09

schavis added the content-lgtm Content changes approved. Merge depends on code review label Nov 13, 2023

vercel bot deployed to Preview November 13, 2023 22:09 View deployment

schavis requested review from schavis and removed request for schavis February 21, 2024 23:44

schavis added the needs-eng-review Community PR waiting for ENG review label May 9, 2024

heatherezell added the waiting-for-response label Jul 27, 2024

stevendpclark closed this Oct 10, 2024

github-actions bot removed the waiting-for-response label Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

marcellanz commented Jul 19, 2022 •

edited

Loading

hashicorp-cla commented Jul 19, 2022 •

edited

Loading

cipherboy commented May 23, 2023

marcellanz commented May 24, 2023

cipherboy commented May 24, 2023

cipherboy left a comment

cipherboy May 24, 2023

marcellanz May 25, 2023

cipherboy May 25, 2023

cipherboy May 24, 2023

cipherboy May 24, 2023

cipherboy May 24, 2023

marcellanz May 25, 2023

cipherboy commented May 24, 2023

marcellanz commented May 24, 2023

hellobontempo commented May 24, 2023

marcellanz commented May 31, 2023

cipherboy commented May 31, 2023

hellobontempo left a comment

hellobontempo Jun 1, 2023

hellobontempo Jun 1, 2023

hellobontempo Jun 1, 2023

hellobontempo Jun 1, 2023

cipherboy commented Jun 21, 2023

marcellanz commented Jun 22, 2023

digital-content-events bot commented Aug 16, 2023 •

edited

Loading

`content/api-docs/secret/transit.mdx`

schavis left a comment

hashishaw commented Nov 20, 2023

dj-thd2 commented Jan 3, 2024

marcellanz commented Jan 10, 2024

heatherezell commented Jul 15, 2024

stevendpclark commented Oct 10, 2024

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

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

Conversation

marcellanz commented Jul 19, 2022 • edited Loading

hashicorp-cla commented Jul 19, 2022 • edited Loading

cipherboy commented May 23, 2023

marcellanz commented May 24, 2023

cipherboy commented May 24, 2023

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

cipherboy commented May 24, 2023

marcellanz commented May 24, 2023

hellobontempo commented May 24, 2023

marcellanz commented May 31, 2023

cipherboy commented May 31, 2023

hellobontempo 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

cipherboy commented Jun 21, 2023

marcellanz commented Jun 22, 2023

digital-content-events bot commented Aug 16, 2023 • edited Loading

📄 Content Checks

content/api-docs/secret/transit.mdx

schavis left a comment

Choose a reason for hiding this comment

hashishaw commented Nov 20, 2023

dj-thd2 commented Jan 3, 2024

marcellanz commented Jan 10, 2024

heatherezell commented Jul 15, 2024

stevendpclark commented Oct 10, 2024

marcellanz commented Jul 19, 2022 •

edited

Loading

hashicorp-cla commented Jul 19, 2022 •

edited

Loading

digital-content-events bot commented Aug 16, 2023 •

edited

Loading

`content/api-docs/secret/transit.mdx`