Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improvement: add signature_bits field to CA and signers #11245

Merged
merged 1 commit into from
Sep 10, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 31, 2021

This change adds the ability to set the signature algorithm of the
CAs that Vault generates and any certificates it signs. This is a
potentially useful stepping stone for a SHA3 transition down the line.

Summary:

  • Adds the field "signature_bits" to CA and Sign endpoints
  • Adds support for SHA256, SHA384 and SHA512 signatures on EC and RSA
    keytypes.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 31, 2021

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – vault-storybook March 31, 2021 05:45 Inactive
@vercel vercel bot temporarily deployed to Preview – vault March 31, 2021 05:45 Inactive
@ghost ghost force-pushed the ext_signatures branch from bae7d0d to 1b64fd8 Compare March 31, 2021 05:47
@vercel vercel bot temporarily deployed to Preview – vault March 31, 2021 05:47 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook March 31, 2021 05:47 Inactive
@sgmiller
Copy link
Collaborator

sgmiller commented Apr 21, 2021

I like the idea, but if bit length that switches on algorithm is a bit opaque. I'd prefer to introduce a field that selects the algorithm based on the name and bit length. KeyBits is a bit different since it's more arbitrary than the possible hash lengths which are often fundamental to the hash function itself.

@ghost
Copy link
Author

ghost commented Apr 28, 2021

I like the idea, but if bit length that switches on algorithm is a bit opaque. I'd prefer to introduce a field that selects the algorithm based on the name and bit length. KeyBits is a bit different since it's more arbitrary than the possible hash lengths which are often fundamental to the hash function itself.

I had similar thoughts as well when I was writing this patch. In the end, my motivation for using a signature_bits field instead of a string/int combination was for (what are my assumptions about this code) motivations in this codebase for matching ECDSA and RSA signatures to the chosen key type, which would appear to be TLS compatibility. While technically possible to pair an EC key with a SHA256WithRSA signature, for example, this does break compatibility with TLSv1.1 and potentially a host of other devices.

Until the SHA3 family, SHAKE or some other alternative is realistically available, for simplicity and compatibility I chose this path instead of adding a string field or a string/int combination.

@sgmiller
Copy link
Collaborator

sgmiller commented Jun 1, 2021

Okay, that seems reasonable. As long as it's possible to add a signature_hash or similar parameter to pick a different family in the future I don't object. If you could tackle that conflict I think we're good.

@sgmiller sgmiller self-requested a review June 1, 2021 16:37
@ghost ghost force-pushed the ext_signatures branch from 1b64fd8 to 9387860 Compare June 1, 2021 18:28
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 1, 2021 18:28 Inactive
@vercel vercel bot temporarily deployed to Preview – vault June 1, 2021 18:28 Inactive
@pmmukh
Copy link
Contributor

pmmukh commented Sep 10, 2021

@jhart-cpi could you please rebase this branch once? after that, long as tests pass, this should be good to merge in.

This change adds the ability to set the signature algorithm of the
CAs that Vault generates and any certificates it signs.  This is a
potentially useful stepping stone for a SHA3 transition down the line.

Summary:
* Adds the field "signature_bits" to CA and Sign endpoints
* Adds support for SHA256, SHA384 and SHA512 signatures on EC and RSA
keytypes.
@ghost ghost force-pushed the ext_signatures branch from f16ee42 to f07bb6a Compare September 10, 2021 19:42
@vercel vercel bot temporarily deployed to Preview – vault September 10, 2021 19:42 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 10, 2021 19:42 Inactive
@ghost
Copy link
Author

ghost commented Sep 10, 2021

@jhart-cpi could you please rebase this branch once? after that, long as tests pass, this should be good to merge in.

Done.

@pmmukh pmmukh merged commit 49c3db0 into hashicorp:main Sep 10, 2021
cipherboy added a commit to cipherboy/vault that referenced this pull request Nov 12, 2021
When using an ECDSA signature with a NIST P-Curve, we should follow
recommendations from BIS (Section 4.2) and Mozilla's root store policy
(section 5.1.2) to ensure that arbitrary selection of signature_bits
does not exceed what the curve is capable of signing.

Related: hashicorp#11245

Signed-off-by: Alexander Scheel <[email protected]>
cipherboy added a commit that referenced this pull request Nov 12, 2021
)

* Restrict ECDSA signatures with NIST P-Curve hashes

When using an ECDSA signature with a NIST P-Curve, we should follow
recommendations from BIS (Section 4.2) and Mozilla's root store policy
(section 5.1.2) to ensure that arbitrary selection of signature_bits
does not exceed what the curve is capable of signing.

Related: #11245

Signed-off-by: Alexander Scheel <[email protected]>

* Switch to certutil.ValidateKeyTypeSignatureLength(...)

Replaces previous calls to certutil.ValidateKeyTypeLength(...) and
certutil.ValidateSignatureLength(...) with a single call, allowing for
curve<->hash validation.

Signed-off-by: Alexander Scheel <[email protected]>

* Switch to autodetection of signature_bits

This enables detection of whether the caller manually specified a value
for signature_bits or not; when not manually specified, we can provision
a value that complies with new NIST P-Curve policy.

Signed-off-by: Alexander Scheel <[email protected]>

* Select hash function length automatically

Due to our change in behavior (to default to -1 as the value to
signature_bits to allow for automatic hash selection), switch
ValidateKeyTypeSignatureLength(...) to accept a pointer to hashBits and
provision it with valid default values.

Signed-off-by: Alexander Scheel <[email protected]>

* Prevent invalid Curve size lookups

Signed-off-by: Alexander Scheel <[email protected]>

* Switch from -1 to 0 as default SignatureBits

Signed-off-by: Alexander Scheel <[email protected]>

* Add changelog entry

Signed-off-by: Alexander Scheel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants