From 1e808b61e5cf50e2a117e427d847fec1922e7529 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 17 Jan 2022 23:36:13 +0100 Subject: [PATCH] Merge logic for X509 and SSH policy --- authority/provisioner/policy.go | 45 +- authority/provisioner/provisioner.go | 7 +- authority/provisioner/sign_options.go | 6 +- authority/provisioner/sign_ssh_options.go | 6 +- policy/{x509/x509.go => engine.go} | 86 +++- policy/{x509/x509_test.go => engine_test.go} | 2 +- policy/{x509 => }/options.go | 22 +- policy/{x509 => }/options_test.go | 2 +- policy/ssh.go | 9 + policy/ssh/options.go | 99 ---- policy/ssh/ssh.go | 472 ------------------- policy/ssh/ssh_test.go | 261 ---------- policy/x509.go | 14 + 13 files changed, 153 insertions(+), 878 deletions(-) rename policy/{x509/x509.go => engine.go} (86%) rename policy/{x509/x509_test.go => engine_test.go} (99%) rename policy/{x509 => }/options.go (97%) rename policy/{x509 => }/options_test.go (99%) create mode 100644 policy/ssh.go delete mode 100644 policy/ssh/options.go delete mode 100644 policy/ssh/ssh.go delete mode 100644 policy/ssh/ssh_test.go create mode 100644 policy/x509.go diff --git a/authority/provisioner/policy.go b/authority/provisioner/policy.go index 1adfd115..5fe31935 100644 --- a/authority/provisioner/policy.go +++ b/authority/provisioner/policy.go @@ -1,70 +1,69 @@ package provisioner import ( - sshpolicy "github.com/smallstep/certificates/policy/ssh" - x509policy "github.com/smallstep/certificates/policy/x509" + "github.com/smallstep/certificates/policy" ) // newX509PolicyEngine creates a new x509 name policy engine -func newX509PolicyEngine(x509Opts *X509Options) (*x509policy.NamePolicyEngine, error) { +func newX509PolicyEngine(x509Opts *X509Options) (policy.X509NamePolicyEngine, error) { if x509Opts == nil { return nil, nil } - options := []x509policy.NamePolicyOption{ - x509policy.WithSubjectCommonNameVerification(), // enable x509 Subject Common Name validation by default + options := []policy.NamePolicyOption{ + policy.WithSubjectCommonNameVerification(), // enable x509 Subject Common Name validation by default } allowed := x509Opts.GetAllowedNameOptions() if allowed != nil && allowed.HasNames() { options = append(options, - x509policy.WithPermittedDNSDomains(allowed.DNSDomains), - x509policy.WithPermittedCIDRs(allowed.IPRanges), // TODO(hs): support IPs in addition to ranges - x509policy.WithPermittedEmailAddresses(allowed.EmailAddresses), - x509policy.WithPermittedURIDomains(allowed.URIDomains), + policy.WithPermittedDNSDomains(allowed.DNSDomains), + policy.WithPermittedCIDRs(allowed.IPRanges), // TODO(hs): support IPs in addition to ranges + policy.WithPermittedEmailAddresses(allowed.EmailAddresses), + policy.WithPermittedURIDomains(allowed.URIDomains), ) } denied := x509Opts.GetDeniedNameOptions() if denied != nil && denied.HasNames() { options = append(options, - x509policy.WithExcludedDNSDomains(denied.DNSDomains), - x509policy.WithExcludedCIDRs(denied.IPRanges), // TODO(hs): support IPs in addition to ranges - x509policy.WithExcludedEmailAddresses(denied.EmailAddresses), - x509policy.WithExcludedURIDomains(denied.URIDomains), + policy.WithExcludedDNSDomains(denied.DNSDomains), + policy.WithExcludedCIDRs(denied.IPRanges), // TODO(hs): support IPs in addition to ranges + policy.WithExcludedEmailAddresses(denied.EmailAddresses), + policy.WithExcludedURIDomains(denied.URIDomains), ) } - return x509policy.New(options...) + return policy.New(options...) } // newSSHPolicyEngine creates a new SSH name policy engine -func newSSHPolicyEngine(sshOpts *SSHOptions) (*sshpolicy.NamePolicyEngine, error) { +func newSSHPolicyEngine(sshOpts *SSHOptions) (policy.SSHNamePolicyEngine, error) { if sshOpts == nil { return nil, nil } - options := []sshpolicy.NamePolicyOption{} + options := []policy.NamePolicyOption{} allowed := sshOpts.GetAllowedNameOptions() if allowed != nil && allowed.HasNames() { options = append(options, - sshpolicy.WithPermittedDNSDomains(allowed.DNSDomains), // TODO(hs): be a bit more lenient w.r.t. the format of domains? I.e. allow "*.localhost" instead of the ".localhost", which is what Name Constraints do. - sshpolicy.WithPermittedEmailAddresses(allowed.EmailAddresses), - sshpolicy.WithPermittedPrincipals(allowed.Principals), + policy.WithPermittedDNSDomains(allowed.DNSDomains), // TODO(hs): be a bit more lenient w.r.t. the format of domains? I.e. allow "*.localhost" instead of the ".localhost", which is what Name Constraints do. + policy.WithPermittedEmailAddresses(allowed.EmailAddresses), + policy.WithPermittedPrincipals(allowed.Principals), ) } denied := sshOpts.GetDeniedNameOptions() if denied != nil && denied.HasNames() { options = append(options, - sshpolicy.WithExcludedDNSDomains(denied.DNSDomains), // TODO(hs): be a bit more lenient w.r.t. the format of domains? I.e. allow "*.localhost" instead of the ".localhost", which is what Name Constraints do. - sshpolicy.WithExcludedEmailAddresses(denied.EmailAddresses), - sshpolicy.WithExcludedPrincipals(denied.Principals), + policy.WithExcludedDNSDomains(denied.DNSDomains), // TODO(hs): be a bit more lenient w.r.t. the format of domains? I.e. allow "*.localhost" instead of the ".localhost", which is what Name Constraints do. + policy.WithExcludedEmailAddresses(denied.EmailAddresses), + policy.WithExcludedPrincipals(denied.Principals), ) } - return sshpolicy.New(options...) + return policy.New(options...) } diff --git a/authority/provisioner/provisioner.go b/authority/provisioner/provisioner.go index 2b98f0cf..a7d6e01d 100644 --- a/authority/provisioner/provisioner.go +++ b/authority/provisioner/provisioner.go @@ -12,8 +12,7 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/db" "github.com/smallstep/certificates/errs" - sshpolicy "github.com/smallstep/certificates/policy/ssh" - x509policy "github.com/smallstep/certificates/policy/x509" + "github.com/smallstep/certificates/policy" "golang.org/x/crypto/ssh" ) @@ -307,8 +306,8 @@ func SanitizeSSHUserPrincipal(email string) string { } type base struct { - x509PolicyEngine *x509policy.NamePolicyEngine - sshPolicyEngine *sshpolicy.NamePolicyEngine + x509PolicyEngine policy.X509NamePolicyEngine + sshPolicyEngine policy.SSHNamePolicyEngine } // AuthorizeSign returns an unimplemented error. Provisioners should overwrite diff --git a/authority/provisioner/sign_options.go b/authority/provisioner/sign_options.go index ccc55435..a0e27f6d 100755 --- a/authority/provisioner/sign_options.go +++ b/authority/provisioner/sign_options.go @@ -16,7 +16,7 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - x509policy "github.com/smallstep/certificates/policy/x509" + "github.com/smallstep/certificates/policy" "go.step.sm/crypto/keyutil" "go.step.sm/crypto/x509util" ) @@ -408,11 +408,11 @@ func (v *validityValidator) Valid(cert *x509.Certificate, o SignOptions) error { // x509NamePolicyValidator validates that the certificate (to be signed) // contains only allowed SANs. type x509NamePolicyValidator struct { - policyEngine *x509policy.NamePolicyEngine + policyEngine policy.X509NamePolicyEngine } // newX509NamePolicyValidator return a new SANs allow/deny validator. -func newX509NamePolicyValidator(engine *x509policy.NamePolicyEngine) *x509NamePolicyValidator { +func newX509NamePolicyValidator(engine policy.X509NamePolicyEngine) *x509NamePolicyValidator { return &x509NamePolicyValidator{ policyEngine: engine, } diff --git a/authority/provisioner/sign_ssh_options.go b/authority/provisioner/sign_ssh_options.go index e5bd2121..e52d3aa7 100644 --- a/authority/provisioner/sign_ssh_options.go +++ b/authority/provisioner/sign_ssh_options.go @@ -10,7 +10,7 @@ import ( "github.com/pkg/errors" "github.com/smallstep/certificates/errs" - sshpolicy "github.com/smallstep/certificates/policy/ssh" + "github.com/smallstep/certificates/policy" "go.step.sm/crypto/keyutil" "golang.org/x/crypto/ssh" ) @@ -448,11 +448,11 @@ func (v sshDefaultPublicKeyValidator) Valid(cert *ssh.Certificate, o SignSSHOpti // sshNamePolicyValidator validates that the certificate (to be signed) // contains only allowed principals. type sshNamePolicyValidator struct { - policyEngine *sshpolicy.NamePolicyEngine + policyEngine policy.SSHNamePolicyEngine } // newSSHNamePolicyValidator return a new SSH allow/deny validator. -func newSSHNamePolicyValidator(engine *sshpolicy.NamePolicyEngine) *sshNamePolicyValidator { +func newSSHNamePolicyValidator(engine policy.SSHNamePolicyEngine) *sshNamePolicyValidator { return &sshNamePolicyValidator{ policyEngine: engine, } diff --git a/policy/x509/x509.go b/policy/engine.go similarity index 86% rename from policy/x509/x509.go rename to policy/engine.go index 5a8337b9..10da6bc9 100755 --- a/policy/x509/x509.go +++ b/policy/engine.go @@ -1,4 +1,4 @@ -package x509policy +package policy import ( "bytes" @@ -12,6 +12,7 @@ import ( "github.com/pkg/errors" "go.step.sm/crypto/x509util" + "golang.org/x/crypto/ssh" ) type CertificateInvalidError struct { @@ -47,7 +48,7 @@ func (e CertificateInvalidError) Error() string { // NamePolicyEngine can be used to check that a CSR or Certificate meets all allowed and // denied names before a CA creates and/or signs the Certificate. -// TODO(hs): the x509 RFC also defines name checks on directory name; support that? +// TODO(hs): the X509 RFC also defines name checks on directory name; support that? // TODO(hs): implement Stringer interface: describe the contents of the NamePolicyEngine? type NamePolicyEngine struct { @@ -65,12 +66,15 @@ type NamePolicyEngine struct { excludedEmailAddresses []string permittedURIDomains []string excludedURIDomains []string + permittedPrincipals []string + excludedPrincipals []string // some internal counts for housekeeping numberOfDNSDomainConstraints int numberOfIPRangeConstraints int numberOfEmailAddressConstraints int numberOfURIDomainConstraints int + numberOfPrincipalConstraints int totalNumberOfPermittedConstraints int totalNumberOfExcludedConstraints int totalNumberOfConstraints int @@ -90,22 +94,25 @@ func New(opts ...NamePolicyOption) (*NamePolicyEngine, error) { e.permittedIPRanges = removeDuplicateIPRanges(e.permittedIPRanges) e.permittedEmailAddresses = removeDuplicates(e.permittedEmailAddresses) e.permittedURIDomains = removeDuplicates(e.permittedURIDomains) + e.permittedPrincipals = removeDuplicates(e.permittedPrincipals) e.excludedDNSDomains = removeDuplicates(e.excludedDNSDomains) e.excludedIPRanges = removeDuplicateIPRanges(e.excludedIPRanges) e.excludedEmailAddresses = removeDuplicates(e.excludedEmailAddresses) e.excludedURIDomains = removeDuplicates(e.excludedURIDomains) + e.excludedPrincipals = removeDuplicates(e.excludedPrincipals) 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.permittedEmailAddresses) + len(e.permittedURIDomains) + len(e.permittedPrincipals) e.totalNumberOfExcludedConstraints = len(e.excludedDNSDomains) + len(e.excludedIPRanges) + - len(e.excludedEmailAddresses) + len(e.excludedURIDomains) + len(e.excludedEmailAddresses) + len(e.excludedURIDomains) + len(e.excludedPrincipals) e.totalNumberOfConstraints = e.totalNumberOfPermittedConstraints + e.totalNumberOfExcludedConstraints @@ -151,7 +158,7 @@ func (e *NamePolicyEngine) AreCertificateNamesAllowed(cert *x509.Certificate) (b if e.verifySubjectCommonName { appendSubjectCommonName(cert.Subject, &dnsNames, &ips, &emails, &uris) } - if err := e.validateNames(dnsNames, ips, emails, uris); err != nil { + if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil { return false, err } return true, nil @@ -165,7 +172,7 @@ func (e *NamePolicyEngine) AreCSRNamesAllowed(csr *x509.CertificateRequest) (boo if e.verifySubjectCommonName { appendSubjectCommonName(csr.Subject, &dnsNames, &ips, &emails, &uris) } - if err := e.validateNames(dnsNames, ips, emails, uris); err != nil { + if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil { return false, err } return true, nil @@ -175,7 +182,7 @@ func (e *NamePolicyEngine) AreCSRNamesAllowed(csr *x509.CertificateRequest) (boo // The SANs are first split into DNS names, IPs, email addresses and URIs. func (e *NamePolicyEngine) AreSANsAllowed(sans []string) (bool, error) { dnsNames, ips, emails, uris := x509util.SplitSANs(sans) - if err := e.validateNames(dnsNames, ips, emails, uris); err != nil { + if err := e.validateNames(dnsNames, ips, emails, uris, []string{}); err != nil { return false, err } return true, nil @@ -183,7 +190,7 @@ func (e *NamePolicyEngine) AreSANsAllowed(sans []string) (bool, error) { // IsDNSAllowed verifies a single DNS domain is allowed. func (e *NamePolicyEngine) IsDNSAllowed(dns string) (bool, error) { - if err := e.validateNames([]string{dns}, []net.IP{}, []string{}, []*url.URL{}); err != nil { + if err := e.validateNames([]string{dns}, []net.IP{}, []string{}, []*url.URL{}, []string{}); err != nil { return false, err } return true, nil @@ -191,7 +198,16 @@ func (e *NamePolicyEngine) IsDNSAllowed(dns string) (bool, error) { // IsIPAllowed verifies a single IP domain is allowed. func (e *NamePolicyEngine) IsIPAllowed(ip net.IP) (bool, error) { - if err := e.validateNames([]string{}, []net.IP{ip}, []string{}, []*url.URL{}); err != nil { + if err := e.validateNames([]string{}, []net.IP{ip}, []string{}, []*url.URL{}, []string{}); err != nil { + return false, err + } + return true, nil +} + +// ArePrincipalsAllowed verifies that all principals in an SSH certificate are allowed. +func (e *NamePolicyEngine) ArePrincipalsAllowed(cert *ssh.Certificate) (bool, error) { + dnsNames, emails, usernames := splitPrincipals(cert.ValidPrincipals) + if err := e.validateNames(dnsNames, []net.IP{}, emails, []*url.URL{}, usernames); err != nil { return false, err } return true, nil @@ -217,10 +233,27 @@ func appendSubjectCommonName(subject pkix.Name, dnsNames *[]string, ips *[]net.I } } +// splitPrincipals splits SSH certificate principals into DNS names, emails and user names. +func splitPrincipals(principals []string) (dnsNames, emails, usernames []string) { + dnsNames = []string{} + emails = []string{} + usernames = []string{} + for _, principal := range principals { + if strings.Contains(principal, "@") { + emails = append(emails, principal) + } else if len(strings.Split(principal, ".")) > 1 { + dnsNames = append(dnsNames, principal) + } else { + usernames = append(usernames, principal) + } + } + return +} + // validateNames verifies that all names are allowed. // Its logic follows that of (a large part of) the (c *Certificate) isValid() function // in https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go -func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailAddresses []string, uris []*url.URL) error { +func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailAddresses []string, uris []*url.URL, usernames []string) error { // nothing to compare against; return early if e.totalNumberOfConstraints == 0 { @@ -309,6 +342,34 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA } } + //"dns": ["*.smallstep.com"], + //"email": ["@smallstep.com", "@google.com"], + //"principal": ["max", "mariano", "mike"] + /* No regexes for now. But if we ever implement them, they'd probably look like this */ + /*"principal": ["foo.smallstep.com", "/^*\.smallstep\.com$/"]*/ + + // Principals can be single user names (mariano, max, mike, ...), hostnames/domains (*.smallstep.com, host.smallstep.com, ...) and "emails" (max@smallstep.com, @smallstep.com, ...) + // All ValidPrincipals can thus be any one of those, and they can be mixed (mike@smallstep.com, mike, ...); we need to split this? + // Should we assume a generic engine, or can we do it host vs. user based? If host vs. user based, then it becomes easier w.r.t. dns; hosts will only be DNS, right? + // If we assume generic, we _may_ have a harder time distinguishing host vs. user certs. We propose to use host + user specific provisioners, though... + // Perhaps we can do some heuristics on the principal names vs. hostnames (i.e. when only a single label and no dot, then it's a user principal) + + for _, username := range usernames { + if e.numberOfPrincipalConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 { + return CertificateInvalidError{ + Reason: x509.CANotAuthorizedForThisName, + Detail: fmt.Sprintf("username principal %q is not permitted by any constraint", username), + } + } + // TODO: some validation? I.e. allowed characters? + if err := checkNameConstraints("username", username, username, + func(parsedName, constraint interface{}) (bool, error) { + return matchUsernameConstraint(parsedName.(string), constraint.(string)) + }, e.permittedPrincipals, e.excludedPrincipals); err != nil { + return err + } + } + // TODO: when the error is not nil and returned up in the above, we can add // additional context to it (i.e. the cert or csr that was inspected). @@ -753,3 +814,8 @@ 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) { + return strings.EqualFold(username, constraint), nil +} diff --git a/policy/x509/x509_test.go b/policy/engine_test.go similarity index 99% rename from policy/x509/x509_test.go rename to policy/engine_test.go index a2977214..2b3484ab 100755 --- a/policy/x509/x509_test.go +++ b/policy/engine_test.go @@ -1,4 +1,4 @@ -package x509policy +package policy import ( "crypto/x509" diff --git a/policy/x509/options.go b/policy/options.go similarity index 97% rename from policy/x509/options.go rename to policy/options.go index ecd793a7..f628a083 100755 --- a/policy/x509/options.go +++ b/policy/options.go @@ -1,4 +1,4 @@ -package x509policy +package policy import ( "fmt" @@ -538,6 +538,26 @@ func AddExcludedURIDomain(uriDomain string) NamePolicyOption { } } +func WithPermittedPrincipals(principals []string) NamePolicyOption { + return func(g *NamePolicyEngine) error { + // for _, principal := range principals { + // // TODO: validation? + // } + g.permittedPrincipals = principals + return nil + } +} + +func WithExcludedPrincipals(principals []string) NamePolicyOption { + return func(g *NamePolicyEngine) error { + // for _, principal := range principals { + // // TODO: validation? + // } + g.excludedPrincipals = principals + return nil + } +} + func normalizeAndValidateDNSDomainConstraint(constraint string) (string, error) { normalizedConstraint := strings.TrimSpace(constraint) if strings.Contains(normalizedConstraint, "..") { diff --git a/policy/x509/options_test.go b/policy/options_test.go similarity index 99% rename from policy/x509/options_test.go rename to policy/options_test.go index 304e208f..5e84d20e 100644 --- a/policy/x509/options_test.go +++ b/policy/options_test.go @@ -1,4 +1,4 @@ -package x509policy +package policy import ( "net" diff --git a/policy/ssh.go b/policy/ssh.go new file mode 100644 index 00000000..0b4290d2 --- /dev/null +++ b/policy/ssh.go @@ -0,0 +1,9 @@ +package policy + +import ( + "golang.org/x/crypto/ssh" +) + +type SSHNamePolicyEngine interface { + ArePrincipalsAllowed(cert *ssh.Certificate) (bool, error) +} diff --git a/policy/ssh/options.go b/policy/ssh/options.go deleted file mode 100644 index 30b68a1d..00000000 --- a/policy/ssh/options.go +++ /dev/null @@ -1,99 +0,0 @@ -package sshpolicy - -import ( - "fmt" - "strings" - - "github.com/pkg/errors" -) - -type NamePolicyOption func(g *NamePolicyEngine) error - -func WithPermittedDNSDomains(domains []string) NamePolicyOption { - return func(g *NamePolicyEngine) error { - for _, domain := range domains { - if err := validateDNSDomainConstraint(domain); err != nil { - return errors.Errorf("cannot parse permitted domain constraint %q", domain) - } - } - g.permittedDNSDomains = domains - return nil - } -} - -func WithExcludedDNSDomains(domains []string) NamePolicyOption { - return func(g *NamePolicyEngine) error { - for _, domain := range domains { - if err := validateDNSDomainConstraint(domain); err != nil { - return errors.Errorf("cannot parse excluded domain constraint %q", domain) - } - } - g.excludedDNSDomains = domains - return nil - } -} - -func WithPermittedEmailAddresses(emailAddresses []string) NamePolicyOption { - return func(g *NamePolicyEngine) error { - for _, email := range emailAddresses { - if err := validateEmailConstraint(email); err != nil { - return err - } - } - g.permittedEmailAddresses = emailAddresses - return nil - } -} - -func WithExcludedEmailAddresses(emailAddresses []string) NamePolicyOption { - return func(g *NamePolicyEngine) error { - for _, email := range emailAddresses { - if err := validateEmailConstraint(email); err != nil { - return err - } - } - g.excludedEmailAddresses = emailAddresses - return nil - } -} - -func WithPermittedPrincipals(principals []string) NamePolicyOption { - return func(g *NamePolicyEngine) error { - // for _, principal := range principals { - // // TODO: validation? - // } - g.permittedPrincipals = principals - return nil - } -} - -func WithExcludedPrincipals(principals []string) NamePolicyOption { - return func(g *NamePolicyEngine) error { - // for _, principal := range principals { - // // TODO: validation? - // } - g.excludedPrincipals = principals - return nil - } -} - -func validateDNSDomainConstraint(domain string) error { - if _, ok := domainToReverseLabels(domain); !ok { - return errors.Errorf("cannot parse permitted domain constraint %q", domain) - } - return nil -} - -func validateEmailConstraint(constraint string) error { - if strings.Contains(constraint, "@") { - _, ok := parseRFC2821Mailbox(constraint) - if !ok { - return fmt.Errorf("cannot parse email constraint %q", constraint) - } - } - _, ok := domainToReverseLabels(constraint) - if !ok { - return fmt.Errorf("cannot parse email domain constraint %q", constraint) - } - return nil -} diff --git a/policy/ssh/ssh.go b/policy/ssh/ssh.go deleted file mode 100644 index dcf5394f..00000000 --- a/policy/ssh/ssh.go +++ /dev/null @@ -1,472 +0,0 @@ -package sshpolicy - -import ( - "bytes" - "crypto/x509" - "fmt" - "reflect" - "strings" - - "github.com/pkg/errors" - "golang.org/x/crypto/ssh" -) - -type CertificateInvalidError struct { - Reason x509.InvalidReason - Detail string -} - -func (e CertificateInvalidError) Error() string { - switch e.Reason { - // TODO: include logical errors for this package; exlude ones that don't make sense for its current use case? - // TODO: currently only CANotAuthorizedForThisName is used by this package; we're not checking the other things in CSRs in this package. - case x509.NotAuthorizedToSign: - return "not authorized to sign other certificates" // TODO: this one doesn't make sense for this pkg - case x509.Expired: - return "csr has expired or is not yet valid: " + e.Detail - case x509.CANotAuthorizedForThisName: - return "not authorized to sign for this name: " + e.Detail - case x509.CANotAuthorizedForExtKeyUsage: - return "not authorized for an extended key usage: " + e.Detail - case x509.TooManyIntermediates: - return "too many intermediates for path length constraint" - case x509.IncompatibleUsage: - return "csr specifies an incompatible key usage" - case x509.NameMismatch: - return "issuer name does not match subject from issuing certificate" - case x509.NameConstraintsWithoutSANs: - return "issuer has name constraints but csr doesn't have a SAN extension" - case x509.UnconstrainedName: - return "issuer has name constraints but csr contains unknown or unconstrained name: " + e.Detail - } - return "unknown error" -} - -type NamePolicyEngine struct { - options []NamePolicyOption - permittedDNSDomains []string - excludedDNSDomains []string - permittedEmailAddresses []string - excludedEmailAddresses []string - permittedPrincipals []string // TODO: rename to usernames, as principals can be host, user@ (like mail) and usernames? - excludedPrincipals []string -} - -func New(opts ...NamePolicyOption) (*NamePolicyEngine, error) { - - e := &NamePolicyEngine{} // TODO: embed an x509 engine instead of building it again? - e.options = append(e.options, opts...) - for _, option := range e.options { - if err := option(e); err != nil { - return nil, err - } - } - - return e, nil -} - -func (e *NamePolicyEngine) ArePrincipalsAllowed(cert *ssh.Certificate) (bool, error) { - dnsNames, emails, userNames := splitPrincipals(cert.ValidPrincipals) - if err := e.validateNames(dnsNames, emails, userNames); err != nil { - return false, err - } - return true, nil -} - -func (e *NamePolicyEngine) validateNames(dnsNames, emails, userNames []string) error { - //"dns": ["*.smallstep.com"], - //"email": ["@smallstep.com", "@google.com"], - //"principal": ["max", "mariano", "mike"] - /* No regexes for now. But if we ever implement them, they'd probably look like this */ - /*"principal": ["foo.smallstep.com", "/^*\.smallstep\.com$/"]*/ - - // Principals can be single user names (mariano, max, mike, ...), hostnames/domains (*.smallstep.com, host.smallstep.com, ...) and "emails" (max@smallstep.com, @smallstep.com, ...) - // All ValidPrincipals can thus be any one of those, and they can be mixed (mike@smallstep.com, mike, ...); we need to split this? - // Should we assume a generic engine, or can we do it host vs. user based? If host vs. user based, then it becomes easier w.r.t. dns; hosts will only be DNS, right? - // If we assume generic, we _may_ have a harder time distinguishing host vs. user certs. We propose to use host + user specific provisioners, though... - // Perhaps we can do some heuristics on the principal names vs. hostnames (i.e. when only a single label and no dot, then it's a user principal) - - for _, dns := range dnsNames { - if _, ok := domainToReverseLabels(dns); !ok { - return errors.Errorf("cannot parse dns %q", dns) - } - if err := checkNameConstraints("dns", dns, dns, - func(parsedName, constraint interface{}) (bool, error) { - return matchDomainConstraint(parsedName.(string), constraint.(string)) - }, e.permittedDNSDomains, e.excludedDNSDomains); err != nil { - return err - } - } - - for _, email := range emails { - mailbox, ok := parseRFC2821Mailbox(email) - if !ok { - return fmt.Errorf("cannot parse rfc822Name %q", mailbox) - } - if err := checkNameConstraints("email", email, mailbox, - func(parsedName, constraint interface{}) (bool, error) { - return matchEmailConstraint(parsedName.(rfc2821Mailbox), constraint.(string)) - }, e.permittedEmailAddresses, e.excludedEmailAddresses); err != nil { - return err - } - } - - for _, userName := range userNames { - // TODO: some validation? I.e. allowed characters? - if err := checkNameConstraints("username", userName, userName, - func(parsedName, constraint interface{}) (bool, error) { - return matchUserNameConstraint(parsedName.(string), constraint.(string)) - }, e.permittedPrincipals, e.excludedPrincipals); err != nil { - return err - } - } - - return nil -} - -// splitPrincipals splits SSH certificate principals into DNS names, emails and user names. -func splitPrincipals(principals []string) (dnsNames, emails, userNames []string) { - dnsNames = []string{} - emails = []string{} - userNames = []string{} - for _, principal := range principals { - if strings.Contains(principal, "@") { - emails = append(emails, principal) - } else if len(strings.Split(principal, ".")) > 1 { - dnsNames = append(dnsNames, principal) - } else { - userNames = append(userNames, principal) - } - } - return -} - -// checkNameConstraints checks that c permits a child certificate to claim the -// given name, of type nameType. The argument parsedName contains the parsed -// form of name, suitable for passing to the match function. The total number -// of comparisons is tracked in the given count and should not exceed the given -// limit. -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go -func checkNameConstraints( - nameType string, - name string, - parsedName interface{}, - match func(parsedName, constraint interface{}) (match bool, err error), - permitted, excluded interface{}) error { - - excludedValue := reflect.ValueOf(excluded) - - // *count += excludedValue.Len() - // if *count > maxConstraintComparisons { - // return x509.CertificateInvalidError{c, x509.TooManyConstraints, ""} - // } - - // TODO: fix the errors; return our own, because we don't have cert ... - - for i := 0; i < excludedValue.Len(); i++ { - constraint := excludedValue.Index(i).Interface() - match, err := match(parsedName, constraint) - if err != nil { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, - Detail: err.Error(), - } - } - - if match { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, - Detail: fmt.Sprintf("%s %q is excluded by constraint %q", nameType, name, constraint), - } - } - } - - permittedValue := reflect.ValueOf(permitted) - - // *count += permittedValue.Len() - // if *count > maxConstraintComparisons { - // return x509.CertificateInvalidError{c, x509.TooManyConstraints, ""} - // } - - ok := true - for i := 0; i < permittedValue.Len(); i++ { - constraint := permittedValue.Index(i).Interface() - var err error - if ok, err = match(parsedName, constraint); err != nil { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, - Detail: err.Error(), - } - } - - if ok { - break - } - } - - if !ok { - return CertificateInvalidError{ - Reason: x509.CANotAuthorizedForThisName, - Detail: fmt.Sprintf("%s %q is not permitted by any constraint", nameType, name), - } - } - - return nil -} - -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go -func matchDomainConstraint(domain, constraint string) (bool, error) { - // The meaning of zero length constraints is not specified, but this - // code follows NSS and accepts them as matching everything. - if constraint == "" { - return true, nil - } - - domainLabels, ok := domainToReverseLabels(domain) - if !ok { - return false, fmt.Errorf("cannot parse domain %q", domain) - } - - // RFC 5280 says that a leading period in a domain name means that at - // least one label must be prepended, but only for URI and email - // constraints, not DNS constraints. The code also supports that - // behavior for DNS constraints. - - mustHaveSubdomains := false - if constraint[0] == '.' { - mustHaveSubdomains = true - constraint = constraint[1:] - } - - constraintLabels, ok := domainToReverseLabels(constraint) - if !ok { - return false, fmt.Errorf("cannot parse domain %q", constraint) - } - - if len(domainLabels) < len(constraintLabels) || - (mustHaveSubdomains && len(domainLabels) == len(constraintLabels)) { - return false, nil - } - - for i, constraintLabel := range constraintLabels { - if !strings.EqualFold(constraintLabel, domainLabels[i]) { - return false, nil - } - } - - return true, nil -} - -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go -func matchEmailConstraint(mailbox rfc2821Mailbox, constraint string) (bool, error) { - // If the constraint contains an @, then it specifies an exact mailbox name. - if strings.Contains(constraint, "@") { - constraintMailbox, ok := parseRFC2821Mailbox(constraint) - if !ok { - return false, fmt.Errorf("cannot parse constraint %q", constraint) - } - return mailbox.local == constraintMailbox.local && strings.EqualFold(mailbox.domain, constraintMailbox.domain), nil - } - - // Otherwise the constraint is like a DNS constraint of the domain part - // of the mailbox. - return matchDomainConstraint(mailbox.domain, constraint) -} - -// matchUserNameConstraint performs a string literal match against a constraint -func matchUserNameConstraint(userName, constraint string) (bool, error) { - return userName == constraint, nil -} - -// TODO: decrease code duplication: single policy engine again, with principals added, but not used in x509? -// Not sure how I'd like to model that in Go, though: use (embedded) structs? interfaces? An x509 name policy engine -// interface could expose the methods that are useful to x509; the SSH name policy engine interfaces could do the -// same for SSH ones. One interface for both (with no methods?); then two, so that not all name policy options -// can be executed on both types? The shared ones could then maybe use the one with no methods? But we need protect -// it from being applied to just any type, of course. Not sure if Go allows us to do something like that, though. -// Maybe some kind of dummy function helps there? - -// domainToReverseLabels converts a textual domain name like foo.example.com to -// the list of labels in reverse order, e.g. ["com", "example", "foo"]. -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go -func domainToReverseLabels(domain string) (reverseLabels []string, ok bool) { - for len(domain) > 0 { - if i := strings.LastIndexByte(domain, '.'); i == -1 { - reverseLabels = append(reverseLabels, domain) - domain = "" - } else { - reverseLabels = append(reverseLabels, domain[i+1:]) - domain = domain[:i] - } - } - - if len(reverseLabels) > 0 && reverseLabels[0] == "" { - // An empty label at the end indicates an absolute value. - return nil, false - } - - for _, label := range reverseLabels { - if label == "" { - // Empty labels are otherwise invalid. - return nil, false - } - - for _, c := range label { - if c < 33 || c > 126 { - // Invalid character. - return nil, false - } - } - } - - return reverseLabels, true -} - -// rfc2821Mailbox represents a “mailbox” (which is an email address to most -// people) by breaking it into the “local” (i.e. before the '@') and “domain” -// parts. -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go -type rfc2821Mailbox struct { - local, domain string -} - -// parseRFC2821Mailbox parses an email address into local and domain parts, -// based on the ABNF for a “Mailbox” from RFC 2821. According to RFC 5280, -// Section 4.2.1.6 that's correct for an rfc822Name from a certificate: “The -// format of an rfc822Name is a "Mailbox" as defined in RFC 2821, Section 4.1.2”. -// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go -func parseRFC2821Mailbox(in string) (mailbox rfc2821Mailbox, ok bool) { - if in == "" { - return mailbox, false - } - - localPartBytes := make([]byte, 0, len(in)/2) - - if in[0] == '"' { - // Quoted-string = DQUOTE *qcontent DQUOTE - // non-whitespace-control = %d1-8 / %d11 / %d12 / %d14-31 / %d127 - // qcontent = qtext / quoted-pair - // qtext = non-whitespace-control / - // %d33 / %d35-91 / %d93-126 - // quoted-pair = ("\" text) / obs-qp - // text = %d1-9 / %d11 / %d12 / %d14-127 / obs-text - // - // (Names beginning with “obs-” are the obsolete syntax from RFC 2822, - // Section 4. Since it has been 16 years, we no longer accept that.) - in = in[1:] - QuotedString: - for { - if in == "" { - return mailbox, false - } - c := in[0] - in = in[1:] - - switch { - case c == '"': - break QuotedString - - case c == '\\': - // quoted-pair - if in == "" { - return mailbox, false - } - if in[0] == 11 || - in[0] == 12 || - (1 <= in[0] && in[0] <= 9) || - (14 <= in[0] && in[0] <= 127) { - localPartBytes = append(localPartBytes, in[0]) - in = in[1:] - } else { - return mailbox, false - } - - case c == 11 || - c == 12 || - // Space (char 32) is not allowed based on the - // BNF, but RFC 3696 gives an example that - // assumes that it is. Several “verified” - // errata continue to argue about this point. - // We choose to accept it. - c == 32 || - c == 33 || - c == 127 || - (1 <= c && c <= 8) || - (14 <= c && c <= 31) || - (35 <= c && c <= 91) || - (93 <= c && c <= 126): - // qtext - localPartBytes = append(localPartBytes, c) - - default: - return mailbox, false - } - } - } else { - // Atom ("." Atom)* - NextChar: - for len(in) > 0 { - // atext from RFC 2822, Section 3.2.4 - c := in[0] - - switch { - case c == '\\': - // Examples given in RFC 3696 suggest that - // escaped characters can appear outside of a - // quoted string. Several “verified” errata - // continue to argue the point. We choose to - // accept it. - in = in[1:] - if in == "" { - return mailbox, false - } - fallthrough - - case ('0' <= c && c <= '9') || - ('a' <= c && c <= 'z') || - ('A' <= c && c <= 'Z') || - c == '!' || c == '#' || c == '$' || c == '%' || - c == '&' || c == '\'' || c == '*' || c == '+' || - c == '-' || c == '/' || c == '=' || c == '?' || - c == '^' || c == '_' || c == '`' || c == '{' || - c == '|' || c == '}' || c == '~' || c == '.': - localPartBytes = append(localPartBytes, in[0]) - in = in[1:] - - default: - break NextChar - } - } - - if len(localPartBytes) == 0 { - return mailbox, false - } - - // From RFC 3696, Section 3: - // “period (".") may also appear, but may not be used to start - // or end the local part, nor may two or more consecutive - // periods appear.” - twoDots := []byte{'.', '.'} - if localPartBytes[0] == '.' || - localPartBytes[len(localPartBytes)-1] == '.' || - bytes.Contains(localPartBytes, twoDots) { - return mailbox, false - } - } - - if in == "" || in[0] != '@' { - return mailbox, false - } - in = in[1:] - - // The RFC species a format for domains, but that's known to be - // violated in practice so we accept that anything after an '@' is the - // domain part. - if _, ok := domainToReverseLabels(in); !ok { - return mailbox, false - } - - mailbox.local = string(localPartBytes) - mailbox.domain = in - return mailbox, true -} diff --git a/policy/ssh/ssh_test.go b/policy/ssh/ssh_test.go deleted file mode 100644 index e56ce592..00000000 --- a/policy/ssh/ssh_test.go +++ /dev/null @@ -1,261 +0,0 @@ -package sshpolicy - -import ( - "testing" - - "golang.org/x/crypto/ssh" -) - -func TestNamePolicyEngine_ArePrincipalsAllowed(t *testing.T) { - type fields struct { - options []NamePolicyOption - permittedDNSDomains []string - excludedDNSDomains []string - permittedEmailAddresses []string - excludedEmailAddresses []string - permittedPrincipals []string - excludedPrincipals []string - } - tests := []struct { - name string - fields fields - cert *ssh.Certificate - want bool - wantErr bool - }{ - { - name: "fail/dns-permitted", - fields: fields{ - permittedDNSDomains: []string{".local"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"host.notlocal"}, - }, - want: false, - wantErr: true, - }, - { - name: "fail/dns-permitted", - fields: fields{ - excludedDNSDomains: []string{".local"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"host.local"}, - }, - want: false, - wantErr: true, - }, - { - name: "fail/mail-permitted", - fields: fields{ - permittedEmailAddresses: []string{"example.local"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"user@example.notlocal"}, - }, - want: false, - wantErr: true, - }, - { - name: "fail/mail-excluded", - fields: fields{ - excludedEmailAddresses: []string{"example.local"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"user@example.local"}, - }, - want: false, - wantErr: true, - }, - { - name: "fail/principal-permitted", - fields: fields{ - permittedPrincipals: []string{"user1"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"user2"}, - }, - want: false, - wantErr: true, - }, - { - name: "fail/principal-excluded", - fields: fields{ - excludedPrincipals: []string{"user"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"user"}, - }, - want: false, - wantErr: true, - }, - { - name: "fail/combined-complex-all-badhost.local", - fields: fields{ - permittedDNSDomains: []string{".local"}, - permittedEmailAddresses: []string{"example.local"}, - permittedPrincipals: []string{"user"}, - excludedDNSDomains: []string{"badhost.local"}, - excludedEmailAddresses: []string{"badmail@example.local"}, - excludedPrincipals: []string{"baduser"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{ - "user", - "user@example.local", - "badhost.local", - }, - }, - want: false, - wantErr: true, - }, - { - name: "ok/no-constraints", - fields: fields{}, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"host.example.com"}, - }, - want: true, - wantErr: false, - }, - { - name: "ok/dns-permitted", - fields: fields{ - permittedDNSDomains: []string{".local"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"example.local"}, - }, - want: true, - wantErr: false, - }, - { - name: "ok/dns-excluded", - fields: fields{ - excludedDNSDomains: []string{".notlocal"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"example.local"}, - }, - want: true, - wantErr: false, - }, - { - name: "ok/mail-permitted", - fields: fields{ - permittedEmailAddresses: []string{"example.local"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"user@example.local"}, - }, - want: true, - wantErr: false, - }, - { - name: "ok/mail-excluded", - fields: fields{ - excludedEmailAddresses: []string{"example.notlocal"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"user@example.local"}, - }, - want: true, - wantErr: false, - }, - { - name: "ok/principal-permitted", - fields: fields{ - permittedPrincipals: []string{"user"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"user"}, - }, - want: true, - wantErr: false, - }, - { - name: "ok/principal-excluded", - fields: fields{ - excludedPrincipals: []string{"someone"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{"user"}, - }, - want: true, - wantErr: false, - }, - { - name: "ok/combined-simple-user-permitted", - fields: fields{ - permittedEmailAddresses: []string{"example.local"}, - permittedPrincipals: []string{"user"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{ - "user", - "user@example.local", - }, - }, - want: true, - wantErr: false, - }, - { - name: "ok/combined-simple-all-permitted", - fields: fields{ - permittedDNSDomains: []string{".local"}, - permittedEmailAddresses: []string{"example.local"}, - permittedPrincipals: []string{"user"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{ - "user", - "user@example.local", - "host.local", - }, - }, - want: true, - wantErr: false, - }, - { - name: "ok/combined-complex-all", - fields: fields{ - permittedDNSDomains: []string{".local"}, - permittedEmailAddresses: []string{"example.local"}, - permittedPrincipals: []string{"user"}, - excludedDNSDomains: []string{"badhost.local"}, - excludedEmailAddresses: []string{"badmail@example.local"}, - excludedPrincipals: []string{"baduser"}, - }, - cert: &ssh.Certificate{ - ValidPrincipals: []string{ - "user", - "user@example.local", - "host.local", - }, - }, - want: true, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - e := &NamePolicyEngine{ - options: tt.fields.options, - permittedDNSDomains: tt.fields.permittedDNSDomains, - excludedDNSDomains: tt.fields.excludedDNSDomains, - permittedEmailAddresses: tt.fields.permittedEmailAddresses, - excludedEmailAddresses: tt.fields.excludedEmailAddresses, - permittedPrincipals: tt.fields.permittedPrincipals, - excludedPrincipals: tt.fields.excludedPrincipals, - } - got, err := e.ArePrincipalsAllowed(tt.cert) - if (err != nil) != tt.wantErr { - t.Errorf("NamePolicyEngine.ArePrincipalsAllowed() error = %v, wantErr %v", err, tt.wantErr) - return - } - if got != tt.want { - t.Errorf("NamePolicyEngine.ArePrincipalsAllowed() = %v, want %v", got, tt.want) - } - }) - } -} diff --git a/policy/x509.go b/policy/x509.go new file mode 100644 index 00000000..0bc35d89 --- /dev/null +++ b/policy/x509.go @@ -0,0 +1,14 @@ +package policy + +import ( + "crypto/x509" + "net" +) + +type X509NamePolicyEngine interface { + AreCertificateNamesAllowed(cert *x509.Certificate) (bool, error) + AreCSRNamesAllowed(csr *x509.CertificateRequest) (bool, error) + AreSANsAllowed(sans []string) (bool, error) + IsDNSAllowed(dns string) (bool, error) + IsIPAllowed(ip net.IP) (bool, error) +}