Allow configuring the possible salt lengths for RSA PSS signatures #16549

trishankatdatadog · 2022-08-03T13:57:32Z

The Golang crypto/rsa package allows for using at least two possible salt lengths (auto and hash for want better terminology) when creating/verifying RSA PSS signatures. However, these options are currently not exposed via the Vault Transit Secrets Engine. These options would allow for cross-platform signature verification without modifying downstream clients (e.g., the securesystemslib package, used by the python-tuf and python-in-toto supply chain security frameworks, which currently assumes a different salt length by default than Vault does).

This fixes issue #7531.

stevendpclark

Thanks for the contribution @trishankatdatadog!

We've discussed it a bit internally and would like to move forward to get this improvement into Vault but would like a few modifications first.

I've made a few comments on the PR itself but the main change we would like is the ability for an end-user to specify a number of bytes on top of allowing the string values "auto" or "hash" within the new field. The new field would need some customized validation, substituting auto, hash for the PSS constants defined in go, or if the string is not blank, attempt to parse as a number and if successful, validate that it is >0 as well as replicate the same upper bounds validation as done in pss.go.

Some tests, documentation updates and a changelog entry would also be appreciated.

Please let me know if anything is unclear and once again thanks for taking the time to submit this improvement!

builtin/logical/transit/path_sign_verify.go

sdk/helper/keysutil/policy.go

trishankatdatadog · 2022-08-05T19:04:52Z

Thanks @stevendpclark! Happy to address these comments.

However, I see another PR #16227 along the same lines which I haven't taken a deep look at yet from @monomosc. How should we go about this?

stevendpclark · 2022-08-08T12:48:45Z

Thanks @stevendpclark! Happy to address these comments.

Great, please let me know if anything is unclear.

However, I see another PR #16227 along the same lines which I haven't taken a deep look at yet from @monomosc. How should we go about this?

That is unfortunate. At the time that I had started looking at your PR, they had yet to sign the CLA so their PR couldn't be used. I'll leave it up to you if you wish to work with them on this PR together or finish this one up yourself, but in my mind at this time, you have first dibs on the PR.

trishankatdatadog · 2022-08-08T23:46:15Z

That is unfortunate. At the time that I had started looking at your PR, they had yet to sign the CLA so their PR couldn't be used. I'll leave it up to you if you wish to work with them on this PR together or finish this one up yourself, but in my mind at this time, you have first dibs on the PR.

Sounds good. Let me see whether I can use some of their work here. Of course, if I do end up using it, I will credit them accordingly.

trishankatdatadog · 2022-08-09T03:10:26Z

@stevendpclark is b1e3beb0671597c81a07d9e73130d5d8f151e159 better?

stevendpclark · 2022-08-09T13:29:28Z

@stevendpclark is b1e3beb better?

@trishankatdatadog Yes thanks!

I'm not sure if you are working up to this but remember that one of the big things we would like to see in this PR is for an end-user to specify a number of bytes on top of allowing the string values "auto" or "hash" within the new field. So with that said I wouldn't define a custom map/type for SaltLength at least within the SDK. I'd keep it as an int within your new SigningOptions struct to allow passing in converted "auto", "hash" values from the pss.go constants (PSSSaltLengthAuto, PSSSaltLengthEqualsHash) or the user specified bytes value.

trishankatdatadog · 2022-08-09T15:41:57Z

I'm not sure if you are working up to this but remember that one of the big things we would like to see in this PR is for an end-user to specify a number of bytes on top of allowing the string values "auto" or "hash" within the new field. So with that said I wouldn't define a custom map/type for SaltLength at least within the SDK. I'd keep it as an int within your new SigningOptions struct to allow passing in converted "auto", "hash" values from the pss.go constants (PSSSaltLengthAuto, PSSSaltLengthEqualsHash) or the user specified bytes value.

My bad, I did recall reading it somewhere, but somehow missed finding the original comment. Let me look into this tonight.

monomosc · 2022-08-10T14:11:58Z

I'm not sure if you are working up to this but remember that one of the big things we would like to see in this PR is for an end-user to specify a number of bytes on top of allowing the string values "auto" or "hash" within the new field. So with that said I wouldn't define a custom map/type for SaltLength at least within the SDK. I'd keep it as an int within your new SigningOptions struct to allow passing in converted "auto", "hash" values from the pss.go constants (PSSSaltLengthAuto, PSSSaltLengthEqualsHash) or the user specified bytes value.

