diff --git a/acme/api/order.go b/acme/api/order.go index e1adebb3..8fe37656 100644 --- a/acme/api/order.go +++ b/acme/api/order.go @@ -105,6 +105,8 @@ func (h *Handler) NewOrder(w http.ResponseWriter, r *http.Request) { // management of allowed/denied names based on just the name, without having bound to EAB. Still, // EAB is not illogical, because that's the way Accounts are connected to an external system and // thus make sense to also set the allowed/denied names based on that info. + // TODO: also perform check on the authority level here already, so that challenges are not performed + // and after that the CA fails to sign it. (i.e. h.ca function?) for _, identifier := range nor.Identifiers { // TODO: gather all errors, so that we can build subproblems; include the nor.Validate() error here too, like in example? diff --git a/authority/admin/api/acme.go b/authority/admin/api/acme.go index 27c3ba6f..131a8fff 100644 --- a/authority/admin/api/acme.go +++ b/authority/admin/api/acme.go @@ -14,7 +14,7 @@ import ( const ( // provisionerContextKey provisioner key - provisionerContextKey = ContextKey("provisioner") + provisionerContextKey = admin.ContextKey("provisioner") ) // CreateExternalAccountKeyRequest is the type for POST /admin/acme/eab requests diff --git a/authority/admin/api/admin.go b/authority/admin/api/admin.go index dd40784b..34db5ea2 100644 --- a/authority/admin/api/admin.go +++ b/authority/admin/api/admin.go @@ -26,8 +26,8 @@ type adminAuthority interface { UpdateProvisioner(ctx context.Context, nu *linkedca.Provisioner) error RemoveProvisioner(ctx context.Context, id string) error GetAuthorityPolicy(ctx context.Context) (*linkedca.Policy, error) - StoreAuthorityPolicy(ctx context.Context, policy *linkedca.Policy) error - UpdateAuthorityPolicy(ctx context.Context, policy *linkedca.Policy) error + StoreAuthorityPolicy(ctx context.Context, admin *linkedca.Admin, policy *linkedca.Policy) error + UpdateAuthorityPolicy(ctx context.Context, admin *linkedca.Admin, policy *linkedca.Policy) error RemoveAuthorityPolicy(ctx context.Context) error } diff --git a/authority/admin/api/admin_test.go b/authority/admin/api/admin_test.go index f1698139..bcea31b5 100644 --- a/authority/admin/api/admin_test.go +++ b/authority/admin/api/admin_test.go @@ -139,11 +139,11 @@ func (m *mockAdminAuthority) GetAuthorityPolicy(ctx context.Context) (*linkedca. return nil, errors.New("not implemented yet") } -func (m *mockAdminAuthority) StoreAuthorityPolicy(ctx context.Context, policy *linkedca.Policy) error { +func (m *mockAdminAuthority) StoreAuthorityPolicy(ctx context.Context, admin *linkedca.Admin, policy *linkedca.Policy) error { return errors.New("not implemented yet") } -func (m *mockAdminAuthority) UpdateAuthorityPolicy(ctx context.Context, policy *linkedca.Policy) error { +func (m *mockAdminAuthority) UpdateAuthorityPolicy(ctx context.Context, admin *linkedca.Admin, policy *linkedca.Policy) error { return errors.New("not implemented yet") } diff --git a/authority/admin/api/handler.go b/authority/admin/api/handler.go index e59b95e0..0dd45cb0 100644 --- a/authority/admin/api/handler.go +++ b/authority/admin/api/handler.go @@ -30,8 +30,7 @@ func NewHandler(auth adminAuthority, adminDB admin.DB, acmeDB acme.DB, acmeRespo func (h *Handler) Route(r api.Router) { authnz := func(next nextHTTP) nextHTTP { - //return h.extractAuthorizeTokenAdmin(h.requireAPIEnabled(next)) - return h.requireAPIEnabled(next) // TODO(hs): remove this; temporarily no auth checks for simple testing... + return h.extractAuthorizeTokenAdmin(h.requireAPIEnabled(next)) } requireEABEnabled := func(next nextHTTP) nextHTTP { diff --git a/authority/admin/api/middleware.go b/authority/admin/api/middleware.go index 62aefdc3..c30c7219 100644 --- a/authority/admin/api/middleware.go +++ b/authority/admin/api/middleware.go @@ -7,6 +7,7 @@ import ( "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority/admin" "github.com/smallstep/certificates/authority/admin/db/nosql" + "go.step.sm/linkedca" ) type nextHTTP = func(http.ResponseWriter, *http.Request) @@ -27,6 +28,7 @@ func (h *Handler) requireAPIEnabled(next nextHTTP) nextHTTP { // extractAuthorizeTokenAdmin is a middleware that extracts and caches the bearer token. func (h *Handler) extractAuthorizeTokenAdmin(next nextHTTP) nextHTTP { return func(w http.ResponseWriter, r *http.Request) { + tok := r.Header.Get("Authorization") if tok == "" { api.WriteError(w, admin.NewError(admin.ErrorUnauthorizedType, @@ -40,7 +42,7 @@ func (h *Handler) extractAuthorizeTokenAdmin(next nextHTTP) nextHTTP { return } - ctx := context.WithValue(r.Context(), adminContextKey, adm) + ctx := context.WithValue(r.Context(), admin.AdminContextKey, adm) next(w, r.WithContext(ctx)) } } @@ -49,13 +51,14 @@ func (h *Handler) extractAuthorizeTokenAdmin(next nextHTTP) nextHTTP { func (h *Handler) checkAction(next nextHTTP, supportedInStandalone bool) nextHTTP { return func(w http.ResponseWriter, r *http.Request) { - // actions allowed in standalone mode are always allowed + // actions allowed in standalone mode are always supported if supportedInStandalone { next(w, r) return } - // when in standalone mode, actions are not supported + // when not in standalone mode and using a nosql.DB backend, + // actions are not supported if _, ok := h.adminDB.(*nosql.DB); ok { api.WriteError(w, admin.NewError(admin.ErrorNotImplementedType, "operation not supported in standalone mode")) @@ -67,11 +70,12 @@ func (h *Handler) checkAction(next nextHTTP, supportedInStandalone bool) nextHTT } } -// ContextKey is the key type for storing and searching for ACME request -// essentials in the context of a request. -type ContextKey string - -const ( - // adminContextKey account key - adminContextKey = ContextKey("admin") -) +// adminFromContext searches the context for a *linkedca.Admin. +// Returns the admin or an error. +func adminFromContext(ctx context.Context) (*linkedca.Admin, error) { + val, ok := ctx.Value(admin.AdminContextKey).(*linkedca.Admin) + if !ok || val == nil { + return nil, admin.NewError(admin.ErrorBadRequestType, "admin not in context") + } + return val, nil +} diff --git a/authority/admin/api/middleware_test.go b/authority/admin/api/middleware_test.go index 7fb4671a..ffa319db 100644 --- a/authority/admin/api/middleware_test.go +++ b/authority/admin/api/middleware_test.go @@ -152,7 +152,7 @@ func TestHandler_extractAuthorizeTokenAdmin(t *testing.T) { req.Header["Authorization"] = []string{"token"} createdAt := time.Now() var deletedAt time.Time - admin := &linkedca.Admin{ + adm := &linkedca.Admin{ Id: "adminID", AuthorityId: "authorityID", Subject: "admin", @@ -164,20 +164,20 @@ func TestHandler_extractAuthorizeTokenAdmin(t *testing.T) { auth := &mockAdminAuthority{ MockAuthorizeAdminToken: func(r *http.Request, token string) (*linkedca.Admin, error) { assert.Equals(t, "token", token) - return admin, nil + return adm, nil }, } next := func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - a := ctx.Value(adminContextKey) // verifying that the context now has a linkedca.Admin + a := ctx.Value(admin.AdminContextKey) // verifying that the context now has a linkedca.Admin adm, ok := a.(*linkedca.Admin) if !ok { t.Errorf("expected *linkedca.Admin; got %T", a) return } opts := []cmp.Option{cmpopts.IgnoreUnexported(linkedca.Admin{}, timestamppb.Timestamp{})} - if !cmp.Equal(admin, adm, opts...) { - t.Errorf("linkedca.Admin diff =\n%s", cmp.Diff(admin, adm, opts...)) + if !cmp.Equal(adm, adm, opts...) { + t.Errorf("linkedca.Admin diff =\n%s", cmp.Diff(adm, adm, opts...)) } w.Write(nil) // mock response with status 200 } diff --git a/authority/admin/api/policy.go b/authority/admin/api/policy.go index c318e5e5..2f64802f 100644 --- a/authority/admin/api/policy.go +++ b/authority/admin/api/policy.go @@ -87,8 +87,14 @@ func (par *PolicyAdminResponder) CreateAuthorityPolicy(w http.ResponseWriter, r return } - if err := par.auth.StoreAuthorityPolicy(ctx, newPolicy); err != nil { - api.WriteError(w, admin.WrapErrorISE(err, "error storing authority policy")) + adm, err := adminFromContext(ctx) + if err != nil { + api.WriteError(w, admin.WrapErrorISE(err, "error retrieving admin from context")) + return + } + + if err := par.auth.StoreAuthorityPolicy(ctx, adm, newPolicy); err != nil { + api.WriteError(w, admin.WrapError(admin.ErrorBadRequestType, err, "error storing authority policy")) return } @@ -103,25 +109,49 @@ func (par *PolicyAdminResponder) CreateAuthorityPolicy(w http.ResponseWriter, r // UpdateAuthorityPolicy handles the PUT /admin/authority/policy request func (par *PolicyAdminResponder) UpdateAuthorityPolicy(w http.ResponseWriter, r *http.Request) { - var policy = new(linkedca.Policy) - if err := api.ReadProtoJSON(r.Body, policy); err != nil { + + ctx := r.Context() + policy, err := par.auth.GetAuthorityPolicy(ctx) + + shouldWriteError := false + if ae, ok := err.(*admin.Error); ok { + shouldWriteError = !ae.IsType(admin.ErrorNotFoundType) + } + + if shouldWriteError { + api.WriteError(w, admin.WrapErrorISE(err, "error retrieving authority policy")) + return + } + + if policy == nil { + api.JSONNotFound(w) + return + } + + var newPolicy = new(linkedca.Policy) + if err := api.ReadProtoJSON(r.Body, newPolicy); err != nil { api.WriteError(w, err) return } - ctx := r.Context() - if err := par.auth.UpdateAuthorityPolicy(ctx, policy); err != nil { - api.WriteError(w, admin.WrapErrorISE(err, "error updating authority policy")) + adm, err := adminFromContext(ctx) + if err != nil { + api.WriteError(w, admin.WrapErrorISE(err, "error retrieving admin from context")) + return + } + + if err := par.auth.UpdateAuthorityPolicy(ctx, adm, newPolicy); err != nil { + api.WriteError(w, admin.WrapError(admin.ErrorBadRequestType, err, "error updating authority policy")) return } - newPolicy, err := par.auth.GetAuthorityPolicy(ctx) + newlyStoredPolicy, err := par.auth.GetAuthorityPolicy(ctx) if err != nil { api.WriteError(w, admin.WrapErrorISE(err, "error retrieving authority policy after updating")) return } - api.ProtoJSONStatus(w, newPolicy, http.StatusOK) + api.ProtoJSONStatus(w, newlyStoredPolicy, http.StatusOK) } // DeleteAuthorityPolicy handles the DELETE /admin/authority/policy request diff --git a/authority/admin/context.go b/authority/admin/context.go new file mode 100644 index 00000000..87bf3e03 --- /dev/null +++ b/authority/admin/context.go @@ -0,0 +1,10 @@ +package admin + +// ContextKey is the key type for storing and searching for +// Admin API objects in request contexts. +type ContextKey string + +const ( + // AdminContextKey account key + AdminContextKey = ContextKey("admin") +) diff --git a/authority/administrator/collection.go b/authority/administrator/collection.go index 88d7bb2c..300c3e4f 100644 --- a/authority/administrator/collection.go +++ b/authority/administrator/collection.go @@ -78,7 +78,7 @@ func (c *Collection) LoadByProvisioner(provName string) ([]*linkedca.Admin, bool } // Store adds an admin to the collection and enforces the uniqueness of -// admin IDs and amdin subject <-> provisioner name combos. +// admin IDs and admin subject <-> provisioner name combos. func (c *Collection) Store(adm *linkedca.Admin, prov provisioner.Interface) error { // Input validation. if adm.ProvisionerId != prov.GetID() { diff --git a/authority/authority.go b/authority/authority.go index aaf0e478..29a10d7e 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -219,15 +219,19 @@ func (a *Authority) reloadPolicyEngines(ctx context.Context) error { if a.config.AuthorityConfig.EnableAdmin { linkedPolicy, err := a.adminDB.GetAuthorityPolicy(ctx) if err != nil { - return admin.WrapErrorISE(err, "error getting policy to initialize authority") + return admin.WrapErrorISE(err, "error getting policy to (re)load policy engines") } policyOptions = policyToCertificates(linkedPolicy) } else { policyOptions = a.config.AuthorityConfig.Policy } - // return early if no policy options set + // if no new or updated policy option is set, clear policy engines that (may have) + // been configured before and return early if policyOptions == nil { + a.x509Policy = nil + a.sshHostPolicy = nil + a.sshUserPolicy = nil return nil } @@ -574,7 +578,7 @@ func (a *Authority) init() error { return err } - // Load Policy Engines + // Load x509 and SSH Policy Engines if err := a.reloadPolicyEngines(context.Background()); err != nil { return err } diff --git a/authority/policy.go b/authority/policy.go index 8ef264d0..db44e5f4 100644 --- a/authority/policy.go +++ b/authority/policy.go @@ -2,10 +2,15 @@ package authority import ( "context" + "fmt" + + "github.com/pkg/errors" - "github.com/smallstep/certificates/authority/admin" - "github.com/smallstep/certificates/authority/policy" "go.step.sm/linkedca" + + "github.com/smallstep/certificates/authority/admin" + authPolicy "github.com/smallstep/certificates/authority/policy" + policy "github.com/smallstep/certificates/policy" ) func (a *Authority) GetAuthorityPolicy(ctx context.Context) (*linkedca.Policy, error) { @@ -20,31 +25,39 @@ func (a *Authority) GetAuthorityPolicy(ctx context.Context) (*linkedca.Policy, e return policy, nil } -func (a *Authority) StoreAuthorityPolicy(ctx context.Context, policy *linkedca.Policy) error { +func (a *Authority) StoreAuthorityPolicy(ctx context.Context, adm *linkedca.Admin, policy *linkedca.Policy) error { a.adminMutex.Lock() defer a.adminMutex.Unlock() + if err := a.checkPolicy(ctx, adm, policy); err != nil { + return err + } + if err := a.adminDB.CreateAuthorityPolicy(ctx, policy); err != nil { return err } if err := a.reloadPolicyEngines(ctx); err != nil { - return admin.WrapErrorISE(err, "error reloading admin resources when creating authority policy") + return admin.WrapErrorISE(err, "error reloading policy engines when creating authority policy") } return nil } -func (a *Authority) UpdateAuthorityPolicy(ctx context.Context, policy *linkedca.Policy) error { +func (a *Authority) UpdateAuthorityPolicy(ctx context.Context, adm *linkedca.Admin, policy *linkedca.Policy) error { a.adminMutex.Lock() defer a.adminMutex.Unlock() + if err := a.checkPolicy(ctx, adm, policy); err != nil { + return err + } + if err := a.adminDB.UpdateAuthorityPolicy(ctx, policy); err != nil { return err } if err := a.reloadPolicyEngines(ctx); err != nil { - return admin.WrapErrorISE(err, "error reloading admin resources when updating authority policy") + return admin.WrapErrorISE(err, "error reloading policy engines when updating authority policy") } return nil @@ -59,34 +72,84 @@ func (a *Authority) RemoveAuthorityPolicy(ctx context.Context) error { } if err := a.reloadPolicyEngines(ctx); err != nil { - return admin.WrapErrorISE(err, "error reloading admin resources when deleting authority policy") + return admin.WrapErrorISE(err, "error reloading policy engines when deleting authority policy") } return nil } -func policyToCertificates(p *linkedca.Policy) *policy.Options { +func (a *Authority) checkPolicy(ctx context.Context, adm *linkedca.Admin, p *linkedca.Policy) error { + + // convert the policy; return early if nil + policyOptions := policyToCertificates(p) + if policyOptions == nil { + return nil + } + + engine, err := authPolicy.NewX509PolicyEngine(policyOptions.GetX509Options()) + if err != nil { + return admin.WrapErrorISE(err, "error creating temporary policy engine") + } + + // TODO(hs): Provide option to force the policy, even when the admin subject would be locked out? + + sans := []string{adm.Subject} + if err := isAllowed(engine, sans); err != nil { + return err + } + + // TODO(hs): perform the check for other admin subjects too? + // What logic to use for that: do all admins need access? Only super admins? At least one? + + return nil +} + +func isAllowed(engine authPolicy.X509Policy, sans []string) error { + var ( + allowed bool + err error + ) + if allowed, err = engine.AreSANsAllowed(sans); err != nil { + var policyErr *policy.NamePolicyError + if errors.As(err, &policyErr); policyErr.Reason == policy.NotAuthorizedForThisName { + return fmt.Errorf("the provided policy would lock out %s from the CA. Please update your policy to include %s as an allowed name", sans, sans) + } else { + return err + } + } + + if !allowed { + return fmt.Errorf("the provided policy would lock out %s from the CA. Please update your policy to include %s as an allowed name", sans, sans) + } + + return nil +} + +func policyToCertificates(p *linkedca.Policy) *authPolicy.Options { + // return early if p == nil { return nil } + // prepare full policy struct - opts := &policy.Options{ - X509: &policy.X509PolicyOptions{ - AllowedNames: &policy.X509NameOptions{}, - DeniedNames: &policy.X509NameOptions{}, + opts := &authPolicy.Options{ + X509: &authPolicy.X509PolicyOptions{ + AllowedNames: &authPolicy.X509NameOptions{}, + DeniedNames: &authPolicy.X509NameOptions{}, }, - SSH: &policy.SSHPolicyOptions{ - Host: &policy.SSHHostCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{}, - DeniedNames: &policy.SSHNameOptions{}, + SSH: &authPolicy.SSHPolicyOptions{ + Host: &authPolicy.SSHHostCertificateOptions{ + AllowedNames: &authPolicy.SSHNameOptions{}, + DeniedNames: &authPolicy.SSHNameOptions{}, }, - User: &policy.SSHUserCertificateOptions{ - AllowedNames: &policy.SSHNameOptions{}, - DeniedNames: &policy.SSHNameOptions{}, + User: &authPolicy.SSHUserCertificateOptions{ + AllowedNames: &authPolicy.SSHNameOptions{}, + DeniedNames: &authPolicy.SSHNameOptions{}, }, }, } + // fill x509 policy configuration if p.X509 != nil { if p.X509.Allow != nil { @@ -102,6 +165,7 @@ func policyToCertificates(p *linkedca.Policy) *policy.Options { opts.X509.DeniedNames.URIDomains = p.X509.Deny.Uris } } + // fill ssh policy configuration if p.Ssh != nil { if p.Ssh.Host != nil { diff --git a/authority/tls.go b/authority/tls.go index 96c80e9a..297c796e 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -191,26 +191,20 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign } } - // If a policy is configured, perform allow/deny policy check on authority level - // TODO: policy currently also applies to admin token certs; how to circumvent? - // Allow any name of an admin in the DB? Or in the admin collection? - todoRemoveThis := false - if todoRemoveThis && a.x509Policy != nil { - allowed, err := a.x509Policy.AreCertificateNamesAllowed(leaf) - if err != nil { - return nil, errs.InternalServerErr(err, - errs.WithKeyVal("csr", csr), - errs.WithKeyVal("signOptions", signOpts), - errs.WithMessage("error creating certificate"), - ) - } - if !allowed { - // TODO: include SANs in error message? - return nil, errs.ApplyOptions( - errs.ForbiddenErr(errors.New("authority not allowed to sign"), "error creating certificate"), - opts..., - ) - } + // Check if authority is allowed to sign the certificate + var allowedToSign bool + if allowedToSign, err = a.isAllowedToSign(leaf); err != nil { + return nil, errs.InternalServerErr(err, + errs.WithKeyVal("csr", csr), + errs.WithKeyVal("signOptions", signOpts), + errs.WithMessage("error creating certificate"), + ) + } + if !allowedToSign { + return nil, errs.ApplyOptions( + errs.ForbiddenErr(errors.New("authority not allowed to sign"), "error creating certificate"), + opts..., + ) } // Sign certificate @@ -236,6 +230,61 @@ func (a *Authority) Sign(csr *x509.CertificateRequest, signOpts provisioner.Sign return fullchain, nil } +// isAllowedToSign checks if the Authority is allowed to sign the X.509 certificate. +// It first checks if the certificate contains an admin subject that exists in the +// collection of admins. The CA is always allowed to sign those. If the cert contains +// different names and a policy is configured, the policy will be executed against +// the cert to see if the CA is allowed to sign it. +func (a *Authority) isAllowedToSign(cert *x509.Certificate) (bool, error) { + + // // check if certificate is an admin identity token certificate and the admin subject exists + // b := isAdminIdentityTokenCertificate(cert) + // _ = b + + // if isAdminIdentityTokenCertificate(cert) && a.admins.HasSubject(cert.Subject.CommonName) { + // return true, nil + // } + + // if no policy is configured, the cert is implicitly allowed + if a.x509Policy == nil { + return true, nil + } + + return a.x509Policy.AreCertificateNamesAllowed(cert) +} + +func isAdminIdentityTokenCertificate(cert *x509.Certificate) bool { + + // TODO: remove this check + + if cert.Issuer.CommonName != "" { + return false + } + + subject := cert.Subject.CommonName + if subject == "" { + return false + } + + dnsNames := cert.DNSNames + if len(dnsNames) != 1 { + return false + } + + if dnsNames[0] != subject { + return false + } + + extras := cert.ExtraExtensions + if len(extras) != 1 { + return false + } + + extra := extras[0] + + return extra.Id.Equal(asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 37476, 9000, 64, 1}) +} + // Renew creates a new Certificate identical to the old certificate, except // with a validity window that begins 'now'. func (a *Authority) Renew(oldCert *x509.Certificate) ([]*x509.Certificate, error) { diff --git a/policy/engine.go b/policy/engine.go index e9038dd0..63d8452a 100755 --- a/policy/engine.go +++ b/policy/engine.go @@ -10,9 +10,10 @@ import ( "reflect" "strings" - "go.step.sm/crypto/x509util" "golang.org/x/crypto/ssh" "golang.org/x/net/idna" + + "go.step.sm/crypto/x509util" ) type NamePolicyReason int @@ -39,7 +40,7 @@ type NamePolicyError struct { Detail string } -func (e NamePolicyError) Error() string { +func (e *NamePolicyError) Error() string { switch e.Reason { case NotAuthorizedForThisName: return "not authorized to sign for this name: " + e.Detail @@ -295,7 +296,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA // then return error, because DNS should be explicitly configured to be allowed in that case. In case there are // (other) excluded constraints, we'll allow a DNS (implicit allow; currently). if e.numberOfDNSDomainConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { - return NamePolicyError{ + return &NamePolicyError{ Reason: NotAuthorizedForThisName, Detail: fmt.Sprintf("dns %q is not explicitly permitted by any constraint", dns), } @@ -307,7 +308,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA } parsedDNS, err := idna.Lookup.ToASCII(dns) if err != nil { - return NamePolicyError{ + return &NamePolicyError{ Reason: CannotParseDomain, Detail: fmt.Sprintf("dns %q cannot be converted to ASCII", dns), } @@ -316,7 +317,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA parsedDNS = "*" + parsedDNS } if _, ok := domainToReverseLabels(parsedDNS); !ok { - return NamePolicyError{ + return &NamePolicyError{ Reason: CannotParseDomain, Detail: fmt.Sprintf("cannot parse dns %q", dns), } @@ -331,7 +332,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA for _, ip := range ips { if e.numberOfIPRangeConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { - return NamePolicyError{ + return &NamePolicyError{ Reason: NotAuthorizedForThisName, Detail: fmt.Sprintf("ip %q is not explicitly permitted by any constraint", ip.String()), } @@ -346,14 +347,14 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA for _, email := range emailAddresses { if e.numberOfEmailAddressConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { - return NamePolicyError{ + return &NamePolicyError{ Reason: NotAuthorizedForThisName, Detail: fmt.Sprintf("email %q is not explicitly permitted by any constraint", email), } } mailbox, ok := parseRFC2821Mailbox(email) if !ok { - return NamePolicyError{ + return &NamePolicyError{ Reason: CannotParseRFC822Name, Detail: fmt.Sprintf("invalid rfc822Name %q", mailbox), } @@ -363,7 +364,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA // https://datatracker.ietf.org/doc/html/rfc5280#section-7.5 domainASCII, err := idna.ToASCII(mailbox.domain) if err != nil { - return NamePolicyError{ + return &NamePolicyError{ Reason: CannotParseDomain, Detail: fmt.Sprintf("cannot parse email domain %q", email), } @@ -381,7 +382,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA for _, uri := range uris { if e.numberOfURIDomainConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { - return NamePolicyError{ + return &NamePolicyError{ Reason: NotAuthorizedForThisName, Detail: fmt.Sprintf("uri %q is not explicitly permitted by any constraint", uri.String()), } @@ -396,7 +397,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA for _, principal := range principals { if e.numberOfPrincipalConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { - return NamePolicyError{ + return &NamePolicyError{ Reason: NotAuthorizedForThisName, Detail: fmt.Sprintf("username principal %q is not explicitly permitted by any constraint", principal), } @@ -431,14 +432,14 @@ func checkNameConstraints( constraint := excludedValue.Index(i).Interface() match, err := match(parsedName, constraint) if err != nil { - return NamePolicyError{ + return &NamePolicyError{ Reason: CannotMatchNameToConstraint, Detail: err.Error(), } } if match { - return NamePolicyError{ + return &NamePolicyError{ Reason: NotAuthorizedForThisName, Detail: fmt.Sprintf("%s %q is excluded by constraint %q", nameType, name, constraint), } @@ -452,7 +453,7 @@ func checkNameConstraints( constraint := permittedValue.Index(i).Interface() var err error if ok, err = match(parsedName, constraint); err != nil { - return NamePolicyError{ + return &NamePolicyError{ Reason: CannotMatchNameToConstraint, Detail: err.Error(), } @@ -464,7 +465,7 @@ func checkNameConstraints( } if !ok { - return NamePolicyError{ + return &NamePolicyError{ Reason: NotAuthorizedForThisName, Detail: fmt.Sprintf("%s %q is not permitted by any constraint", nameType, name), } diff --git a/policy/engine_test.go b/policy/engine_test.go index 0259e8de..f7a4b20a 100755 --- a/policy/engine_test.go +++ b/policy/engine_test.go @@ -13,7 +13,7 @@ import ( ) // TODO(hs): the functionality in the policy engine is a nice candidate for trying fuzzing on -// TODO(hs): more complex uses cases that combine multiple names and permitted/excluded entries +// TODO(hs): more complex use cases that combine multiple names and permitted/excluded entries func TestNamePolicyEngine_matchDomainConstraint(t *testing.T) { tests := []struct { diff --git a/policy/options_test.go b/policy/options_test.go index 0fc54aa2..8a64f282 100644 --- a/policy/options_test.go +++ b/policy/options_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/smallstep/assert" )