diff --git a/crypto/cert_manager.go b/crypto/cert_manager.go index b067c775..2228d0f3 100644 --- a/crypto/cert_manager.go +++ b/crypto/cert_manager.go @@ -16,12 +16,12 @@ type CertManager interface { } type certManager struct { - chain [][]byte + chain []*x509.Certificate } var _ CertManager = &certManager{} -var errNoCertificateChain = errors.New("No certicifate chain loaded") +var errNoCertificateChain = errors.New("CertManager BUG: No certicifate chain loaded") // NewCertManager creates a new CertManager func NewCertManager() CertManager { @@ -30,12 +30,21 @@ func NewCertManager() CertManager { // SetData takes the byte-slice sent in the SHLO and decompresses it into the certificate chain func (c *certManager) SetData(data []byte) error { - chain, err := decompressChain(data) + byteChain, err := decompressChain(data) if err != nil { return qerr.Error(qerr.InvalidCryptoMessageParameter, "Certificate data invalid") } - c.chain = chain + chain := make([]*x509.Certificate, len(byteChain), len(byteChain)) + for i, data := range byteChain { + cert, err := x509.ParseCertificate(data) + if err != nil { + return err + } + chain[i] = cert + } + + c.chain = chain return nil } @@ -45,21 +54,15 @@ func (c *certManager) GetLeafCert() []byte { if len(c.chain) == 0 { return nil } - return c.chain[0] + return c.chain[0].Raw } func (c *certManager) VerifyServerProof(proof, chlo, serverConfigData []byte) (bool, error) { - leafCert := c.GetLeafCert() - if leafCert == nil { + if len(c.chain) == 0 { return false, errNoCertificateChain } - cert, err := x509.ParseCertificate(leafCert) - if err != nil { - return false, err - } - - return verifyServerProof(proof, cert, chlo, serverConfigData), nil + return verifyServerProof(proof, c.chain[0], chlo, serverConfigData), nil } func (c *certManager) Verify(hostname string) error { @@ -78,12 +81,7 @@ func (c *certManager) Verify(hostname string) error { if len(c.chain) > 1 { intermediates := x509.NewCertPool() for i := 1; i < len(c.chain); i++ { - var cert *x509.Certificate - cert, err = x509.ParseCertificate(c.chain[i]) - if err != nil { - return err - } - intermediates.AddCert(cert) + intermediates.AddCert(c.chain[i]) } opts.Intermediates = intermediates } diff --git a/crypto/cert_manager_test.go b/crypto/cert_manager_test.go index 73983a63..0d0ed575 100644 --- a/crypto/cert_manager_test.go +++ b/crypto/cert_manager_test.go @@ -19,9 +19,19 @@ import ( var _ = Describe("Cert Manager", func() { var cm *certManager + var cert1, cert2 []byte BeforeEach(func() { cm = NewCertManager().(*certManager) + key1, err := rsa.GenerateKey(rand.Reader, 512) + Expect(err).ToNot(HaveOccurred()) + key2, err := rsa.GenerateKey(rand.Reader, 512) + Expect(err).ToNot(HaveOccurred()) + template := &x509.Certificate{SerialNumber: big.NewInt(1)} + cert1, err = x509.CreateCertificate(rand.Reader, template, template, &key1.PublicKey, key1) + Expect(err).ToNot(HaveOccurred()) + cert2, err = x509.CreateCertificate(rand.Reader, template, template, &key2.PublicKey, key2) + Expect(err).ToNot(HaveOccurred()) }) It("errors when given invalid data", func() { @@ -29,22 +39,39 @@ var _ = Describe("Cert Manager", func() { Expect(err).To(MatchError(qerr.Error(qerr.InvalidCryptoMessageParameter, "Certificate data invalid"))) }) - It("decompresses a certificate chain", func() { - cert1 := []byte{0xde, 0xca, 0xfb, 0xad} - cert2 := []byte{0xde, 0xad, 0xbe, 0xef, 0x13, 0x37} - chain := [][]byte{cert1, cert2} - compressed, err := compressChain(chain, nil, nil) - Expect(err).ToNot(HaveOccurred()) - err = cm.SetData(compressed) - Expect(err).ToNot(HaveOccurred()) - Expect(cm.chain).To(Equal(chain)) + Context("setting the data", func() { + It("decompresses a certificate chain", func() { + chain := [][]byte{cert1, cert2} + compressed, err := compressChain(chain, nil, nil) + Expect(err).ToNot(HaveOccurred()) + err = cm.SetData(compressed) + Expect(err).ToNot(HaveOccurred()) + Expect(cm.chain[0].Raw).To(Equal(cert1)) + Expect(cm.chain[1].Raw).To(Equal(cert2)) + }) + + It("errors if it can't decompress the chain", func() { + err := cm.SetData([]byte("invalid data")) + Expect(err).To(MatchError(qerr.Error(qerr.InvalidCryptoMessageParameter, "Certificate data invalid"))) + }) + + It("errors if it can't parse a certificate", func() { + chain := [][]byte{[]byte("cert1"), []byte("cert2")} + compressed, err := compressChain(chain, nil, nil) + Expect(err).ToNot(HaveOccurred()) + err = cm.SetData(compressed) + _, ok := err.(asn1.StructuralError) + Expect(ok).To(BeTrue()) + }) }) Context("getting the leaf cert", func() { It("gets it", func() { - cert1 := []byte{0xc1} - cert2 := []byte{0xc2} - cm.chain = [][]byte{cert1, cert2} + xcert1, err := x509.ParseCertificate(cert1) + Expect(err).ToNot(HaveOccurred()) + xcert2, err := x509.ParseCertificate(cert2) + Expect(err).ToNot(HaveOccurred()) + cm.chain = []*x509.Certificate{xcert1, xcert2} leafCert := cm.GetLeafCert() Expect(leafCert).To(Equal(cert1)) }) @@ -61,15 +88,6 @@ var _ = Describe("Cert Manager", func() { Expect(err).To(MatchError(errNoCertificateChain)) Expect(valid).To(BeFalse()) }) - - It("errors when it can't parse the certificate", func() { - cert := []byte("invalid cert") - cm.chain = [][]byte{cert} - valid, err := cm.VerifyServerProof([]byte("proof"), []byte("chlo"), []byte("scfg")) - Expect(err).To(HaveOccurred()) - Expect(err).ToNot(MatchError(errNoCertificateChain)) - Expect(valid).To(BeFalse()) - }) }) Context("verifying the certificate chain", func() { @@ -86,26 +104,18 @@ var _ = Describe("Cert Manager", func() { It("accepts a valid certificate", func() { cc := NewCertChain(testdata.GetTLSConfig()).(*certChain) - cert, err := cc.getCertForSNI("quic.clemente.io") + tlsCert, err := cc.getCertForSNI("quic.clemente.io") Expect(err).ToNot(HaveOccurred()) - cm.chain = cert.Certificate + for _, data := range tlsCert.Certificate { + var cert *x509.Certificate + cert, err = x509.ParseCertificate(data) + Expect(err).ToNot(HaveOccurred()) + cm.chain = append(cm.chain, cert) + } err = cm.Verify("quic.clemente.io") Expect(err).ToNot(HaveOccurred()) }) - It("errors if it can't parse an intermediate certificate", func() { - cc := NewCertChain(testdata.GetTLSConfig()).(*certChain) - cert, err := cc.getCertForSNI("quic.clemente.io") - Expect(err).ToNot(HaveOccurred()) - cm.chain = cert.Certificate - Expect(cm.chain).To(HaveLen(2)) - cm.chain[1] = []byte("invalid intermediate") - err = cm.Verify("quic.clemente.io") - Expect(err).To(HaveOccurred()) - _, ok := err.(asn1.StructuralError) - Expect(ok).To(BeTrue()) - }) - It("doesn't accept an expired certificate", func() { if runtime.GOOS == "windows" { // certificate validation works different on windows, see https://golang.org/src/crypto/x509/verify.go line 238 @@ -119,7 +129,7 @@ var _ = Describe("Cert Manager", func() { } leafCert := getCertificate(template) - cm.chain = [][]byte{leafCert.Raw} + cm.chain = []*x509.Certificate{leafCert} err := cm.Verify("") Expect(err).To(HaveOccurred()) Expect(err.(x509.CertificateInvalidError).Reason).To(Equal(x509.Expired)) @@ -138,7 +148,7 @@ var _ = Describe("Cert Manager", func() { } leafCert := getCertificate(template) - cm.chain = [][]byte{leafCert.Raw} + cm.chain = []*x509.Certificate{leafCert} err := cm.Verify("") Expect(err).To(HaveOccurred()) Expect(err.(x509.CertificateInvalidError).Reason).To(Equal(x509.Expired)) @@ -158,7 +168,7 @@ var _ = Describe("Cert Manager", func() { } leafCert := getCertificate(template) - cm.chain = [][]byte{leafCert.Raw} + cm.chain = []*x509.Certificate{leafCert} err := cm.Verify("quic.clemente.io") Expect(err).To(HaveOccurred()) _, ok := err.(x509.HostnameError) @@ -170,12 +180,6 @@ var _ = Describe("Cert Manager", func() { Expect(err).To(HaveOccurred()) }) - It("errors if it can't parse the leaf certificate", func() { - cm.chain = [][]byte{[]byte("invalid leaf cert")} - err := cm.Verify("example.com") - Expect(err).To(HaveOccurred()) - }) - // this tests relies on LetsEncrypt not being contained in the Root CAs It("rejects valid certificate with missing certificate chain", func() { if runtime.GOOS == "windows" { @@ -183,8 +187,10 @@ var _ = Describe("Cert Manager", func() { } cert := testdata.GetCertificate() - cm.chain = [][]byte{cert.Certificate[0]} - err := cm.Verify("quic.clemente.io") + xcert, err := x509.ParseCertificate(cert.Certificate[0]) + Expect(err).ToNot(HaveOccurred()) + cm.chain = []*x509.Certificate{xcert} + err = cm.Verify("quic.clemente.io") _, ok := err.(x509.UnknownAuthorityError) Expect(ok).To(BeTrue()) })