My bad, I did recall reading it somewhere, but somehow missed finding the original comment. Let me look into this tonight.

Hi, you are of course free to use code from my (older) PR for this subject if you want. #16549

High level, my PR added the pss_salt_length as an (optional) Integer. This automatically solves all problems, as the golang default value for an integer is 0, and thus will not change behavior. -1 would correlate to auto (see https://pkg.go.dev/crypto/rsa#pkg-constants) and any other number would declare the desired salt length directly.

I have also added some sanity unit-tests; I suggest copying them as well.

Lastly there is an open question as to how to handle invalid salt lengths. Every RSA-Key-Size corresponds to a maximum legal salt length; exceeding it willr esult in a fmt.Error with some internal error message by crypto/rsa.

trishankatdatadog · 2022-08-10T14:54:12Z

Hi, you are of course free to use code from my (older) PR for this subject if you want. #16549

Thanks! I am writing something independently now, so let me finish it, and we'll compare and see 🙂 I think we are basically using the same approach.

trishankatdatadog · 2022-08-11T01:06:46Z

@stevendpclark @monomosc Generalised the salt length to an int, and have fairly comprehensive unit tests. Pls let me know what you think, thx.

monomosc · 2022-08-11T11:40:20Z

Your tests now do fail though; I (as a customer) would be happy with the solution though.

stevendpclark

@trishankatdatadog thanks for the unit tests and changes to the SDK. I've added a few more comments, sorry if I wasn't clear previously, my comments were directed to the SDK api only not the end-user facing api.

builtin/logical/transit/path_sign_verify.go

sdk/helper/keysutil/policy.go

builtin/logical/transit/path_sign_verify.go

trishankatdatadog · 2022-08-11T17:51:16Z

Your tests now do fail though; I (as a customer) would be happy with the solution though.

Glad you like it. Let me address @stevendpclark's comments, and then I think we'll all be happy (well, after I also look at the failing tests, could be a red herring).

@trishankatdatadog

- Found by @trishankatdatadog in PR #16549, we were masking errors coming out of the rsa verification calls as verfication errors and not returning when they were usage errors.

@trishankatdatadog

…16695) - Found by @trishankatdatadog in PR #16549, we were masking errors coming out of the rsa verification calls as verfication errors and not returning when they were usage errors.

trishankatdatadog · 2022-08-16T03:57:21Z

