From 18235e36247172bdab5023beddb700ff2235831c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wilson=20J=C3=BAnior?= Date: Thu, 8 Jul 2021 16:24:38 -0300 Subject: [PATCH 1/2] Improve ParseSingleCertificateChain when no root is present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes when the certificate chain does not have a root CA, in which case the chain should contain all available intermediates and ca.crt should contain the rootmost certificate. Co-authored-by: Josh Van Leeuwen Signed-off-by: Wilson Júnior Signed-off-by: Ashley Davis --- pkg/internal/vault/vault_test.go | 28 ++++++++++++++++++++++++---- pkg/util/pki/parse.go | 14 +++++++++++++- pkg/util/pki/parse_test.go | 5 +++++ 3 files changed, 42 insertions(+), 5 deletions(-) diff --git a/pkg/internal/vault/vault_test.go b/pkg/internal/vault/vault_test.go index fa3916a2a..c00acc773 100644 --- a/pkg/internal/vault/vault_test.go +++ b/pkg/internal/vault/vault_test.go @@ -222,6 +222,12 @@ func TestSign(t *testing.T) { t.FailNow() } + rootBundleData, err := bundlePEM(testIntermediateCa, testRootCa) + if err != nil { + t.Errorf("failed to encode root bundle for testing: %s", err) + t.FailNow() + } + tests := map[string]testSignT{ "a garbage csr should return err": { csrPEM: []byte("a bad csr"), @@ -241,7 +247,7 @@ func TestSign(t *testing.T) { expectedCA: "", }, - "a good csr and good response should return a certificate": { + "a good csr and good response with no root should return a certificate with the intermediate in the chain and as the CA": { csrPEM: csrPEM, issuer: gen.Issuer("vault-issuer", gen.SetIssuerVault(cmapi.VaultIssuer{}), @@ -251,10 +257,24 @@ func TestSign(t *testing.T) { Body: ioutil.NopCloser(bytes.NewReader(bundleData))}, }, nil), expectedErr: nil, - expectedCert: testLeafCertificate, + expectedCert: testLeafCertificate + testIntermediateCa, expectedCA: testIntermediateCa, }, + "a good csr and good response with a root should return a certificate without the root in the chain but with the root as the CA": { + csrPEM: csrPEM, + issuer: gen.Issuer("vault-issuer", + gen.SetIssuerVault(cmapi.VaultIssuer{}), + ), + fakeClient: vaultfake.NewFakeClient().WithRawRequest(&vault.Response{ + Response: &http.Response{ + Body: ioutil.NopCloser(bytes.NewReader(rootBundleData))}, + }, nil), + expectedErr: nil, + expectedCert: testLeafCertificate + testIntermediateCa, + expectedCA: testRootCa, + }, + "vault issuer with namespace specified": { csrPEM: csrPEM, issuer: gen.Issuer("vault-issuer", @@ -265,7 +285,7 @@ func TestSign(t *testing.T) { Body: ioutil.NopCloser(bytes.NewReader(bundleData))}, }, nil), expectedErr: nil, - expectedCert: testLeafCertificate, + expectedCert: testLeafCertificate + testIntermediateCa, expectedCA: testIntermediateCa, }, } @@ -321,7 +341,7 @@ func TestExtractCertificatesFromVaultCertificateSecret(t *testing.T) { tests := map[string]testExtractCertificatesFromVaultCertT{ "when a Vault engine is a root CA": { secret: signedCertificateSecret(testIntermediateCa), - expectedCert: testLeafCertificate, + expectedCert: testLeafCertificate + testIntermediateCa, expectedCA: testIntermediateCa, }, "when a Vault engine is an intermediate CA, and its parent is a root CA": { diff --git a/pkg/util/pki/parse.go b/pkg/util/pki/parse.go index 99df003fc..a24accb7b 100644 --- a/pkg/util/pki/parse.go +++ b/pkg/util/pki/parse.go @@ -252,8 +252,14 @@ func (c *chainNode) toBundleAndCA() (PEMBundle, error) { for { // If the issuer is nil, we have hit the root of the chain. Assign the CA - // to this certificate and stop traversing. + // to this certificate and stop traversing. If the certificate at the root + // of the chain is not self-signed (i.e. is not a root CA), then also append + // that certificate to the chain. + if c.issuer == nil { + if !isSelfSignedCertificate(c.cert) { + certs = append(certs, c.cert) + } ca = c.cert break } @@ -336,3 +342,9 @@ func (c *chainNode) root() *chainNode { return c } + +// isSelfSignedCertificate returns true if the given X.509 certificate has been +// signed by itself, which would make it a "root" certificate. +func isSelfSignedCertificate(cert *x509.Certificate) bool { + return cert.CheckSignatureFrom(cert) == nil +} diff --git a/pkg/util/pki/parse_test.go b/pkg/util/pki/parse_test.go index 4f01f48a2..52fc85b2f 100644 --- a/pkg/util/pki/parse_test.go +++ b/pkg/util/pki/parse_test.go @@ -324,6 +324,11 @@ func TestParseSingleCertificateChain(t *testing.T) { expPEMBundle: PEMBundle{}, expErr: true, }, + "if certificate chain does not have a root ca, should append all intermediates to ChainPEM and use the root-most cert as CAPEM": { + inputBundle: joinPEM(intA1.pem, intA2.pem, leaf.pem), + expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem, intA2.pem, intA1.pem), CAPEM: intA1.pem}, + expErr: false, + }, } for name, test := range tests { From 2ee4abeb24e8326867740a1fb4aa6e8b448b47cd Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Tue, 27 Jul 2021 16:26:44 +0100 Subject: [PATCH 2/2] handle individual certs in ParseSingleCertificateChain roots are handled differently because they're their own CAs also adds test cases for each of: - a lone leaf - a lone intermediate - a lone root Signed-off-by: Ashley Davis --- pkg/util/pki/parse.go | 16 ++++++++++++++-- pkg/util/pki/parse_test.go | 25 ++++++++++++++++++++----- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/pkg/util/pki/parse.go b/pkg/util/pki/parse.go index a24accb7b..3958e10e7 100644 --- a/pkg/util/pki/parse.go +++ b/pkg/util/pki/parse.go @@ -256,10 +256,17 @@ func (c *chainNode) toBundleAndCA() (PEMBundle, error) { // of the chain is not self-signed (i.e. is not a root CA), then also append // that certificate to the chain. + // Root certificates are omitted from the chain as per + // https://datatracker.ietf.org/doc/html/rfc5246#section-7.4.2 + // > [T]he self-signed certificate that specifies the root certificate authority + // > MAY be omitted from the chain, under the assumption that the remote end must + // > already possess it in order to validate it in any case. + if c.issuer == nil { - if !isSelfSignedCertificate(c.cert) { + if len(certs) > 0 && !isSelfSignedCertificate(c.cert) { certs = append(certs, c.cert) } + ca = c.cert break } @@ -276,8 +283,13 @@ func (c *chainNode) toBundleAndCA() (PEMBundle, error) { } // If no certificates parsed, then CA is the only certificate and should be - // the chain + // the chain. If the CA is also self-signed, then by definition it's also the + // issuer and so can be placed in CAPEM too. if len(certs) == 0 { + if isSelfSignedCertificate(ca) { + return PEMBundle{ChainPEM: caPEM, CAPEM: caPEM}, nil + } + return PEMBundle{ChainPEM: caPEM}, nil } diff --git a/pkg/util/pki/parse_test.go b/pkg/util/pki/parse_test.go index 52fc85b2f..e874964d2 100644 --- a/pkg/util/pki/parse_test.go +++ b/pkg/util/pki/parse_test.go @@ -258,16 +258,16 @@ func TestParseSingleCertificateChain(t *testing.T) { expPEMBundle PEMBundle expErr bool }{ - "if single certificate passed, return single certificate": { - inputBundle: root.pem, - expPEMBundle: PEMBundle{ChainPEM: root.pem}, - expErr: false, - }, "if two certificate chain passed in order, should return single ca and certificate": { inputBundle: joinPEM(intA1.pem, root.pem), expPEMBundle: PEMBundle{ChainPEM: intA1.pem, CAPEM: root.pem}, expErr: false, }, + "if two certificate chain passed with leaf and intermediate, should return both certs in chain with intermediate as CA": { + inputBundle: joinPEM(leaf.pem, intA2.pem), + expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem, intA2.pem), CAPEM: intA2.pem}, + expErr: false, + }, "if two certificate chain passed out of order, should return single ca and certificate": { inputBundle: joinPEM(root.pem, intA1.pem), expPEMBundle: PEMBundle{ChainPEM: intA1.pem, CAPEM: root.pem}, @@ -329,6 +329,21 @@ func TestParseSingleCertificateChain(t *testing.T) { expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem, intA2.pem, intA1.pem), CAPEM: intA1.pem}, expErr: false, }, + "if only a single leaf certificate was parsed, ChainPEM should contain a single leaf certificate and CAPEM should remain empty": { + inputBundle: joinPEM(leaf.pem), + expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem), CAPEM: nil}, + expErr: false, + }, + "if only a single intermediate certificate was parsed, ChainPEM should contain a single intermediate certificate and CAPEM should remain empty": { + inputBundle: joinPEM(intA1.pem), + expPEMBundle: PEMBundle{ChainPEM: joinPEM(intA1.pem), CAPEM: nil}, + expErr: false, + }, + "if only a single root certificate was parsed, ChainPEM should contain a single root certificate and CAPEM should also contain that root": { + inputBundle: joinPEM(root.pem), + expPEMBundle: PEMBundle{ChainPEM: joinPEM(root.pem), CAPEM: root.pem}, + expErr: false, + }, } for name, test := range tests {