Archives/smallstep-certificates
2
0
Fork 0
mirror of https://github.com/smallstep/certificates.git synced 2024-11-17 15:29:21 +00:00
Code Issues Packages Projects Releases Wiki Activity

Clean up, improve test cases and coverage

Browse Source
This commit is contained in:
Herman Slatman 2022-01-18 14:39:21 +01:00
parent 1e808b61e5
commit 6440870a80
No known key found for this signature in database
GPG Key ID: F4D8A44EA0A75A4F
10 changed files with 723 additions and 123 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

0
acme/api/order.go Executable file → Normal file
View File

0
authority/provisioner/acme.go Executable file → Normal file
View File

0
authority/provisioner/jwk.go Executable file → Normal file
View File

4
authority/provisioner/options.go Executable file → Normal file
View File

@ -90,7 +90,7 @@ func (o *X509Options) GetDeniedNameOptions() *DeniedX509NameOptions {
// AllowedX509NameOptions models the allowed names
type AllowedX509NameOptions struct {
DNSDomains []string `json:"dns,omitempty"`
IPRanges []string `json:"ip,omitempty"` // TODO(hs): support IPs as well as ranges
IPRanges []string `json:"ip,omitempty"`
EmailAddresses []string `json:"email,omitempty"`
URIDomains []string `json:"uri,omitempty"`
}
@ -98,7 +98,7 @@ type AllowedX509NameOptions struct {
// DeniedX509NameOptions models the denied names
type DeniedX509NameOptions struct {
DNSDomains []string `json:"dns,omitempty"`
IPRanges []string `json:"ip,omitempty"` // TODO(hs): support IPs as well as ranges
IPRanges []string `json:"ip,omitempty"`
EmailAddresses []string `json:"email,omitempty"`
URIDomains []string `json:"uri,omitempty"`
}

4
authority/provisioner/policy.go
View File

@ -19,7 +19,7 @@ func newX509PolicyEngine(x509Opts *X509Options) (policy.X509NamePolicyEngine, er
if allowed != nil && allowed.HasNames() {
options = append(options,
policy.WithPermittedDNSDomains(allowed.DNSDomains),
policy.WithPermittedCIDRs(allowed.IPRanges), // TODO(hs): support IPs in addition to ranges
policy.WithPermittedIPsOrCIDRs(allowed.IPRanges),
policy.WithPermittedEmailAddresses(allowed.EmailAddresses),
policy.WithPermittedURIDomains(allowed.URIDomains),
)
@ -29,7 +29,7 @@ func newX509PolicyEngine(x509Opts *X509Options) (policy.X509NamePolicyEngine, er
if denied != nil && denied.HasNames() {
options = append(options,
policy.WithExcludedDNSDomains(denied.DNSDomains),
policy.WithExcludedCIDRs(denied.IPRanges), // TODO(hs): support IPs in addition to ranges
policy.WithExcludedIPsOrCIDRs(denied.IPRanges),
policy.WithExcludedEmailAddresses(denied.EmailAddresses),
policy.WithExcludedURIDomains(denied.URIDomains),
)

0
authority/provisioner/sign_options.go Executable file → Normal file
View File

138
policy/engine.go
View File

@ -15,33 +15,25 @@ import (
"golang.org/x/crypto/ssh"
)
type CertificateInvalidError struct {
Reason x509.InvalidReason
type NamePolicyReason int
const (
// NotAuthorizedForThisName results when an instance of
// NamePolicyEngine determines that there's a constraint which
// doesn't permit a DNS or another type of SAN to be signed
// (or otherwise used).
NotAuthorizedForThisName NamePolicyReason = iota
)
type NamePolicyError struct {
Reason NamePolicyReason
Detail string
}
func (e CertificateInvalidError) Error() string {
func (e NamePolicyError) 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:
case NotAuthorizedForThisName:
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"
}
@ -126,7 +118,7 @@ func removeDuplicates(strSlice []string) []string {
keys := make(map[string]bool)
result := []string{}
for _, item := range strSlice {
if _, value := keys[item]; !value {
if _, value := keys[item]; !value && item != "" { // skip empty constraints
keys[item] = true
result = append(result, item)
}
@ -206,8 +198,8 @@ func (e *NamePolicyEngine) IsIPAllowed(ip net.IP) (bool, error) {
// 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 {
dnsNames, ips, emails, usernames := splitPrincipals(cert.ValidPrincipals)
if err := e.validateNames(dnsNames, ips, emails, []*url.URL{}, usernames); err != nil {
return false, err
}
return true, nil
@ -233,14 +225,17 @@ 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) {
// splitPrincipals splits SSH certificate principals into DNS names, emails and usernames.
func splitPrincipals(principals []string) (dnsNames []string, ips []net.IP, emails, usernames []string) {
dnsNames = []string{}
ips = []net.IP{}
emails = []string{}
usernames = []string{}
for _, principal := range principals {
if strings.Contains(principal, "@") {
emails = append(emails, principal)
} else if ip := net.ParseIP(principal); ip != nil {
ips = append(ips, ip)
} else if len(strings.Split(principal, ".")) > 1 {
dnsNames = append(dnsNames, principal)
} else {
@ -260,7 +255,6 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA
return nil
}
// TODO: return our own type(s) of error?
// TODO: implement check that requires at least a single name in all of the SANs + subject?
// TODO: set limit on total of all names validated? In x509 there's a limit on the number of comparisons
@ -277,9 +271,9 @@ 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 CertificateInvalidError{
Reason: x509.CANotAuthorizedForThisName,
Detail: fmt.Sprintf("dns %q is not permitted by any constraint", dns), // TODO(hs): change this error (message)
return NamePolicyError{
Reason: NotAuthorizedForThisName,
Detail: fmt.Sprintf("dns %q is not explicitly permitted by any constraint", dns),
}
}
if _, ok := domainToReverseLabels(dns); !ok {
@ -295,9 +289,9 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA
for _, ip := range ips {
if e.numberOfIPRangeConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 {
return CertificateInvalidError{
Reason: x509.CANotAuthorizedForThisName,
Detail: fmt.Sprintf("ip %q is not permitted by any constraint", ip.String()),
return NamePolicyError{
Reason: NotAuthorizedForThisName,
Detail: fmt.Sprintf("ip %q is not explicitly permitted by any constraint", ip.String()),
}
}
if err := checkNameConstraints("ip", ip.String(), ip,
@ -310,9 +304,9 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA
for _, email := range emailAddresses {
if e.numberOfEmailAddressConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 {
return CertificateInvalidError{
Reason: x509.CANotAuthorizedForThisName,
Detail: fmt.Sprintf("email %q is not permitted by any constraint", email),
return NamePolicyError{
Reason: NotAuthorizedForThisName,
Detail: fmt.Sprintf("email %q is not explicitly permitted by any constraint", email),
}
}
mailbox, ok := parseRFC2821Mailbox(email)
@ -329,9 +323,9 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA
for _, uri := range uris {
if e.numberOfURIDomainConstraints == 0 && e.totalNumberOfPermittedConstraints > 0 {
return CertificateInvalidError{
Reason: x509.CANotAuthorizedForThisName,
Detail: fmt.Sprintf("uri %q is not permitted by any constraint", uri.String()),
return NamePolicyError{
Reason: NotAuthorizedForThisName,
Detail: fmt.Sprintf("uri %q is not explicitly permitted by any constraint", uri.String()),
}
}
if err := checkNameConstraints("uri", uri.String(), uri,
@ -342,23 +336,11 @@ 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),
return NamePolicyError{
Reason: NotAuthorizedForThisName,
Detail: fmt.Sprintf("username principal %q is not explicity permitted by any constraint", username),
}
}
// TODO: some validation? I.e. allowed characters?
@ -370,7 +352,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA
}
}
// TODO: when the error is not nil and returned up in the above, we can add
// TODO(hs): 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).
// TODO(hs): validate other types of SANs? The Go std library skips those.
@ -382,8 +364,7 @@ func (e *NamePolicyEngine) validateNames(dnsNames []string, ips []net.IP, emailA
// 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. The total number of comparisons is tracked in the given
// count and should not exceed the given limit.
// to the match function.
// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go
func checkNameConstraints(
nameType string,
@ -394,26 +375,19 @@ func checkNameConstraints(
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,
return NamePolicyError{
Reason: NotAuthorizedForThisName,
Detail: err.Error(),
}
}
if match {
return CertificateInvalidError{
Reason: x509.CANotAuthorizedForThisName,
return NamePolicyError{
Reason: NotAuthorizedForThisName,
Detail: fmt.Sprintf("%s %q is excluded by constraint %q", nameType, name, constraint),
}
}
@ -421,18 +395,13 @@ func checkNameConstraints(
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,
return NamePolicyError{
Reason: NotAuthorizedForThisName,
Detail: err.Error(),
}
}
@ -443,8 +412,8 @@ func checkNameConstraints(
}
if !ok {
return CertificateInvalidError{
Reason: x509.CANotAuthorizedForThisName,
return NamePolicyError{
Reason: NotAuthorizedForThisName,
Detail: fmt.Sprintf("%s %q is not permitted by any constraint", nameType, name),
}
}
@ -651,7 +620,6 @@ func (e *NamePolicyEngine) matchDomainConstraint(domain, constraint string) (boo
}
// Block domains that start with just a period
// TODO(hs): check if we should allow domains starting with "." at all; not sure if this is allowed in x509 names and certs.
if domain[0] == '.' {
return false, nil
}
@ -744,19 +712,11 @@ func matchIPConstraint(ip net.IP, constraint *net.IPNet) (bool, error) {
// }
// }
// if isIPv4(ip) != isIPv4(constraint.IP) { // TODO(hs): this check seems to do what the above intended to do?
// return false, nil
// }
contained := constraint.Contains(ip) // TODO(hs): validate that this is the correct behavior; also check IPv4-in-IPv6 (again)
return contained, nil
}
func isIPv4(ip net.IP) bool {
return ip.To4() != nil
}
// SOURCE: https://cs.opensource.google/go/go/+/refs/tags/go1.17.5:src/crypto/x509/verify.go
func (e *NamePolicyEngine) matchEmailConstraint(mailbox rfc2821Mailbox, constraint string) (bool, error) {
// TODO(hs): handle literal wildcard case for emails? Does that even make sense?
@ -817,5 +777,9 @@ func (e *NamePolicyEngine) matchURIConstraint(uri *url.URL, constraint string) (
// matchUsernameConstraint performs a string literal match against a constraint.
func matchUsernameConstraint(username, constraint string) (bool, error) {
// allow any plain principal username
if constraint == "*" {
return true, nil
}
return strings.EqualFold(username, constraint), nil
}

484
policy/engine_test.go
View File

@ -8,11 +8,11 @@ import (
"testing"
"github.com/smallstep/assert"
"golang.org/x/crypto/ssh"
)
// 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): check errors (reasons) are as expected
func TestNamePolicyEngine_matchDomainConstraint(t *testing.T) {
tests := []struct {
@ -135,6 +135,22 @@ func TestNamePolicyEngine_matchDomainConstraint(t *testing.T) {
want: false,
wantErr: false,
},
{
name: "false/idna-internationalized-domain-name",
engine: &NamePolicyEngine{},
domain: "JP納豆.例.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/
constraint: ".例.jp",
want: false,
wantErr: true,
},
{
name: "false/idna-internationalized-domain-name-constraint",
engine: &NamePolicyEngine{},
domain: "xn--jp-cd2fp15c.xn--fsq.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/
constraint: ".例.jp",
want: false,
wantErr: true,
},
{
name: "ok/empty-constraint",
engine: &NamePolicyEngine{},
@ -169,6 +185,22 @@ func TestNamePolicyEngine_matchDomainConstraint(t *testing.T) {
want: true,
wantErr: false,
},
{
name: "ok/different-case",
engine: &NamePolicyEngine{},
domain: "WWW.EXAMPLE.com",
constraint: "www.example.com",
want: true,
wantErr: false,
},
{
name: "ok/idna-internationalized-domain-name-punycode",
engine: &NamePolicyEngine{},
domain: "xn--jp-cd2fp15c.xn--fsq.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/
constraint: ".xn--fsq.jp",
want: true,
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@ -413,6 +445,17 @@ func TestNamePolicyEngine_matchEmailConstraint(t *testing.T) {
want: true,
wantErr: false,
},
{
name: "ok/different-case",
engine: &NamePolicyEngine{},
mailbox: rfc2821Mailbox{
local: "mail",
domain: "EXAMPLE.com",
},
constraint: "example.com", // "wildcard" for 'example.com'
want: true,
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@ -558,6 +601,17 @@ func TestNamePolicyEngine_matchURIConstraint(t *testing.T) {
want: true,
wantErr: false,
},
{
name: "ok/different-case",
engine: &NamePolicyEngine{},
uri: &url.URL{
Scheme: "https",
Host: "www.EXAMPLE.local",
},
constraint: ".example.local", // using x509 period as the "wildcard"; expects a single subdomain
want: true,
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@ -573,7 +627,23 @@ func TestNamePolicyEngine_matchURIConstraint(t *testing.T) {
}
}
func TestNamePolicyEngine_AreCertificateNamesAllowed(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
options []NamePolicyOption
@ -2048,14 +2118,422 @@ func TestNamePolicyEngine_AreCertificateNamesAllowed(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
engine, err := New(tt.options...)
assert.FatalError(t, err)
got, err := engine.AreCertificateNamesAllowed(tt.cert) // TODO: perform tests on CSR, sans, etc. too
got, err := engine.AreCertificateNamesAllowed(tt.cert)
if (err != nil) != tt.wantErr {
t.Errorf("NamePolicyEngine.AreCertificateNamesAllowed() error = %v, wantErr %v", err, tt.wantErr)
return
}
if err != nil {
assert.NotEquals(t, "", err.Error()) // TODO(hs): implement a more specific error comparison?
}
if got != tt.want {
t.Errorf("NamePolicyEngine.AreCertificateNamesAllowed() = %v, want %v", got, tt.want)
}
// Perform the same tests for a CSR, which are similar to Certificates
csr := &x509.CertificateRequest{
Subject: tt.cert.Subject,
DNSNames: tt.cert.DNSNames,
EmailAddresses: tt.cert.EmailAddresses,
IPAddresses: tt.cert.IPAddresses,
URIs: tt.cert.URIs,
}
got, err = engine.AreCSRNamesAllowed(csr)
if (err != nil) != tt.wantErr {
t.Errorf("NamePolicyEngine.AreCSRNamesAllowed() error = %v, wantErr %v", err, tt.wantErr)
return
}
if err != nil {
assert.NotEquals(t, "", err.Error())
}
if got != tt.want {
t.Errorf("NamePolicyEngine.AreCSRNamesAllowed() = %v, want %v", got, tt.want)
}
// 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)
got, err = engine.AreSANsAllowed(sans)
if (err != nil) != tt.wantErr {
t.Errorf("NamePolicyEngine.AreSANsAllowed() error = %v, wantErr %v", err, tt.wantErr)
return
}
if err != nil {
assert.NotEquals(t, "", err.Error())
}
if got != tt.want {
t.Errorf("NamePolicyEngine.AreSANsAllowed() = %v, want %v", got, tt.want)
}
})
}
}
func TestNamePolicyEngine_SSH_ArePrincipalsAllowed(t *testing.T) {
tests := []struct {
name string
options []NamePolicyOption
cert *ssh.Certificate
want bool
wantErr bool
}{
{
name: "fail/with-permitted-dns-domain",
options: []NamePolicyOption{
WithPermittedDNSDomain("*.local"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"host.example.com",
},
},
want: false,
wantErr: true,
},
{
name: "fail/with-excluded-dns-domain",
options: []NamePolicyOption{
WithExcludedDNSDomain("*.local"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"host.local",
},
},
want: false,
wantErr: true,
},
{
name: "fail/with-permitted-ip",
options: []NamePolicyOption{
WithPermittedCIDR("127.0.0.1/24"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"192.168.0.22",
},
},
want: false,
wantErr: true,
},
{
name: "fail/with-excluded-ip",
options: []NamePolicyOption{
WithExcludedCIDR("127.0.0.1/24"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"127.0.0.0",
},
},
want: false,
wantErr: true,
},
{
name: "fail/with-permitted-email",
options: []NamePolicyOption{
WithPermittedEmailAddress("@example.com"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"mail@local",
},
},
want: false,
wantErr: true,
},
{
name: "fail/with-excluded-email",
options: []NamePolicyOption{
WithExcludedEmailAddress("@example.com"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"mail@example.com",
},
},
want: false,
wantErr: true,
},
{
name: "fail/with-permitted-principals",
options: []NamePolicyOption{
WithPermittedPrincipals([]string{"user"}),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"root",
},
},
want: false,
wantErr: true,
},
{
name: "fail/with-excluded-principals",
options: []NamePolicyOption{
WithExcludedPrincipals([]string{"user"}),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"user",
},
},
want: false,
wantErr: true,
},
{
name: "fail/with-permitted-principal-as-mail",
options: []NamePolicyOption{
WithPermittedPrincipals([]string{"ops"}),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"ops@work", // this is (currently) parsed as an email-like principal; not allowed with just "ops" as the permitted principal
},
},
want: false,
wantErr: true,
},
{
name: "fail/principal-with-permitted-dns-domain", // when only DNS is permitted, username principals are not allowed.
options: []NamePolicyOption{
WithPermittedDNSDomain("*.local"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"user",
},
},
want: false,
wantErr: true,
},
{
name: "fail/principal-with-permitted-ip-range", // when only IPs are permitted, username principals are not allowed.
options: []NamePolicyOption{
WithPermittedCIDR("127.0.0.1/24"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"user",
},
},
want: false,
wantErr: true,
},
{
name: "fail/principal-with-permitted-email", // when only emails are permitted, username principals are not allowed.
options: []NamePolicyOption{
WithPermittedEmailAddress("@example.com"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"user",
},
},
want: false,
wantErr: true,
},
{
name: "fail/combined-user",
options: []NamePolicyOption{
WithPermittedEmailAddress("@smallstep.com"),
WithExcludedEmailAddress("root@smallstep.com"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"someone@smallstep.com",
"someone",
},
},
want: false,
wantErr: true,
},
{
name: "fail/combined-user-with-excluded-user-principal",
options: []NamePolicyOption{
WithPermittedEmailAddress("@smallstep.com"),
WithExcludedPrincipals([]string{"root"}),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"someone@smallstep.com",
"root",
},
},
want: false,
wantErr: true,
},
{
name: "ok/with-permitted-dns-domain",
options: []NamePolicyOption{
WithPermittedDNSDomain("*.local"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"host.local",
},
},
want: true,
wantErr: false,
},
{
name: "ok/with-excluded-dns-domain",
options: []NamePolicyOption{
WithExcludedDNSDomain("*.example.com"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"host.local",
},
},
want: true,
wantErr: false,
},
{
name: "ok/with-permitted-ip",
options: []NamePolicyOption{
WithPermittedCIDR("127.0.0.1/24"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"127.0.0.33",
},
},
want: true,
wantErr: false,
},
{
name: "ok/with-excluded-ip",
options: []NamePolicyOption{
WithExcludedCIDR("127.0.0.1/24"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"192.168.0.35",
},
},
want: true,
wantErr: false,
},
{
name: "ok/with-permitted-email",
options: []NamePolicyOption{
WithPermittedEmailAddress("@example.com"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"mail@example.com",
},
},
want: true,
wantErr: false,
},
{
name: "ok/with-excluded-email",
options: []NamePolicyOption{
WithExcludedEmailAddress("@example.com"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"mail@local",
},
},
want: true,
wantErr: false,
},
{
name: "ok/with-permitted-principals",
options: []NamePolicyOption{
WithPermittedPrincipals([]string{"*"}),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"user",
},
},
want: true,
wantErr: false,
},
{
name: "ok/with-excluded-principals",
options: []NamePolicyOption{
WithExcludedPrincipals([]string{"user"}),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"root",
},
},
want: true,
wantErr: false,
},
{
name: "ok/combined-user",
options: []NamePolicyOption{
WithPermittedEmailAddress("@smallstep.com"),
WithPermittedPrincipals([]string{"*"}), // without specifying the wildcard, "someone" would not be allowed.
WithExcludedEmailAddress("root@smallstep.com"),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"someone@smallstep.com",
"someone",
},
},
want: true,
wantErr: false,
},
{
name: "ok/combined-user-with-excluded-user-principal",
options: []NamePolicyOption{
WithPermittedEmailAddress("@smallstep.com"),
WithExcludedEmailAddress("root@smallstep.com"),
WithExcludedPrincipals([]string{"root"}), // unlike the previous test, this implicitly allows any other username principal
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"someone@smallstep.com",
"someone",
},
},
want: true,
wantErr: false,
},
{
name: "ok/combined-simple-all",
options: []NamePolicyOption{
WithPermittedDNSDomain("*.local"),
WithPermittedCIDR("127.0.0.1/24"),
WithPermittedEmailAddress("@example.local"),
WithPermittedPrincipals([]string{"user"}),
WithExcludedDNSDomain("badhost.local"),
WithExcludedCIDR("127.0.0.128/25"),
WithExcludedEmailAddress("badmail@example.local"),
WithExcludedPrincipals([]string{"root"}),
},
cert: &ssh.Certificate{
ValidPrincipals: []string{
"example.local",
"127.0.0.1",
"user@example.local",
"user",
},
},
want: true,
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
engine, err := New(tt.options...)
assert.FatalError(t, err)
got, err := engine.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)
}
})
}
}

90
policy/options.go
View File

@ -204,6 +204,42 @@ func AddExcludedCIDRs(cidrs []string) NamePolicyOption {
}
}
func WithPermittedIPsOrCIDRs(ipsOrCIDRs []string) NamePolicyOption {
return func(e *NamePolicyEngine) error {
networks := make([]*net.IPNet, len(ipsOrCIDRs))
for i, ipOrCIDR := range ipsOrCIDRs {
_, nw, err := net.ParseCIDR(ipOrCIDR)
if err == nil {
networks[i] = nw
} else if ip := net.ParseIP(ipOrCIDR); ip != nil {
networks[i] = networkFor(ip)
} else {
return errors.Errorf("cannot parse permitted constraint %q as IP nor CIDR", ipOrCIDR)
}
}
e.permittedIPRanges = networks
return nil
}
}
func WithExcludedIPsOrCIDRs(ipsOrCIDRs []string) NamePolicyOption {
return func(e *NamePolicyEngine) error {
networks := make([]*net.IPNet, len(ipsOrCIDRs))
for i, ipOrCIDR := range ipsOrCIDRs {
_, nw, err := net.ParseCIDR(ipOrCIDR)
if err == nil {
networks[i] = nw
} else if ip := net.ParseIP(ipOrCIDR); ip != nil {
networks[i] = networkFor(ip)
} else {
return errors.Errorf("cannot parse excluded constraint %q as IP nor CIDR", ipOrCIDR)
}
}
e.excludedIPRanges = networks
return nil
}
}
func WithPermittedCIDR(cidr string) NamePolicyOption {
return func(e *NamePolicyEngine) error {
_, nw, err := net.ParseCIDR(cidr)
@ -228,16 +264,7 @@ func AddPermittedCIDR(cidr string) NamePolicyOption {
func WithPermittedIP(ip net.IP) NamePolicyOption {
return func(e *NamePolicyEngine) error {
var mask net.IPMask
if !isIPv4(ip) {
mask = net.CIDRMask(128, 128)
} else {
mask = net.CIDRMask(32, 32)
}
nw := &net.IPNet{
IP: ip,
Mask: mask,
}
nw := networkFor(ip)
e.permittedIPRanges = []*net.IPNet{nw}
return nil
}
@ -245,16 +272,7 @@ func WithPermittedIP(ip net.IP) NamePolicyOption {
func AddPermittedIP(ip net.IP) NamePolicyOption {
return func(e *NamePolicyEngine) error {
var mask net.IPMask
if !isIPv4(ip) {
mask = net.CIDRMask(128, 128)
} else {
mask = net.CIDRMask(32, 32)
}
nw := &net.IPNet{
IP: ip,
Mask: mask,
}
nw := networkFor(ip)
e.permittedIPRanges = append(e.permittedIPRanges, nw)
return nil
}
@ -540,9 +558,7 @@ func AddExcludedURIDomain(uriDomain string) NamePolicyOption {
func WithPermittedPrincipals(principals []string) NamePolicyOption {
return func(g *NamePolicyEngine) error {
// for _, principal := range principals {
// // TODO: validation?
// }
// TODO(hs): normalize and parse principal into the right type? Seems the safe thing to do.
g.permittedPrincipals = principals
return nil
}
@ -550,16 +566,32 @@ func WithPermittedPrincipals(principals []string) NamePolicyOption {
func WithExcludedPrincipals(principals []string) NamePolicyOption {
return func(g *NamePolicyEngine) error {
// for _, principal := range principals {
// // TODO: validation?
// }
// TODO(hs): normalize and parse principal into the right type? Seems the safe thing to do.
g.excludedPrincipals = principals
return nil
}
}
func networkFor(ip net.IP) *net.IPNet {
var mask net.IPMask
if !isIPv4(ip) {
mask = net.CIDRMask(128, 128)
} else {
mask = net.CIDRMask(32, 32)
}
nw := &net.IPNet{
IP: ip,
Mask: mask,
}
return nw
}
func isIPv4(ip net.IP) bool {
return ip.To4() != nil
}
func normalizeAndValidateDNSDomainConstraint(constraint string) (string, error) {
normalizedConstraint := strings.TrimSpace(constraint)
normalizedConstraint := strings.ToLower(strings.TrimSpace(constraint))
if strings.Contains(normalizedConstraint, "..") {
return "", errors.Errorf("domain constraint %q cannot have empty labels", constraint)
}
@ -576,7 +608,7 @@ func normalizeAndValidateDNSDomainConstraint(constraint string) (string, error)
}
func normalizeAndValidateEmailConstraint(constraint string) (string, error) {
normalizedConstraint := strings.TrimSpace(constraint)
normalizedConstraint := strings.ToLower(strings.TrimSpace(constraint))
if strings.Contains(normalizedConstraint, "*") {
return "", fmt.Errorf("email constraint %q cannot contain asterisk", constraint)
}
@ -601,7 +633,7 @@ func normalizeAndValidateEmailConstraint(constraint string) (string, error) {
}
func normalizeAndValidateURIDomainConstraint(constraint string) (string, error) {
normalizedConstraint := strings.TrimSpace(constraint)
normalizedConstraint := strings.ToLower(strings.TrimSpace(constraint))
if strings.Contains(normalizedConstraint, "..") {
return "", errors.Errorf("URI domain constraint %q cannot have empty labels", constraint)
}

126
policy/options_test.go
View File

@ -33,6 +33,18 @@ func Test_normalizeAndValidateDNSDomainConstraint(t *testing.T) {
want: "",
wantErr: true,
},
{
name: "false/idna-internationalized-domain-name",
constraint: ".例.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/
want: "",
wantErr: true,
},
{
name: "false/idna-internationalized-domain-name-constraint",
constraint: ".例.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/
want: "",
wantErr: true,
},
{
name: "ok/wildcard",
constraint: "*.local",
@ -45,6 +57,12 @@ func Test_normalizeAndValidateDNSDomainConstraint(t *testing.T) {
want: "example.local",
wantErr: false,
},
{
name: "ok/idna-internationalized-domain-name-punycode",
constraint: ".xn--fsq.jp", // Example value from https://www.w3.org/International/articles/idn-and-iri/
want: ".xn--fsq.jp",
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
@ -297,6 +315,42 @@ func TestNew(t *testing.T) {
wantErr: true,
}
},
"fail/with-permitted-ipsOrCIDRs-cidr": func(t *testing.T) test {
return test{
options: []NamePolicyOption{
WithPermittedIPsOrCIDRs([]string{"127.0.0.1//24"}),
},
want: nil,
wantErr: true,
}
},
"fail/with-permitted-ipsOrCIDRs-ip": func(t *testing.T) test {
return test{
options: []NamePolicyOption{
WithPermittedIPsOrCIDRs([]string{"127.0.0:1"}),
},
want: nil,
wantErr: true,
}
},
"fail/with-excluded-ipsOrCIDRs-cidr": func(t *testing.T) test {
return test{
options: []NamePolicyOption{
WithExcludedIPsOrCIDRs([]string{"127.0.0.1//24"}),
},
want: nil,
wantErr: true,
}
},
"fail/with-excluded-ipsOrCIDRs-ip": func(t *testing.T) test {
return test{
options: []NamePolicyOption{
WithExcludedIPsOrCIDRs([]string{"127.0.0:1"}),
},
want: nil,
wantErr: true,
}
},
"fail/with-permitted-cidr": func(t *testing.T) test {
return test{
options: []NamePolicyOption{
@ -828,6 +882,48 @@ func TestNew(t *testing.T) {
wantErr: false,
}
},
"ok/with-permitted-ipsOrCIDRs-cidr": func(t *testing.T) test {
_, nw1, err := net.ParseCIDR("127.0.0.1/24")
assert.FatalError(t, err)
_, nw2, err := net.ParseCIDR("192.168.0.31/32")
assert.FatalError(t, err)
options := []NamePolicyOption{
WithPermittedIPsOrCIDRs([]string{"127.0.0.1/24", "192.168.0.31"}),
}
return test{
options: options,
want: &NamePolicyEngine{
permittedIPRanges: []*net.IPNet{
nw1, nw2,
},
numberOfIPRangeConstraints: 2,
totalNumberOfPermittedConstraints: 2,
totalNumberOfConstraints: 2,
},
wantErr: false,
}
},
"ok/with-excluded-ipsOrCIDRs-cidr": func(t *testing.T) test {
_, nw1, err := net.ParseCIDR("127.0.0.1/24")
assert.FatalError(t, err)
_, nw2, err := net.ParseCIDR("192.168.0.31/32")
assert.FatalError(t, err)
options := []NamePolicyOption{
WithExcludedIPsOrCIDRs([]string{"127.0.0.1/24", "192.168.0.31"}),
}
return test{
options: options,
want: &NamePolicyEngine{
excludedIPRanges: []*net.IPNet{
nw1, nw2,
},
numberOfIPRangeConstraints: 2,
totalNumberOfExcludedConstraints: 2,
totalNumberOfConstraints: 2,
},
wantErr: false,
}
},
"ok/with-permitted-cidr": func(t *testing.T) test {
_, nw1, err := net.ParseCIDR("127.0.0.1/24")
assert.FatalError(t, err)
@ -1322,6 +1418,36 @@ func TestNew(t *testing.T) {
wantErr: false,
}
},
"ok/with-permitted-principals": func(t *testing.T) test {
options := []NamePolicyOption{
WithPermittedPrincipals([]string{"root", "ops"}),
}
return test{
options: options,
want: &NamePolicyEngine{
permittedPrincipals: []string{"root", "ops"},
numberOfPrincipalConstraints: 2,
totalNumberOfPermittedConstraints: 2,
totalNumberOfConstraints: 2,
},
wantErr: false,
}
},
"ok/with-excluded-principals": func(t *testing.T) test {
options := []NamePolicyOption{
WithExcludedPrincipals([]string{"root", "ops"}),
}
return test{
options: options,
want: &NamePolicyEngine{
excludedPrincipals: []string{"root", "ops"},
numberOfPrincipalConstraints: 2,
totalNumberOfExcludedConstraints: 2,
totalNumberOfConstraints: 2,
},
wantErr: false,
}
},
}
for name, prep := range tests {
tc := prep(t)