From 2b7f6931f3fc8d5e9010a758c07bfeb0657bb36e Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 28 Apr 2022 14:49:23 +0200 Subject: [PATCH] Change Subject Common Name verification Subject Common Names can now also be configured to be allowed or denied, similar to SANs. When a Subject Common Name is not explicitly allowed or denied, its type will be determined and its value will be validated according to the constraints for that type of name (i.e. URI). --- api/read/read_test.go | 3 +- authority/admin/api/policy_test.go | 2 - authority/policy.go | 7 +- authority/policy/options.go | 19 +--- authority/policy/options_test.go | 40 ------- authority/policy/policy.go | 6 +- authority/policy_test.go | 53 ++++----- authority/provisioner/options.go | 12 -- authority/provisioner/options_test.go | 35 ------ authority/tls_test.go | 4 +- policy/engine.go | 60 ++++------ policy/engine_test.go | 157 ++++++++++++++++++-------- policy/options.go | 16 ++- policy/validate.go | 77 ++++++++++--- 14 files changed, 246 insertions(+), 245 deletions(-) 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 }