weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

fix check for self-signed certs in EncodeX509Chain

Browse Source 
see also https://github.com/jetstack/cert-manager/issues/4142

EncodeX509Chain checked for self-signed certs by comparing the subject
and issuer of the cert in question, which is invalid since it's
perfectly fine for those to match.

the correct behavior is to use cert.CheckSignatureFrom(cert). this bug
was exposed in 1.4 when ParseSingleCertificateChain started using
EncodeX509Chain in the critical path of several issuers; when end-users
had leaf certificates with subjects matching their issuer's subject, the
bug was triggered.

includes newly written tests for EncodeX509Chain and a test for
ParseSingleCertificateChain

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
This commit is contained in:
Ashley Davis 2021-07-21 16:33:07 +01:00
parent 31360580f0
commit 17ec9ea8e7
No known key found for this signature in database
GPG Key ID: DD14CC017E32BEB1
3 changed files with 106 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
pkg/util/pki/csr.go
View File

@ -450,7 +450,7 @@ func EncodeCSR(template *x509.CertificateRequest, key crypto.Signer) ([]byte, er
return derBytes, nil
}
// EncodeX509 will encode a *x509.Certificate into PEM format.
// EncodeX509 will encode a single *x509.Certificate into PEM format.
func EncodeX509(cert *x509.Certificate) ([]byte, error) {
caPem := bytes.NewBuffer([]byte{})
err := pem.Encode(caPem, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
@ -461,14 +461,24 @@ func EncodeX509(cert *x509.Certificate) ([]byte, error) {
return caPem.Bytes(), nil
}
// EncodeX509Chain will encode an *x509.Certificate chain into PEM format.
// EncodeX509Chain will encode a list of *x509.Certificates into a PEM format chain.
// Self-signed certificates are not included as per
// https://datatracker.ietf.org/doc/html/rfc5246#section-7.4.2
// Certificates are output in the order they're given; if the input is not ordered
// as specified in RFC5246 section 7.4.2, the resulting chain might not be valid
// for use in TLS.
func EncodeX509Chain(certs []*x509.Certificate) ([]byte, error) {
caPem := bytes.NewBuffer([]byte{})
for _, cert := range certs {
if bytes.Equal(cert.RawIssuer, cert.RawSubject) {
if cert == nil {
continue
}
if cert.CheckSignatureFrom(cert) == nil {
// Don't include self-signed certificate
continue
}
err := pem.Encode(caPem, &pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})
if err != nil {
return nil, err

77
pkg/util/pki/csr_test.go
View File

@ -656,6 +656,7 @@ func TestSignCSRTemplate(t *testing.T) {
wantErr: true,
},
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
actualBundle, err := SignCSRTemplate(test.caCerts, test.caKey, test.template)
@ -669,15 +670,91 @@ func TestSignCSRTemplate(t *testing.T) {
// into strings and do a textual diff.
expected, _ := DecodeX509CertificateBytes(test.expectedCertPem)
actual, _ := DecodeX509CertificateBytes(actualBundle.ChainPEM)
assert.Equal(t, expected.Subject.String(), actual.Subject.String())
}
if !bytes.Equal(test.expectedCaCertPem, actualBundle.CAPEM) {
// To help us identify where the mismatch is, we decode turn the
// into strings and do a textual diff.
expected, _ := DecodeX509CertificateBytes(test.expectedCaCertPem)
actual, _ := DecodeX509CertificateBytes(actualBundle.CAPEM)
assert.Equal(t, expected.Subject.String(), actual.Subject.String())
}
})
}
}
func TestEncodeX509Chain(t *testing.T) {
root := mustCreateBundle(t, nil, "root")
intA1 := mustCreateBundle(t, root, "intA-1")
intA2 := mustCreateBundle(t, intA1, "intA-2")
leafA1 := mustCreateBundle(t, intA1, "leaf-a1")
leafA2 := mustCreateBundle(t, intA2, "leaf-a2")
leafInterCN := mustCreateBundle(t, intA1, intA1.cert.Subject.CommonName)
tests := map[string]struct {
inputCerts []*x509.Certificate
expChain []byte
expErr bool
}{
"simple 3 cert chain should be encoded in the same order as passed, with no root": {
inputCerts: []*x509.Certificate{root.cert, intA1.cert, leafA1.cert},
expChain: joinPEM(intA1.pem, leafA1.pem),
expErr: false,
},
"simple 4 cert chain should be encoded in the same order as passed, with no root": {
inputCerts: []*x509.Certificate{root.cert, intA1.cert, intA2.cert, leafA2.cert},
expChain: joinPEM(intA1.pem, intA2.pem, leafA2.pem),
expErr: false,
},
"3 cert chain with no leaf be encoded in the same order as passed, with no root": {
inputCerts: []*x509.Certificate{root.cert, intA1.cert, intA2.cert},
expChain: joinPEM(intA1.pem, intA2.pem),
expErr: false,
},
"chain with a non-root cert where issuer matches subject should include that cert but not root": {
// see https://github.com/jetstack/cert-manager/issues/4142#issuecomment-884248923
inputCerts: []*x509.Certificate{root.cert, intA1.cert, leafInterCN.cert},
expChain: joinPEM(intA1.pem, leafInterCN.pem),
expErr: false,
},
"empty input chain should result in no output and no error": {
inputCerts: []*x509.Certificate{},
expChain: []byte(""),
expErr: false,
},
"chain with just a root should result in no output and no error": {
inputCerts: []*x509.Certificate{root.cert},
expChain: []byte(""),
expErr: false,
},
"chain with just a leaf should result in just the leaf": {
inputCerts: []*x509.Certificate{leafA1.cert},
expChain: leafA1.pem,
expErr: false,
},
"nil certs are ignored": {
inputCerts: []*x509.Certificate{leafA1.cert, nil},
expChain: leafA1.pem,
expErr: false,
},
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
chainOut, err := EncodeX509Chain(test.inputCerts)
if (err != nil) != test.expErr {
t.Errorf("unexpected error, exp=%t got=%v",
test.expErr, err)
}
if !reflect.DeepEqual(chainOut, test.expChain) {
t.Errorf("unexpected output from EncodeX509Chain, exp=%+s got=%+s",
test.expChain, chainOut)
}
})
}
}

