From ef110a94df7fce0b2a52277c6448b379cb4d940a Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 21 Apr 2022 23:45:05 +0200 Subject: [PATCH] Change pointer booleans to regular boolean configuration --- authority/admin/api/policy.go | 16 ------ authority/admin/api/policy_test.go | 85 +----------------------------- authority/policy.go | 9 ++-- authority/policy/options.go | 19 +++---- authority/policy/options_test.go | 32 +++++------ authority/policy_test.go | 36 ++++++------- 6 files changed, 43 insertions(+), 154 deletions(-) diff --git a/authority/admin/api/policy.go b/authority/admin/api/policy.go index 639bdbf2..c697b67a 100644 --- a/authority/admin/api/policy.go +++ b/authority/admin/api/policy.go @@ -5,7 +5,6 @@ import ( "net/http" "go.step.sm/linkedca" - "google.golang.org/protobuf/types/known/wrapperspb" "github.com/smallstep/certificates/acme" "github.com/smallstep/certificates/api/read" @@ -97,8 +96,6 @@ func (par *PolicyAdminResponder) CreateAuthorityPolicy(w http.ResponseWriter, r return } - applyConditionalDefaults(newPolicy) - newPolicy.Deduplicate() adm := linkedca.AdminFromContext(ctx) @@ -234,8 +231,6 @@ func (par *PolicyAdminResponder) CreateProvisionerPolicy(w http.ResponseWriter, return } - applyConditionalDefaults(newPolicy) - newPolicy.Deduplicate() prov.Policy = newPolicy @@ -454,14 +449,3 @@ func isBadRequest(err error) bool { isPolicyError := errors.As(err, &pe) return isPolicyError && (pe.Typ == authority.AdminLockOut || pe.Typ == authority.EvaluationFailure || pe.Typ == authority.ConfigurationFailure) } - -// applyConditionalDefaults applies default settings in case they're not provided -// in the request body. -func applyConditionalDefaults(p *linkedca.Policy) { - if p.GetX509() == nil { - return - } - if p.GetX509().GetVerifySubjectCommonName() == nil { - p.X509.VerifySubjectCommonName = &wrapperspb.BoolValue{Value: true} - } -} diff --git a/authority/admin/api/policy_test.go b/authority/admin/api/policy_test.go index 224ab18d..ee97c2cc 100644 --- a/authority/admin/api/policy_test.go +++ b/authority/admin/api/policy_test.go @@ -12,7 +12,6 @@ import ( "testing" "google.golang.org/protobuf/encoding/protojson" - "google.golang.org/protobuf/types/known/wrapperspb" "go.step.sm/linkedca" @@ -310,7 +309,7 @@ func TestPolicyAdminResponder_CreateAuthorityPolicy(t *testing.T) { Allow: &linkedca.X509Names{ Dns: []string{"*.local"}, }, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: true}, + DisableSubjectCommonNameVerification: false, }, } body, err := protojson.Marshal(policy) @@ -1047,7 +1046,7 @@ func TestPolicyAdminResponder_CreateProvisionerPolicy(t *testing.T) { Allow: &linkedca.X509Names{ Dns: []string{"*.local"}, }, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: true}, + DisableSubjectCommonNameVerification: false, }, } body, err := protojson.Marshal(policy) @@ -2077,86 +2076,6 @@ func TestPolicyAdminResponder_DeleteACMEAccountPolicy(t *testing.T) { } } -func Test_applyConditionalDefaults(t *testing.T) { - tests := []struct { - name string - policy *linkedca.Policy - expected *linkedca.Policy - }{ - { - name: "no-x509", - policy: &linkedca.Policy{ - Ssh: &linkedca.SSHPolicy{}, - }, - expected: &linkedca.Policy{ - Ssh: &linkedca.SSHPolicy{}, - }, - }, - { - name: "with-x509-verify-subject-common-name", - policy: &linkedca.Policy{ - X509: &linkedca.X509Policy{ - Allow: &linkedca.X509Names{ - Dns: []string{"*.local"}, - }, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: true}, - }, - }, - expected: &linkedca.Policy{ - X509: &linkedca.X509Policy{ - Allow: &linkedca.X509Names{ - Dns: []string{"*.local"}, - }, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: true}, - }, - }, - }, - { - name: "without-x509-verify-subject-common-name", - policy: &linkedca.Policy{ - X509: &linkedca.X509Policy{ - Allow: &linkedca.X509Names{ - Dns: []string{"*.local"}, - }, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: false}, - }, - }, - expected: &linkedca.Policy{ - X509: &linkedca.X509Policy{ - Allow: &linkedca.X509Names{ - Dns: []string{"*.local"}, - }, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: false}, - }, - }, - }, - { - name: "no-x509-verify-subject-common-name", - policy: &linkedca.Policy{ - X509: &linkedca.X509Policy{ - Allow: &linkedca.X509Names{ - Dns: []string{"*.local"}, - }, - }, - }, - expected: &linkedca.Policy{ - X509: &linkedca.X509Policy{ - Allow: &linkedca.X509Names{ - Dns: []string{"*.local"}, - }, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: true}, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - applyConditionalDefaults(tt.policy) - assert.Equals(t, tt.expected, tt.policy) - }) - } -} - func Test_isBadRequest(t *testing.T) { tests := []struct { name string diff --git a/authority/policy.go b/authority/policy.go index dd24ecf7..d0da3634 100644 --- a/authority/policy.go +++ b/authority/policy.go @@ -350,12 +350,9 @@ func policyToCertificates(p *linkedca.Policy) *authPolicy.Options { opts.X509.DeniedNames.URIDomains = deny.Uris } } - if v := x509.GetAllowWildcardLiteral(); v != nil { - opts.X509.AllowWildcardLiteral = &v.Value - } - if v := x509.GetVerifySubjectCommonName(); v != nil { - opts.X509.VerifySubjectCommonName = &v.Value - } + + opts.X509.AllowWildcardLiteral = x509.AllowWildcardLiteral + opts.X509.DisableSubjectCommonNameVerification = x509.DisableSubjectCommonNameVerification } // fill ssh policy configuration diff --git a/authority/policy/options.go b/authority/policy/options.go index 68efe45a..ff0eec3d 100644 --- a/authority/policy/options.go +++ b/authority/policy/options.go @@ -44,10 +44,10 @@ type X509PolicyOptions struct { // AllowWildcardLiteral indicates if literal wildcard names // such as *.example.com and @example.com are allowed. Defaults // to false. - AllowWildcardLiteral *bool `json:"allow_wildcard_literal,omitempty"` - // VerifySubjectCommonName indicates if the Subject Common Name - // is verified in addition to the SANs. Defaults to true. - VerifySubjectCommonName *bool `json:"verify_subject_common_name,omitempty"` + AllowWildcardLiteral bool `json:"allow_wildcard_literal,omitempty"` + // DisableSubjectCommonNameVerification indicates if the Subject Common Name + // is verified in addition to the SANs. Defaults to false. + DisableSubjectCommonNameVerification bool `json:"disable_subject_common_name_verification,omitempty"` } // X509NameOptions models the X509 name policy configuration. @@ -83,21 +83,22 @@ func (o *X509PolicyOptions) GetDeniedNameOptions() *X509NameOptions { return o.DeniedNames } +// IsWildcardLiteralAllowed returns whether the authority allows +// literal wildcard domains and other names to be signed. func (o *X509PolicyOptions) IsWildcardLiteralAllowed() bool { if o == nil { return true } - return o.AllowWildcardLiteral != nil && *o.AllowWildcardLiteral + return o.AllowWildcardLiteral } +// ShouldVerifySubjectCommonName returns whether the authority +// should verify the Subject Common Name in addition to the SANs. func (o *X509PolicyOptions) ShouldVerifySubjectCommonName() bool { if o == nil { return false } - if o.VerifySubjectCommonName == nil { - return true - } - return *o.VerifySubjectCommonName + return !o.DisableSubjectCommonNameVerification } // SSHPolicyOptionsInterface is an interface for providers of diff --git a/authority/policy/options_test.go b/authority/policy/options_test.go index 49330f08..ebcd90fe 100644 --- a/authority/policy/options_test.go +++ b/authority/policy/options_test.go @@ -5,8 +5,6 @@ import ( ) func TestX509PolicyOptions_IsWildcardLiteralAllowed(t *testing.T) { - trueValue := true - falseValue := false tests := []struct { name string options *X509PolicyOptions @@ -18,23 +16,21 @@ func TestX509PolicyOptions_IsWildcardLiteralAllowed(t *testing.T) { want: true, }, { - name: "nil", - options: &X509PolicyOptions{ - AllowWildcardLiteral: nil, - }, - want: false, + name: "not-set", + options: &X509PolicyOptions{}, + want: false, }, { name: "set-true", options: &X509PolicyOptions{ - AllowWildcardLiteral: &trueValue, + AllowWildcardLiteral: true, }, want: true, }, { name: "set-false", options: &X509PolicyOptions{ - AllowWildcardLiteral: &falseValue, + AllowWildcardLiteral: false, }, want: false, }, @@ -49,8 +45,6 @@ func TestX509PolicyOptions_IsWildcardLiteralAllowed(t *testing.T) { } func TestX509PolicyOptions_ShouldVerifySubjectCommonName(t *testing.T) { - trueValue := true - falseValue := false tests := []struct { name string options *X509PolicyOptions @@ -62,25 +56,23 @@ func TestX509PolicyOptions_ShouldVerifySubjectCommonName(t *testing.T) { want: false, }, { - name: "nil", - options: &X509PolicyOptions{ - VerifySubjectCommonName: nil, - }, - want: true, + name: "not-set", + options: &X509PolicyOptions{}, + want: true, }, { name: "set-true", options: &X509PolicyOptions{ - VerifySubjectCommonName: &trueValue, + DisableSubjectCommonNameVerification: true, }, - want: true, + want: false, }, { name: "set-false", options: &X509PolicyOptions{ - VerifySubjectCommonName: &falseValue, + DisableSubjectCommonNameVerification: false, }, - want: false, + want: true, }, } for _, tt := range tests { diff --git a/authority/policy_test.go b/authority/policy_test.go index 514a7a51..bc121a79 100644 --- a/authority/policy_test.go +++ b/authority/policy_test.go @@ -7,7 +7,6 @@ import ( "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" - "google.golang.org/protobuf/types/known/wrapperspb" "go.step.sm/linkedca" @@ -193,8 +192,6 @@ func TestAuthority_checkPolicy(t *testing.T) { } func Test_policyToCertificates(t *testing.T) { - trueValue := true - falseValue := false tests := []struct { name string policy *linkedca.Policy @@ -217,8 +214,8 @@ func Test_policyToCertificates(t *testing.T) { Allow: &linkedca.X509Names{ Dns: []string{"*.local"}, }, - AllowWildcardLiteral: &wrapperspb.BoolValue{Value: false}, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: true}, + AllowWildcardLiteral: false, + DisableSubjectCommonNameVerification: false, }, }, want: &policy.Options{ @@ -226,8 +223,8 @@ func Test_policyToCertificates(t *testing.T) { AllowedNames: &policy.X509NameOptions{ DNSDomains: []string{"*.local"}, }, - AllowWildcardLiteral: &falseValue, - VerifySubjectCommonName: &trueValue, + AllowWildcardLiteral: false, + DisableSubjectCommonNameVerification: false, }, }, }, @@ -247,8 +244,8 @@ func Test_policyToCertificates(t *testing.T) { Emails: []string{"badhost.example.com"}, Uris: []string{"https://badhost.local"}, }, - AllowWildcardLiteral: &wrapperspb.BoolValue{Value: true}, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: true}, + AllowWildcardLiteral: true, + DisableSubjectCommonNameVerification: false, }, Ssh: &linkedca.SSHPolicy{ Host: &linkedca.SSHHostPolicy{ @@ -289,8 +286,8 @@ func Test_policyToCertificates(t *testing.T) { EmailAddresses: []string{"badhost.example.com"}, URIDomains: []string{"https://badhost.local"}, }, - AllowWildcardLiteral: &trueValue, - VerifySubjectCommonName: &trueValue, + AllowWildcardLiteral: true, + DisableSubjectCommonNameVerification: false, }, SSH: &policy.SSHPolicyOptions{ Host: &policy.SSHHostCertificateOptions{ @@ -335,7 +332,6 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { sshUserPolicy bool sshHostPolicy bool } - trueValue := true tests := []struct { name string config *config.Config @@ -517,8 +513,8 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { DeniedNames: &policy.X509NameOptions{ DNSDomains: []string{"badhost.local"}, }, - AllowWildcardLiteral: &trueValue, - VerifySubjectCommonName: &trueValue, + AllowWildcardLiteral: true, + DisableSubjectCommonNameVerification: false, }, }, }, @@ -637,8 +633,8 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { DeniedNames: &policy.X509NameOptions{ DNSDomains: []string{"badhost.local"}, }, - AllowWildcardLiteral: &trueValue, - VerifySubjectCommonName: &trueValue, + AllowWildcardLiteral: true, + DisableSubjectCommonNameVerification: false, }, SSH: &policy.SSHPolicyOptions{ Host: &policy.SSHHostCertificateOptions{ @@ -770,8 +766,8 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { Deny: &linkedca.X509Names{ Dns: []string{"badhost.local"}, }, - AllowWildcardLiteral: &wrapperspb.BoolValue{Value: true}, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: true}, + AllowWildcardLiteral: true, + DisableSubjectCommonNameVerification: false, }, Ssh: &linkedca.SSHPolicy{ Host: &linkedca.SSHHostPolicy{ @@ -835,8 +831,8 @@ func TestAuthority_reloadPolicyEngines(t *testing.T) { Deny: &linkedca.X509Names{ Dns: []string{"badhost.local"}, }, - AllowWildcardLiteral: &wrapperspb.BoolValue{Value: true}, - VerifySubjectCommonName: &wrapperspb.BoolValue{Value: true}, + AllowWildcardLiteral: true, + DisableSubjectCommonNameVerification: false, }, }, nil },