From 41a2622736d27329435c77d5efa25f7897a83902 Mon Sep 17 00:00:00 2001 From: wasaga Date: Thu, 24 Jun 2021 18:40:59 -0400 Subject: [PATCH] certs: reject certs from databroker if they conflict with local (#2309) --- config/certs.go | 79 ++++++++++++++++++++++++++++ config/certs_test.go | 43 +++++++++++++++ config/options.go | 72 ++++++++++++++++++++----- internal/databroker/config_source.go | 2 +- pkg/cryptutil/certificates.go | 45 +++++++++++++++- 5 files changed, 225 insertions(+), 16 deletions(-) create mode 100644 config/certs.go create mode 100644 config/certs_test.go diff --git a/config/certs.go b/config/certs.go new file mode 100644 index 000000000..99d57e0b2 --- /dev/null +++ b/config/certs.go @@ -0,0 +1,79 @@ +package config + +import ( + "crypto/x509" + "strings" +) + +type certUsage byte +type certsIndex map[string]map[string]certUsage + +const ( + certUsageServerAuth = certUsage(1 << iota) + certUsageClientAuth +) + +func splitDomainName(name string) (prefix, suffix string) { + dot := strings.IndexRune(name, '.') + if dot < 0 { + dot = 0 // i.e. `localhost` + } + return name[0:dot], name[dot:] +} + +func getCertUsage(cert *x509.Certificate) certUsage { + var usage certUsage + for _, ex := range cert.ExtKeyUsage { + switch ex { + case x509.ExtKeyUsageClientAuth: + usage |= certUsageClientAuth + case x509.ExtKeyUsageServerAuth: + usage |= certUsageServerAuth + } + } + return usage +} + +func (c certsIndex) addCert(cert *x509.Certificate) { + usage := getCertUsage(cert) + for _, name := range cert.DNSNames { + c.add(name, usage) + } +} + +func (c certsIndex) matchCert(cert *x509.Certificate) (bool, string) { + usage := getCertUsage(cert) + for _, name := range cert.DNSNames { + if c.match(name, usage) { + return true, name + } + } + return false, "" +} + +func (c certsIndex) add(name string, usage certUsage) { + prefix, suffix := splitDomainName(name) + names := c[suffix] + if names == nil { + names = make(map[string]certUsage) + c[suffix] = names + } + names[prefix] = usage +} + +func (c certsIndex) match(name string, usage certUsage) bool { + prefix, suffix := splitDomainName(name) + names := c[suffix] + if names == nil { + return false + } + if prefix != "*" { + return names["*"]&usage != 0 || names[prefix]&usage != 0 + } + for _, u := range names { + if u&usage != 0 { + return true + } + } + return false +} diff --git a/config/certs_test.go b/config/certs_test.go new file mode 100644 index 000000000..c9eaf4818 --- /dev/null +++ b/config/certs_test.go @@ -0,0 +1,43 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCertOverlap(t *testing.T) { + testCases := []struct { + names []string + test string + match bool + }{ + {[]string{"aa.bb.cc", "cc.bb.aa"}, "aa.bb.c", false}, + {[]string{"aa.bb.cc"}, "aa.bb.cc", true}, + {[]string{"*.bb.cc"}, "aa.bb.cc", true}, + {[]string{"a1.bb.cc", "a2.bb.cc"}, "*.bb.cc", true}, + {[]string{"*.bb.cc", "a2.bb.cc"}, "*.bb.cc", true}, + {[]string{"*.aa.bb.cc"}, "*.bb.cc", false}, + {[]string{"*.aa.bb.cc"}, "aa.bb.cc", false}, + {[]string{"bb.cc"}, "*.bb.cc", false}, + } + t.Run("match mix mode", func(t *testing.T) { + for _, tc := range testCases { + idx := make(certsIndex) + for _, name := range tc.names { + idx.add(name, certUsageServerAuth|certUsageClientAuth) + } + assert.Equalf(t, tc.match, idx.match(tc.test, certUsageServerAuth), "%v", tc) + } + }) + t.Run("different cert usages never match", func(t *testing.T) { + for _, tc := range testCases { + idx := make(certsIndex) + for _, name := range tc.names { + idx.add(name, certUsageServerAuth) + } + assert.Equalf(t, false, idx.match(tc.test, certUsageClientAuth), "%v", tc) + } + }) + +} diff --git a/config/options.go b/config/options.go index ba5fa9bab..55fdb8aee 100644 --- a/config/options.go +++ b/config/options.go @@ -990,8 +990,64 @@ func (o *Options) Checksum() uint64 { return hashutil.MustHash(o) } +func (o Options) indexCerts(ctx context.Context) certsIndex { + idx := make(certsIndex) + + if o.CertFile != "" { + cert, err := cryptutil.ParsePEMCertificateFromFile(o.CertFile) + if err != nil { + log.Error(ctx).Err(err).Str("file", o.CertFile).Msg("parsing local cert: skipped") + } else { + idx.addCert(cert) + } + } else if o.Cert != "" { + if data, err := base64.StdEncoding.DecodeString(o.Cert); err != nil { + log.Error(ctx).Err(err).Msg("bad base64 for local cert: skipped") + } else if cert, err := cryptutil.ParsePEMCertificate(data); err != nil { + log.Error(ctx).Err(err).Msg("parsing local cert: skipped") + } else { + idx.addCert(cert) + } + } + + for _, c := range o.CertificateFiles { + cert, err := cryptutil.ParsePEMCertificateFromFile(c.CertFile) + if err != nil { + log.Error(ctx).Err(err).Str("file", c.CertFile).Msg("parsing local cert: skipped") + } + idx.addCert(cert) + } + return idx +} + +func (o *Options) applyExternalCerts(ctx context.Context, certs []*config.Settings_Certificate) { + idx := o.indexCerts(ctx) + for _, c := range certs { + cert, err := cryptutil.ParsePEMCertificate(c.CertBytes) + if err != nil { + log.Error(ctx).Err(err).Msg("parsing cert from databroker: skipped") + continue + } + if overlaps, name := idx.matchCert(cert); overlaps { + log.Error(ctx).Err(err).Str("domain", name).Msg("overlaps with local certs: skipped") + continue + } + cfp := certificateFilePair{ + CertFile: c.CertFile, + KeyFile: c.KeyFile, + } + if cfp.CertFile == "" { + cfp.CertFile = base64.StdEncoding.EncodeToString(c.CertBytes) + } + if cfp.KeyFile == "" { + cfp.KeyFile = base64.StdEncoding.EncodeToString(c.KeyBytes) + } + o.CertificateFiles = append(o.CertificateFiles, cfp) + } +} + // ApplySettings modifies the config options using the given protobuf settings. -func (o *Options) ApplySettings(settings *config.Settings) { +func (o *Options) ApplySettings(ctx context.Context, settings *config.Settings) { if settings == nil { return } @@ -1023,19 +1079,7 @@ func (o *Options) ApplySettings(settings *config.Settings) { if settings.DnsLookupFamily != nil { o.DNSLookupFamily = settings.GetDnsLookupFamily() } - for _, c := range settings.Certificates { - cfp := certificateFilePair{ - CertFile: c.CertFile, - KeyFile: c.KeyFile, - } - if cfp.CertFile == "" { - cfp.CertFile = base64.StdEncoding.EncodeToString(c.CertBytes) - } - if cfp.KeyFile == "" { - cfp.KeyFile = base64.StdEncoding.EncodeToString(c.KeyBytes) - } - o.CertificateFiles = append(o.CertificateFiles, cfp) - } + o.applyExternalCerts(ctx, settings.GetCertificates()) if settings.HttpRedirectAddr != nil { o.HTTPRedirectAddr = settings.GetHttpRedirectAddr() } diff --git a/internal/databroker/config_source.go b/internal/databroker/config_source.go index 7f5f189c8..b94db3128 100644 --- a/internal/databroker/config_source.go +++ b/internal/databroker/config_source.go @@ -91,7 +91,7 @@ func (src *ConfigSource) rebuild(ctx context.Context, firstTime firstTime) { // add all the config policies to the list for id, cfgpb := range src.dbConfigs { - cfg.Options.ApplySettings(cfgpb.Settings) + cfg.Options.ApplySettings(ctx, cfgpb.Settings) var errCount uint64 err := cfg.Options.Validate() diff --git a/pkg/cryptutil/certificates.go b/pkg/cryptutil/certificates.go index d0656270b..f21f04665 100644 --- a/pkg/cryptutil/certificates.go +++ b/pkg/cryptutil/certificates.go @@ -11,13 +11,17 @@ import ( "encoding/pem" "errors" "fmt" + "io" "math/big" "net" "os" "time" ) -const crlPemType = "X509 CRL" +const ( + crlPemType = "X509 CRL" + maxCertFileSize = 1 << 16 +) // CertificateFromBase64 returns an X509 pair from a base64 encoded blob. func CertificateFromBase64(cert, key string) (*tls.Certificate, error) { @@ -211,3 +215,42 @@ func GenerateSelfSignedCertificate(domain string) (*tls.Certificate, error) { return &cert, nil } + +// ParsePEMCertificate parses PEM encoded certificate block +func ParsePEMCertificate(raw []byte) (*x509.Certificate, error) { + data := raw + for { + var block *pem.Block + block, data = pem.Decode(data) + if block == nil { + break + } + + if block.Type != "CERTIFICATE" || len(block.Headers) != 0 { + continue + } + + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, fmt.Errorf("invalid certificate: %w", err) + } + return cert, nil + } + return nil, fmt.Errorf("no certificate block found") +} + +// ParsePEMCertificateFromFile decodes PEM certificate from file +func ParsePEMCertificateFromFile(file string) (*x509.Certificate, error) { + fd, err := os.Open(file) + if err != nil { + return nil, fmt.Errorf("open file: %w", err) + } + defer func() { + _ = fd.Close() + }() + raw, err := io.ReadAll(io.LimitReader(fd, maxCertFileSize)) + if err != nil { + return nil, fmt.Errorf("read file: %w", err) + } + return ParsePEMCertificate(raw) +}