From 931656bd6868a87eef4814cc02b5125a907662bf Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Sat, 24 Feb 2024 02:26:00 +0300 Subject: [PATCH] acmeserver: add policy field to define allow/deny rules (#5796) * acmeserver: support specifying the allowed challenge types * add caddyfile adapt tests * acmeserver: add `policy` field to define allow/deny rules * allow `omitempty` to work * add caddyfile support for `policy` * remove "uri domain" policy * fmt the files * add docs * do not support `CommonName`; the field is deprecated * r/DNSDomains/Domains/g * Caddyfile docs * add tests * move `Policy` to top of file --- caddytest/integration/acme_test.go | 71 +-------- caddytest/integration/acmeserver_test.go | 176 +++++++++++++++++++++ modules/caddypki/acmeserver/acmeserver.go | 8 +- modules/caddypki/acmeserver/caddyfile.go | 58 ++++++- modules/caddypki/acmeserver/policy.go | 83 ++++++++++ modules/caddypki/acmeserver/policy_test.go | 176 +++++++++++++++++++++ 6 files changed, 506 insertions(+), 66 deletions(-) create mode 100644 modules/caddypki/acmeserver/policy.go create mode 100644 modules/caddypki/acmeserver/policy_test.go diff --git a/caddytest/integration/acme_test.go b/caddytest/integration/acme_test.go index 45db8f01..840af023 100644 --- a/caddytest/integration/acme_test.go +++ b/caddytest/integration/acme_test.go @@ -5,13 +5,9 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/rand" - "crypto/tls" - "crypto/x509" "fmt" "net" "net/http" - "os" - "path/filepath" "strings" "testing" @@ -48,37 +44,11 @@ func TestACMEServerWithDefaults(t *testing.T) { } `, "caddyfile") - datadir := caddy.AppDataDir() - rootCertsGlob := filepath.Join(datadir, "pki", "authorities", "local", "*.crt") - matches, err := filepath.Glob(rootCertsGlob) - if err != nil { - t.Errorf("could not find root certs: %s", err) - return - } - certPool := x509.NewCertPool() - for _, m := range matches { - certPem, err := os.ReadFile(m) - if err != nil { - t.Errorf("reading cert file '%s' error: %s", m, err) - return - } - if !certPool.AppendCertsFromPEM(certPem) { - t.Errorf("failed to append the cert: %s", m) - return - } - } - client := acmez.Client{ Client: &acme.Client{ - Directory: "https://acme.localhost:9443/acme/local/directory", - HTTPClient: &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - RootCAs: certPool, - }, - }, - }, - Logger: logger, + Directory: "https://acme.localhost:9443/acme/local/directory", + HTTPClient: tester.Client, + Logger: logger, }, ChallengeSolvers: map[string]acmez.Solver{ acme.ChallengeTypeHTTP01: &naiveHTTPSolver{logger: logger}, @@ -143,37 +113,11 @@ func TestACMEServerWithMismatchedChallenges(t *testing.T) { } `, "caddyfile") - datadir := caddy.AppDataDir() - rootCertsGlob := filepath.Join(datadir, "pki", "authorities", "local", "*.crt") - matches, err := filepath.Glob(rootCertsGlob) - if err != nil { - t.Errorf("could not find root certs: %s", err) - return - } - certPool := x509.NewCertPool() - for _, m := range matches { - certPem, err := os.ReadFile(m) - if err != nil { - t.Errorf("reading cert file '%s' error: %s", m, err) - return - } - if !certPool.AppendCertsFromPEM(certPem) { - t.Errorf("failed to append the cert: %s", m) - return - } - } - client := acmez.Client{ Client: &acme.Client{ - Directory: "https://acme.localhost:9443/acme/local/directory", - HTTPClient: &http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{ - RootCAs: certPool, - }, - }, - }, - Logger: logger, + Directory: "https://acme.localhost:9443/acme/local/directory", + HTTPClient: tester.Client, + Logger: logger, }, ChallengeSolvers: map[string]acmez.Solver{ acme.ChallengeTypeHTTP01: &naiveHTTPSolver{logger: logger}, @@ -224,12 +168,13 @@ type naiveHTTPSolver struct { func (s *naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) error { smallstepacme.InsecurePortHTTP01 = acmeChallengePort s.srv = &http.Server{ - Addr: fmt.Sprintf("localhost:%d", acmeChallengePort), + Addr: fmt.Sprintf(":%d", acmeChallengePort), Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { host, _, err := net.SplitHostPort(r.Host) if err != nil { host = r.Host } + s.logger.Info("received request on challenge server", zap.String("path", r.URL.Path)) if r.Method == "GET" && r.URL.Path == challenge.HTTP01ResourcePath() && strings.EqualFold(host, challenge.Identifier.Value) { w.Header().Add("Content-Type", "text/plain") w.Write([]byte(challenge.KeyAuthorization)) diff --git a/caddytest/integration/acmeserver_test.go b/caddytest/integration/acmeserver_test.go index 0323d5cd..435bfc7b 100644 --- a/caddytest/integration/acmeserver_test.go +++ b/caddytest/integration/acmeserver_test.go @@ -1,9 +1,17 @@ package integration import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "strings" "testing" "github.com/caddyserver/caddy/v2/caddytest" + "github.com/mholt/acmez" + "github.com/mholt/acmez/acme" + "go.uber.org/zap" ) func TestACMEServerDirectory(t *testing.T) { @@ -31,3 +39,171 @@ func TestACMEServerDirectory(t *testing.T) { `{"newNonce":"https://acme.localhost:9443/acme/local/new-nonce","newAccount":"https://acme.localhost:9443/acme/local/new-account","newOrder":"https://acme.localhost:9443/acme/local/new-order","revokeCert":"https://acme.localhost:9443/acme/local/revoke-cert","keyChange":"https://acme.localhost:9443/acme/local/key-change"} `) } + +func TestACMEServerAllowPolicy(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(` + { + skip_install_trust + local_certs + admin localhost:2999 + http_port 9080 + https_port 9443 + pki { + ca local { + name "Caddy Local Authority" + } + } + } + acme.localhost { + acme_server { + challenges http-01 + allow { + domains localhost + } + } + } + `, "caddyfile") + + ctx := context.Background() + logger, err := zap.NewDevelopment() + if err != nil { + t.Error(err) + return + } + + client := acmez.Client{ + Client: &acme.Client{ + Directory: "https://acme.localhost:9443/acme/local/directory", + HTTPClient: tester.Client, + Logger: logger, + }, + ChallengeSolvers: map[string]acmez.Solver{ + acme.ChallengeTypeHTTP01: &naiveHTTPSolver{logger: logger}, + }, + } + + accountPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Errorf("generating account key: %v", err) + } + account := acme.Account{ + Contact: []string{"mailto:you@example.com"}, + TermsOfServiceAgreed: true, + PrivateKey: accountPrivateKey, + } + account, err = client.NewAccount(ctx, account) + if err != nil { + t.Errorf("new account: %v", err) + return + } + + // Every certificate needs a key. + certPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Errorf("generating certificate key: %v", err) + return + } + { + certs, err := client.ObtainCertificate( + ctx, + account, + certPrivateKey, + []string{"localhost"}, + ) + if err != nil { + t.Errorf("obtaining certificate for allowed domain: %v", err) + return + } + + // ACME servers should usually give you the entire certificate chain + // in PEM format, and sometimes even alternate chains! It's up to you + // which one(s) to store and use, but whatever you do, be sure to + // store the certificate and key somewhere safe and secure, i.e. don't + // lose them! + for _, cert := range certs { + t.Logf("Certificate %q:\n%s\n\n", cert.URL, cert.ChainPEM) + } + } + { + _, err := client.ObtainCertificate(ctx, account, certPrivateKey, []string{"not-matching.localhost"}) + if err == nil { + t.Errorf("obtaining certificate for 'not-matching.localhost' domain") + } else if err != nil && !strings.Contains(err.Error(), "urn:ietf:params:acme:error:rejectedIdentifier") { + t.Logf("unexpected error: %v", err) + } + } +} + +func TestACMEServerDenyPolicy(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(` + { + skip_install_trust + local_certs + admin localhost:2999 + http_port 9080 + https_port 9443 + pki { + ca local { + name "Caddy Local Authority" + } + } + } + acme.localhost { + acme_server { + deny { + domains deny.localhost + } + } + } + `, "caddyfile") + + ctx := context.Background() + logger, err := zap.NewDevelopment() + if err != nil { + t.Error(err) + return + } + + client := acmez.Client{ + Client: &acme.Client{ + Directory: "https://acme.localhost:9443/acme/local/directory", + HTTPClient: tester.Client, + Logger: logger, + }, + ChallengeSolvers: map[string]acmez.Solver{ + acme.ChallengeTypeHTTP01: &naiveHTTPSolver{logger: logger}, + }, + } + + accountPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Errorf("generating account key: %v", err) + } + account := acme.Account{ + Contact: []string{"mailto:you@example.com"}, + TermsOfServiceAgreed: true, + PrivateKey: accountPrivateKey, + } + account, err = client.NewAccount(ctx, account) + if err != nil { + t.Errorf("new account: %v", err) + return + } + + // Every certificate needs a key. + certPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Errorf("generating certificate key: %v", err) + return + } + { + _, err := client.ObtainCertificate(ctx, account, certPrivateKey, []string{"deny.localhost"}) + if err == nil { + t.Errorf("obtaining certificate for 'deny.localhost' domain") + } else if err != nil && !strings.Contains(err.Error(), "urn:ietf:params:acme:error:rejectedIdentifier") { + t.Logf("unexpected error: %v", err) + } + } +} diff --git a/modules/caddypki/acmeserver/acmeserver.go b/modules/caddypki/acmeserver/acmeserver.go index e0588c52..eaf6e930 100644 --- a/modules/caddypki/acmeserver/acmeserver.go +++ b/modules/caddypki/acmeserver/acmeserver.go @@ -96,6 +96,9 @@ type Handler struct { // "http-01", "dns-01", "tls-alpn-01" Challenges ACMEChallenges `json:"challenges,omitempty" ` + // The policy to use for issuing certificates + Policy *Policy `json:"policy,omitempty"` + logger *zap.Logger resolvers []caddy.NetworkAddress ctx caddy.Context @@ -165,7 +168,10 @@ func (ash *Handler) Provision(ctx caddy.Context) error { &provisioner.ACME{ Name: ash.CA, Challenges: ash.Challenges.toSmallstepType(), - Type: provisioner.TypeACME.String(), + Options: &provisioner.Options{ + X509: ash.Policy.normalizeRules(), + }, + Type: provisioner.TypeACME.String(), Claims: &provisioner.Claims{ MinTLSDur: &provisioner.Duration{Duration: 5 * time.Minute}, MaxTLSDur: &provisioner.Duration{Duration: 24 * time.Hour * 365}, diff --git a/modules/caddypki/acmeserver/caddyfile.go b/modules/caddypki/acmeserver/caddyfile.go index 864a94c5..7eaaec49 100644 --- a/modules/caddypki/acmeserver/caddyfile.go +++ b/modules/caddypki/acmeserver/caddyfile.go @@ -33,6 +33,15 @@ func init() { // lifetime // resolvers // challenges +// allow_wildcard_names +// allow { +// domains +// ip_ranges +// } +// deny { +// domains +// ip_ranges +// } // } func parseACMEServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) { h.Next() // consume directive name @@ -60,7 +69,6 @@ func parseACMEServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error ca = new(caddypki.CA) } ca.ID = acmeServer.CA - case "lifetime": if !h.NextArg() { return nil, h.ArgErr() @@ -70,7 +78,6 @@ func parseACMEServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error if err != nil { return nil, err } - if d := time.Duration(ca.IntermediateLifetime); d > 0 && dur > d { return nil, h.Errf("certificate lifetime (%s) exceeds intermediate certificate lifetime (%s)", dur, d) } @@ -82,6 +89,53 @@ func parseACMEServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error } case "challenges": acmeServer.Challenges = append(acmeServer.Challenges, stringToChallenges(h.RemainingArgs())...) + case "allow_wildcard_names": + if acmeServer.Policy == nil { + acmeServer.Policy = &Policy{} + } + acmeServer.Policy.AllowWildcardNames = true + case "allow": + r := &RuleSet{} + for h.Next() { + for h.NextBlock(h.Nesting() - 1) { + if h.CountRemainingArgs() == 0 { + return nil, h.ArgErr() // TODO: + } + switch h.Val() { + case "domains": + r.Domains = append(r.Domains, h.RemainingArgs()...) + case "ip_ranges": + r.IPRanges = append(r.IPRanges, h.RemainingArgs()...) + default: + return nil, h.Errf("unrecognized 'allow' subdirective: %s", h.Val()) + } + } + } + if acmeServer.Policy == nil { + acmeServer.Policy = &Policy{} + } + acmeServer.Policy.Allow = r + case "deny": + r := &RuleSet{} + for h.Next() { + for h.NextBlock(h.Nesting() - 1) { + if h.CountRemainingArgs() == 0 { + return nil, h.ArgErr() // TODO: + } + switch h.Val() { + case "domains": + r.Domains = append(r.Domains, h.RemainingArgs()...) + case "ip_ranges": + r.IPRanges = append(r.IPRanges, h.RemainingArgs()...) + default: + return nil, h.Errf("unrecognized 'deny' subdirective: %s", h.Val()) + } + } + } + if acmeServer.Policy == nil { + acmeServer.Policy = &Policy{} + } + acmeServer.Policy.Deny = r default: return nil, h.Errf("unrecognized ACME server directive: %s", h.Val()) } diff --git a/modules/caddypki/acmeserver/policy.go b/modules/caddypki/acmeserver/policy.go new file mode 100644 index 00000000..96137e6e --- /dev/null +++ b/modules/caddypki/acmeserver/policy.go @@ -0,0 +1,83 @@ +package acmeserver + +import ( + "github.com/smallstep/certificates/authority/policy" + "github.com/smallstep/certificates/authority/provisioner" +) + +// Policy defines the criteria for the ACME server +// of when to issue a certificate. Refer to the +// [Certificate Issuance Policy](https://smallstep.com/docs/step-ca/policies/) +// on Smallstep website for the evaluation criteria. +type Policy struct { + // If a rule set is configured to allow a certain type of name, + // all other types of names are automatically denied. + Allow *RuleSet `json:"allow,omitempty"` + + // If a rule set is configured to deny a certain type of name, + // all other types of names are still allowed. + Deny *RuleSet `json:"deny,omitempty"` + + // If set to true, the ACME server will allow issuing wildcard certificates. + AllowWildcardNames bool `json:"allow_wildcard_names,omitempty"` +} + +// RuleSet is the specific set of SAN criteria for a certificate +// to be issued or denied. +type RuleSet struct { + // Domains is a list of DNS domains that are allowed to be issued. + // It can be in the form of FQDN for specific domain name, or + // a wildcard domain name format, e.g. *.example.com, to allow + // sub-domains of a domain. + Domains []string `json:"domains,omitempty"` + + // IP ranges in the form of CIDR notation or specific IP addresses + // to be approved or denied for certificates. Non-CIDR IP addresses + // are matched exactly. + IPRanges []string `json:"ip_ranges,omitempty"` +} + +// normalizeAllowRules returns `nil` if policy is nil, the `Allow` rule is `nil`, +// or all rules within the `Allow` rule are empty. Otherwise, it returns the X509NameOptions +// with the content of the `Allow` rule. +func (p *Policy) normalizeAllowRules() *policy.X509NameOptions { + if (p == nil) || (p.Allow == nil) || (len(p.Allow.Domains) == 0 && len(p.Allow.IPRanges) == 0) { + return nil + } + return &policy.X509NameOptions{ + DNSDomains: p.Allow.Domains, + IPRanges: p.Allow.IPRanges, + } +} + +// normalizeDenyRules returns `nil` if policy is nil, the `Deny` rule is `nil`, +// or all rules within the `Deny` rule are empty. Otherwise, it returns the X509NameOptions +// with the content of the `Deny` rule. +func (p *Policy) normalizeDenyRules() *policy.X509NameOptions { + if (p == nil) || (p.Deny == nil) || (len(p.Deny.Domains) == 0 && len(p.Deny.IPRanges) == 0) { + return nil + } + return &policy.X509NameOptions{ + DNSDomains: p.Deny.Domains, + IPRanges: p.Deny.IPRanges, + } +} + +// normalizeRules returns `nil` if policy is nil, the `Allow` and `Deny` rules are `nil`, +func (p *Policy) normalizeRules() *provisioner.X509Options { + if p == nil { + return nil + } + + allow := p.normalizeAllowRules() + deny := p.normalizeDenyRules() + if allow == nil && deny == nil && !p.AllowWildcardNames { + return nil + } + + return &provisioner.X509Options{ + AllowedNames: allow, + DeniedNames: deny, + AllowWildcardNames: p.AllowWildcardNames, + } +} diff --git a/modules/caddypki/acmeserver/policy_test.go b/modules/caddypki/acmeserver/policy_test.go new file mode 100644 index 00000000..02d7856d --- /dev/null +++ b/modules/caddypki/acmeserver/policy_test.go @@ -0,0 +1,176 @@ +package acmeserver + +import ( + "reflect" + "testing" + + "github.com/smallstep/certificates/authority/policy" + "github.com/smallstep/certificates/authority/provisioner" +) + +func TestPolicyNormalizeAllowRules(t *testing.T) { + type fields struct { + Allow *RuleSet + Deny *RuleSet + AllowWildcardNames bool + } + tests := []struct { + name string + fields fields + want *policy.X509NameOptions + }{ + { + name: "providing no rules results in 'nil'", + fields: fields{}, + want: nil, + }, + { + name: "providing 'nil' Allow rules results in 'nil', regardless of Deny rules", + fields: fields{ + Allow: nil, + Deny: &RuleSet{}, + AllowWildcardNames: true, + }, + want: nil, + }, + { + name: "providing empty Allow rules results in 'nil', regardless of Deny rules", + fields: fields{ + Allow: &RuleSet{ + Domains: []string{}, + IPRanges: []string{}, + }, + }, + want: nil, + }, + { + name: "rules configured in Allow are returned in X509NameOptions", + fields: fields{ + Allow: &RuleSet{ + Domains: []string{"example.com"}, + IPRanges: []string{"127.0.0.1/32"}, + }, + }, + want: &policy.X509NameOptions{ + DNSDomains: []string{"example.com"}, + IPRanges: []string{"127.0.0.1/32"}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := &Policy{ + Allow: tt.fields.Allow, + Deny: tt.fields.Deny, + AllowWildcardNames: tt.fields.AllowWildcardNames, + } + if got := p.normalizeAllowRules(); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Policy.normalizeAllowRules() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestPolicy_normalizeDenyRules(t *testing.T) { + type fields struct { + Allow *RuleSet + Deny *RuleSet + AllowWildcardNames bool + } + tests := []struct { + name string + fields fields + want *policy.X509NameOptions + }{ + { + name: "providing no rules results in 'nil'", + fields: fields{}, + want: nil, + }, + { + name: "providing 'nil' Deny rules results in 'nil', regardless of Allow rules", + fields: fields{ + Deny: nil, + Allow: &RuleSet{}, + AllowWildcardNames: true, + }, + want: nil, + }, + { + name: "providing empty Deny rules results in 'nil', regardless of Allow rules", + fields: fields{ + Deny: &RuleSet{ + Domains: []string{}, + IPRanges: []string{}, + }, + }, + want: nil, + }, + { + name: "rules configured in Deny are returned in X509NameOptions", + fields: fields{ + Deny: &RuleSet{ + Domains: []string{"example.com"}, + IPRanges: []string{"127.0.0.1/32"}, + }, + }, + want: &policy.X509NameOptions{ + DNSDomains: []string{"example.com"}, + IPRanges: []string{"127.0.0.1/32"}, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := &Policy{ + Allow: tt.fields.Allow, + Deny: tt.fields.Deny, + AllowWildcardNames: tt.fields.AllowWildcardNames, + } + if got := p.normalizeDenyRules(); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Policy.normalizeDenyRules() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestPolicy_normalizeRules(t *testing.T) { + tests := []struct { + name string + policy *Policy + want *provisioner.X509Options + }{ + { + name: "'nil' policy results in 'nil' options", + policy: nil, + want: nil, + }, + { + name: "'nil' Allow/Deny rules and disallowing wildcard names result in 'nil' X509Options", + policy: &Policy{ + Allow: nil, + Deny: nil, + AllowWildcardNames: false, + }, + want: nil, + }, + { + name: "'nil' Allow/Deny rules and allowing wildcard names result in 'nil' Allow/Deny rules in X509Options but allowing wildcard names in X509Options", + policy: &Policy{ + Allow: nil, + Deny: nil, + AllowWildcardNames: true, + }, + want: &provisioner.X509Options{ + AllowWildcardNames: true, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.policy.normalizeRules(); !reflect.DeepEqual(got, tt.want) { + t.Errorf("Policy.normalizeRules() = %v, want %v", got, tt.want) + } + }) + } +}