@stevendpclark ok, almost done: just want to add a unit test (if not already there) for being able to automatically verify RSA PSS signatures from previous versions of Vault (shouldn't be a problem, but still).

trishankatdatadog · 2022-08-18T03:36:19Z

@stevendpclark OK, I have added about every unit test I can think of for both the SDK and HTTP API.

Of course, just like everyone else, I want to make sure we don't even accidentally break verification of signatures from previous versions of Vault, but I'm fairly certain that these unit tests already cover that case. Please advise. Thanks!

stevendpclark

Hi @trishankatdatadog,

Sorry for a bit of delay, took some PTO time and been playing catch up. This looks absolutely great, wonderful work! I have one small nit about a test, see comment below.

Other then that all that is missing is a documentation update to the api docs (see https://github.com/hashicorp/vault/blob/main/website/content/api-docs/secret/transit.mdx) and a changelog entry (see https://github.com/hashicorp/vault/blob/main/CONTRIBUTING.md#changelog-entries)

sdk/helper/keysutil/policy_test.go

trishankatdatadog · 2022-08-30T21:44:54Z

@stevendpclark Done, PTAL, thanks!

stevendpclark · 2022-08-31T13:41:53Z

Hi @trishankatdatadog, this looks great! I have no additional comments.

Would you be able to rebase this on top of the current main branch to pull in the fixes for the test-go-remote-docker and test-go-reace-remote-docker failures, that way I can merge it in. Sorry about all the back and forth...

This reverts commit 8adfc15fe3303b8fdf9f094ea246945ab1364077.

also, add marshaling for both SDK and HTTP requests

trishankatdatadog · 2022-08-31T16:07:25Z

Would you be able to rebase this on top of the current main branch to pull in the fixes for the test-go-remote-docker and test-go-reace-remote-docker failures, that way I can merge it in. Sorry about all the back and forth...

No problem, and looks like it worked! Only Vercel left 🙂

stevendpclark · 2022-08-31T16:28:35Z

Thanks for the contribution @trishankatdatadog and being so great with all the feedback, much appreciated!

trishankatdatadog · 2022-08-31T17:47:15Z

Thanks for the contribution @trishankatdatadog and being so great with all the feedback, much appreciated!

Thank you for being so responsive and shepherding the process to help us fix the issue, much appreciated too 🙂

trishankatdatadog · 2022-08-31T17:49:44Z

Oh, BTW: in which release could we expect this code?

stevendpclark · 2022-08-31T18:00:51Z

Oh, BTW: in which release could we expect this code?

It will make it out into the next major release of Vault so 1.12...

ofek · 2022-09-01T14:03:38Z

When is 1.12 expected to be released?

stevendpclark · 2022-10-04T17:20:12Z

When is 1.12 expected to be released?

Hi @ofek, rc1 for Vault 1.12.0 is out now if you would like to test this, binaries available at: https://releases.hashicorp.com/vault/1.12.0-rc1/

trishankatdatadog mentioned this pull request Aug 3, 2022

Allow configuring the possible salt lengths for RSA PSS signatures hvac/hvac#846

Merged

ccapurso added secret/transit cryptosec labels Aug 3, 2022

trishankatdatadog mentioned this pull request Aug 4, 2022

Cross-platform verification of RSA PSS signatures secure-systems-lab/securesystemslib#421

Closed

stevendpclark requested changes Aug 4, 2022

View reviewed changes

builtin/logical/transit/path_sign_verify.go Outdated Show resolved Hide resolved

builtin/logical/transit/path_sign_verify.go Outdated Show resolved Hide resolved

sdk/helper/keysutil/policy.go Outdated Show resolved Hide resolved

monomosc mentioned this pull request Aug 5, 2022

builtin/transit add pss_salt_length parameter #16227

Closed

trishankatdatadog mentioned this pull request Aug 11, 2022

Fix RSA PSS salt lengths secure-systems-lab/securesystemslib#422

Merged

4 tasks

stevendpclark reviewed Aug 11, 2022

View reviewed changes

builtin/logical/transit/path_sign_verify.go Outdated Show resolved Hide resolved

sdk/helper/keysutil/policy.go Show resolved Hide resolved

sdk/helper/keysutil/policy.go Outdated Show resolved Hide resolved

builtin/logical/transit/path_sign_verify.go Outdated Show resolved Hide resolved

stevendpclark mentioned this pull request Aug 11, 2022

Do not mask RSA verification misuse errors as verification failures #16695

Merged

trishankatdatadog force-pushed the trishankatdatadog/rsassa-pss-salt-lengths branch 2 times, most recently from bef8669 to 4ff3817 Compare August 16, 2022 03:33

stevendpclark reviewed Aug 19, 2022

View reviewed changes

sdk/helper/keysutil/policy_test.go Outdated Show resolved Hide resolved

trishankatdatadog mentioned this pull request Aug 24, 2022

feat: Support ecdsa and RSA keys (#270 with backwards compatibility) theupdateframework/go-tuf#357

Merged

5 tasks

trishankatdatadog force-pushed the trishankatdatadog/rsassa-pss-salt-lengths branch from 1c7c4a0 to 6a250ee Compare August 30, 2022 20:52

trishankatdatadog requested a review from taoism4504 as a code owner August 30, 2022 21:37

trishankatdatadog added 12 commits August 31, 2022 11:33

accommodate salt lengths for RSA PSS

1427b22

address feedback

0a2a79b

generalise salt length to an int

e79a052

fix error reporting

f0069bc

Revert "fix error reporting"

d456f53

This reverts commit 8adfc15fe3303b8fdf9f094ea246945ab1364077.

fix a faulty check

695c37e

check for min/max salt lengths

78a7e48

stringly-typed HTTP param

3e17c2d

unit tests for sign/verify HTTP requests

c4ce833

also, add marshaling for both SDK and HTTP requests

randomly sample valid salt length

450c94b

add changelog

34d5b4d

add documentation

0ec0679

trishankatdatadog force-pushed the trishankatdatadog/rsassa-pss-salt-lengths branch from 2820548 to 0ec0679 Compare August 31, 2022 15:34

stevendpclark approved these changes Aug 31, 2022

View reviewed changes

stevendpclark merged commit 754c119 into hashicorp:main Aug 31, 2022

This was referenced Aug 31, 2022

Add ability to set PSS salt length for transit/sign endpoint #7531

Closed

Add 'pss_salt_length' parameter in transit/sign endpoint (#7531) #7532

Closed

stevendpclark added this to the 1.12.0-rc1 milestone Aug 31, 2022

trishankatdatadog deleted the trishankatdatadog/rsassa-pss-salt-lengths branch August 31, 2022 17:46

stevendpclark mentioned this pull request Sep 6, 2022

Address failures in FIPS builds around new transit RSA PSS tests #17024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring the possible salt lengths for RSA PSS signatures #16549

Allow configuring the possible salt lengths for RSA PSS signatures #16549

trishankatdatadog commented Aug 3, 2022

stevendpclark left a comment

trishankatdatadog commented Aug 5, 2022

stevendpclark commented Aug 8, 2022 •

edited

Loading

trishankatdatadog commented Aug 8, 2022

trishankatdatadog commented Aug 9, 2022

stevendpclark commented Aug 9, 2022 •

edited

Loading

trishankatdatadog commented Aug 9, 2022

monomosc commented Aug 10, 2022 •

edited

Loading

trishankatdatadog commented Aug 10, 2022

trishankatdatadog commented Aug 11, 2022 •

edited

Loading

monomosc commented Aug 11, 2022 •

edited

Loading

stevendpclark left a comment

trishankatdatadog commented Aug 11, 2022

trishankatdatadog commented Aug 16, 2022

trishankatdatadog commented Aug 18, 2022

stevendpclark left a comment

trishankatdatadog commented Aug 30, 2022

stevendpclark commented Aug 31, 2022 •

edited

Loading

trishankatdatadog commented Aug 31, 2022

stevendpclark commented Aug 31, 2022

trishankatdatadog commented Aug 31, 2022

trishankatdatadog commented Aug 31, 2022

stevendpclark commented Aug 31, 2022

ofek commented Sep 1, 2022

stevendpclark commented Oct 4, 2022

Allow configuring the possible salt lengths for RSA PSS signatures #16549

Allow configuring the possible salt lengths for RSA PSS signatures #16549

Conversation

trishankatdatadog commented Aug 3, 2022

stevendpclark left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Aug 5, 2022

stevendpclark commented Aug 8, 2022 • edited Loading

trishankatdatadog commented Aug 8, 2022

trishankatdatadog commented Aug 9, 2022

stevendpclark commented Aug 9, 2022 • edited Loading

trishankatdatadog commented Aug 9, 2022

monomosc commented Aug 10, 2022 • edited Loading

trishankatdatadog commented Aug 10, 2022

trishankatdatadog commented Aug 11, 2022 • edited Loading

monomosc commented Aug 11, 2022 • edited Loading

stevendpclark left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Aug 11, 2022

trishankatdatadog commented Aug 16, 2022

trishankatdatadog commented Aug 18, 2022

stevendpclark left a comment

Choose a reason for hiding this comment

trishankatdatadog commented Aug 30, 2022

stevendpclark commented Aug 31, 2022 • edited Loading

trishankatdatadog commented Aug 31, 2022

stevendpclark commented Aug 31, 2022

trishankatdatadog commented Aug 31, 2022

trishankatdatadog commented Aug 31, 2022

stevendpclark commented Aug 31, 2022

ofek commented Sep 1, 2022

stevendpclark commented Oct 4, 2022

stevendpclark commented Aug 8, 2022 •

edited

Loading

stevendpclark commented Aug 9, 2022 •

edited

Loading

monomosc commented Aug 10, 2022 •

edited

Loading

trishankatdatadog commented Aug 11, 2022 •

edited

Loading

monomosc commented Aug 11, 2022 •

edited

Loading

stevendpclark commented Aug 31, 2022 •

edited

Loading