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

PKI: Add support for signature_bits param to the intermediate/generate api #17388

Merged
merged 3 commits into from
Oct 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions builtin/logical/pki/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2539,7 +2539,7 @@ func TestBackend_ConsulSignLeafWithLegacyRole(t *testing.T) {

signedCert := parseCert(t, certAsPem)
rootCert := parseCert(t, rootCaPem)
requireSignedBy(t, signedCert, rootCert.PublicKey)
requireSignedBy(t, signedCert, rootCert)
}

func TestBackend_SignSelfIssued(t *testing.T) {
Expand Down Expand Up @@ -4097,7 +4097,7 @@ func runFullCAChainTest(t *testing.T, keyType string) {

rootCaCert := parseCert(t, rootCert)
intermediaryCaCert := parseCert(t, intermediateCert)
requireSignedBy(t, intermediaryCaCert, rootCaCert.PublicKey)
requireSignedBy(t, intermediaryCaCert, rootCaCert)
intermediateCaChain := intermediateSignedData["ca_chain"].([]string)

require.Equal(t, parseCert(t, intermediateCaChain[0]), intermediaryCaCert, "intermediate signed cert should have been part of ca_chain")
Expand Down Expand Up @@ -4191,7 +4191,7 @@ func runFullCAChainTest(t *testing.T, keyType string) {
issuedCrt := parseCert(t, issueCrtAsPem)

// Verify that the certificates are signed by the intermediary CA key...
requireSignedBy(t, issuedCrt, intermediaryCaCert.PublicKey)
requireSignedBy(t, issuedCrt, intermediaryCaCert)

// Test that we can request that the root ca certificate not appear in the ca_chain field
resp, err = CBWrite(b_ext, s_ext, "issue/example", map[string]interface{}{
Expand Down
60 changes: 60 additions & 0 deletions builtin/logical/pki/integation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@ package pki

import (
"context"
"crypto"
"crypto/ecdsa"
"crypto/ed25519"
"crypto/rsa"
"crypto/x509"
"encoding/pem"
"fmt"
"testing"

"github.com/hashicorp/vault/sdk/logical"
Expand Down Expand Up @@ -297,6 +304,59 @@ func TestIntegration_SetSignedWithBackwardsPemBundles(t *testing.T) {
require.False(t, resp.IsError(), "got an error issuing a leaf cert from int ca: %#v", resp)
}

func TestIntegration_CSRGeneration(t *testing.T) {
t.Parallel()
b, s := createBackendWithStorage(t)
testCases := []struct {
keyType string
usePss bool
keyBits int
sigBits int
expectedPublicKeyType crypto.PublicKey
expectedSignature x509.SignatureAlgorithm
}{
{"rsa", false, 2048, 0, &rsa.PublicKey{}, x509.SHA256WithRSA},
{"rsa", false, 2048, 384, &rsa.PublicKey{}, x509.SHA384WithRSA},
// Add back once https://github.com/golang/go/issues/45990 is fixed.
// {"rsa", true, 2048, 0, &rsa.PublicKey{}, x509.SHA256WithRSAPSS},
// {"rsa", true, 2048, 512, &rsa.PublicKey{}, x509.SHA512WithRSAPSS},
{"ec", false, 224, 0, &ecdsa.PublicKey{}, x509.ECDSAWithSHA256},
{"ec", false, 256, 0, &ecdsa.PublicKey{}, x509.ECDSAWithSHA256},
{"ec", false, 384, 0, &ecdsa.PublicKey{}, x509.ECDSAWithSHA384},
{"ec", false, 521, 0, &ecdsa.PublicKey{}, x509.ECDSAWithSHA512},
{"ec", false, 521, 224, &ecdsa.PublicKey{}, x509.ECDSAWithSHA512}, // We ignore signature_bits for ec
{"ed25519", false, 0, 0, ed25519.PublicKey{}, x509.PureEd25519}, // We ignore both fields for ed25519
}
for _, tc := range testCases {
keyTypeName := tc.keyType
if tc.usePss {
keyTypeName = tc.keyType + "-pss"
}
testName := fmt.Sprintf("%s-%d-%d", keyTypeName, tc.keyBits, tc.sigBits)
t.Run(testName, func(t *testing.T) {
resp, err := CBWrite(b, s, "intermediate/generate/internal", map[string]interface{}{
"common_name": "myint.com",
"key_type": tc.keyType,
"key_bits": tc.keyBits,
"signature_bits": tc.sigBits,
"use_pss": tc.usePss,
})
requireSuccessNonNilResponse(t, resp, err)
requireFieldsSetInResp(t, resp, "csr")

csrString := resp.Data["csr"].(string)
pemBlock, _ := pem.Decode([]byte(csrString))
require.NotNil(t, pemBlock, "failed to parse returned csr pem block")
csr, err := x509.ParseCertificateRequest(pemBlock.Bytes)
require.NoError(t, err, "failed parsing certificate request")

require.Equal(t, tc.expectedSignature, csr.SignatureAlgorithm,
"Expected %s, got %s", tc.expectedSignature.String(), csr.SignatureAlgorithm.String())
require.IsType(t, tc.expectedPublicKeyType, csr.PublicKey)
})
}
}

func genTestRootCa(t *testing.T, b *backend, s logical.Storage) (issuerID, keyID) {
return genTestRootCaWithIssuerName(t, b, s, "")
}
Expand Down
12 changes: 2 additions & 10 deletions builtin/logical/pki/path_intermediate.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,7 @@ func (b *backend) pathGenerateIntermediate(ctx context.Context, req *logical.Req
data.Raw["exported"] = "existing"
}

// Nasty hack part two. :-) For generation of CSRs, certutil presently doesn't
// support configuration of this. However, because we need generation parameters,
// which create a role and attempt to read this parameter, we need to provide
// a value (which will be ignored). Hence, we stub in the missing parameters here,
// including its schema, just enough for it to work..
data.Schema["signature_bits"] = &framework.FieldSchema{
Type: framework.TypeInt,
Default: 0,
}
data.Raw["signature_bits"] = 0
// Remove this once https://github.com/golang/go/issues/45990 is fixed
data.Schema["use_pss"] = &framework.FieldSchema{
Type: framework.TypeBool,
Default: false,
Expand All @@ -95,6 +86,7 @@ func (b *backend) pathGenerateIntermediate(ctx context.Context, req *logical.Req
req: req,
apiData: data,
}

parsedBundle, warnings, err := generateIntermediateCSR(sc, input, b.Backend.GetRandomReader())
if err != nil {
switch err.(type) {
Expand Down
7 changes: 2 additions & 5 deletions builtin/logical/pki/path_manage_issuers.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,8 @@ workaround in some compatibility scenarios
with Active Directory Certificate Services.`,
}

// Signature bits isn't respected on intermediate generation, as this
// only impacts the CSR's internal signature and doesn't impact the
// signed certificate's bits (that's on the /sign-intermediate
// endpoints). Remove it from the list of fields to avoid confusion.
delete(ret.Fields, "signature_bits")
// At this time Go does not support signing CSRs using PSS signatures, see
// https://github.com/golang/go/issues/45990
delete(ret.Fields, "use_pss")

return ret
Expand Down
65 changes: 3 additions & 62 deletions builtin/logical/pki/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,12 @@ package pki
import (
"context"
"crypto"
"crypto/ecdsa"
"crypto/ed25519"
"crypto/rand"
"crypto/rsa"
"crypto/sha256"
"crypto/sha512"
"crypto/x509"
"crypto/x509/pkix"
"encoding/pem"
"fmt"
"hash"
"io"
"strings"
"testing"
Expand Down Expand Up @@ -52,66 +47,12 @@ func mountPKIEndpoint(t testing.TB, client *api.Client, path string) {
}

// Signing helpers
func requireSignedBy(t *testing.T, cert *x509.Certificate, key crypto.PublicKey) {
switch typedKey := key.(type) {
case *rsa.PublicKey:
requireRSASignedBy(t, cert, typedKey)
case *ecdsa.PublicKey:
requireECDSASignedBy(t, cert, typedKey)
case ed25519.PublicKey:
requireED25519SignedBy(t, cert, typedKey)
default:
require.Fail(t, "unknown public key type %#v", key)
func requireSignedBy(t *testing.T, cert *x509.Certificate, signingCert *x509.Certificate) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 -- I had thought of this change a while ago, but never actually submitted it, thank you!

if err := cert.CheckSignatureFrom(signingCert); err != nil {
t.Fatalf("signature verification failed: %v", err)
}
}

func requireRSASignedBy(t *testing.T, cert *x509.Certificate, key *rsa.PublicKey) {
require.Contains(t, []x509.SignatureAlgorithm{x509.SHA256WithRSA, x509.SHA512WithRSA},
cert.SignatureAlgorithm, "only sha256 signatures supported")

var hasher hash.Hash
var hashAlgo crypto.Hash

switch cert.SignatureAlgorithm {
case x509.SHA256WithRSA:
hasher = sha256.New()
hashAlgo = crypto.SHA256
case x509.SHA512WithRSA:
hasher = sha512.New()
hashAlgo = crypto.SHA512
}

hasher.Write(cert.RawTBSCertificate)
hashData := hasher.Sum(nil)

err := rsa.VerifyPKCS1v15(key, hashAlgo, hashData, cert.Signature)
require.NoError(t, err, "the certificate was not signed by the expected public rsa key.")
}

func requireECDSASignedBy(t *testing.T, cert *x509.Certificate, key *ecdsa.PublicKey) {
require.Contains(t, []x509.SignatureAlgorithm{x509.ECDSAWithSHA256, x509.ECDSAWithSHA512},
cert.SignatureAlgorithm, "only ecdsa signatures supported")

var hasher hash.Hash
switch cert.SignatureAlgorithm {
case x509.ECDSAWithSHA256:
hasher = sha256.New()
case x509.ECDSAWithSHA512:
hasher = sha512.New()
}

hasher.Write(cert.RawTBSCertificate)
hashData := hasher.Sum(nil)

verify := ecdsa.VerifyASN1(key, hashData, cert.Signature)
require.True(t, verify, "the certificate was not signed by the expected public ecdsa key.")
}

func requireED25519SignedBy(t *testing.T, cert *x509.Certificate, key ed25519.PublicKey) {
require.Equal(t, x509.PureEd25519, cert.SignatureAlgorithm)
ed25519.Verify(key, cert.RawTBSCertificate, cert.Signature)
}

// Certificate helper
func parseCert(t *testing.T, pemCert string) *x509.Certificate {
block, _ := pem.Decode([]byte(pemCert))
Expand Down
3 changes: 3 additions & 0 deletions changelog/17388.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:improvement
secrets/pki: Add support to specify signature bits when generating CSRs through intermediate/generate apis
```
46 changes: 42 additions & 4 deletions sdk/helper/certutil/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -789,7 +789,7 @@ func CreateCertificateWithKeyGenerator(data *CreationBundle, randReader io.Reade
return createCertificate(data, randReader, keyGenerator)
}

// Set correct correct RSA sig algo
// Set correct RSA sig algo
func certTemplateSetSigAlgo(certTemplate *x509.Certificate, data *CreationBundle) {
if data.Params.UsePSS {
switch data.Params.SignatureBits {
Expand All @@ -812,6 +812,35 @@ func certTemplateSetSigAlgo(certTemplate *x509.Certificate, data *CreationBundle
}
}

// selectSignatureAlgorithmForRSA returns the proper x509.SignatureAlgorithm based on various properties set in the
// Creation Bundle parameter. This method will default to a SHA256 signature algorithm if the requested signature
// bits is not set/unknown.
func selectSignatureAlgorithmForRSA(data *CreationBundle) x509.SignatureAlgorithm {
if data.Params.UsePSS {
switch data.Params.SignatureBits {
case 256:
return x509.SHA256WithRSAPSS
case 384:
return x509.SHA384WithRSAPSS
case 512:
return x509.SHA512WithRSAPSS
default:
return x509.SHA256WithRSAPSS
}
}

switch data.Params.SignatureBits {
case 256:
return x509.SHA256WithRSA
case 384:
return x509.SHA384WithRSA
case 512:
return x509.SHA512WithRSA
default:
return x509.SHA256WithRSA
}
}

func createCertificate(data *CreationBundle, randReader io.Reader, privateKeyGenerator KeyGenerator) (*ParsedCertBundle, error) {
var err error
result := &ParsedCertBundle{}
Expand Down Expand Up @@ -878,7 +907,11 @@ func createCertificate(data *CreationBundle, randReader io.Reader, privateKeyGen

var certBytes []byte
if data.SigningBundle != nil {
switch data.SigningBundle.PrivateKeyType {
privateKeyType := data.SigningBundle.PrivateKeyType
if privateKeyType == ManagedPrivateKey {
privateKeyType = GetPrivateKeyTypeFromSigner(data.SigningBundle.PrivateKey)
}
switch privateKeyType {
case RSAPrivateKey:
certTemplateSetSigAlgo(certTemplate, data)
case Ed25519PrivateKey:
Expand Down Expand Up @@ -1049,9 +1082,10 @@ func createCSR(data *CreationBundle, addBasicConstraints bool, randReader io.Rea

switch data.Params.KeyType {
case "rsa":
csrTemplate.SignatureAlgorithm = x509.SHA256WithRSA
// use specified RSA algorithm defaulting to the appropriate SHA256 RSA signature type
csrTemplate.SignatureAlgorithm = selectSignatureAlgorithmForRSA(data)
case "ec":
csrTemplate.SignatureAlgorithm = x509.ECDSAWithSHA256
csrTemplate.SignatureAlgorithm = selectSignatureAlgorithmForECDSA(result.PrivateKey.Public(), data.Params.SignatureBits)
case "ed25519":
csrTemplate.SignatureAlgorithm = x509.PureEd25519
}
Expand All @@ -1067,6 +1101,10 @@ func createCSR(data *CreationBundle, addBasicConstraints bool, randReader io.Rea
return nil, errutil.InternalError{Err: fmt.Sprintf("unable to parse created certificate: %v", err)}
}

if err = result.CSR.CheckSignature(); err != nil {
return nil, errors.New("failed signature validation for CSR")
}

return result, nil
}

Expand Down
10 changes: 10 additions & 0 deletions website/content/api-docs/secret/pki.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -1764,6 +1764,16 @@ generated depending on the `type` request parameter.
name, or by identifier) to use for generating this request. Only suitable
for `type=existing` requests.

- `signature_bits` `(int: 0)` - Specifies the number of bits to use in
the signature algorithm; accepts 256 for SHA-2-256, 384 for SHA-2-384,
and 512 for SHA-2-512. Defaults to 0 to automatically detect based
on key length (SHA-2-256 for RSA keys, and matching the curve size
for NIST P-Curves).

~> **Note**: ECDSA and Ed25519 issuers do not follow configuration of the
`signature_bits` value; only RSA issuers will change signature types
based on this parameter.

- `exclude_cn_from_sans` `(bool: false)` - If true, the given `common_name` will
not be included in DNS or Email Subject Alternate Names (as appropriate).
Useful if the CN is not a hostname or email address, but is instead some
Expand Down
6 changes: 6 additions & 0 deletions website/content/docs/secrets/pki/considerations.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,12 @@ Additionally, some implementations allow rsaPSS OID certificates to contain
restrictions on signature parameters allowed by this certificate, but Go and
Vault do not support adding such restrictions.

At this time Go lacks support for CSRs with the PSS signature algorithm. If
using a GCP managed key with a RSA PSS algorithm as a backing CA key,
attempting to generate a CSR will fail signature verification. In this case
the CSR will need to be generated outside of Vault and the signed version
can be imported into the mount.

## Issuer Subjects and CRLs

As noted on several [GitHub issues](https://github.com/hashicorp/vault/issues/10176),
Expand Down