24
pkg/util/pki/parse_test.go
View File

@ -219,7 +219,7 @@ func mustCreateBundle(t *testing.T, issuer *testBundle, name string) *testBundle
)
if issuer == nil {
// Selfsigned (no issuer)
// No issuer implies the cert should be self signed
issuerKey = pk
issuerCert = template
} else {
@ -235,6 +235,14 @@ func mustCreateBundle(t *testing.T, issuer *testBundle, name string) *testBundle
return &testBundle{pem: certPEM, cert: cert, pk: pk}
}
func joinPEM(first []byte, rest ...[]byte) []byte {
for _, b := range rest {
first = append(first, b...)
}
return first
}
func TestParseSingleCertificateChain(t *testing.T) {
root := mustCreateBundle(t, nil, "root")
intA1 := mustCreateBundle(t, root, "intA-1")
@ -242,15 +250,9 @@ func TestParseSingleCertificateChain(t *testing.T) {
intB1 := mustCreateBundle(t, root, "intB-1")
intB2 := mustCreateBundle(t, intB1, "intB-2")
leaf := mustCreateBundle(t, intA2, "leaf")
leafInterCN := mustCreateBundle(t, intA2, intA2.cert.Subject.CommonName)
random := mustCreateBundle(t, nil, "random")
joinPEM := func(first []byte, rest ...[]byte) []byte {
for _, b := range rest {
first = append(first, b...)
}
return first
}
tests := map[string]struct {
inputBundle []byte
expPEMBundle PEMBundle
@ -286,6 +288,12 @@ func TestParseSingleCertificateChain(t *testing.T) {
expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem, intA2.pem, intA1.pem), CAPEM: root.pem},
expErr: false,
},
"if certificate chain has two certs with the same CN, shouldn't affect output": {
// see https://github.com/jetstack/cert-manager/issues/4142
inputBundle: joinPEM(leafInterCN.pem, intA1.pem, intA2.pem, root.pem),
expPEMBundle: PEMBundle{ChainPEM: joinPEM(leafInterCN.pem, intA2.pem, intA1.pem), CAPEM: root.pem},
expErr: false,
},
"if 4 certificate chain passed out of order, should return single ca and chain in order": {
inputBundle: joinPEM(root.pem, intA1.pem, leaf.pem, intA2.pem),
expPEMBundle: PEMBundle{ChainPEM: joinPEM(leaf.pem, intA2.pem, intA1.pem), CAPEM: root.pem},