diff --git a/api/read/read_test.go b/api/read/read_test.go index 8696ba78..72100584 100644 --- a/api/read/read_test.go +++ b/api/read/read_test.go @@ -4,7 +4,6 @@ import ( "encoding/json" "errors" "io" - "io/ioutil" "net/http" "net/http/httptest" "reflect" @@ -146,7 +145,7 @@ func Test_badProtoJSONError_Render(t *testing.T) { res := w.Result() defer res.Body.Close() - data, err := ioutil.ReadAll(res.Body) + data, err := io.ReadAll(res.Body) assert.NoError(t, err) v := struct { diff --git a/authority/admin/api/policy_test.go b/authority/admin/api/policy_test.go index d0c97729..b5987104 100644 --- a/authority/admin/api/policy_test.go +++ b/authority/admin/api/policy_test.go @@ -312,7 +312,6 @@ func TestPolicyAdminResponder_CreateAuthorityPolicy(t *testing.T) { Allow: &linkedca.X509Names{ Dns: []string{"*.local"}, }, - DisableSubjectCommonNameVerification: false, }, } body, err := protojson.Marshal(policy) @@ -1030,7 +1029,6 @@ func TestPolicyAdminResponder_CreateProvisionerPolicy(t *testing.T) { Allow: &linkedca.X509Names{ Dns: []string{"*.local"}, }, - DisableSubjectCommonNameVerification: false, }, } body, err := protojson.Marshal(policy) diff --git a/authority/policy.go b/authority/policy.go index 47104e0e..dd38fd4a 100644 --- a/authority/policy.go +++ b/authority/policy.go @@ -288,6 +288,9 @@ func policyToCertificates(p *linkedca.Policy) *authPolicy.Options { if allow.Uris != nil { opts.X509.AllowedNames.URIDomains = allow.Uris } + if allow.CommonNames != nil { + opts.X509.AllowedNames.CommonNames = allow.CommonNames + } } if deny := x509.GetDeny(); deny != nil { opts.X509.DeniedNames = &authPolicy.X509NameOptions{} @@ -303,10 +306,12 @@ func policyToCertificates(p *linkedca.Policy) *authPolicy.Options { if deny.Uris != nil { opts.X509.DeniedNames.URIDomains = deny.Uris } + if deny.CommonNames != nil { + opts.X509.DeniedNames.CommonNames = deny.CommonNames + } } opts.X509.AllowWildcardLiteral = x509.AllowWildcardLiteral - opts.X509.DisableCommonNameVerification = x509.DisableSubjectCommonNameVerification } // fill ssh policy configuration diff --git a/authority/policy/options.go b/authority/policy/options.go index c4b7b9ce..0f50083d 100644 --- a/authority/policy/options.go +++ b/authority/policy/options.go @@ -31,7 +31,6 @@ type X509PolicyOptionsInterface interface { GetAllowedNameOptions() *X509NameOptions GetDeniedNameOptions() *X509NameOptions IsWildcardLiteralAllowed() bool - ShouldVerifyCommonName() bool } // X509PolicyOptions is a container for x509 allowed and denied @@ -47,15 +46,11 @@ type X509PolicyOptions struct { // such as *.example.com and @example.com are allowed. Defaults // to false. AllowWildcardLiteral bool `json:"allowWildcardLiteral,omitempty"` - - // DisableCommonNameVerification indicates if the Subject Common Name - // is verified in addition to the SANs. Defaults to false, resulting in - // Common Names being verified. - DisableCommonNameVerification bool `json:"disableCommonNameVerification,omitempty"` } // X509NameOptions models the X509 name policy configuration. type X509NameOptions struct { + CommonNames []string `json:"cn,omitempty"` DNSDomains []string `json:"dns,omitempty"` IPRanges []string `json:"ip,omitempty"` EmailAddresses []string `json:"email,omitempty"` @@ -65,7 +60,8 @@ type X509NameOptions struct { // HasNames checks if the AllowedNameOptions has one or more // names configured. func (o *X509NameOptions) HasNames() bool { - return len(o.DNSDomains) > 0 || + return len(o.CommonNames) > 0 || + len(o.DNSDomains) > 0 || len(o.IPRanges) > 0 || len(o.EmailAddresses) > 0 || len(o.URIDomains) > 0 @@ -96,15 +92,6 @@ func (o *X509PolicyOptions) IsWildcardLiteralAllowed() bool { return o.AllowWildcardLiteral } -// ShouldVerifyCommonName returns whether the authority -// should verify the Subject Common Name in addition to the SANs. -func (o *X509PolicyOptions) ShouldVerifyCommonName() bool { - if o == nil { - return false - } - return !o.DisableCommonNameVerification -} - // SSHPolicyOptionsInterface is an interface for providers of // SSH user and host name policy configuration. type SSHPolicyOptionsInterface interface { diff --git a/authority/policy/options_test.go b/authority/policy/options_test.go index b4f456a1..d7d42093 100644 --- a/authority/policy/options_test.go +++ b/authority/policy/options_test.go @@ -43,43 +43,3 @@ func TestX509PolicyOptions_IsWildcardLiteralAllowed(t *testing.T) { }) } } - -func TestX509PolicyOptions_ShouldVerifySubjectCommonName(t *testing.T) { - tests := []struct { - name string - options *X509PolicyOptions - want bool - }{ - { - name: "nil-options", - options: nil, - want: false, - }, - { - name: "not-set", - options: &X509PolicyOptions{}, - want: true, - }, - { - name: "set-true", - options: &X509PolicyOptions{ - DisableCommonNameVerification: true, - }, - want: false, - }, - { - name: "set-false", - options: &X509PolicyOptions{ - DisableCommonNameVerification: false, - }, - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := tt.options.ShouldVerifyCommonName(); got != tt.want { - t.Errorf("X509PolicyOptions.ShouldVerifySubjectCommonName() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/authority/policy/policy.go b/authority/policy/policy.go index b68bcb19..f5f0fce3 100644 --- a/authority/policy/policy.go +++ b/authority/policy/policy.go @@ -28,6 +28,7 @@ func NewX509PolicyEngine(policyOptions X509PolicyOptionsInterface) (X509Policy, allowed := policyOptions.GetAllowedNameOptions() if allowed != nil && allowed.HasNames() { options = append(options, + policy.WithPermittedCommonNames(allowed.CommonNames...), policy.WithPermittedDNSDomains(allowed.DNSDomains...), policy.WithPermittedIPsOrCIDRs(allowed.IPRanges...), policy.WithPermittedEmailAddresses(allowed.EmailAddresses...), @@ -38,6 +39,7 @@ func NewX509PolicyEngine(policyOptions X509PolicyOptionsInterface) (X509Policy, denied := policyOptions.GetDeniedNameOptions() if denied != nil && denied.HasNames() { options = append(options, + policy.WithExcludedCommonNames(denied.CommonNames...), policy.WithExcludedDNSDomains(denied.DNSDomains...), policy.WithExcludedIPsOrCIDRs(denied.IPRanges...), policy.WithExcludedEmailAddresses(denied.EmailAddresses...), @@ -50,10 +52,6 @@ func NewX509PolicyEngine(policyOptions X509PolicyOptionsInterface) (X509Policy, return nil, nil } - if policyOptions.ShouldVerifyCommonName() { - options = append(options, policy.WithSubjectCommonNameVerification()) - } - if policyOptions.IsWildcardLiteralAllowed() { options = append(options, policy.WithAllowLiteralWildcardNames()) } diff --git a/authority/policy_test.go b/authority/policy_test.go index 40af879a..3f03abd9 100644 --- a/authority/policy_test.go +++ b/authority/policy_test.go @@ -218,8 +218,7 @@ func Test_policyToCertificates(t *testing.T) { Allow: &linkedca.X509Names{ Dns: []string{"*.local"}, }, - AllowWildcardLiteral: false, - DisableSubjectCommonNameVerification: false, + AllowWildcardLiteral: false, }, }, want: &policy.Options{ @@ -227,8 +226,7 @@ func Test_policyToCertificates(t *testing.T) { AllowedNames: &policy.X509NameOptions{ DNSDomains: []string{"*.local"}, }, - AllowWildcardLiteral: false, - DisableCommonNameVerification: false, + AllowWildcardLiteral: false, }, }, }, @@ -237,19 +235,20 @@ func Test_policyToCertificates(t *testing.T) { policy: &linkedca.Policy{ X509: &linkedca.X509Policy{ Allow: &linkedca.X509Names{ - Dns: []string{"step"}, - Ips: []string{"127.0.0.1/24"}, - Emails: []string{"*.example.com"}, - Uris: []string{"https://*.local"}, + Dns: []string{"step"}, + Ips: []string{"127.0.0.1/24"}, + Emails: []string{"*.example.com"}, + Uris: []string{"https://*.local"}, + CommonNames: []string{"some name"}, }, Deny: &linkedca.X509Names{ - Dns: []string{"bad"}, - Ips: []string{"127.0.0.30"}, - Emails: []string{"badhost.example.com"}, - Uris: []string{"https://badhost.local"}, + Dns: []string{"bad"}, + Ips: []string{"127.0.0.30"}, + Emails: []string{"badhost.example.com"}, + Uris: []string{"https://badhost.local"}, + CommonNames: []string{"another name"}, }, - AllowWildcardLiteral: true, - DisableSubjectCommonNameVerification: false, + AllowWildcardLiteral: true, }, Ssh: &linkedca.SSHPolicy{ Host: &linkedca.SSHHostPolicy{ @@ -283,15 +282,16 @@ func Test_policyToCertificates(t *testing.T) { IPRanges: []string{"127.0.0.1/24"}, EmailAddresses: []string{"*.example.com"}, URIDomains: []string{"https://*.local"}, + CommonNames: []string{"some name"}, }, DeniedNames: &policy.X509NameOptions{ DNSDomains: []string{"bad"}, IPRanges: []string{"127.0.0.30"}, EmailAddresses: []string{"badhost.example.com"}, URIDomains: []string{"https://badhost.local"}, + CommonNames: []string{"another name"}, }, - AllowWildcardLiteral: true, - DisableCommonNameVerification: false, + AllowWildcardLiteral: true, }, SSH: &policy.SSHPolicyOptions{ Host: &policy.SSHHostCertificateOptions{ @@ -369,8 +369,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { DeniedNames: &policy.X509NameOptions{ DNSDomains: []string{"badhost.local"}, }, - AllowWildcardLiteral: true, - DisableCommonNameVerification: false, + AllowWildcardLiteral: true, }, } @@ -429,8 +428,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { DeniedNames: &policy.X509NameOptions{ DNSDomains: []string{"badhost.local"}, }, - AllowWildcardLiteral: true, - DisableCommonNameVerification: false, + AllowWildcardLiteral: true, }, SSH: &policy.SSHPolicyOptions{ Host: &policy.SSHHostCertificateOptions{ @@ -488,8 +486,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { DeniedNames: &policy.X509NameOptions{ DNSDomains: []string{"badhost.local"}, }, - AllowWildcardLiteral: true, - DisableCommonNameVerification: false, + AllowWildcardLiteral: true, }, SSH: &policy.SSHPolicyOptions{ Host: &policy.SSHHostCertificateOptions{ @@ -700,8 +697,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { DeniedNames: &policy.X509NameOptions{ DNSDomains: []string{"badhost.local"}, }, - AllowWildcardLiteral: true, - DisableCommonNameVerification: false, + AllowWildcardLiteral: true, }, }, }, @@ -800,8 +796,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { DeniedNames: &policy.X509NameOptions{ DNSDomains: []string{"badhost.local"}, }, - AllowWildcardLiteral: true, - DisableCommonNameVerification: false, + AllowWildcardLiteral: true, }, SSH: &policy.SSHPolicyOptions{ Host: &policy.SSHHostCertificateOptions{ @@ -916,8 +911,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { Deny: &linkedca.X509Names{ Dns: []string{"badhost.local"}, }, - AllowWildcardLiteral: true, - DisableSubjectCommonNameVerification: false, + AllowWildcardLiteral: true, }, Ssh: &linkedca.SSHPolicy{ Host: &linkedca.SSHHostPolicy{ @@ -982,8 +976,7 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { Deny: &linkedca.X509Names{ Dns: []string{"badhost.local"}, }, - AllowWildcardLiteral: true, - DisableSubjectCommonNameVerification: false, + AllowWildcardLiteral: true, }, }, nil }, diff --git a/authority/provisioner/options.go b/authority/provisioner/options.go index 406b23b4..50af8396 100644 --- a/authority/provisioner/options.go +++ b/authority/provisioner/options.go @@ -70,11 +70,6 @@ type X509Options struct { // such as *.example.com and @example.com are allowed. Defaults // to false. AllowWildcardLiteral bool `json:"-"` - - // DisableCommonNameVerification indicates if the Subject Common Name - // is verified in addition to the SANs. Defaults to false, resulting - // in Common Names to be verified. - DisableCommonNameVerification bool `json:"-"` } // HasTemplate returns true if a template is defined in the provisioner options. @@ -107,13 +102,6 @@ func (o *X509Options) IsWildcardLiteralAllowed() bool { return o.AllowWildcardLiteral } -func (o *X509Options) ShouldVerifyCommonName() bool { - if o == nil { - return false - } - return !o.DisableCommonNameVerification -} - // TemplateOptions generates a CertificateOptions with the template and data // defined in the ProvisionerOptions, the provisioner generated data, and the // user data provided in the request. If no template has been provided, diff --git a/authority/provisioner/options_test.go b/authority/provisioner/options_test.go index 2edcdf3e..7883d045 100644 --- a/authority/provisioner/options_test.go +++ b/authority/provisioner/options_test.go @@ -322,38 +322,3 @@ func TestX509Options_IsWildcardLiteralAllowed(t *testing.T) { }) } } - -func TestX509Options_ShouldVerifySubjectCommonName(t *testing.T) { - tests := []struct { - name string - options *X509Options - want bool - }{ - { - name: "nil-options", - options: nil, - want: false, - }, - { - name: "set-true", - options: &X509Options{ - DisableCommonNameVerification: true, - }, - want: false, - }, - { - name: "set-false", - options: &X509Options{ - DisableCommonNameVerification: false, - }, - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := tt.options.ShouldVerifyCommonName(); got != tt.want { - t.Errorf("X509PolicyOptions.ShouldVerifySubjectCommonName() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/authority/tls_test.go b/authority/tls_test.go index 3739dbff..9330f0a3 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -701,9 +701,9 @@ ZYtQ9Ot36qc= options := &policy.Options{ X509: &policy.X509PolicyOptions{ AllowedNames: &policy.X509NameOptions{ - DNSDomains: []string{"*.smallstep.com"}, + CommonNames: []string{"smallstep test"}, + DNSDomains: []string{"*.smallstep.com"}, }, - DisableCommonNameVerification: true, // TODO(hs): allows "smallstep test"; do we want to keep it like this? }, } engine, err := policy.New(options) diff --git a/policy/engine.go b/policy/engine.go index 3d8a4755..d1fb4928 100755 --- a/policy/engine.go +++ b/policy/engine.go @@ -2,7 +2,6 @@ package policy import ( "crypto/x509" - "crypto/x509/pkix" "fmt" "net" "net/url" @@ -33,6 +32,7 @@ const ( type NameType string const ( + CNNameType NameType = "cn" DNSNameType NameType = "dns" IPNameType NameType = "ip" EmailNameType NameType = "email" @@ -80,6 +80,8 @@ type NamePolicyEngine struct { allowLiteralWildcardNames bool // permitted and exluded constraints similar to x509 Name Constraints + permittedCommonNames []string + excludedCommonNames []string permittedDNSDomains []string excludedDNSDomains []string permittedIPRanges []*net.IPNet @@ -92,6 +94,7 @@ type NamePolicyEngine struct { excludedPrincipals []string // some internal counts for housekeeping + numberOfCommonNameConstraints int numberOfDNSDomainConstraints int numberOfIPRangeConstraints int numberOfEmailAddressConstraints int @@ -112,29 +115,34 @@ func New(opts ...NamePolicyOption) (*NamePolicyEngine, error) { } } + e.permittedCommonNames = removeDuplicates(e.permittedCommonNames) e.permittedDNSDomains = removeDuplicates(e.permittedDNSDomains) e.permittedIPRanges = removeDuplicateIPNets(e.permittedIPRanges) e.permittedEmailAddresses = removeDuplicates(e.permittedEmailAddresses) e.permittedURIDomains = removeDuplicates(e.permittedURIDomains) e.permittedPrincipals = removeDuplicates(e.permittedPrincipals) + e.excludedCommonNames = removeDuplicates(e.excludedCommonNames) e.excludedDNSDomains = removeDuplicates(e.excludedDNSDomains) e.excludedIPRanges = removeDuplicateIPNets(e.excludedIPRanges) e.excludedEmailAddresses = removeDuplicates(e.excludedEmailAddresses) e.excludedURIDomains = removeDuplicates(e.excludedURIDomains) e.excludedPrincipals = removeDuplicates(e.excludedPrincipals) + e.numberOfCommonNameConstraints = len(e.permittedCommonNames) + len(e.excludedCommonNames) e.numberOfDNSDomainConstraints = len(e.permittedDNSDomains) + len(e.excludedDNSDomains) e.numberOfIPRangeConstraints = len(e.permittedIPRanges) + len(e.excludedIPRanges) e.numberOfEmailAddressConstraints = len(e.permittedEmailAddresses) + len(e.excludedEmailAddresses) e.numberOfURIDomainConstraints = len(e.permittedURIDomains) + len(e.excludedURIDomains) e.numberOfPrincipalConstraints = len(e.permittedPrincipals) + len(e.excludedPrincipals) - e.totalNumberOfPermittedConstraints = len(e.permittedDNSDomains) + len(e.permittedIPRanges) + - len(e.permittedEmailAddresses) + len(e.permittedURIDomains) + len(e.permittedPrincipals) + e.totalNumberOfPermittedConstraints = len(e.permittedCommonNames) + len(e.permittedDNSDomains) + + len(e.permittedIPRanges) + len(e.permittedEmailAddresses) + len(e.permittedURIDomains) + + len(e.permittedPrincipals) - e.totalNumberOfExcludedConstraints = len(e.excludedDNSDomains) + len(e.excludedIPRanges) + - len(e.excludedEmailAddresses) + len(e.excludedURIDomains) + len(e.excludedPrincipals) + e.totalNumberOfExcludedConstraints = len(e.excludedCommonNames) + len(e.excludedDNSDomains) + + len(e.excludedIPRanges) + len(e.excludedEmailAddresses) + len(e.excludedURIDomains) + + len(e.excludedPrincipals) e.totalNumberOfConstraints = e.totalNumberOfPermittedConstraints + e.totalNumberOfExcludedConstraints @@ -198,29 +206,27 @@ func removeDuplicateIPNets(items []*net.IPNet) (ret []*net.IPNet) { // IsX509CertificateAllowed verifies that all SANs in a Certificate are allowed. func (e *NamePolicyEngine) IsX509CertificateAllowed(cert *x509.Certificate) error { - dnsNames, ips, emails, uris := cert.DNSNames, cert.IPAddresses, cert.EmailAddresses, cert.URIs - // when Subject Common Name must be verified in addition to the SANs, it is - // added to the appropriate slice of names. - if e.verifySubjectCommonName { - appendSubjectCommonName(cert.Subject, &dnsNames, &ips, &emails, &uris) - } - if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil { + if err := e.validateNames(cert.DNSNames, cert.IPAddresses, cert.EmailAddresses, cert.URIs, []string{}); err != nil { return err } + + if e.verifySubjectCommonName { + return e.validateCommonName(cert.Subject.CommonName) + } + return nil } // IsX509CertificateRequestAllowed verifies that all names in the CSR are allowed. func (e *NamePolicyEngine) IsX509CertificateRequestAllowed(csr *x509.CertificateRequest) error { - dnsNames, ips, emails, uris := csr.DNSNames, csr.IPAddresses, csr.EmailAddresses, csr.URIs - // when Subject Common Name must be verified in addition to the SANs, it is - // added to the appropriate slice of names. - if e.verifySubjectCommonName { - appendSubjectCommonName(csr.Subject, &dnsNames, &ips, &emails, &uris) - } - if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil { + if err := e.validateNames(csr.DNSNames, csr.IPAddresses, csr.EmailAddresses, csr.URIs, []string{}); err != nil { return err } + + if e.verifySubjectCommonName { + return e.validateCommonName(csr.Subject.CommonName) + } + return nil } @@ -262,22 +268,6 @@ func (e *NamePolicyEngine) IsSSHCertificateAllowed(cert *ssh.Certificate) error return nil } -// appendSubjectCommonName appends the Subject Common Name to the appropriate slice of names. The logic is -// similar as x509util.SplitSANs: if the subject can be parsed as an IP, it's added to the ips. If it can -// be parsed as an URL, it is added to the URIs. If it contains an @, it is added to emails. When it's none -// of these, it's added to the DNS names. -func appendSubjectCommonName(subject pkix.Name, dnsNames *[]string, ips *[]net.IP, emails *[]string, uris *[]*url.URL) { - commonName := subject.CommonName - if commonName == "" { - return - } - subjectDNSNames, subjectIPs, subjectEmails, subjectURIs := x509util.SplitSANs([]string{commonName}) - *dnsNames = append(*dnsNames, subjectDNSNames...) - *ips = append(*ips, subjectIPs...) - *emails = append(*emails, subjectEmails...) - *uris = append(*uris, subjectURIs...) -} - // splitPrincipals splits SSH certificate principals into DNS names, emails and usernames. func splitSSHPrincipals(cert *ssh.Certificate) (dnsNames []string, ips []net.IP, emails, principals []string, err error) { dnsNames = []string{} diff --git a/policy/engine_test.go b/policy/engine_test.go index deec2ff9..dd6db586 100755 --- a/policy/engine_test.go +++ b/policy/engine_test.go @@ -610,22 +610,6 @@ func TestNamePolicyEngine_matchURIConstraint(t *testing.T) { } } -func extractSANs(cert *x509.Certificate, includeSubject bool) []string { - sans := []string{} - sans = append(sans, cert.DNSNames...) - for _, ip := range cert.IPAddresses { - sans = append(sans, ip.String()) - } - sans = append(sans, cert.EmailAddresses...) - for _, uri := range cert.URIs { - sans = append(sans, uri.String()) - } - if includeSubject && cert.Subject.CommonName != "" { - sans = append(sans, cert.Subject.CommonName) - } - return sans -} - func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { tests := []struct { name string @@ -1140,6 +1124,42 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { }, }, // SUBJECT FAILURE TESTS + { + name: "fail/subject-permitted-no-match", + options: []NamePolicyOption{ + WithSubjectCommonNameVerification(), + WithPermittedCommonNames("this name is allowed", "and this one too"), + }, + cert: &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "some certificate name", + }, + }, + want: false, + wantErr: &NamePolicyError{ + Reason: NotAllowed, // only permitted names allowed + NameType: CNNameType, + Name: "some certificate name", + }, + }, + { + name: "fail/subject-excluded-match", + options: []NamePolicyOption{ + WithSubjectCommonNameVerification(), + WithExcludedCommonNames("this name is not allowed"), + }, + cert: &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "this name is not allowed", + }, + }, + want: false, + wantErr: &NamePolicyError{ + Reason: CannotParseDomain, // CN cannot be parsed as DNS in this case + NameType: CNNameType, + Name: "this name is not allowed", + }, + }, { name: "fail/subject-dns-no-domain", options: []NamePolicyOption{ @@ -1154,7 +1174,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: CannotParseDomain, - NameType: DNSNameType, + NameType: CNNameType, Name: "name with space.local", }, }, @@ -1172,7 +1192,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: DNSNameType, + NameType: CNNameType, Name: "example.notlocal", }, }, @@ -1190,7 +1210,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: DNSNameType, + NameType: CNNameType, Name: "example.local", }, }, @@ -1213,7 +1233,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: IPNameType, + NameType: CNNameType, Name: "10.10.10.10", }, }, @@ -1236,7 +1256,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: IPNameType, + NameType: CNNameType, Name: "127.0.0.30", }, }, @@ -1259,7 +1279,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: IPNameType, + NameType: CNNameType, Name: "2002:db8:85a3::8a2e:370:7339", }, }, @@ -1282,7 +1302,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: IPNameType, + NameType: CNNameType, Name: "2001:db8:85a3::8a2e:370:7339", }, }, @@ -1300,7 +1320,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: EmailNameType, + NameType: CNNameType, Name: "mail@smallstep.com", }, }, @@ -1318,7 +1338,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: EmailNameType, + NameType: CNNameType, Name: "mail@example.local", }, }, @@ -1336,7 +1356,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: URINameType, + NameType: CNNameType, Name: "https://www.google.com", }, }, @@ -1354,7 +1374,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: URINameType, + NameType: CNNameType, Name: "https://www.example.com", }, }, @@ -1575,7 +1595,7 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { }, // COMBINED FAILURE TESTS { - name: "fail/combined-simple-all-badhost.local", + name: "fail/combined-simple-all-badhost.local-common-name", options: []NamePolicyOption{ WithSubjectCommonNameVerification(), WithPermittedDNSDomains("*.local"), @@ -1604,10 +1624,43 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { want: false, wantErr: &NamePolicyError{ Reason: NotAllowed, - NameType: DNSNameType, + NameType: CNNameType, Name: "badhost.local", }, }, + { + name: "fail/combined-simple-all-anotherbadhost.local-dns", + options: []NamePolicyOption{ + WithPermittedDNSDomains("*.local"), + WithPermittedCIDRs("127.0.0.1/24"), + WithPermittedEmailAddresses("@example.local"), + WithPermittedURIDomains("*.example.local"), + WithExcludedDNSDomains("anotherbadhost.local"), + WithExcludedCIDRs("127.0.0.128/25"), + WithExcludedEmailAddresses("badmail@example.local"), + WithExcludedURIDomains("badwww.example.local"), + }, + cert: &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "badhost.local", + }, + DNSNames: []string{"anotherbadhost.local"}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.40")}, + EmailAddresses: []string{"mail@example.local"}, + URIs: []*url.URL{ + { + Scheme: "https", + Host: "www.example.local", + }, + }, + }, + want: false, + wantErr: &NamePolicyError{ + Reason: NotAllowed, + NameType: DNSNameType, + Name: "anotherbadhost.local", + }, + }, { name: "fail/combined-simple-all-badmail@example.local", options: []NamePolicyOption{ @@ -1715,6 +1768,32 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { }, want: true, }, + { + name: "ok/subject-permitted-match", + options: []NamePolicyOption{ + WithSubjectCommonNameVerification(), + WithPermittedCommonNames("this name is allowed"), + }, + cert: &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "this name is allowed", + }, + }, + want: true, + }, + { + name: "ok/subject-excluded-match", + options: []NamePolicyOption{ + WithSubjectCommonNameVerification(), + WithExcludedCommonNames("this name is not allowed"), + }, + cert: &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "some other name", + }, + }, + want: true, + }, // SINGLE SAN TYPE PERMITTED SUCCESS TESTS { name: "ok/dns-permitted", @@ -2433,26 +2512,6 @@ func TestNamePolicyEngine_X509_AllAllowed(t *testing.T) { assert.NotEqual(t, "", npe.Detail()) //assert.Equals(t, tt.err.Reason, npe.Reason) // NOTE: reason detail is skipped; it's a detail } - - // Perform the same tests for a slice of SANs - includeSubject := engine.verifySubjectCommonName // copy behavior of the engine when Subject has to be included as a SAN - sans := extractSANs(tt.cert, includeSubject) - gotErr = engine.AreSANsAllowed(sans) - wantErr = tt.wantErr != nil - if (gotErr != nil) != wantErr { - t.Errorf("NamePolicyEngine.AreSANsAllowed() error = %v, wantErr %v", gotErr, tt.wantErr) - return - } - if gotErr != nil { - var npe *NamePolicyError - assert.True(t, errors.As(gotErr, &npe)) - assert.NotEqual(t, "", npe.Error()) - assert.Equal(t, tt.wantErr.Reason, npe.Reason) - assert.Equal(t, tt.wantErr.NameType, npe.NameType) - assert.Equal(t, tt.wantErr.Name, npe.Name) - assert.NotEqual(t, "", npe.Detail()) - //assert.Equals(t, tt.err.Reason, npe.Reason) // NOTE: reason detail is skipped; it's a detail - } }) } } diff --git a/policy/options.go b/policy/options.go index d244a311..79507f43 100755 --- a/policy/options.go +++ b/policy/options.go @@ -26,6 +26,20 @@ func WithAllowLiteralWildcardNames() NamePolicyOption { } } +func WithPermittedCommonNames(commonNames ...string) NamePolicyOption { + return func(g *NamePolicyEngine) error { + g.permittedCommonNames = commonNames + return nil + } +} + +func WithExcludedCommonNames(commonNames ...string) NamePolicyOption { + return func(g *NamePolicyEngine) error { + g.excludedCommonNames = commonNames + return nil + } +} + func WithPermittedDNSDomains(domains ...string) NamePolicyOption { return func(e *NamePolicyEngine) error { normalizedDomains := make([]string, len(domains)) @@ -198,7 +212,6 @@ func WithExcludedURIDomains(domains ...string) NamePolicyOption { func WithPermittedPrincipals(principals ...string) NamePolicyOption { return func(g *NamePolicyEngine) error { - // TODO(hs): normalize and parse principal into the right type? Seems the safe thing to do. g.permittedPrincipals = principals return nil } @@ -206,7 +219,6 @@ func WithPermittedPrincipals(principals ...string) NamePolicyOption { func WithExcludedPrincipals(principals ...string) NamePolicyOption { return func(g *NamePolicyEngine) error { - // TODO(hs): normalize and parse principal into the right type? Seems the safe thing to do. g.excludedPrincipals = principals return nil } diff --git a/policy/validate.go b/policy/validate.go index fff7120d..abd150db 100644 --- a/policy/validate.go +++ b/policy/validate.go @@ -15,6 +15,8 @@ import ( "strings" "golang.org/x/net/idna" + + "go.step.sm/crypto/x509util" ) // validateNames verifies that all names are allowed. @@ -71,7 +73,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA detail: fmt.Sprintf("cannot parse dns %q", dns), } } - if err := checkNameConstraints("dns", dns, parsedDNS, + if err := checkNameConstraints(DNSNameType, dns, parsedDNS, func(parsedName, constraint interface{}) (bool, error) { return e.matchDomainConstraint(parsedName.(string), constraint.(string)) }, e.permittedDNSDomains, e.excludedDNSDomains); err != nil { @@ -88,7 +90,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA detail: fmt.Sprintf("ip %q is not explicitly permitted by any constraint", ip.String()), } } - if err := checkNameConstraints("ip", ip.String(), ip, + if err := checkNameConstraints(IPNameType, ip.String(), ip, func(parsedName, constraint interface{}) (bool, error) { return matchIPConstraint(parsedName.(net.IP), constraint.(*net.IPNet)) }, e.permittedIPRanges, e.excludedIPRanges); err != nil { @@ -127,7 +129,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA } } mailbox.domain = domainASCII - if err := checkNameConstraints("email", email, mailbox, + if err := checkNameConstraints(EmailNameType, email, mailbox, func(parsedName, constraint interface{}) (bool, error) { return e.matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string)) }, e.permittedEmailAddresses, e.excludedEmailAddresses); err != nil { @@ -148,7 +150,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA } // TODO(hs): ideally we'd like the uri.String() to be the original contents; now // it's transformed into ASCII. Prevent that here? - if err := checkNameConstraints("uri", uri.String(), uri, + if err := checkNameConstraints(URINameType, uri.String(), uri, func(parsedName, constraint interface{}) (bool, error) { return e.matchURIConstraint(parsedName.(*url.URL), constraint.(string)) }, e.permittedURIDomains, e.excludedURIDomains); err != nil { @@ -166,9 +168,9 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA } } // TODO: some validation? I.e. allowed characters? - if err := checkNameConstraints("principal", principal, principal, + if err := checkNameConstraints(PrincipalNameType, principal, principal, func(parsedName, constraint interface{}) (bool, error) { - return matchUsernameConstraint(parsedName.(string), constraint.(string)) + return matchPrincipalConstraint(parsedName.(string), constraint.(string)) }, e.permittedPrincipals, e.excludedPrincipals); err != nil { return err } @@ -178,11 +180,51 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA return nil } +// validateCommonName verifies that the Subject Common Name is allowed +func (e *NamePolicyEngine) validateCommonName(commonName string) error { + + // nothing to compare against; return early + if e.totalNumberOfConstraints == 0 { + return nil + } + + // empty common names are not validated + if commonName == "" { + return nil + } + + if e.numberOfCommonNameConstraints > 0 { + // Check the Common Name using its dedicated matcher if constraints have been + // configured. If no error is returned from matching, the Common Name was + // explicitly allowed and nil is returned immediately. + if err := checkNameConstraints(CNNameType, commonName, commonName, + func(parsedName, constraint interface{}) (bool, error) { + return matchCommonNameConstraint(parsedName.(string), constraint.(string)) + }, e.permittedCommonNames, e.excludedCommonNames); err == nil { + return nil + } + } + + // When an error was returned or when no constraints were configured for Common Names, + // the Common Name should be validated against the other types of constraints too, + // according to what type it is. + dnsNames, ips, emails, uris := x509util.SplitSANs([]string{commonName}) + + err := e.validateNames(dnsNames, ips, emails, uris, []string{}) + + if pe, ok := err.(*NamePolicyError); ok { + // override the name type with CN + pe.NameType = CNNameType + } + + return err +} + // checkNameConstraints checks that a name, of type nameType is permitted. // The argument parsedName contains the parsed form of name, suitable for passing // to the match function. func checkNameConstraints( - nameType string, + nameType NameType, name string, parsedName interface{}, match func(parsedName, constraint interface{}) (match bool, err error), @@ -196,7 +238,7 @@ func checkNameConstraints( if err != nil { return &NamePolicyError{ Reason: CannotMatchNameToConstraint, - NameType: NameType(nameType), + NameType: nameType, Name: name, detail: err.Error(), } @@ -205,7 +247,7 @@ func checkNameConstraints( if match { return &NamePolicyError{ Reason: NotAllowed, - NameType: NameType(nameType), + NameType: nameType, Name: name, detail: fmt.Sprintf("%s %q is excluded by constraint %q", nameType, name, constraint), } @@ -221,7 +263,7 @@ func checkNameConstraints( if ok, err = match(parsedName, constraint); err != nil { return &NamePolicyError{ Reason: CannotMatchNameToConstraint, - NameType: NameType(nameType), + NameType: nameType, Name: name, detail: err.Error(), } @@ -235,7 +277,7 @@ func checkNameConstraints( if !ok { return &NamePolicyError{ Reason: NotAllowed, - NameType: NameType(nameType), + NameType: nameType, Name: name, detail: fmt.Sprintf("%s %q is not permitted by any constraint", nameType, name), } @@ -591,11 +633,16 @@ func (e *NamePolicyEngine) matchURIConstraint(uri *url.URL, constraint string) ( return e.matchDomainConstraint(host, constraint) } -// matchUsernameConstraint performs a string literal match against a constraint. -func matchUsernameConstraint(username, constraint string) (bool, error) { - // allow any plain principal username +// matchPrincipalConstraint performs a string literal equality check against a constraint. +func matchPrincipalConstraint(principal, constraint string) (bool, error) { + // allow any plain principal when wildcard constraint is used if constraint == "*" { return true, nil } - return strings.EqualFold(username, constraint), nil + return strings.EqualFold(principal, constraint), nil +} + +// matchCommonNameConstraint performs a string literal equality check against constraint. +func matchCommonNameConstraint(commonName, constraint string) (bool, error) { + return strings.EqualFold(commonName, constraint), nil }