From 10ecf10248c72ecb599b54af8039e957a4bb0485 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Mon, 3 Oct 2022 12:39:54 -0400 Subject: [PATCH] PKI: Add support for signature_bits param to the intermediate/generate api (#17388) * PKI: Add support for signature_bits param to the intermediate/generate api - Mainly to work properly with GCP backed managed keys, we need to issue signatures that would match the GCP key algorithm. - At this time due to https://github.com/golang/go/issues/45990 we can't issue PSS signed CSRs, as the libraries in Go always request a PKCS1v15. - Add an extra check in intermediate/generate that validates the CSR's signature before providing it back to the client in case we generated a bad signature such as if an end-user used a GCP backed managed key with a RSA PSS algorithm. - GCP ignores the requested signature type and always signs with the key's algorithm which can lead to a CSR that says it is signed with a PKCS1v15 algorithm but is actually a RSA PSS signature * Add cl * PR feedback --- builtin/logical/pki/backend_test.go | 6 +- builtin/logical/pki/integation_test.go | 60 +++++++++++++++++ builtin/logical/pki/path_intermediate.go | 12 +--- builtin/logical/pki/path_manage_issuers.go | 7 +- builtin/logical/pki/test_helpers.go | 65 +------------------ changelog/17388.txt | 3 + sdk/helper/certutil/helpers.go | 46 +++++++++++-- website/content/api-docs/secret/pki.mdx | 10 +++ .../docs/secrets/pki/considerations.mdx | 6 ++ 9 files changed, 131 insertions(+), 84 deletions(-) create mode 100644 changelog/17388.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 190c336b8..23eb3c15c 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -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) { @@ -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") @@ -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{}{ diff --git a/builtin/logical/pki/integation_test.go b/builtin/logical/pki/integation_test.go index ed2b45940..a655662ec 100644 --- a/builtin/logical/pki/integation_test.go +++ b/builtin/logical/pki/integation_test.go @@ -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" @@ -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, "") } diff --git a/builtin/logical/pki/path_intermediate.go b/builtin/logical/pki/path_intermediate.go index c87584221..cfcff87b0 100644 --- a/builtin/logical/pki/path_intermediate.go +++ b/builtin/logical/pki/path_intermediate.go @@ -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, @@ -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) { diff --git a/builtin/logical/pki/path_manage_issuers.go b/builtin/logical/pki/path_manage_issuers.go index a22df03a4..11a7311ff 100644 --- a/builtin/logical/pki/path_manage_issuers.go +++ b/builtin/logical/pki/path_manage_issuers.go @@ -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 diff --git a/builtin/logical/pki/test_helpers.go b/builtin/logical/pki/test_helpers.go index 907bafe58..acf083ced 100644 --- a/builtin/logical/pki/test_helpers.go +++ b/builtin/logical/pki/test_helpers.go @@ -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" @@ -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) { + 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)) diff --git a/changelog/17388.txt b/changelog/17388.txt new file mode 100644 index 000000000..e3a98d871 --- /dev/null +++ b/changelog/17388.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: Add support to specify signature bits when generating CSRs through intermediate/generate apis +``` diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index 56ab5324a..c69e80fb4 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -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 { @@ -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{} @@ -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: @@ -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 } @@ -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 } diff --git a/website/content/api-docs/secret/pki.mdx b/website/content/api-docs/secret/pki.mdx index 0dd9374c6..63bbad807 100644 --- a/website/content/api-docs/secret/pki.mdx +++ b/website/content/api-docs/secret/pki.mdx @@ -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 diff --git a/website/content/docs/secrets/pki/considerations.mdx b/website/content/docs/secrets/pki/considerations.mdx index 043e79b9a..d4ebe3ed4 100644 --- a/website/content/docs/secrets/pki/considerations.mdx +++ b/website/content/docs/secrets/pki/considerations.mdx @@ -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),