From 0263468424fb2f7ff9de791b6fd1c50c7650d3d9 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Mon, 19 Sep 2022 19:45:13 -0700 Subject: [PATCH 01/28] Initial work on name constraints validation Issue #1060 --- authority/internal/constraints/constraints.go | 106 ++++++ .../internal/constraints/constraints_test.go | 140 +++++++ authority/internal/constraints/verify.go | 358 ++++++++++++++++++ 3 files changed, 604 insertions(+) create mode 100644 authority/internal/constraints/constraints.go create mode 100644 authority/internal/constraints/constraints_test.go create mode 100644 authority/internal/constraints/verify.go diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go new file mode 100644 index 00000000..a9dcf715 --- /dev/null +++ b/authority/internal/constraints/constraints.go @@ -0,0 +1,106 @@ +package constraints + +import ( + "crypto/x509" + "fmt" + "net" + "net/url" +) + +var oidExtensionNameConstraints = []int{2, 5, 29, 30} + +type ConstraintError struct { + Type string + Name string +} + +func (e ConstraintError) Error() string { + return fmt.Sprintf("%s %q is not allowed", e.Type, e.Name) +} + +type service struct { + hasNameConstraints bool + permittedDNSDomains []string + excludedDNSDomains []string + permittedIPRanges []*net.IPNet + excludedIPRanges []*net.IPNet + permittedEmailAddresses []string + excludedEmailAddresses []string + permittedURIDomains []string + excludedURIDomains []string +} + +func New(chain ...*x509.Certificate) *service { + s := new(service) + for _, crt := range chain { + s.permittedDNSDomains = append(s.permittedDNSDomains, crt.PermittedDNSDomains...) + s.excludedDNSDomains = append(s.excludedDNSDomains, crt.ExcludedDNSDomains...) + s.permittedIPRanges = append(s.permittedIPRanges, crt.PermittedIPRanges...) + s.excludedIPRanges = append(s.excludedIPRanges, crt.ExcludedIPRanges...) + s.permittedEmailAddresses = append(s.permittedEmailAddresses, crt.PermittedEmailAddresses...) + s.excludedEmailAddresses = append(s.excludedEmailAddresses, crt.ExcludedEmailAddresses...) + s.permittedURIDomains = append(s.permittedURIDomains, crt.PermittedURIDomains...) + s.excludedURIDomains = append(s.excludedURIDomains, crt.ExcludedURIDomains...) + + if !s.hasNameConstraints { + for _, ext := range crt.Extensions { + if ext.Id.Equal(oidExtensionNameConstraints) { + s.hasNameConstraints = true + break + } + } + } + } + return s +} + +// Validates +func (s *service) Validate(dnsNames []string, ipAddresses []*net.IP, emailAddresses []string, uris []*url.URL) error { + if !s.hasNameConstraints { + return nil + } + + for _, name := range dnsNames { + if err := checkNameConstraints("DNS name", name, name, s.permittedDNSDomains, s.excludedDNSDomains, + func(parsedName, constraint any) (bool, error) { + return matchDomainConstraint(parsedName.(string), constraint.(string)) + }, + ); err != nil { + return err + } + } + + for _, ip := range ipAddresses { + if err := checkNameConstraints("IP address", ip.String(), ip, s.permittedIPRanges, s.excludedIPRanges, + func(parsedName, constraint any) (bool, error) { + return matchIPConstraint(parsedName.(net.IP), constraint.(*net.IPNet)) + }); err != nil { + return err + } + } + + for _, email := range emailAddresses { + mailbox, ok := parseRFC2821Mailbox(email) + if !ok { + return fmt.Errorf("cannot parse rfc822Name %q", email) + } + if err := checkNameConstraints("Email address", email, mailbox, s.permittedEmailAddresses, s.excludedEmailAddresses, + func(parsedName, constraint any) (bool, error) { + return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string)) + }, + ); err != nil { + return err + } + } + + for _, uri := range uris { + if err := checkNameConstraints("URI", uri.String(), uri, s.permittedURIDomains, s.excludedURIDomains, + func(parsedName, constraint any) (bool, error) { + return matchURIConstraint(parsedName.(*url.URL), constraint.(string)) + }); err != nil { + return err + } + } + + return nil +} diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go new file mode 100644 index 00000000..1194f6ce --- /dev/null +++ b/authority/internal/constraints/constraints_test.go @@ -0,0 +1,140 @@ +package constraints + +import ( + "crypto/x509" + "net" + "net/url" + "reflect" + "testing" + + "go.step.sm/crypto/minica" +) + +func TestNew(t *testing.T) { + ca1, err := minica.New() + if err != nil { + t.Fatal(err) + } + + ca2, err := minica.New( + minica.WithIntermediateTemplate(`{ + "subject": {{ toJson .Subject }}, + "keyUsage": ["certSign", "crlSign"], + "basicConstraints": { + "isCA": true, + "maxPathLen": 0 + }, + "nameConstraints": { + "critical": true, + "permittedDNSDomains": ["internal.example.org"], + "excludedDNSDomains": ["internal.example.com"], + "permittedIPRanges": ["192.168.1.0/24", "192.168.2.1/32"], + "excludedIPRanges": ["192.168.3.0/24", "192.168.4.0/28"], + "permittedEmailAddresses": ["root@example.org", "example.org", ".acme.org"], + "excludedEmailAddresses": ["root@example.com", "example.com", ".acme.com"], + "permittedURIDomains": ["host.example.org", ".acme.org"], + "excludedURIDomains": ["host.example.com", ".acme.com"] + } + }`), + ) + if err != nil { + t.Fatal(err) + } + + type args struct { + chain []*x509.Certificate + } + tests := []struct { + name string + args args + want *service + }{ + {"ok", args{[]*x509.Certificate{ca1.Intermediate, ca1.Root}}, &service{ + hasNameConstraints: false, + }}, + {"ok with constraints", args{[]*x509.Certificate{ca2.Intermediate, ca2.Root}}, &service{ + hasNameConstraints: true, + permittedDNSDomains: []string{"internal.example.org"}, + excludedDNSDomains: []string{"internal.example.com"}, + permittedIPRanges: []*net.IPNet{ + {IP: net.ParseIP("192.168.1.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}, + {IP: net.ParseIP("192.168.2.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, + }, + excludedIPRanges: []*net.IPNet{ + {IP: net.ParseIP("192.168.3.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}, + {IP: net.ParseIP("192.168.4.0").To4(), Mask: net.IPMask{255, 255, 255, 240}}, + }, + permittedEmailAddresses: []string{"root@example.org", "example.org", ".acme.org"}, + excludedEmailAddresses: []string{"root@example.com", "example.com", ".acme.com"}, + permittedURIDomains: []string{"host.example.org", ".acme.org"}, + excludedURIDomains: []string{"host.example.com", ".acme.com"}, + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := New(tt.args.chain...); !reflect.DeepEqual(got, tt.want) { + t.Errorf("New() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_service_Validate(t *testing.T) { + + type fields struct { + hasNameConstraints bool + permittedDNSDomains []string + excludedDNSDomains []string + permittedIPRanges []*net.IPNet + excludedIPRanges []*net.IPNet + permittedEmailAddresses []string + excludedEmailAddresses []string + permittedURIDomains []string + excludedURIDomains []string + } + type args struct { + dnsNames []string + ipAddresses []*net.IP + emailAddresses []string + uris []*url.URL + } + tests := []struct { + name string + fields fields + args args + wantErr bool + }{ + {"ok", fields{hasNameConstraints: false}, args{ + dnsNames: []string{"example.com", "host.example.com"}, + ipAddresses: []*net.IP{{192, 168, 1, 1}, {0x26, 0x00, 0x1f, 0x1c, 0x47, 0x1, 0x9d, 0x00, 0xc3, 0xa7, 0x66, 0x94, 0x87, 0x0f, 0x20, 0x72}}, + emailAddresses: []string{"root@example.com"}, + uris: []*url.URL{{Scheme: "https", Host: "example.com", Path: "/uuid/c6d1a755-0c12-431e-9136-b64cb3173ec7"}}, + }, false}, + // {"ok dns", fields{}, args{}, false}, + // {"ok ip", fields{}, args{}, false}, + // {"ok email", fields{}, args{}, false}, + // {"ok uri", fields{}, args{}, false}, + // {"fail dns", fields{}, args{}, true}, + // {"fail ip", fields{}, args{}, true}, + // {"fail email", fields{}, args{}, true}, + // {"fail uri", fields{}, args{}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &service{ + hasNameConstraints: tt.fields.hasNameConstraints, + permittedDNSDomains: tt.fields.permittedDNSDomains, + excludedDNSDomains: tt.fields.excludedDNSDomains, + permittedIPRanges: tt.fields.permittedIPRanges, + excludedIPRanges: tt.fields.excludedIPRanges, + permittedEmailAddresses: tt.fields.permittedEmailAddresses, + excludedEmailAddresses: tt.fields.excludedEmailAddresses, + permittedURIDomains: tt.fields.permittedURIDomains, + excludedURIDomains: tt.fields.excludedURIDomains, + } + if err := s.Validate(tt.args.dnsNames, tt.args.ipAddresses, tt.args.emailAddresses, tt.args.uris); (err != nil) != tt.wantErr { + t.Errorf("service.Validate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/authority/internal/constraints/verify.go b/authority/internal/constraints/verify.go new file mode 100644 index 00000000..981cd35e --- /dev/null +++ b/authority/internal/constraints/verify.go @@ -0,0 +1,358 @@ +// Copyright (c) 2009 The Go Authors. All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +package constraints + +import ( + "bytes" + "fmt" + "net" + "net/url" + "reflect" + "strings" +) + +func checkNameConstraints(nameType string, name string, parsedName any, permitted, excluded any, match func(name, constraint any) (bool, error)) error { + excludedValue := reflect.ValueOf(excluded) + for i := 0; i < excludedValue.Len(); i++ { + constraint := excludedValue.Index(i).Interface() + match, err := match(parsedName, constraint) + if err != nil { + return err + } + + if match { + return fmt.Errorf("%s %q is excluded by constraint %q", nameType, name, constraint) + } + } + + var ( + err error + ok = true + ) + + permittedValue := reflect.ValueOf(permitted) + for i := 0; i < permittedValue.Len(); i++ { + constraint := permittedValue.Index(i).Interface() + if ok, err = match(parsedName, constraint); err != nil { + return err + } + if ok { + break + } + } + if !ok { + return fmt.Errorf("%s %q is not permitted by any constraint", nameType, name) + } + + return nil +} + +func matchDomainConstraint(domain, constraint string) (bool, error) { + // The meaning of zero length constraints is not specified, but this + // code follows NSS and accepts them as matching everything. + if len(constraint) == 0 { + return true, nil + } + + domainLabels, ok := domainToReverseLabels(domain) + if !ok { + return false, fmt.Errorf("x509: internal error: cannot parse domain %q", domain) + } + + // RFC 5280 says that a leading period in a domain name means that at + // least one label must be prepended, but only for URI and email + // constraints, not DNS constraints. The code also supports that + // behaviour for DNS constraints. + + mustHaveSubdomains := false + if constraint[0] == '.' { + mustHaveSubdomains = true + constraint = constraint[1:] + } + + constraintLabels, ok := domainToReverseLabels(constraint) + if !ok { + return false, fmt.Errorf("x509: internal error: cannot parse domain %q", constraint) + } + + if len(domainLabels) < len(constraintLabels) || + (mustHaveSubdomains && len(domainLabels) == len(constraintLabels)) { + return false, nil + } + + for i, constraintLabel := range constraintLabels { + if !strings.EqualFold(constraintLabel, domainLabels[i]) { + return false, nil + } + } + + return true, nil +} + +func matchIPConstraint(ip net.IP, constraint *net.IPNet) (bool, error) { + if len(ip) != len(constraint.IP) { + return false, nil + } + + for i := range ip { + if mask := constraint.Mask[i]; ip[i]&mask != constraint.IP[i]&mask { + return false, nil + } + } + + return true, nil +} + +func matchEmailConstraint(mailbox rfc2821Mailbox, constraint string) (bool, error) { + // If the constraint contains an @, then it specifies an exact mailbox + // name. + if strings.Contains(constraint, "@") { + constraintMailbox, ok := parseRFC2821Mailbox(constraint) + if !ok { + return false, fmt.Errorf("x509: internal error: cannot parse constraint %q", constraint) + } + return mailbox.local == constraintMailbox.local && strings.EqualFold(mailbox.domain, constraintMailbox.domain), nil + } + + // Otherwise the constraint is like a DNS constraint of the domain part + // of the mailbox. + return matchDomainConstraint(mailbox.domain, constraint) +} + +func matchURIConstraint(uri *url.URL, constraint string) (bool, error) { + // From RFC 5280, Section 4.2.1.10: + // “a uniformResourceIdentifier that does not include an authority + // component with a host name specified as a fully qualified domain + // name (e.g., if the URI either does not include an authority + // component or includes an authority component in which the host name + // is specified as an IP address), then the application MUST reject the + // certificate.” + + host := uri.Host + if len(host) == 0 { + return false, fmt.Errorf("URI with empty host (%q) cannot be matched against constraints", uri.String()) + } + + if strings.Contains(host, ":") && !strings.HasSuffix(host, "]") { + var err error + host, _, err = net.SplitHostPort(uri.Host) + if err != nil { + return false, err + } + } + + if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") || + net.ParseIP(host) != nil { + return false, fmt.Errorf("URI with IP (%q) cannot be matched against constraints", uri.String()) + } + + return matchDomainConstraint(host, constraint) +} + +// domainToReverseLabels converts a textual domain name like foo.example.com to +// the list of labels in reverse order, e.g. ["com", "example", "foo"]. +func domainToReverseLabels(domain string) (reverseLabels []string, ok bool) { + for len(domain) > 0 { + if i := strings.LastIndexByte(domain, '.'); i == -1 { + reverseLabels = append(reverseLabels, domain) + domain = "" + } else { + reverseLabels = append(reverseLabels, domain[i+1:]) + domain = domain[:i] + } + } + + if len(reverseLabels) > 0 && len(reverseLabels[0]) == 0 { + // An empty label at the end indicates an absolute value. + return nil, false + } + + for _, label := range reverseLabels { + if len(label) == 0 { + // Empty labels are otherwise invalid. + return nil, false + } + + for _, c := range label { + if c < 33 || c > 126 { + // Invalid character. + return nil, false + } + } + } + + return reverseLabels, true +} + +// rfc2821Mailbox represents a “mailbox” (which is an email address to most +// people) by breaking it into the “local” (i.e. before the '@') and “domain” +// parts. +type rfc2821Mailbox struct { + local, domain string +} + +// parseRFC2821Mailbox parses an email address into local and domain parts, +// based on the ABNF for a “Mailbox” from RFC 2821. According to RFC 5280, +// Section 4.2.1.6 that's correct for an rfc822Name from a certificate: “The +// format of an rfc822Name is a "Mailbox" as defined in RFC 2821, Section 4.1.2”. +func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) { + if len(in) == 0 { + return mailbox, false + } + + localPartBytes := make([]byte, 0, len(in)/2) + + if in[0] == '"' { + // Quoted-string = DQUOTE *qcontent DQUOTE + // non-whitespace-control = %d1-8 / %d11 / %d12 / %d14-31 / %d127 + // qcontent = qtext / quoted-pair + // qtext = non-whitespace-control / + // %d33 / %d35-91 / %d93-126 + // quoted-pair = ("\" text) / obs-qp + // text = %d1-9 / %d11 / %d12 / %d14-127 / obs-text + // + // (Names beginning with “obs-” are the obsolete syntax from RFC 2822, + // Section 4. Since it has been 16 years, we no longer accept that.) + in = in[1:] + QuotedString: + for { + if len(in) == 0 { + return mailbox, false + } + c := in[0] + in = in[1:] + + switch { + case c == '"': + break QuotedString + + case c == '\\': + // quoted-pair + if len(in) == 0 { + return mailbox, false + } + if in[0] == 11 || + in[0] == 12 || + (1 <= in[0] && in[0] <= 9) || + (14 <= in[0] && in[0] <= 127) { + localPartBytes = append(localPartBytes, in[0]) + in = in[1:] + } else { + return mailbox, false + } + + case c == 11 || + c == 12 || + // Space (char 32) is not allowed based on the + // BNF, but RFC 3696 gives an example that + // assumes that it is. Several “verified” + // errata continue to argue about this point. + // We choose to accept it. + c == 32 || + c == 33 || + c == 127 || + (1 <= c && c <= 8) || + (14 <= c && c <= 31) || + (35 <= c && c <= 91) || + (93 <= c && c <= 126): + // qtext + localPartBytes = append(localPartBytes, c) + + default: + return mailbox, false + } + } + } else { + // Atom ("." Atom)* + NextChar: + for len(in) > 0 { + // atext from RFC 2822, Section 3.2.4 + c := in[0] + + switch { + case c == '\\': + // Examples given in RFC 3696 suggest that + // escaped characters can appear outside of a + // quoted string. Several “verified” errata + // continue to argue the point. We choose to + // accept it. + in = in[1:] + if len(in) == 0 { + return mailbox, false + } + fallthrough + + case ('0' <= c && c <= '9') || + ('a' <= c && c <= 'z') || + ('A' <= c && c <= 'Z') || + c == '!' || c == '#' || c == '$' || c == '%' || + c == '&' || c == '\'' || c == '*' || c == '+' || + c == '-' || c == '/' || c == '=' || c == '?' || + c == '^' || c == '_' || c == '`' || c == '{' || + c == '|' || c == '}' || c == '~' || c == '.': + localPartBytes = append(localPartBytes, in[0]) + in = in[1:] + + default: + break NextChar + } + } + + if len(localPartBytes) == 0 { + return mailbox, false + } + + // From RFC 3696, Section 3: + // “period (".") may also appear, but may not be used to start + // or end the local part, nor may two or more consecutive + // periods appear.” + twoDots := []byte{'.', '.'} + if localPartBytes[0] == '.' || + localPartBytes[len(localPartBytes)-1] == '.' || + bytes.Contains(localPartBytes, twoDots) { + return mailbox, false + } + } + + if len(in) == 0 || in[0] != '@' { + return mailbox, false + } + in = in[1:] + + // The RFC species a format for domains, but that's known to be + // violated in practice so we accept that anything after an '@' is the + // domain part. + if _, ok := domainToReverseLabels(in); !ok { + return mailbox, false + } + + mailbox.local = string(localPartBytes) + mailbox.domain = in + return mailbox, true +} From 6686f0437d9d239a8d29ea0c0fcd2870fe6225f1 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 10:23:51 -0700 Subject: [PATCH 02/28] Remove x509 prefixes --- authority/internal/constraints/verify.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/authority/internal/constraints/verify.go b/authority/internal/constraints/verify.go index 981cd35e..767fe304 100644 --- a/authority/internal/constraints/verify.go +++ b/authority/internal/constraints/verify.go @@ -82,7 +82,7 @@ func matchDomainConstraint(domain, constraint string) (bool, error) { domainLabels, ok := domainToReverseLabels(domain) if !ok { - return false, fmt.Errorf("x509: internal error: cannot parse domain %q", domain) + return false, fmt.Errorf("internal error: cannot parse domain %q", domain) } // RFC 5280 says that a leading period in a domain name means that at @@ -98,7 +98,7 @@ func matchDomainConstraint(domain, constraint string) (bool, error) { constraintLabels, ok := domainToReverseLabels(constraint) if !ok { - return false, fmt.Errorf("x509: internal error: cannot parse domain %q", constraint) + return false, fmt.Errorf("internal error: cannot parse domain %q", constraint) } if len(domainLabels) < len(constraintLabels) || @@ -135,7 +135,7 @@ func matchEmailConstraint(mailbox rfc2821Mailbox, constraint string) (bool, erro if strings.Contains(constraint, "@") { constraintMailbox, ok := parseRFC2821Mailbox(constraint) if !ok { - return false, fmt.Errorf("x509: internal error: cannot parse constraint %q", constraint) + return false, fmt.Errorf("internal error: cannot parse constraint %q", constraint) } return mailbox.local == constraintMailbox.local && strings.EqualFold(mailbox.domain, constraintMailbox.domain), nil } From 495494ce8f7d85f379251f729c4748cc90e22644 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 10:36:44 -0700 Subject: [PATCH 03/28] Return a typed error --- authority/internal/constraints/constraints.go | 13 ++++++---- authority/internal/constraints/verify.go | 24 +++++++++++++++---- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index a9dcf715..0e8c4f27 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -10,12 +10,13 @@ import ( var oidExtensionNameConstraints = []int{2, 5, 29, 30} type ConstraintError struct { - Type string - Name string + Type string + Name string + Detail string } func (e ConstraintError) Error() string { - return fmt.Sprintf("%s %q is not allowed", e.Type, e.Name) + return e.Detail } type service struct { @@ -74,7 +75,8 @@ func (s *service) Validate(dnsNames []string, ipAddresses []*net.IP, emailAddres if err := checkNameConstraints("IP address", ip.String(), ip, s.permittedIPRanges, s.excludedIPRanges, func(parsedName, constraint any) (bool, error) { return matchIPConstraint(parsedName.(net.IP), constraint.(*net.IPNet)) - }); err != nil { + }, + ); err != nil { return err } } @@ -97,7 +99,8 @@ func (s *service) Validate(dnsNames []string, ipAddresses []*net.IP, emailAddres if err := checkNameConstraints("URI", uri.String(), uri, s.permittedURIDomains, s.excludedURIDomains, func(parsedName, constraint any) (bool, error) { return matchURIConstraint(parsedName.(*url.URL), constraint.(string)) - }); err != nil { + }, + ); err != nil { return err } } diff --git a/authority/internal/constraints/verify.go b/authority/internal/constraints/verify.go index 767fe304..552c5ea2 100644 --- a/authority/internal/constraints/verify.go +++ b/authority/internal/constraints/verify.go @@ -43,11 +43,19 @@ func checkNameConstraints(nameType string, name string, parsedName any, permitte constraint := excludedValue.Index(i).Interface() match, err := match(parsedName, constraint) if err != nil { - return err + return ConstraintError{ + Type: nameType, + Name: name, + Detail: err.Error(), + } } if match { - return fmt.Errorf("%s %q is excluded by constraint %q", nameType, name, constraint) + return ConstraintError{ + Type: nameType, + Name: name, + Detail: fmt.Sprintf("%s %q is excluded by constraint %q", nameType, name, constraint), + } } } @@ -60,14 +68,22 @@ func checkNameConstraints(nameType string, name string, parsedName any, permitte for i := 0; i < permittedValue.Len(); i++ { constraint := permittedValue.Index(i).Interface() if ok, err = match(parsedName, constraint); err != nil { - return err + return ConstraintError{ + Type: nameType, + Name: name, + Detail: err.Error(), + } } if ok { break } } if !ok { - return fmt.Errorf("%s %q is not permitted by any constraint", nameType, name) + return ConstraintError{ + Type: nameType, + Name: name, + Detail: fmt.Sprintf("%s %q is not permitted by any constraint", nameType, name), + } } return nil From 7bea2f4d0e64b6bd7d837d6ea1711b9ef22bb2b6 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 11:33:36 -0700 Subject: [PATCH 04/28] Add more constraint unit tests --- authority/internal/constraints/constraints.go | 10 +- .../internal/constraints/constraints_test.go | 104 ++++++++++++++++-- 2 files changed, 102 insertions(+), 12 deletions(-) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 0e8c4f27..5e27c41f 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -9,12 +9,15 @@ import ( var oidExtensionNameConstraints = []int{2, 5, 29, 30} +// ConstraintError is the typed error that will be returned if a constraint +// error is found. type ConstraintError struct { Type string Name string Detail string } +// Error implements the error interface. func (e ConstraintError) Error() string { return e.Detail } @@ -31,6 +34,8 @@ type service struct { excludedURIDomains []string } +// New creates a constraint validation service that contains the given chain of +// certificates. func New(chain ...*x509.Certificate) *service { s := new(service) for _, crt := range chain { @@ -55,8 +60,9 @@ func New(chain ...*x509.Certificate) *service { return s } -// Validates -func (s *service) Validate(dnsNames []string, ipAddresses []*net.IP, emailAddresses []string, uris []*url.URL) error { +// Validate checks the given names with the name constraints defined in the +// service. +func (s *service) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresses []string, uris []*url.URL) error { if !s.hasNameConstraints { return nil } diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index 1194f6ce..69aa51ad 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -94,7 +94,7 @@ func Test_service_Validate(t *testing.T) { } type args struct { dnsNames []string - ipAddresses []*net.IP + ipAddresses []net.IP emailAddresses []string uris []*url.URL } @@ -106,18 +106,102 @@ func Test_service_Validate(t *testing.T) { }{ {"ok", fields{hasNameConstraints: false}, args{ dnsNames: []string{"example.com", "host.example.com"}, - ipAddresses: []*net.IP{{192, 168, 1, 1}, {0x26, 0x00, 0x1f, 0x1c, 0x47, 0x1, 0x9d, 0x00, 0xc3, 0xa7, 0x66, 0x94, 0x87, 0x0f, 0x20, 0x72}}, + ipAddresses: []net.IP{{192, 168, 1, 1}, {0x26, 0x00, 0x1f, 0x1c, 0x47, 0x1, 0x9d, 0x00, 0xc3, 0xa7, 0x66, 0x94, 0x87, 0x0f, 0x20, 0x72}}, emailAddresses: []string{"root@example.com"}, uris: []*url.URL{{Scheme: "https", Host: "example.com", Path: "/uuid/c6d1a755-0c12-431e-9136-b64cb3173ec7"}}, }, false}, - // {"ok dns", fields{}, args{}, false}, - // {"ok ip", fields{}, args{}, false}, - // {"ok email", fields{}, args{}, false}, - // {"ok uri", fields{}, args{}, false}, - // {"fail dns", fields{}, args{}, true}, - // {"fail ip", fields{}, args{}, true}, - // {"fail email", fields{}, args{}, true}, - // {"fail uri", fields{}, args{}, true}, + {"ok permitted dns ", fields{ + hasNameConstraints: true, + permittedDNSDomains: []string{"example.com"}, + }, args{dnsNames: []string{"example.com", "www.example.com"}}, false}, + {"ok not excluded dns", fields{ + hasNameConstraints: true, + excludedDNSDomains: []string{"example.org"}, + }, args{dnsNames: []string{"example.com", "www.example.com"}}, false}, + {"ok permitted ip", fields{ + hasNameConstraints: true, + permittedIPRanges: []*net.IPNet{ + {IP: net.ParseIP("192.168.1.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}, + {IP: net.ParseIP("192.168.2.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, + }, + }, args{ipAddresses: []net.IP{{192, 168, 1, 10}, {192, 168, 2, 1}}}, false}, + {"ok not excluded ip", fields{ + hasNameConstraints: true, + excludedIPRanges: []*net.IPNet{ + {IP: net.ParseIP("192.168.1.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}, + {IP: net.ParseIP("192.168.2.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, + }, + }, args{ipAddresses: []net.IP{{192, 168, 2, 2}, {192, 168, 3, 1}}}, false}, + {"ok permitted emails ", fields{ + hasNameConstraints: true, + permittedEmailAddresses: []string{"root@example.com", "acme.org", ".acme.com"}, + }, args{emailAddresses: []string{"root@example.com", "name@acme.org", "name@coyote.acme.com", `"(quoted)"@www.acme.com`}}, false}, + {"ok not excluded emails", fields{ + hasNameConstraints: true, + excludedEmailAddresses: []string{"root@example.com", "acme.org", ".acme.com"}, + }, args{emailAddresses: []string{"name@example.com", "root@acme.com", "root@other.com"}}, false}, + {"ok permitted uris ", fields{ + hasNameConstraints: true, + permittedURIDomains: []string{"example.com", ".acme.com"}, + }, args{uris: []*url.URL{{Scheme: "https", Host: "example.com", Path: "/path"}, {Scheme: "https", Host: "www.acme.com", Path: "/path"}}}, false}, + {"ok not excluded uris", fields{ + hasNameConstraints: true, + excludedURIDomains: []string{"example.com", ".acme.com"}, + }, args{uris: []*url.URL{{Scheme: "https", Host: "example.org", Path: "/path"}, {Scheme: "https", Host: "acme.com", Path: "/path"}}}, false}, + {"fail permitted dns ", fields{ + hasNameConstraints: true, + permittedDNSDomains: []string{"example.com"}, + }, args{dnsNames: []string{"www.example.com", "www.example.org"}}, true}, + {"fail not excluded dns", fields{ + hasNameConstraints: true, + excludedDNSDomains: []string{"example.org"}, + }, args{dnsNames: []string{"example.com", "www.example.org"}}, true}, + {"fail permitted ip", fields{ + hasNameConstraints: true, + permittedIPRanges: []*net.IPNet{ + {IP: net.ParseIP("192.168.1.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}, + {IP: net.ParseIP("192.168.2.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, + }, + }, args{ipAddresses: []net.IP{{192, 168, 1, 10}, {192, 168, 2, 10}}}, true}, + {"fail not excluded ip", fields{ + hasNameConstraints: true, + excludedIPRanges: []*net.IPNet{ + {IP: net.ParseIP("192.168.1.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}, + {IP: net.ParseIP("192.168.2.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, + }, + }, args{ipAddresses: []net.IP{{192, 168, 2, 2}, {192, 168, 1, 1}}}, true}, + {"fail permitted emails ", fields{ + hasNameConstraints: true, + permittedEmailAddresses: []string{"root@example.com", "acme.org", ".acme.com"}, + }, args{emailAddresses: []string{"root@example.com", "name@acme.org", "name@acme.com"}}, true}, + {"fail not excluded emails", fields{ + hasNameConstraints: true, + excludedEmailAddresses: []string{"root@example.com", "acme.org", ".acme.com"}, + }, args{emailAddresses: []string{"name@example.com", "root@example.com"}}, true}, + {"fail permitted uris ", fields{ + hasNameConstraints: true, + permittedURIDomains: []string{"example.com", ".acme.com"}, + }, args{uris: []*url.URL{{Scheme: "https", Host: "example.com", Path: "/path"}, {Scheme: "https", Host: "acme.com", Path: "/path"}}}, true}, + {"fail not excluded uris", fields{ + hasNameConstraints: true, + excludedURIDomains: []string{"example.com", ".acme.com"}, + }, args{uris: []*url.URL{{Scheme: "https", Host: "www.example.com", Path: "/path"}, {Scheme: "https", Host: "acme.com", Path: "/path"}}}, true}, + {"fail parse emails ", fields{ + hasNameConstraints: true, + permittedEmailAddresses: []string{"example.com"}, + }, args{emailAddresses: []string{`(notquoted)@example.com`}}, true}, + {"fail match dns", fields{ + hasNameConstraints: true, + permittedDNSDomains: []string{"example.com"}, + }, args{dnsNames: []string{`www.example.com.`}}, true}, + {"fail match email", fields{ + hasNameConstraints: true, + excludedEmailAddresses: []string{`(notquoted)@example.com`}, + }, args{emailAddresses: []string{`ok@example.com`}}, true}, + {"fail match uri", fields{ + hasNameConstraints: true, + permittedURIDomains: []string{"example.com"}, + }, args{uris: []*url.URL{{Scheme: "urn", Opaque: "uuid:36efb1ae-6617-4b23-b799-874a37aaea1c"}}}, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 45e594f98cfcca46e1119d474895650b31871572 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 11:36:45 -0700 Subject: [PATCH 05/28] Make the constraint service public --- authority/internal/constraints/constraints.go | 10 ++++++---- authority/internal/constraints/constraints_test.go | 8 ++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 5e27c41f..997b53aa 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -22,7 +22,9 @@ func (e ConstraintError) Error() string { return e.Detail } -type service struct { +// Service implements a constraint validator for DNS names, IP addresses, Email +// addresses and URIs. +type Service struct { hasNameConstraints bool permittedDNSDomains []string excludedDNSDomains []string @@ -36,8 +38,8 @@ type service struct { // New creates a constraint validation service that contains the given chain of // certificates. -func New(chain ...*x509.Certificate) *service { - s := new(service) +func New(chain ...*x509.Certificate) *Service { + s := new(Service) for _, crt := range chain { s.permittedDNSDomains = append(s.permittedDNSDomains, crt.PermittedDNSDomains...) s.excludedDNSDomains = append(s.excludedDNSDomains, crt.ExcludedDNSDomains...) @@ -62,7 +64,7 @@ func New(chain ...*x509.Certificate) *service { // Validate checks the given names with the name constraints defined in the // service. -func (s *service) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresses []string, uris []*url.URL) error { +func (s *Service) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresses []string, uris []*url.URL) error { if !s.hasNameConstraints { return nil } diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index 69aa51ad..34e204e0 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -47,12 +47,12 @@ func TestNew(t *testing.T) { tests := []struct { name string args args - want *service + want *Service }{ - {"ok", args{[]*x509.Certificate{ca1.Intermediate, ca1.Root}}, &service{ + {"ok", args{[]*x509.Certificate{ca1.Intermediate, ca1.Root}}, &Service{ hasNameConstraints: false, }}, - {"ok with constraints", args{[]*x509.Certificate{ca2.Intermediate, ca2.Root}}, &service{ + {"ok with constraints", args{[]*x509.Certificate{ca2.Intermediate, ca2.Root}}, &Service{ hasNameConstraints: true, permittedDNSDomains: []string{"internal.example.org"}, excludedDNSDomains: []string{"internal.example.com"}, @@ -205,7 +205,7 @@ func Test_service_Validate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := &service{ + s := &Service{ hasNameConstraints: tt.fields.hasNameConstraints, permittedDNSDomains: tt.fields.permittedDNSDomains, excludedDNSDomains: tt.fields.excludedDNSDomains, From 2a15e3eee1fc0d4a068557ba1569409b82dbd26a Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 11:38:32 -0700 Subject: [PATCH 06/28] Rename constraint.Service to constraint.Engine --- authority/authority.go | 2 ++ authority/internal/constraints/constraints.go | 12 ++++++------ authority/internal/constraints/constraints_test.go | 8 ++++---- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index 73aa9cca..fe00eff2 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -80,6 +80,8 @@ type Authority struct { authorizeRenewFunc provisioner.AuthorizeRenewFunc authorizeSSHRenewFunc provisioner.AuthorizeSSHRenewFunc + // Constraint engine + // Policy engines policyEngine *policy.Engine diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 997b53aa..3a320481 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -22,9 +22,9 @@ func (e ConstraintError) Error() string { return e.Detail } -// Service implements a constraint validator for DNS names, IP addresses, Email +// Engine implements a constraint validator for DNS names, IP addresses, Email // addresses and URIs. -type Service struct { +type Engine struct { hasNameConstraints bool permittedDNSDomains []string excludedDNSDomains []string @@ -36,10 +36,10 @@ type Service struct { excludedURIDomains []string } -// New creates a constraint validation service that contains the given chain of +// New creates a constraint validation engine that contains the given chain of // certificates. -func New(chain ...*x509.Certificate) *Service { - s := new(Service) +func New(chain ...*x509.Certificate) *Engine { + s := new(Engine) for _, crt := range chain { s.permittedDNSDomains = append(s.permittedDNSDomains, crt.PermittedDNSDomains...) s.excludedDNSDomains = append(s.excludedDNSDomains, crt.ExcludedDNSDomains...) @@ -64,7 +64,7 @@ func New(chain ...*x509.Certificate) *Service { // Validate checks the given names with the name constraints defined in the // service. -func (s *Service) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresses []string, uris []*url.URL) error { +func (s *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresses []string, uris []*url.URL) error { if !s.hasNameConstraints { return nil } diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index 34e204e0..cb750c52 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -47,12 +47,12 @@ func TestNew(t *testing.T) { tests := []struct { name string args args - want *Service + want *Engine }{ - {"ok", args{[]*x509.Certificate{ca1.Intermediate, ca1.Root}}, &Service{ + {"ok", args{[]*x509.Certificate{ca1.Intermediate, ca1.Root}}, &Engine{ hasNameConstraints: false, }}, - {"ok with constraints", args{[]*x509.Certificate{ca2.Intermediate, ca2.Root}}, &Service{ + {"ok with constraints", args{[]*x509.Certificate{ca2.Intermediate, ca2.Root}}, &Engine{ hasNameConstraints: true, permittedDNSDomains: []string{"internal.example.org"}, excludedDNSDomains: []string{"internal.example.com"}, @@ -205,7 +205,7 @@ func Test_service_Validate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := &Service{ + s := &Engine{ hasNameConstraints: tt.fields.hasNameConstraints, permittedDNSDomains: tt.fields.permittedDNSDomains, excludedDNSDomains: tt.fields.excludedDNSDomains, From 8b54e25f645af25372ec876ed5a2ec6c858c8e4b Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 12:33:03 -0700 Subject: [PATCH 07/28] Allow nil engines --- authority/internal/constraints/constraints.go | 2 +- authority/internal/constraints/constraints_test.go | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 3a320481..c994c7a6 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -65,7 +65,7 @@ func New(chain ...*x509.Certificate) *Engine { // Validate checks the given names with the name constraints defined in the // service. func (s *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresses []string, uris []*url.URL) error { - if !s.hasNameConstraints { + if s == nil || !s.hasNameConstraints { return nil } diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index cb750c52..f70d39cb 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -222,3 +222,10 @@ func Test_service_Validate(t *testing.T) { }) } } + +func Test_service_Validate_nil(t *testing.T) { + var s *Engine + if err := s.Validate([]string{"www.example.com"}, nil, nil, nil); err != nil { + t.Errorf("service.Validate() error = %v, wantErr false", err) + } +} From 2959aa676de9ddd3614cb0c6cf398fd77fa5d9c1 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 13:12:34 -0700 Subject: [PATCH 08/28] Add helper ValidateCertificate --- authority/internal/constraints/constraints.go | 6 ++ .../internal/constraints/constraints_test.go | 72 +++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index c994c7a6..327379b7 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -115,3 +115,9 @@ func (s *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresse return nil } + +// ValidateCertificate validates the DNS names, IP addresses, Email address and +// URIs present in the given certificate. +func (s *Engine) ValidateCertificate(cert *x509.Certificate) error { + return s.Validate(cert.DNSNames, cert.IPAddresses, cert.EmailAddresses, cert.URIs) +} diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index f70d39cb..f4f909bf 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -229,3 +229,75 @@ func Test_service_Validate_nil(t *testing.T) { t.Errorf("service.Validate() error = %v, wantErr false", err) } } + +func TestEngine_ValidateCertificate(t *testing.T) { + type fields struct { + hasNameConstraints bool + permittedDNSDomains []string + excludedDNSDomains []string + permittedIPRanges []*net.IPNet + excludedIPRanges []*net.IPNet + permittedEmailAddresses []string + excludedEmailAddresses []string + permittedURIDomains []string + excludedURIDomains []string + } + type args struct { + cert *x509.Certificate + } + tests := []struct { + name string + fields fields + args args + wantErr bool + }{ + {"ok", fields{hasNameConstraints: false}, args{&x509.Certificate{ + DNSNames: []string{"example.com"}, + IPAddresses: []net.IP{{127, 0, 0, 1}}, + EmailAddresses: []string{"info@example.com"}, + URIs: []*url.URL{{Scheme: "https", Host: "uuid.example.com", Path: "/dc4c76b5-5262-4551-a881-48094a604d63"}}, + }}, false}, + {"ok with constraints", fields{ + hasNameConstraints: false, + permittedDNSDomains: []string{"example.com"}, + permittedIPRanges: []*net.IPNet{ + {IP: net.ParseIP("127.0.0.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, + {IP: net.ParseIP("10.3.0.0").To4(), Mask: net.IPMask{255, 255, 0, 0}}, + }, + permittedEmailAddresses: []string{"example.com"}, + permittedURIDomains: []string{".example.com"}, + }, args{&x509.Certificate{ + DNSNames: []string{"www.example.com"}, + IPAddresses: []net.IP{{127, 0, 0, 1}, {10, 3, 1, 1}}, + EmailAddresses: []string{"info@example.com"}, + URIs: []*url.URL{{Scheme: "https", Host: "uuid.example.com", Path: "/dc4c76b5-5262-4551-a881-48094a604d63"}}, + }}, false}, + {"fail", fields{ + hasNameConstraints: true, + permittedURIDomains: []string{".example.com"}, + }, args{&x509.Certificate{ + DNSNames: []string{"example.com"}, + IPAddresses: []net.IP{{127, 0, 0, 1}}, + EmailAddresses: []string{"info@example.com"}, + URIs: []*url.URL{{Scheme: "https", Host: "uuid.example.org", Path: "/dc4c76b5-5262-4551-a881-48094a604d63"}}, + }}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s := &Engine{ + hasNameConstraints: tt.fields.hasNameConstraints, + permittedDNSDomains: tt.fields.permittedDNSDomains, + excludedDNSDomains: tt.fields.excludedDNSDomains, + permittedIPRanges: tt.fields.permittedIPRanges, + excludedIPRanges: tt.fields.excludedIPRanges, + permittedEmailAddresses: tt.fields.permittedEmailAddresses, + excludedEmailAddresses: tt.fields.excludedEmailAddresses, + permittedURIDomains: tt.fields.permittedURIDomains, + excludedURIDomains: tt.fields.excludedURIDomains, + } + if err := s.ValidateCertificate(tt.args.cert); (err != nil) != tt.wantErr { + t.Errorf("Engine.ValidateCertificate() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} From 75bff055fcf639e7358dc9743828915b8ece5cec Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 14:45:47 -0700 Subject: [PATCH 09/28] Add StatusCoder to ConstraintError --- authority/internal/constraints/constraints.go | 46 +++++++++++-------- .../internal/constraints/constraints_test.go | 12 ++--- 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 327379b7..29b5be56 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -4,6 +4,7 @@ import ( "crypto/x509" "fmt" "net" + "net/http" "net/url" ) @@ -22,6 +23,11 @@ func (e ConstraintError) Error() string { return e.Detail } +// StatusCode implements an status coder interface. +func (e ConstraintError) StatusCode() int { + return http.StatusForbidden +} + // Engine implements a constraint validator for DNS names, IP addresses, Email // addresses and URIs. type Engine struct { @@ -39,38 +45,38 @@ type Engine struct { // New creates a constraint validation engine that contains the given chain of // certificates. func New(chain ...*x509.Certificate) *Engine { - s := new(Engine) + e := new(Engine) for _, crt := range chain { - s.permittedDNSDomains = append(s.permittedDNSDomains, crt.PermittedDNSDomains...) - s.excludedDNSDomains = append(s.excludedDNSDomains, crt.ExcludedDNSDomains...) - s.permittedIPRanges = append(s.permittedIPRanges, crt.PermittedIPRanges...) - s.excludedIPRanges = append(s.excludedIPRanges, crt.ExcludedIPRanges...) - s.permittedEmailAddresses = append(s.permittedEmailAddresses, crt.PermittedEmailAddresses...) - s.excludedEmailAddresses = append(s.excludedEmailAddresses, crt.ExcludedEmailAddresses...) - s.permittedURIDomains = append(s.permittedURIDomains, crt.PermittedURIDomains...) - s.excludedURIDomains = append(s.excludedURIDomains, crt.ExcludedURIDomains...) + e.permittedDNSDomains = append(e.permittedDNSDomains, crt.PermittedDNSDomains...) + e.excludedDNSDomains = append(e.excludedDNSDomains, crt.ExcludedDNSDomains...) + e.permittedIPRanges = append(e.permittedIPRanges, crt.PermittedIPRanges...) + e.excludedIPRanges = append(e.excludedIPRanges, crt.ExcludedIPRanges...) + e.permittedEmailAddresses = append(e.permittedEmailAddresses, crt.PermittedEmailAddresses...) + e.excludedEmailAddresses = append(e.excludedEmailAddresses, crt.ExcludedEmailAddresses...) + e.permittedURIDomains = append(e.permittedURIDomains, crt.PermittedURIDomains...) + e.excludedURIDomains = append(e.excludedURIDomains, crt.ExcludedURIDomains...) - if !s.hasNameConstraints { + if !e.hasNameConstraints { for _, ext := range crt.Extensions { if ext.Id.Equal(oidExtensionNameConstraints) { - s.hasNameConstraints = true + e.hasNameConstraints = true break } } } } - return s + return e } // Validate checks the given names with the name constraints defined in the // service. -func (s *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresses []string, uris []*url.URL) error { - if s == nil || !s.hasNameConstraints { +func (e *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresses []string, uris []*url.URL) error { + if e == nil || !e.hasNameConstraints { return nil } for _, name := range dnsNames { - if err := checkNameConstraints("DNS name", name, name, s.permittedDNSDomains, s.excludedDNSDomains, + if err := checkNameConstraints("DNS name", name, name, e.permittedDNSDomains, e.excludedDNSDomains, func(parsedName, constraint any) (bool, error) { return matchDomainConstraint(parsedName.(string), constraint.(string)) }, @@ -80,7 +86,7 @@ func (s *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresse } for _, ip := range ipAddresses { - if err := checkNameConstraints("IP address", ip.String(), ip, s.permittedIPRanges, s.excludedIPRanges, + if err := checkNameConstraints("IP address", ip.String(), ip, e.permittedIPRanges, e.excludedIPRanges, func(parsedName, constraint any) (bool, error) { return matchIPConstraint(parsedName.(net.IP), constraint.(*net.IPNet)) }, @@ -94,7 +100,7 @@ func (s *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresse if !ok { return fmt.Errorf("cannot parse rfc822Name %q", email) } - if err := checkNameConstraints("Email address", email, mailbox, s.permittedEmailAddresses, s.excludedEmailAddresses, + if err := checkNameConstraints("Email address", email, mailbox, e.permittedEmailAddresses, e.excludedEmailAddresses, func(parsedName, constraint any) (bool, error) { return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string)) }, @@ -104,7 +110,7 @@ func (s *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresse } for _, uri := range uris { - if err := checkNameConstraints("URI", uri.String(), uri, s.permittedURIDomains, s.excludedURIDomains, + if err := checkNameConstraints("URI", uri.String(), uri, e.permittedURIDomains, e.excludedURIDomains, func(parsedName, constraint any) (bool, error) { return matchURIConstraint(parsedName.(*url.URL), constraint.(string)) }, @@ -118,6 +124,6 @@ func (s *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresse // ValidateCertificate validates the DNS names, IP addresses, Email address and // URIs present in the given certificate. -func (s *Engine) ValidateCertificate(cert *x509.Certificate) error { - return s.Validate(cert.DNSNames, cert.IPAddresses, cert.EmailAddresses, cert.URIs) +func (e *Engine) ValidateCertificate(cert *x509.Certificate) error { + return e.Validate(cert.DNSNames, cert.IPAddresses, cert.EmailAddresses, cert.URIs) } diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index f4f909bf..dff8d043 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -205,7 +205,7 @@ func Test_service_Validate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := &Engine{ + e := &Engine{ hasNameConstraints: tt.fields.hasNameConstraints, permittedDNSDomains: tt.fields.permittedDNSDomains, excludedDNSDomains: tt.fields.excludedDNSDomains, @@ -216,7 +216,7 @@ func Test_service_Validate(t *testing.T) { permittedURIDomains: tt.fields.permittedURIDomains, excludedURIDomains: tt.fields.excludedURIDomains, } - if err := s.Validate(tt.args.dnsNames, tt.args.ipAddresses, tt.args.emailAddresses, tt.args.uris); (err != nil) != tt.wantErr { + if err := e.Validate(tt.args.dnsNames, tt.args.ipAddresses, tt.args.emailAddresses, tt.args.uris); (err != nil) != tt.wantErr { t.Errorf("service.Validate() error = %v, wantErr %v", err, tt.wantErr) } }) @@ -224,8 +224,8 @@ func Test_service_Validate(t *testing.T) { } func Test_service_Validate_nil(t *testing.T) { - var s *Engine - if err := s.Validate([]string{"www.example.com"}, nil, nil, nil); err != nil { + var e *Engine + if err := e.Validate([]string{"www.example.com"}, nil, nil, nil); err != nil { t.Errorf("service.Validate() error = %v, wantErr false", err) } } @@ -284,7 +284,7 @@ func TestEngine_ValidateCertificate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := &Engine{ + e := &Engine{ hasNameConstraints: tt.fields.hasNameConstraints, permittedDNSDomains: tt.fields.permittedDNSDomains, excludedDNSDomains: tt.fields.excludedDNSDomains, @@ -295,7 +295,7 @@ func TestEngine_ValidateCertificate(t *testing.T) { permittedURIDomains: tt.fields.permittedURIDomains, excludedURIDomains: tt.fields.excludedURIDomains, } - if err := s.ValidateCertificate(tt.args.cert); (err != nil) != tt.wantErr { + if err := e.ValidateCertificate(tt.args.cert); (err != nil) != tt.wantErr { t.Errorf("Engine.ValidateCertificate() error = %v, wantErr %v", err, tt.wantErr) } }) From 3f58f30b21c219fdc13e9e20b14e54b27d9d70f8 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 15:53:08 -0700 Subject: [PATCH 10/28] Name tests properly --- authority/internal/constraints/constraints_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index dff8d043..8070e8ad 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -79,8 +79,7 @@ func TestNew(t *testing.T) { } } -func Test_service_Validate(t *testing.T) { - +func TestEngine_Validate(t *testing.T) { type fields struct { hasNameConstraints bool permittedDNSDomains []string @@ -223,7 +222,7 @@ func Test_service_Validate(t *testing.T) { } } -func Test_service_Validate_nil(t *testing.T) { +func TestEngine_Validate_nil(t *testing.T) { var e *Engine if err := e.Validate([]string{"www.example.com"}, nil, nil, nil); err != nil { t.Errorf("service.Validate() error = %v, wantErr false", err) From 89b6aa924a8700c3275edd1173500b63d29b3d07 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 18:44:15 -0700 Subject: [PATCH 11/28] Normalize IPs in matchIPConstraint --- authority/internal/constraints/constraints_test.go | 9 +++++---- authority/internal/constraints/verify.go | 13 +++++++++++-- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index 8070e8ad..d6d13eb4 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -105,7 +105,7 @@ func TestEngine_Validate(t *testing.T) { }{ {"ok", fields{hasNameConstraints: false}, args{ dnsNames: []string{"example.com", "host.example.com"}, - ipAddresses: []net.IP{{192, 168, 1, 1}, {0x26, 0x00, 0x1f, 0x1c, 0x47, 0x1, 0x9d, 0x00, 0xc3, 0xa7, 0x66, 0x94, 0x87, 0x0f, 0x20, 0x72}}, + ipAddresses: []net.IP{{192, 168, 1, 1}, {0x26, 0x00, 0x1f, 0x1c, 0x47, 0x01, 0x9d, 0x00, 0xc3, 0xa7, 0x66, 0x94, 0x87, 0x0f, 0x20, 0x72}}, emailAddresses: []string{"root@example.com"}, uris: []*url.URL{{Scheme: "https", Host: "example.com", Path: "/uuid/c6d1a755-0c12-431e-9136-b64cb3173ec7"}}, }, false}, @@ -120,14 +120,15 @@ func TestEngine_Validate(t *testing.T) { {"ok permitted ip", fields{ hasNameConstraints: true, permittedIPRanges: []*net.IPNet{ - {IP: net.ParseIP("192.168.1.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}, + {IP: net.ParseIP("192.168.1.0"), Mask: net.IPMask{255, 255, 255, 0}}, {IP: net.ParseIP("192.168.2.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, + {IP: net.ParseIP("2600:1700:22f8:2600:e559:bd88:350a:34d6"), Mask: net.IPMask{255, 255, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}}, }, - }, args{ipAddresses: []net.IP{{192, 168, 1, 10}, {192, 168, 2, 1}}}, false}, + }, args{ipAddresses: []net.IP{{192, 168, 1, 10}, {192, 168, 2, 1}, {0x26, 0x0, 0x17, 0x00, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0xa, 0xb, 0xc}}}, false}, {"ok not excluded ip", fields{ hasNameConstraints: true, excludedIPRanges: []*net.IPNet{ - {IP: net.ParseIP("192.168.1.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}, + {IP: net.ParseIP("192.168.1.0"), Mask: net.IPMask{255, 255, 255, 0}}, {IP: net.ParseIP("192.168.2.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, }, }, args{ipAddresses: []net.IP{{192, 168, 2, 2}, {192, 168, 3, 1}}}, false}, diff --git a/authority/internal/constraints/verify.go b/authority/internal/constraints/verify.go index 552c5ea2..120942b6 100644 --- a/authority/internal/constraints/verify.go +++ b/authority/internal/constraints/verify.go @@ -131,13 +131,22 @@ func matchDomainConstraint(domain, constraint string) (bool, error) { return true, nil } +func normalizeIP(ip net.IP) net.IP { + if ip4 := ip.To4(); ip4 != nil { + return ip4 + } + return ip +} + func matchIPConstraint(ip net.IP, constraint *net.IPNet) (bool, error) { - if len(ip) != len(constraint.IP) { + ip = normalizeIP(ip) + constraintIP := normalizeIP(constraint.IP) + if len(ip) != len(constraintIP) { return false, nil } for i := range ip { - if mask := constraint.Mask[i]; ip[i]&mask != constraint.IP[i]&mask { + if mask := constraint.Mask[i]; ip[i]&mask != constraintIP[i]&mask { return false, nil } } From debe565e42e04c15a9b5a91322b6012588c2df3e Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Tue, 20 Sep 2022 18:52:47 -0700 Subject: [PATCH 12/28] Validate constraints on Sign and Renew/Rekey Fixes #1060 --- authority/authority.go | 46 +++++++++++++++++------ authority/options.go | 28 +++++++++++++- authority/tls.go | 9 +++++ authority/tls_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 154 insertions(+), 13 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index fe00eff2..95e58294 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -1,6 +1,7 @@ package authority import ( + "bytes" "context" "crypto" "crypto/sha256" @@ -24,6 +25,7 @@ import ( adminDBNosql "github.com/smallstep/certificates/authority/admin/db/nosql" "github.com/smallstep/certificates/authority/administrator" "github.com/smallstep/certificates/authority/config" + "github.com/smallstep/certificates/authority/internal/constraints" "github.com/smallstep/certificates/authority/policy" "github.com/smallstep/certificates/authority/provisioner" "github.com/smallstep/certificates/cas" @@ -46,14 +48,15 @@ type Authority struct { linkedCAToken string // X509 CA - password []byte - issuerPassword []byte - x509CAService cas.CertificateAuthorityService - rootX509Certs []*x509.Certificate - rootX509CertPool *x509.CertPool - federatedX509Certs []*x509.Certificate - certificates *sync.Map - x509Enforcers []provisioner.CertificateEnforcer + password []byte + issuerPassword []byte + x509CAService cas.CertificateAuthorityService + rootX509Certs []*x509.Certificate + rootX509CertPool *x509.CertPool + federatedX509Certs []*x509.Certificate + intermediateX509Certs []*x509.Certificate + certificates *sync.Map + x509Enforcers []provisioner.CertificateEnforcer // SCEP CA scepService *scep.Service @@ -80,10 +83,9 @@ type Authority struct { authorizeRenewFunc provisioner.AuthorizeRenewFunc authorizeSSHRenewFunc provisioner.AuthorizeSSHRenewFunc - // Constraint engine - - // Policy engines - policyEngine *policy.Engine + // Constraints and Policy engines + constraintsEngine *constraints.Engine + policyEngine *policy.Engine adminMutex sync.RWMutex @@ -375,6 +377,11 @@ func (a *Authority) init() error { if err != nil { return err } + // If not defined with an option, add intermediates to the the list + // of used for constraints purposes. + if len(a.intermediateX509Certs) == 0 { + a.intermediateX509Certs = append(a.intermediateX509Certs, options.CertificateChain...) + } } a.x509CAService, err = cas.New(ctx, options) if err != nil { @@ -612,6 +619,21 @@ func (a *Authority) init() error { return err } + // Load X509 constraints engine. + // + // This is currently only available in CA mode. + if size := len(a.intermediateX509Certs); size > 0 { + last := a.intermediateX509Certs[size-1] + constraintCerts := make([]*x509.Certificate, 0, size+1) + constraintCerts = append(constraintCerts, a.intermediateX509Certs...) + for _, root := range a.rootX509Certs { + if bytes.Equal(last.RawIssuer, root.RawSubject) && bytes.Equal(last.AuthorityKeyId, root.SubjectKeyId) { + constraintCerts = append(constraintCerts, root) + } + } + a.constraintsEngine = constraints.New(constraintCerts...) + } + // Load x509 and SSH Policy Engines if err := a.reloadPolicyEngines(ctx); err != nil { return err diff --git a/authority/options.go b/authority/options.go index 9cef89f0..cc2fc532 100644 --- a/authority/options.go +++ b/authority/options.go @@ -151,16 +151,23 @@ func WithKeyManager(k kms.KeyManager) Option { // WithX509Signer defines the signer used to sign X509 certificates. func WithX509Signer(crt *x509.Certificate, s crypto.Signer) Option { + return WithX509SignerChain([]*x509.Certificate{crt}, s) +} + +// WithX509SignerChain defines the signer used to sign X509 certificates. This +// option is similar to WithX509Signer but it supports a chain of intermediates. +func WithX509SignerChain(issuerChain []*x509.Certificate, s crypto.Signer) Option { return func(a *Authority) error { srv, err := cas.New(context.Background(), casapi.Options{ Type: casapi.SoftCAS, Signer: s, - CertificateChain: []*x509.Certificate{crt}, + CertificateChain: issuerChain, }) if err != nil { return err } a.x509CAService = srv + a.intermediateX509Certs = append(a.intermediateX509Certs, issuerChain...) return nil } } @@ -233,6 +240,25 @@ func WithX509FederatedCerts(certs ...*x509.Certificate) Option { } } +// WithX509RootCerts is an option that allows to define the list of intermediate +// certificates that the CA will be using. This option will replace any +// intermediate certificate defined before. +// +// Note that these certificates will not be bundled with the certificates signed +// by the CA, the CAS service will take care of that, although they should +// match, this is not guaranteed. These certificates will be mainly used for +// constraint purposes. +// +// This option should only used on specific configurations, for example when +// WithX509SignerFunc is used, as we don't know the list of intermediates on +// advance. +func WithX509IntermediateCerts(intermediateCerts ...*x509.Certificate) Option { + return func(a *Authority) error { + a.intermediateX509Certs = intermediateCerts + return nil + } +} + // WithX509RootBundle is an option that allows to define the list of root // certificates. This option will replace any root certificate defined before. func WithX509RootBundle(pemCerts []byte) Option { diff --git a/authority/tls.go b/authority/tls.go index 632ac238..dadf6d99 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -256,6 +256,9 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign // isAllowedToSignX509Certificate checks if the Authority is allowed // to sign the X.509 certificate. func (a *Authority) isAllowedToSignX509Certificate(cert *x509.Certificate) error { + if err := a.constraintsEngine.ValidateCertificate(cert); err != nil { + return err + } return a.policyEngine.IsX509CertificateAllowed(cert) } @@ -351,6 +354,12 @@ func (a *Authority) Rekey(oldCert *x509.Certificate, pk crypto.PublicKey) ([]*x5 newCert.ExtraExtensions = append(newCert.ExtraExtensions, ext) } + // Check if the certificate is allowed to be renewed, policies or + // constraints might change over time. + if err := a.isAllowedToSignX509Certificate(newCert); err != nil { + return nil, err + } + resp, err := a.x509CAService.RenewCertificate(&casapi.RenewCertificateRequest{ Template: newCert, Lifetime: lifetime, diff --git a/authority/tls_test.go b/authority/tls_test.go index a8521b51..b3156a71 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -22,6 +22,7 @@ import ( "go.step.sm/crypto/jose" "go.step.sm/crypto/keyutil" + "go.step.sm/crypto/minica" "go.step.sm/crypto/pemutil" "go.step.sm/crypto/x509util" @@ -1589,3 +1590,86 @@ func TestAuthority_Revoke(t *testing.T) { }) } } + +func TestAuthority_constraints(t *testing.T) { + ca, err := minica.New( + minica.WithIntermediateTemplate(`{ + "subject": {{ toJson .Subject }}, + "keyUsage": ["certSign", "crlSign"], + "basicConstraints": { + "isCA": true, + "maxPathLen": 0 + }, + "nameConstraints": { + "critical": true, + "permittedDNSDomains": ["internal.example.org"], + "excludedDNSDomains": ["internal.example.com"], + "permittedIPRanges": ["192.168.1.0/24", "192.168.2.1/32"], + "excludedIPRanges": ["192.168.3.0/24", "192.168.4.0/28"], + "permittedEmailAddresses": ["root@example.org", "example.org", ".acme.org"], + "excludedEmailAddresses": ["root@example.com", "example.com", ".acme.com"], + "permittedURIDomains": ["uuid.example.org", ".acme.org"], + "excludedURIDomains": ["uuid.example.com", ".acme.com"] + } + }`), + ) + if err != nil { + t.Fatal(err) + } + + auth, err := NewEmbedded(WithX509RootCerts(ca.Root), WithX509Signer(ca.Intermediate, ca.Signer)) + if err != nil { + t.Fatal(err) + } + signer, err := keyutil.GenerateDefaultSigner() + if err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + sans []string + wantErr bool + }{ + {"ok dns", []string{"internal.example.org", "host.internal.example.org"}, false}, + {"ok ip", []string{"192.168.1.10", "192.168.2.1"}, false}, + {"ok email", []string{"root@example.org", "info@example.org", "info@www.acme.org"}, false}, + {"ok uri", []string{"https://uuid.example.org/b908d973-5167-4a62-abe3-6beda358d82a", "https://uuid.acme.org/1724aae1-1bb3-44fb-83c3-9a1a18df67c8"}, false}, + {"fail permitted dns", []string{"internal.acme.org"}, true}, + {"fail excluded dns", []string{"internal.example.com"}, true}, + {"fail permitted ips", []string{"192.168.2.10"}, true}, + {"fail excluded ips", []string{"192.168.3.1"}, true}, + {"fail permitted emails", []string{"root@acme.org"}, true}, + {"fail excluded emails", []string{"root@example.com"}, true}, + {"fail permitted uris", []string{"https://acme.org/uuid/7848819c-9d0b-4e12-bbff-cd66079a3444"}, true}, + {"fail excluded uris", []string{"https://uuid.example.com/d325eda7-6356-4d60-b8f6-3d64724afeb3"}, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + csr, err := x509util.CreateCertificateRequest(tt.sans[0], tt.sans, signer) + if err != nil { + t.Fatal(err) + } + cert, err := ca.SignCSR(csr) + if err != nil { + t.Fatal(err) + } + + data := x509util.CreateTemplateData(tt.sans[0], tt.sans) + templateOption, err := provisioner.TemplateOptions(nil, data) + if err != nil { + t.Fatal(err) + } + + _, err = auth.Sign(csr, provisioner.SignOptions{}, templateOption) + if (err != nil) != tt.wantErr { + t.Errorf("Authority.Sign() error = %v, wantErr %v", err, tt.wantErr) + } + + _, err = auth.Renew(cert) + if (err != nil) != tt.wantErr { + t.Errorf("Authority.Renew() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} From a6e85cbbf608f9f01094dcf7977f1f4396a627ad Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 21 Sep 2022 14:56:15 -0700 Subject: [PATCH 13/28] Fix linter errors --- authority/internal/constraints/verify.go | 28 ++++++++++++------------ 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/authority/internal/constraints/verify.go b/authority/internal/constraints/verify.go index 120942b6..5d070f1e 100644 --- a/authority/internal/constraints/verify.go +++ b/authority/internal/constraints/verify.go @@ -37,7 +37,7 @@ import ( "strings" ) -func checkNameConstraints(nameType string, name string, parsedName any, permitted, excluded any, match func(name, constraint any) (bool, error)) error { +func checkNameConstraints(nameType, name string, parsedName, permitted, excluded any, match func(name, constraint any) (bool, error)) error { excludedValue := reflect.ValueOf(excluded) for i := 0; i < excludedValue.Len(); i++ { constraint := excludedValue.Index(i).Interface() @@ -92,7 +92,7 @@ func checkNameConstraints(nameType string, name string, parsedName any, permitte func matchDomainConstraint(domain, constraint string) (bool, error) { // The meaning of zero length constraints is not specified, but this // code follows NSS and accepts them as matching everything. - if len(constraint) == 0 { + if constraint == "" { return true, nil } @@ -101,10 +101,10 @@ func matchDomainConstraint(domain, constraint string) (bool, error) { return false, fmt.Errorf("internal error: cannot parse domain %q", domain) } - // RFC 5280 says that a leading period in a domain name means that at - // least one label must be prepended, but only for URI and email - // constraints, not DNS constraints. The code also supports that - // behaviour for DNS constraints. + // RFC 5280 says that a leading period in a domain name means that at least + // one label must be prepended, but only for URI and email constraints, not + // DNS constraints. The code also supports that behavior for DNS + // constraints. mustHaveSubdomains := false if constraint[0] == '.' { @@ -180,7 +180,7 @@ func matchURIConstraint(uri *url.URL, constraint string) (bool, error) { // certificate.” host := uri.Host - if len(host) == 0 { + if host == "" { return false, fmt.Errorf("URI with empty host (%q) cannot be matched against constraints", uri.String()) } @@ -213,13 +213,13 @@ func domainToReverseLabels(domain string) (reverseLabels []string, ok bool) { } } - if len(reverseLabels) > 0 && len(reverseLabels[0]) == 0 { + if len(reverseLabels) > 0 && reverseLabels[0] == "" { // An empty label at the end indicates an absolute value. return nil, false } for _, label := range reverseLabels { - if len(label) == 0 { + if label == "" { // Empty labels are otherwise invalid. return nil, false } @@ -247,7 +247,7 @@ type rfc2821Mailbox struct { // Section 4.2.1.6 that's correct for an rfc822Name from a certificate: “The // format of an rfc822Name is a "Mailbox" as defined in RFC 2821, Section 4.1.2”. func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) { - if len(in) == 0 { + if in == "" { return mailbox, false } @@ -267,7 +267,7 @@ func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) { in = in[1:] QuotedString: for { - if len(in) == 0 { + if in == "" { return mailbox, false } c := in[0] @@ -279,7 +279,7 @@ func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) { case c == '\\': // quoted-pair - if len(in) == 0 { + if in == "" { return mailbox, false } if in[0] == 11 || @@ -328,7 +328,7 @@ func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) { // continue to argue the point. We choose to // accept it. in = in[1:] - if len(in) == 0 { + if in == "" { return mailbox, false } fallthrough @@ -365,7 +365,7 @@ func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) { } } - if len(in) == 0 || in[0] != '@' { + if in == "" || in[0] != '@' { return mailbox, false } in = in[1:] From 4b79405dace6bf5e527fda034e2c5ac8e7fd4a82 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 21 Sep 2022 15:54:28 -0700 Subject: [PATCH 14/28] Check constraints and policy for leaf certificates too --- authority/tls.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/authority/tls.go b/authority/tls.go index f9ab705b..5f0bdd26 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -630,6 +630,18 @@ func (a *Authority) GetTLSCertificate() (*tls.Certificate, error) { certTpl.NotBefore = now.Add(-1 * time.Minute) certTpl.NotAfter = now.Add(24 * time.Hour) + // Policy and constraints require this fields to be set. At this moment they + // are only present in the extra extension. + certTpl.DNSNames = cr.DNSNames + certTpl.IPAddresses = cr.IPAddresses + certTpl.EmailAddresses = cr.EmailAddresses + certTpl.URIs = cr.URIs + + // Fail if name constraints or policy does not allow the server names. + if err := a.isAllowedToSignX509Certificate(certTpl); err != nil { + return fatal(err) + } + resp, err := a.x509CAService.CreateCertificate(&casapi.CreateCertificateRequest{ Template: certTpl, CSR: cr, From 72e2c4eb2e7a943a59b8de401bf678ed6043e3da Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 21 Sep 2022 18:35:18 -0700 Subject: [PATCH 15/28] Render proper policy and constrains errors --- authority/internal/constraints/constraints.go | 17 +++++++++++--- authority/ssh.go | 14 +++--------- authority/tls.go | 22 +++++++++---------- policy/engine.go | 20 ++++++++++++++++- 4 files changed, 47 insertions(+), 26 deletions(-) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 29b5be56..2755e102 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -6,6 +6,8 @@ import ( "net" "net/http" "net/url" + + "github.com/smallstep/certificates/errs" ) var oidExtensionNameConstraints = []int{2, 5, 29, 30} @@ -23,9 +25,18 @@ func (e ConstraintError) Error() string { return e.Detail } -// StatusCode implements an status coder interface. -func (e ConstraintError) StatusCode() int { - return http.StatusForbidden +// As implements the As(any) bool interface and allows to use "errors.As()" to +// convert the ConstraintError to an errs.Error. +func (e ConstraintError) As(v any) bool { + if err, ok := v.(**errs.Error); ok { + *err = &errs.Error{ + Status: http.StatusForbidden, + Msg: e.Detail, + Err: e, + } + return true + } + return false } // Engine implements a constraint validator for DNS names, IP addresses, Email diff --git a/authority/ssh.go b/authority/ssh.go index 55861ce7..1b243b39 100644 --- a/authority/ssh.go +++ b/authority/ssh.go @@ -6,7 +6,6 @@ import ( "crypto/x509" "encoding/binary" "errors" - "fmt" "net/http" "strings" "time" @@ -20,7 +19,6 @@ import ( "github.com/smallstep/certificates/authority/provisioner" "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" - policy "github.com/smallstep/certificates/policy" "github.com/smallstep/certificates/templates" ) @@ -255,15 +253,9 @@ func (a *Authority) SignSSH(ctx context.Context, key ssh.PublicKey, opts provisi // Check if authority is allowed to sign the certificate if err := a.isAllowedToSignSSHCertificate(certTpl); err != nil { - var pe *policy.NamePolicyError - if errors.As(err, &pe) && pe.Reason == policy.NotAllowed { - return nil, &errs.Error{ - // NOTE: custom forbidden error, so that denied name is sent to client - // as well as shown in the logs. - Status: http.StatusForbidden, - Err: fmt.Errorf("authority not allowed to sign: %w", err), - Msg: fmt.Sprintf("The request was forbidden by the certificate authority: %s", err.Error()), - } + var ee *errs.Error + if errors.As(err, &ee) { + return nil, ee } return nil, errs.InternalServerErr(err, errs.WithMessage("authority.SignSSH: error creating ssh certificate"), diff --git a/authority/tls.go b/authority/tls.go index 5f0bdd26..4f5570b7 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -28,7 +28,6 @@ import ( casapi "github.com/smallstep/certificates/cas/apiv1" "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" - "github.com/smallstep/certificates/policy" ) // GetTLSOptions returns the tls options configured. @@ -213,15 +212,9 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign // Check if authority is allowed to sign the certificate if err := a.isAllowedToSignX509Certificate(leaf); err != nil { - var pe *policy.NamePolicyError - if errors.As(err, &pe) && pe.Reason == policy.NotAllowed { - return nil, errs.ApplyOptions(&errs.Error{ - // NOTE: custom forbidden error, so that denied name is sent to client - // as well as shown in the logs. - Status: http.StatusForbidden, - Err: fmt.Errorf("authority not allowed to sign: %w", err), - Msg: fmt.Sprintf("The request was forbidden by the certificate authority: %s", err.Error()), - }, opts...) + var ee *errs.Error + if errors.As(err, &ee) { + return nil, ee } return nil, errs.InternalServerErr(err, errs.WithKeyVal("csr", csr), @@ -358,7 +351,14 @@ func (a *Authority) Rekey(oldCert *x509.Certificate, pk crypto.PublicKey) ([]*x5 // Check if the certificate is allowed to be renewed, policies or // constraints might change over time. if err := a.isAllowedToSignX509Certificate(newCert); err != nil { - return nil, err + var ee *errs.Error + if errors.As(err, &ee) { + return nil, ee + } + return nil, errs.InternalServerErr(err, + errs.WithKeyVal("serialNumber", oldCert.SerialNumber.String()), + errs.WithMessage("error renewing certificate"), + ) } resp, err := a.x509CAService.RenewCertificate(&casapi.RenewCertificateRequest{ diff --git a/policy/engine.go b/policy/engine.go index 4a0baa8f..c02fd7a9 100755 --- a/policy/engine.go +++ b/policy/engine.go @@ -4,11 +4,13 @@ import ( "crypto/x509" "fmt" "net" + "net/http" "net/url" + "go.step.sm/crypto/x509util" "golang.org/x/crypto/ssh" - "go.step.sm/crypto/x509util" + "github.com/smallstep/certificates/errs" ) type NamePolicyReason int @@ -62,6 +64,22 @@ func (e *NamePolicyError) Error() string { } } +// As implements the As(any) bool interface and allows to use "errors.As()" to +// convert a NotAllowed NamePolicyError to an errs.Error. +func (e *NamePolicyError) As(v any) bool { + if e.Reason == NotAllowed { + if err, ok := v.(**errs.Error); ok { + *err = &errs.Error{ + Status: http.StatusForbidden, + Msg: fmt.Sprintf("The request was forbidden by the certificate authority: %s", e.Error()), + Err: e, + } + return true + } + } + return false +} + func (e *NamePolicyError) Detail() string { return e.detail } From d68c765e20565217aa69fbb53458e4181d862d79 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 21 Sep 2022 18:46:34 -0700 Subject: [PATCH 16/28] Add context to errors --- authority/tls.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index 4f5570b7..32b85e11 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -214,7 +214,7 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign if err := a.isAllowedToSignX509Certificate(leaf); err != nil { var ee *errs.Error if errors.As(err, &ee) { - return nil, ee + return nil, errs.ApplyOptions(ee, opts...) } return nil, errs.InternalServerErr(err, errs.WithKeyVal("csr", csr), @@ -353,7 +353,7 @@ func (a *Authority) Rekey(oldCert *x509.Certificate, pk crypto.PublicKey) ([]*x5 if err := a.isAllowedToSignX509Certificate(newCert); err != nil { var ee *errs.Error if errors.As(err, &ee) { - return nil, ee + return nil, errs.ApplyOptions(ee, opts...) } return nil, errs.InternalServerErr(err, errs.WithKeyVal("serialNumber", oldCert.SerialNumber.String()), From 15dc7901e515ed1535b3912eadd6d6efb19a130f Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Wed, 21 Sep 2022 18:46:46 -0700 Subject: [PATCH 17/28] Fix unit tests --- authority/tls_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/tls_test.go b/authority/tls_test.go index f5f767db..e8a80760 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -543,7 +543,7 @@ ZYtQ9Ot36qc= notBefore: signOpts.NotBefore.Time().Truncate(time.Second), notAfter: signOpts.NotAfter.Time().Truncate(time.Second), extensionsCount: 6, - err: errors.New("authority not allowed to sign"), + err: errors.New("dns name \"test.smallstep.com\" not allowed"), code: http.StatusForbidden, } }, From 23045e1812ce402e1847cbb01bc9cd0c08192646 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 22 Sep 2022 11:05:06 -0700 Subject: [PATCH 18/28] Clarify comments by code review --- authority/authority.go | 3 ++- authority/options.go | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index 87600d1a..e71461e1 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -378,7 +378,8 @@ func (a *Authority) init() error { return err } // If not defined with an option, add intermediates to the the list - // of used for constraints purposes. + // of certificates used for name constraints validation at issuance + // time. if len(a.intermediateX509Certs) == 0 { a.intermediateX509Certs = append(a.intermediateX509Certs, options.CertificateChain...) } diff --git a/authority/options.go b/authority/options.go index cc2fc532..09aaac84 100644 --- a/authority/options.go +++ b/authority/options.go @@ -240,16 +240,16 @@ func WithX509FederatedCerts(certs ...*x509.Certificate) Option { } } -// WithX509RootCerts is an option that allows to define the list of intermediate -// certificates that the CA will be using. This option will replace any -// intermediate certificate defined before. +// WithX509IntermediateCerts is an option that allows to define the list of +// intermediate certificates that the CA will be using. This option will replace +// any intermediate certificate defined before. // // Note that these certificates will not be bundled with the certificates signed // by the CA, the CAS service will take care of that, although they should // match, this is not guaranteed. These certificates will be mainly used for // constraint purposes. // -// This option should only used on specific configurations, for example when +// This option should only be used on specific configurations, for example when // WithX509SignerFunc is used, as we don't know the list of intermediates on // advance. func WithX509IntermediateCerts(intermediateCerts ...*x509.Certificate) Option { From 0214e015a0adf247f7c3f96047ad9f12822051da Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 22 Sep 2022 11:07:22 -0700 Subject: [PATCH 19/28] Clarify comments by code review --- authority/options.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/authority/options.go b/authority/options.go index 09aaac84..c5ee0cd5 100644 --- a/authority/options.go +++ b/authority/options.go @@ -245,9 +245,9 @@ func WithX509FederatedCerts(certs ...*x509.Certificate) Option { // any intermediate certificate defined before. // // Note that these certificates will not be bundled with the certificates signed -// by the CA, the CAS service will take care of that, although they should -// match, this is not guaranteed. These certificates will be mainly used for -// constraint purposes. +// by the CA, because the CAS service will take care of that. They should match, +// but that's not guaranteed. These certificates will be mainly used for name +// constraint validation before a certificate is issued. // // This option should only be used on specific configurations, for example when // WithX509SignerFunc is used, as we don't know the list of intermediates on From b94c0d09bec050b0c1ab581dc7444dbec181cda9 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 22 Sep 2022 11:07:28 -0700 Subject: [PATCH 20/28] Set up test properly --- authority/internal/constraints/constraints_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index d6d13eb4..f65e1768 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -258,7 +258,7 @@ func TestEngine_ValidateCertificate(t *testing.T) { URIs: []*url.URL{{Scheme: "https", Host: "uuid.example.com", Path: "/dc4c76b5-5262-4551-a881-48094a604d63"}}, }}, false}, {"ok with constraints", fields{ - hasNameConstraints: false, + hasNameConstraints: true, permittedDNSDomains: []string{"example.com"}, permittedIPRanges: []*net.IPNet{ {IP: net.ParseIP("127.0.0.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, From 246566a1951f0afbd750a09116942d7030eff0bb Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 22 Sep 2022 11:35:11 -0700 Subject: [PATCH 21/28] Change way to get hasNameConstraints --- authority/internal/constraints/constraints.go | 15 ++++----- .../internal/constraints/constraints_test.go | 31 +++++++++++++++++++ 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 2755e102..9aa6d9c3 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -66,16 +66,13 @@ func New(chain ...*x509.Certificate) *Engine { e.excludedEmailAddresses = append(e.excludedEmailAddresses, crt.ExcludedEmailAddresses...) e.permittedURIDomains = append(e.permittedURIDomains, crt.PermittedURIDomains...) e.excludedURIDomains = append(e.excludedURIDomains, crt.ExcludedURIDomains...) - - if !e.hasNameConstraints { - for _, ext := range crt.Extensions { - if ext.Id.Equal(oidExtensionNameConstraints) { - e.hasNameConstraints = true - break - } - } - } } + + e.hasNameConstraints = len(e.permittedDNSDomains) > 0 || len(e.excludedDNSDomains) > 0 || + len(e.permittedIPRanges) > 0 || len(e.excludedIPRanges) > 0 || + len(e.permittedEmailAddresses) > 0 || len(e.excludedEmailAddresses) > 0 || + len(e.permittedURIDomains) > 0 || len(e.excludedURIDomains) > 0 + return e } diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index f65e1768..aaf5d410 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -79,6 +79,37 @@ func TestNew(t *testing.T) { } } +func TestNew_hasNameConstraints(t *testing.T) { + tests := []struct { + name string + fn func(c *x509.Certificate) + want bool + }{ + {"no constraints", func(c *x509.Certificate) {}, false}, + {"permittedDNSDomains", func(c *x509.Certificate) { c.PermittedDNSDomains = []string{"constraint"} }, true}, + {"excludedDNSDomains", func(c *x509.Certificate) { c.ExcludedDNSDomains = []string{"constraint"} }, true}, + {"permittedIPRanges", func(c *x509.Certificate) { + c.PermittedIPRanges = []*net.IPNet{{IP: net.ParseIP("192.168.3.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}} + }, true}, + {"excludedIPRanges", func(c *x509.Certificate) { + c.ExcludedIPRanges = []*net.IPNet{{IP: net.ParseIP("192.168.3.0").To4(), Mask: net.IPMask{255, 255, 255, 0}}} + }, true}, + {"permittedEmailAddresses", func(c *x509.Certificate) { c.PermittedEmailAddresses = []string{"constraint"} }, true}, + {"excludedEmailAddresses", func(c *x509.Certificate) { c.ExcludedEmailAddresses = []string{"constraint"} }, true}, + {"permittedURIDomains", func(c *x509.Certificate) { c.PermittedURIDomains = []string{"constraint"} }, true}, + {"excludedURIDomains", func(c *x509.Certificate) { c.ExcludedURIDomains = []string{"constraint"} }, true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cert := &x509.Certificate{} + tt.fn(cert) + if e := New(cert); e.hasNameConstraints != tt.want { + t.Errorf("Engine.hasNameConstraints = %v, want %v", e.hasNameConstraints, tt.want) + } + }) + } +} + func TestEngine_Validate(t *testing.T) { type fields struct { hasNameConstraints bool From ccd93684c3763879412d285be1a6dbb5be79ee34 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 22 Sep 2022 11:54:21 -0700 Subject: [PATCH 22/28] Remove unused variable --- authority/internal/constraints/constraints.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 9aa6d9c3..7d39d540 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -10,8 +10,6 @@ import ( "github.com/smallstep/certificates/errs" ) -var oidExtensionNameConstraints = []int{2, 5, 29, 30} - // ConstraintError is the typed error that will be returned if a constraint // error is found. type ConstraintError struct { From 2eba5326db6d427cd9c0bc7338a0a21a0309df94 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Thu, 22 Sep 2022 12:17:16 -0700 Subject: [PATCH 23/28] Remove policy validation on renew --- authority/tls.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index 32b85e11..29053ddf 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -348,9 +348,12 @@ func (a *Authority) Rekey(oldCert *x509.Certificate, pk crypto.PublicKey) ([]*x5 newCert.ExtraExtensions = append(newCert.ExtraExtensions, ext) } - // Check if the certificate is allowed to be renewed, policies or - // constraints might change over time. - if err := a.isAllowedToSignX509Certificate(newCert); err != nil { + // Check if the certificate is allowed to be renewed, name constraints might + // change over time. + // + // TODO(hslatman,maraino): consider adding policies too and consider if + // RenewSSH should check policies. + if err := a.constraintsEngine.ValidateCertificate(newCert); err != nil { var ee *errs.Error if errors.As(err, &ee) { return nil, errs.ApplyOptions(ee, opts...) From 965d59c0a856be6d30245cb40f4cd3631850b3a9 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 23 Sep 2022 10:50:44 -0700 Subject: [PATCH 24/28] Fix comment typos and extra white spaces --- authority/authority.go | 4 ++-- authority/internal/constraints/constraints.go | 4 ++-- authority/internal/constraints/constraints_test.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index e71461e1..7d527b13 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -377,8 +377,8 @@ func (a *Authority) init() error { if err != nil { return err } - // If not defined with an option, add intermediates to the the list - // of certificates used for name constraints validation at issuance + // If not defined with an option, add intermediates to the list of + // certificates used for name constraints validation at issuance // time. if len(a.intermediateX509Certs) == 0 { a.intermediateX509Certs = append(a.intermediateX509Certs, options.CertificateChain...) diff --git a/authority/internal/constraints/constraints.go b/authority/internal/constraints/constraints.go index 7d39d540..a1cbde7e 100644 --- a/authority/internal/constraints/constraints.go +++ b/authority/internal/constraints/constraints.go @@ -128,8 +128,8 @@ func (e *Engine) Validate(dnsNames []string, ipAddresses []net.IP, emailAddresse return nil } -// ValidateCertificate validates the DNS names, IP addresses, Email address and -// URIs present in the given certificate. +// ValidateCertificate validates the DNS names, IP addresses, Email addresses +// and URIs present in the given certificate. func (e *Engine) ValidateCertificate(cert *x509.Certificate) error { return e.Validate(cert.DNSNames, cert.IPAddresses, cert.EmailAddresses, cert.URIs) } diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index aaf5d410..d2ad8f94 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -140,7 +140,7 @@ func TestEngine_Validate(t *testing.T) { emailAddresses: []string{"root@example.com"}, uris: []*url.URL{{Scheme: "https", Host: "example.com", Path: "/uuid/c6d1a755-0c12-431e-9136-b64cb3173ec7"}}, }, false}, - {"ok permitted dns ", fields{ + {"ok permitted dns", fields{ hasNameConstraints: true, permittedDNSDomains: []string{"example.com"}, }, args{dnsNames: []string{"example.com", "www.example.com"}}, false}, @@ -163,7 +163,7 @@ func TestEngine_Validate(t *testing.T) { {IP: net.ParseIP("192.168.2.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, }, }, args{ipAddresses: []net.IP{{192, 168, 2, 2}, {192, 168, 3, 1}}}, false}, - {"ok permitted emails ", fields{ + {"ok permitted emails", fields{ hasNameConstraints: true, permittedEmailAddresses: []string{"root@example.com", "acme.org", ".acme.com"}, }, args{emailAddresses: []string{"root@example.com", "name@acme.org", "name@coyote.acme.com", `"(quoted)"@www.acme.com`}}, false}, From 8374c0d26e3abd38edfd75440e23bd720475c7ec Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 23 Sep 2022 10:52:24 -0700 Subject: [PATCH 25/28] Fix some more extra white spaces --- authority/internal/constraints/constraints_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/authority/internal/constraints/constraints_test.go b/authority/internal/constraints/constraints_test.go index d2ad8f94..0f6d0ef1 100644 --- a/authority/internal/constraints/constraints_test.go +++ b/authority/internal/constraints/constraints_test.go @@ -171,7 +171,7 @@ func TestEngine_Validate(t *testing.T) { hasNameConstraints: true, excludedEmailAddresses: []string{"root@example.com", "acme.org", ".acme.com"}, }, args{emailAddresses: []string{"name@example.com", "root@acme.com", "root@other.com"}}, false}, - {"ok permitted uris ", fields{ + {"ok permitted uris", fields{ hasNameConstraints: true, permittedURIDomains: []string{"example.com", ".acme.com"}, }, args{uris: []*url.URL{{Scheme: "https", Host: "example.com", Path: "/path"}, {Scheme: "https", Host: "www.acme.com", Path: "/path"}}}, false}, @@ -179,7 +179,7 @@ func TestEngine_Validate(t *testing.T) { hasNameConstraints: true, excludedURIDomains: []string{"example.com", ".acme.com"}, }, args{uris: []*url.URL{{Scheme: "https", Host: "example.org", Path: "/path"}, {Scheme: "https", Host: "acme.com", Path: "/path"}}}, false}, - {"fail permitted dns ", fields{ + {"fail permitted dns", fields{ hasNameConstraints: true, permittedDNSDomains: []string{"example.com"}, }, args{dnsNames: []string{"www.example.com", "www.example.org"}}, true}, @@ -201,7 +201,7 @@ func TestEngine_Validate(t *testing.T) { {IP: net.ParseIP("192.168.2.1").To4(), Mask: net.IPMask{255, 255, 255, 255}}, }, }, args{ipAddresses: []net.IP{{192, 168, 2, 2}, {192, 168, 1, 1}}}, true}, - {"fail permitted emails ", fields{ + {"fail permitted emails", fields{ hasNameConstraints: true, permittedEmailAddresses: []string{"root@example.com", "acme.org", ".acme.com"}, }, args{emailAddresses: []string{"root@example.com", "name@acme.org", "name@acme.com"}}, true}, @@ -209,7 +209,7 @@ func TestEngine_Validate(t *testing.T) { hasNameConstraints: true, excludedEmailAddresses: []string{"root@example.com", "acme.org", ".acme.com"}, }, args{emailAddresses: []string{"name@example.com", "root@example.com"}}, true}, - {"fail permitted uris ", fields{ + {"fail permitted uris", fields{ hasNameConstraints: true, permittedURIDomains: []string{"example.com", ".acme.com"}, }, args{uris: []*url.URL{{Scheme: "https", Host: "example.com", Path: "/path"}, {Scheme: "https", Host: "acme.com", Path: "/path"}}}, true}, @@ -217,7 +217,7 @@ func TestEngine_Validate(t *testing.T) { hasNameConstraints: true, excludedURIDomains: []string{"example.com", ".acme.com"}, }, args{uris: []*url.URL{{Scheme: "https", Host: "www.example.com", Path: "/path"}, {Scheme: "https", Host: "acme.com", Path: "/path"}}}, true}, - {"fail parse emails ", fields{ + {"fail parse emails", fields{ hasNameConstraints: true, permittedEmailAddresses: []string{"example.com"}, }, args{emailAddresses: []string{`(notquoted)@example.com`}}, true}, From 0bedd228505fbbcac3776ccfc99beaf24de42d4c Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 23 Sep 2022 10:55:20 -0700 Subject: [PATCH 26/28] Fix typos in WithX509IntermediateCerts comment --- authority/options.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/options.go b/authority/options.go index c5ee0cd5..8e1a01ff 100644 --- a/authority/options.go +++ b/authority/options.go @@ -246,11 +246,11 @@ func WithX509FederatedCerts(certs ...*x509.Certificate) Option { // // Note that these certificates will not be bundled with the certificates signed // by the CA, because the CAS service will take care of that. They should match, -// but that's not guaranteed. These certificates will be mainly used for name +// but that's not guaranteed. These certificates will be mainly used for name // constraint validation before a certificate is issued. // // This option should only be used on specific configurations, for example when -// WithX509SignerFunc is used, as we don't know the list of intermediates on +// WithX509SignerFunc is used, as we don't know the list of intermediates in // advance. func WithX509IntermediateCerts(intermediateCerts ...*x509.Certificate) Option { return func(a *Authority) error { From c9e7af37226e45fc2e655cce2a40ee8e57ed2a09 Mon Sep 17 00:00:00 2001 From: Mariano Cano Date: Fri, 23 Sep 2022 11:04:27 -0700 Subject: [PATCH 27/28] Use only name constraints in GetTLSCertificate --- authority/tls.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authority/tls.go b/authority/tls.go index 29053ddf..efabc8f2 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -640,8 +640,8 @@ func (a *Authority) GetTLSCertificate() (*tls.Certificate, error) { certTpl.EmailAddresses = cr.EmailAddresses certTpl.URIs = cr.URIs - // Fail if name constraints or policy does not allow the server names. - if err := a.isAllowedToSignX509Certificate(certTpl); err != nil { + // Fail if name constraints do not allow the server names. + if err := a.constraintsEngine.ValidateCertificate(certTpl); err != nil { return fatal(err) }