From 3a39de16d4de7702d0b8bdd098cd6cca1c7c8ec3 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Sat, 2 Sep 2023 00:57:14 +0300 Subject: [PATCH 01/13] acmeserver: support specifying the allowed challenge types --- modules/caddypki/acmeserver/acmeserver.go | 12 +++- modules/caddypki/acmeserver/caddyfile.go | 5 ++ modules/caddypki/acmeserver/challenges.go | 70 +++++++++++++++++++++++ 3 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 modules/caddypki/acmeserver/challenges.go diff --git a/modules/caddypki/acmeserver/acmeserver.go b/modules/caddypki/acmeserver/acmeserver.go index 86896150c4a..bfd67517684 100644 --- a/modules/caddypki/acmeserver/acmeserver.go +++ b/modules/caddypki/acmeserver/acmeserver.go @@ -91,6 +91,8 @@ type Handler struct { // than 1 resolver address, one is chosen at random. Resolvers []string `json:"resolvers,omitempty"` + Challenges ACMEChallenges `json:"challenges,omitempty"` + logger *zap.Logger resolvers []caddy.NetworkAddress ctx caddy.Context @@ -125,6 +127,11 @@ func (ash *Handler) Provision(ctx caddy.Context) error { if ash.Lifetime == 0 { ash.Lifetime = caddy.Duration(12 * time.Hour) } + if len(ash.Challenges) > 0 { + if err := ash.Challenges.validate(); err != nil { + return err + } + } // get a reference to the configured CA appModule, err := ctx.App("pki") @@ -153,8 +160,9 @@ func (ash *Handler) Provision(ctx caddy.Context) error { AuthConfig: &authority.AuthConfig{ Provisioners: provisioner.List{ &provisioner.ACME{ - Name: ash.CA, - Type: provisioner.TypeACME.String(), + Name: ash.CA, + Challenges: ash.Challenges.toSmallstepType(), + 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 3b52113b527..e9c31d89b64 100644 --- a/modules/caddypki/acmeserver/caddyfile.go +++ b/modules/caddypki/acmeserver/caddyfile.go @@ -32,6 +32,7 @@ func init() { // ca // lifetime // resolvers +// challenges // } func parseACMEServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) { if !h.Next() { @@ -81,6 +82,10 @@ func parseACMEServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error if len(acmeServer.Resolvers) == 0 { return nil, h.Errf("must specify at least one resolver address") } + case "challenges": + acmeServer.Challenges = append(acmeServer.Challenges, stringToChallenges(h.RemainingArgs())...) + default: + return nil, h.Errf("unrecognized ACME server directive: %s", h.Val()) } } } diff --git a/modules/caddypki/acmeserver/challenges.go b/modules/caddypki/acmeserver/challenges.go new file mode 100644 index 00000000000..3d590b45dfb --- /dev/null +++ b/modules/caddypki/acmeserver/challenges.go @@ -0,0 +1,70 @@ +package acmeserver + +import ( + "encoding/json" + "fmt" + "strings" + + "github.com/smallstep/certificates/authority/provisioner" +) + +type ACMEChallenge string + +const ( + HTTP_01 ACMEChallenge = "http-01" + DNS_01 ACMEChallenge = "dns-01" + TLS_ALPN_01 ACMEChallenge = "tls-alpn-01" +) + +// validate checks if the given challenge is supported. +func (c ACMEChallenge) validate() error { + switch c { + case HTTP_01, DNS_01, TLS_ALPN_01: + return nil + default: + return fmt.Errorf("acme challenge %q is not supported", c) + } +} + +func (c *ACMEChallenge) UnmarshalJSON(b []byte) error { + var s string + if err := json.Unmarshal(b, &s); err != nil { + return err + } + *c = ACMEChallenge(strings.ToLower(s)) + return nil +} + +// String returns a string representation of the challenge. +func (c ACMEChallenge) String() string { + return strings.ToLower(string(c)) +} + +type ACMEChallenges []ACMEChallenge + +// validate checks if the given challenges are supported. +func (c ACMEChallenges) validate() error { + for _, ch := range c { + if err := ch.validate(); err != nil { + return err + } + } + return nil +} +func (c ACMEChallenges) toSmallstepType() []provisioner.ACMEChallenge { + if len(c) == 0 { + return nil + } + ac := make([]provisioner.ACMEChallenge, len(c)) + for i, ch := range c { + ac[i] = provisioner.ACMEChallenge(ch) + } + return ac +} +func stringToChallenges(chs []string) ACMEChallenges { + challenges := make(ACMEChallenges, len(chs)) + for i, ch := range chs { + challenges[i] = ACMEChallenge(ch) + } + return challenges +} From cb2b84402333442eb33b9da1e28f835f32b615c7 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Sat, 2 Sep 2023 20:50:44 +0300 Subject: [PATCH 02/13] add caddyfile adapt tests --- .../acme_server_custom_challenges.txt | 65 ++++++++++++++++++ .../acme_server_default_challenges.txt | 62 +++++++++++++++++ .../acme_server_multi_custom_challenges.txt | 66 +++++++++++++++++++ modules/caddypki/acmeserver/challenges.go | 4 +- 4 files changed, 196 insertions(+), 1 deletion(-) create mode 100644 caddytest/integration/caddyfile_adapt/acme_server_custom_challenges.txt create mode 100644 caddytest/integration/caddyfile_adapt/acme_server_default_challenges.txt create mode 100644 caddytest/integration/caddyfile_adapt/acme_server_multi_custom_challenges.txt diff --git a/caddytest/integration/caddyfile_adapt/acme_server_custom_challenges.txt b/caddytest/integration/caddyfile_adapt/acme_server_custom_challenges.txt new file mode 100644 index 00000000000..2a7a5149243 --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/acme_server_custom_challenges.txt @@ -0,0 +1,65 @@ +{ + pki { + ca custom-ca { + name "Custom CA" + } + } +} + +acme.example.com { + acme_server { + ca custom-ca + challenges dns-01 + } +} +---------- +{ + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":443" + ], + "routes": [ + { + "match": [ + { + "host": [ + "acme.example.com" + ] + } + ], + "handle": [ + { + "handler": "subroute", + "routes": [ + { + "handle": [ + { + "ca": "custom-ca", + "challenges": [ + "dns-01" + ], + "handler": "acme_server" + } + ] + } + ] + } + ], + "terminal": true + } + ] + } + } + }, + "pki": { + "certificate_authorities": { + "custom-ca": { + "name": "Custom CA" + } + } + } + } +} diff --git a/caddytest/integration/caddyfile_adapt/acme_server_default_challenges.txt b/caddytest/integration/caddyfile_adapt/acme_server_default_challenges.txt new file mode 100644 index 00000000000..26d345047c9 --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/acme_server_default_challenges.txt @@ -0,0 +1,62 @@ +{ + pki { + ca custom-ca { + name "Custom CA" + } + } +} + +acme.example.com { + acme_server { + ca custom-ca + challenges + } +} +---------- +{ + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":443" + ], + "routes": [ + { + "match": [ + { + "host": [ + "acme.example.com" + ] + } + ], + "handle": [ + { + "handler": "subroute", + "routes": [ + { + "handle": [ + { + "ca": "custom-ca", + "handler": "acme_server" + } + ] + } + ] + } + ], + "terminal": true + } + ] + } + } + }, + "pki": { + "certificate_authorities": { + "custom-ca": { + "name": "Custom CA" + } + } + } + } +} diff --git a/caddytest/integration/caddyfile_adapt/acme_server_multi_custom_challenges.txt b/caddytest/integration/caddyfile_adapt/acme_server_multi_custom_challenges.txt new file mode 100644 index 00000000000..7fe3ca663db --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/acme_server_multi_custom_challenges.txt @@ -0,0 +1,66 @@ +{ + pki { + ca custom-ca { + name "Custom CA" + } + } +} + +acme.example.com { + acme_server { + ca custom-ca + challenges dns-01 http-01 + } +} +---------- +{ + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":443" + ], + "routes": [ + { + "match": [ + { + "host": [ + "acme.example.com" + ] + } + ], + "handle": [ + { + "handler": "subroute", + "routes": [ + { + "handle": [ + { + "ca": "custom-ca", + "challenges": [ + "dns-01", + "http-01" + ], + "handler": "acme_server" + } + ] + } + ] + } + ], + "terminal": true + } + ] + } + } + }, + "pki": { + "certificate_authorities": { + "custom-ca": { + "name": "Custom CA" + } + } + } + } +} diff --git a/modules/caddypki/acmeserver/challenges.go b/modules/caddypki/acmeserver/challenges.go index 3d590b45dfb..adcd8d6dac0 100644 --- a/modules/caddypki/acmeserver/challenges.go +++ b/modules/caddypki/acmeserver/challenges.go @@ -31,7 +31,7 @@ func (c *ACMEChallenge) UnmarshalJSON(b []byte) error { if err := json.Unmarshal(b, &s); err != nil { return err } - *c = ACMEChallenge(strings.ToLower(s)) + *c = ACMEChallenge(strings.ToLower(strings.TrimSpace(s))) return nil } @@ -51,6 +51,7 @@ func (c ACMEChallenges) validate() error { } return nil } + func (c ACMEChallenges) toSmallstepType() []provisioner.ACMEChallenge { if len(c) == 0 { return nil @@ -61,6 +62,7 @@ func (c ACMEChallenges) toSmallstepType() []provisioner.ACMEChallenge { } return ac } + func stringToChallenges(chs []string) ACMEChallenges { challenges := make(ACMEChallenges, len(chs)) for i, ch := range chs { From 001465c0f6196f43085069f5a8995da7dcf16ba0 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Fri, 29 Dec 2023 17:14:51 +0300 Subject: [PATCH 03/13] introduce basic acme_server test --- caddytest/integration/acme_test.go | 170 +++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 caddytest/integration/acme_test.go diff --git a/caddytest/integration/acme_test.go b/caddytest/integration/acme_test.go new file mode 100644 index 00000000000..3ed9495f998 --- /dev/null +++ b/caddytest/integration/acme_test.go @@ -0,0 +1,170 @@ +package integration + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "log" + "net" + "net/http" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/caddyserver/caddy/v2" + "github.com/caddyserver/caddy/v2/caddytest" + "github.com/mholt/acmez" + "github.com/mholt/acmez/acme" + "go.uber.org/zap" +) + +// Test the basic functionality of Caddy's ACME server +func TestACMEServerWithDefaults(t *testing.T) { + ctx := context.Background() + // Logging is important - replace with your own zap logger + logger, err := zap.NewDevelopment() + if err != nil { + t.Error(err) + return + } + + tester := caddytest.NewTester(t) + tester.InitServer(` + { + skip_install_trust + admin localhost:2999 + http_port 9080 + https_port 9443 + local_certs + } + acme.localhost { + acme_server + } + `, "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, + }, + ChallengeSolvers: map[string]acmez.Solver{ + acme.ChallengeTypeHTTP01: naiveHTTPSolver{logger: logger}, + }, + } + + // Before you can get a cert, you'll need an account registered with + // the ACME CA; it needs a private key which should obviously be + // different from any key used for certificates! + 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, + } + + // If the account is new, we need to create it; only do this once! + // then be sure to securely store the account key and metadata so + // you can reuse it later! + account, err = client.NewAccount(ctx, account) + if err != nil { + t.Errorf("new account: %v", err) + } + + // Every certificate needs a key. + certPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Errorf("generating certificate key: %v", err) + } + + certs, err := client.ObtainCertificate(ctx, account, certPrivateKey, []string{"acme-client.localhost"}) + if err != nil { + t.Errorf("obtaining certificate: %v", err) + } + + // 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) + } +} + +// naiveHTTPSolver is a no-op acmez.Solver for example purposes only. +type naiveHTTPSolver struct { + srv *http.Server + logger *zap.Logger +} + +func (s naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) error { + s.srv = &http.Server{ + Addr: ":80", + Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + host, _, err := net.SplitHostPort(r.Host) + if err != nil { + host = r.Host + } + 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)) + r.Close = true + s.logger.Info("served key authentication", + zap.String("identifier", challenge.Identifier.Value), + zap.String("challenge", "http-01"), + zap.String("remote", r.RemoteAddr), + ) + } + }), + } + l, err := net.Listen("tcp", ":80") + if err != nil { + return err + } + s.logger.Info("present challenge", zap.Any("challenge", challenge)) + go s.srv.Serve(l) + return nil +} + +func (s naiveHTTPSolver) CleanUp(ctx context.Context, chal acme.Challenge) error { + log.Printf("[DEBUG] cleanup: %#v", chal) + if s.srv != nil { + s.srv.Close() + } + return nil +} From 621783beecb9d3f1e6d352262dea8c778abfbc54 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Fri, 29 Dec 2023 17:23:47 +0300 Subject: [PATCH 04/13] skip acme test on unsuitable environments --- caddytest/integration/acme_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/caddytest/integration/acme_test.go b/caddytest/integration/acme_test.go index 3ed9495f998..8860c567a86 100644 --- a/caddytest/integration/acme_test.go +++ b/caddytest/integration/acme_test.go @@ -31,6 +31,16 @@ func TestACMEServerWithDefaults(t *testing.T) { t.Error(err) return } + l, err := net.Listen("tcp", ":80") + if err != nil && strings.Contains(err.Error(), "permission") { + t.Skip("cannot listen on port 80") + return + } else if err != nil { + t.Logf("error listening on port 80: %s", err) + t.Error(err) + t.Skip() + } + l.Close() tester := caddytest.NewTester(t) tester.InitServer(` From 59bedb1d2fe913afac049b624e5a6b08437c686c Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Sat, 30 Dec 2023 13:22:51 +0300 Subject: [PATCH 05/13] skip integration tests of ACME --- caddytest/integration/acme_test.go | 32 ++++++++++-------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/caddytest/integration/acme_test.go b/caddytest/integration/acme_test.go index 8860c567a86..d402f638d6f 100644 --- a/caddytest/integration/acme_test.go +++ b/caddytest/integration/acme_test.go @@ -7,7 +7,6 @@ import ( "crypto/rand" "crypto/tls" "crypto/x509" - "log" "net" "net/http" "os" @@ -19,28 +18,20 @@ import ( "github.com/caddyserver/caddy/v2/caddytest" "github.com/mholt/acmez" "github.com/mholt/acmez/acme" + smallstepacme "github.com/smallstep/certificates/acme" "go.uber.org/zap" ) // Test the basic functionality of Caddy's ACME server func TestACMEServerWithDefaults(t *testing.T) { + t.Skip("TODO: troubleshoot the test; it succeeds randomly with small probability") + ctx := context.Background() - // Logging is important - replace with your own zap logger logger, err := zap.NewDevelopment() if err != nil { t.Error(err) return } - l, err := net.Listen("tcp", ":80") - if err != nil && strings.Contains(err.Error(), "permission") { - t.Skip("cannot listen on port 80") - return - } else if err != nil { - t.Logf("error listening on port 80: %s", err) - t.Error(err) - t.Skip() - } - l.Close() tester := caddytest.NewTester(t) tester.InitServer(` @@ -93,9 +84,6 @@ func TestACMEServerWithDefaults(t *testing.T) { }, } - // Before you can get a cert, you'll need an account registered with - // the ACME CA; it needs a private key which should obviously be - // different from any key used for certificates! accountPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) if err != nil { t.Errorf("generating account key: %v", err) @@ -105,24 +93,23 @@ func TestACMEServerWithDefaults(t *testing.T) { TermsOfServiceAgreed: true, PrivateKey: accountPrivateKey, } - - // If the account is new, we need to create it; only do this once! - // then be sure to securely store the account key and metadata so - // you can reuse it later! 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{"acme-client.localhost"}) if err != nil { t.Errorf("obtaining certificate: %v", err) + return } // ACME servers should usually give you the entire certificate chain @@ -142,6 +129,7 @@ type naiveHTTPSolver struct { } func (s naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) error { + smallstepacme.InsecurePortHTTP01 = 8080 s.srv = &http.Server{ Addr: ":80", Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -150,7 +138,6 @@ func (s naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) host = r.Host } 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)) r.Close = true @@ -171,8 +158,9 @@ func (s naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) return nil } -func (s naiveHTTPSolver) CleanUp(ctx context.Context, chal acme.Challenge) error { - log.Printf("[DEBUG] cleanup: %#v", chal) +func (s naiveHTTPSolver) CleanUp(ctx context.Context, challenge acme.Challenge) error { + s.logger.Info("cleanup", zap.Any("challenge", challenge)) + smallstepacme.InsecurePortHTTP01 = 80 if s.srv != nil { s.srv.Close() } From 5f8c209721ea1c272091f7ad681325fd5ff77435 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Mon, 1 Jan 2024 21:16:30 +0300 Subject: [PATCH 06/13] documentation --- modules/caddypki/acmeserver/acmeserver.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/modules/caddypki/acmeserver/acmeserver.go b/modules/caddypki/acmeserver/acmeserver.go index d687be3505c..e0588c527a8 100644 --- a/modules/caddypki/acmeserver/acmeserver.go +++ b/modules/caddypki/acmeserver/acmeserver.go @@ -91,7 +91,10 @@ type Handler struct { // than 1 resolver address, one is chosen at random. Resolvers []string `json:"resolvers,omitempty"` - Challenges ACMEChallenges `json:"challenges,omitempty"` + // Specify the set of enabled ACME challenges. An empty or absent value + // means all challenges are enabled. Accepted values are: + // "http-01", "dns-01", "tls-alpn-01" + Challenges ACMEChallenges `json:"challenges,omitempty" ` logger *zap.Logger resolvers []caddy.NetworkAddress From 9d984eab74d21a480572b7d1bd3f76d31bcf8fd3 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Fri, 5 Jan 2024 23:35:03 +0300 Subject: [PATCH 07/13] add negative-scenario test for mismatched allowed challenges --- caddytest/integration/acme_test.go | 94 ++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/caddytest/integration/acme_test.go b/caddytest/integration/acme_test.go index d402f638d6f..dad1fb51e62 100644 --- a/caddytest/integration/acme_test.go +++ b/caddytest/integration/acme_test.go @@ -122,6 +122,100 @@ func TestACMEServerWithDefaults(t *testing.T) { } } +func TestACMEServerWithMismatchedChallenges(t *testing.T) { + // t.Skip("TODO: troubleshoot the test; it succeeds randomly with small probability") + + ctx := context.Background() + logger := caddy.Log().Named("acmez") + + tester := caddytest.NewTester(t) + tester.InitServer(` + { + skip_install_trust + admin localhost:2999 + http_port 9080 + https_port 9443 + local_certs + } + acme.localhost { + acme_server { + challenges tls-alpn-01 + } + } + `, "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, + }, + 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{"acme-client.localhost"}) + if len(certs) > 0 { + t.Errorf("expected '0' certificates, but received '%d'", len(certs)) + } + if err == nil { + t.Error("expected errors, but received none") + } + const expectedErrMsg = "no solvers available for remaining challenges (configured=[http-01] offered=[tls-alpn-01] remaining=[tls-alpn-01])" + if !strings.Contains(err.Error(), expectedErrMsg) { + t.Errorf(`received error message does not match expectation: expected="%s" received="%s"`, expectedErrMsg, err.Error()) + } +} + // naiveHTTPSolver is a no-op acmez.Solver for example purposes only. type naiveHTTPSolver struct { srv *http.Server From 39c37a71f936e625cd64769f54926a354681f8a8 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Fri, 5 Jan 2024 23:54:35 +0300 Subject: [PATCH 08/13] a bit more docs --- modules/caddypki/acmeserver/challenges.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/modules/caddypki/acmeserver/challenges.go b/modules/caddypki/acmeserver/challenges.go index adcd8d6dac0..eff1d7bb422 100644 --- a/modules/caddypki/acmeserver/challenges.go +++ b/modules/caddypki/acmeserver/challenges.go @@ -8,6 +8,7 @@ import ( "github.com/smallstep/certificates/authority/provisioner" ) +// an opaque string type to represent the supported ACME challenges type ACMEChallenge string const ( @@ -26,6 +27,9 @@ func (c ACMEChallenge) validate() error { } } +// The unmarshaller first marshals the value into a string. Then it +// trims any space around it and lowercase it for normaliztion. The +// method does not and should not validate the value within accepted enums. func (c *ACMEChallenge) UnmarshalJSON(b []byte) error { var s string if err := json.Unmarshal(b, &s); err != nil { @@ -40,6 +44,7 @@ func (c ACMEChallenge) String() string { return strings.ToLower(string(c)) } +// container for a collective of ACME challenges type ACMEChallenges []ACMEChallenge // validate checks if the given challenges are supported. From 9d87007bb0797a6fb5fe93ecef380706831d50c0 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Tue, 30 Jan 2024 15:04:42 +0300 Subject: [PATCH 09/13] fix tests for ACME challenges --- caddytest/integration/acme_test.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/caddytest/integration/acme_test.go b/caddytest/integration/acme_test.go index dad1fb51e62..ef34a932e24 100644 --- a/caddytest/integration/acme_test.go +++ b/caddytest/integration/acme_test.go @@ -24,8 +24,6 @@ import ( // Test the basic functionality of Caddy's ACME server func TestACMEServerWithDefaults(t *testing.T) { - t.Skip("TODO: troubleshoot the test; it succeeds randomly with small probability") - ctx := context.Background() logger, err := zap.NewDevelopment() if err != nil { @@ -80,7 +78,7 @@ func TestACMEServerWithDefaults(t *testing.T) { Logger: logger, }, ChallengeSolvers: map[string]acmez.Solver{ - acme.ChallengeTypeHTTP01: naiveHTTPSolver{logger: logger}, + acme.ChallengeTypeHTTP01: &naiveHTTPSolver{logger: logger}, }, } @@ -106,7 +104,7 @@ func TestACMEServerWithDefaults(t *testing.T) { return } - certs, err := client.ObtainCertificate(ctx, account, certPrivateKey, []string{"acme-client.localhost"}) + certs, err := client.ObtainCertificate(ctx, account, certPrivateKey, []string{"localhost"}) if err != nil { t.Errorf("obtaining certificate: %v", err) return @@ -123,8 +121,6 @@ func TestACMEServerWithDefaults(t *testing.T) { } func TestACMEServerWithMismatchedChallenges(t *testing.T) { - // t.Skip("TODO: troubleshoot the test; it succeeds randomly with small probability") - ctx := context.Background() logger := caddy.Log().Named("acmez") @@ -177,7 +173,7 @@ func TestACMEServerWithMismatchedChallenges(t *testing.T) { Logger: logger, }, ChallengeSolvers: map[string]acmez.Solver{ - acme.ChallengeTypeHTTP01: naiveHTTPSolver{logger: logger}, + acme.ChallengeTypeHTTP01: &naiveHTTPSolver{logger: logger}, }, } @@ -203,7 +199,7 @@ func TestACMEServerWithMismatchedChallenges(t *testing.T) { return } - certs, err := client.ObtainCertificate(ctx, account, certPrivateKey, []string{"acme-client.localhost"}) + certs, err := client.ObtainCertificate(ctx, account, certPrivateKey, []string{"localhost"}) if len(certs) > 0 { t.Errorf("expected '0' certificates, but received '%d'", len(certs)) } @@ -222,10 +218,9 @@ type naiveHTTPSolver struct { logger *zap.Logger } -func (s naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) error { - smallstepacme.InsecurePortHTTP01 = 8080 +func (s *naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) error { s.srv = &http.Server{ - Addr: ":80", + Addr: "localhost:80", Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { host, _, err := net.SplitHostPort(r.Host) if err != nil { From 9deff5d2d41f646ce1a04780ac02c0329cacaf67 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Tue, 30 Jan 2024 15:06:32 +0300 Subject: [PATCH 10/13] appease the linter --- modules/caddypki/acmeserver/caddyfile.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/caddypki/acmeserver/caddyfile.go b/modules/caddypki/acmeserver/caddyfile.go index 1b514c1c60d..864a94c534c 100644 --- a/modules/caddypki/acmeserver/caddyfile.go +++ b/modules/caddypki/acmeserver/caddyfile.go @@ -84,7 +84,6 @@ func parseACMEServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error acmeServer.Challenges = append(acmeServer.Challenges, stringToChallenges(h.RemainingArgs())...) default: return nil, h.Errf("unrecognized ACME server directive: %s", h.Val()) - } } From c9c6478b0900a2f05912383fedcfdf73ebf9db45 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Tue, 30 Jan 2024 15:10:39 +0300 Subject: [PATCH 11/13] skip ACME tests on s390x --- caddytest/integration/acme_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/caddytest/integration/acme_test.go b/caddytest/integration/acme_test.go index ef34a932e24..74e35a2a8cb 100644 --- a/caddytest/integration/acme_test.go +++ b/caddytest/integration/acme_test.go @@ -11,6 +11,7 @@ import ( "net/http" "os" "path/filepath" + "runtime" "strings" "testing" @@ -24,6 +25,10 @@ import ( // Test the basic functionality of Caddy's ACME server func TestACMEServerWithDefaults(t *testing.T) { + if runtime.GOARCH == "s390x" { + t.Skip("skipping test on s390x") + return + } ctx := context.Background() logger, err := zap.NewDevelopment() if err != nil { @@ -121,6 +126,10 @@ func TestACMEServerWithDefaults(t *testing.T) { } func TestACMEServerWithMismatchedChallenges(t *testing.T) { + if runtime.GOARCH == "s390x" { + t.Skip("skipping test on s390x") + return + } ctx := context.Background() logger := caddy.Log().Named("acmez") From e418b4e1c88302a3a157b18ab72485fbfef8a304 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Tue, 30 Jan 2024 19:24:13 +0300 Subject: [PATCH 12/13] enable ACME challenge tests on all machines --- caddytest/integration/acme_test.go | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/caddytest/integration/acme_test.go b/caddytest/integration/acme_test.go index 74e35a2a8cb..00a3a6c4e96 100644 --- a/caddytest/integration/acme_test.go +++ b/caddytest/integration/acme_test.go @@ -7,11 +7,11 @@ import ( "crypto/rand" "crypto/tls" "crypto/x509" + "fmt" "net" "net/http" "os" "path/filepath" - "runtime" "strings" "testing" @@ -23,12 +23,10 @@ import ( "go.uber.org/zap" ) +const acmeChallengePort = 8080 + // Test the basic functionality of Caddy's ACME server func TestACMEServerWithDefaults(t *testing.T) { - if runtime.GOARCH == "s390x" { - t.Skip("skipping test on s390x") - return - } ctx := context.Background() logger, err := zap.NewDevelopment() if err != nil { @@ -126,10 +124,6 @@ func TestACMEServerWithDefaults(t *testing.T) { } func TestACMEServerWithMismatchedChallenges(t *testing.T) { - if runtime.GOARCH == "s390x" { - t.Skip("skipping test on s390x") - return - } ctx := context.Background() logger := caddy.Log().Named("acmez") @@ -228,8 +222,9 @@ type naiveHTTPSolver struct { } func (s *naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) error { + smallstepacme.InsecurePortHTTP01 = acmeChallengePort s.srv = &http.Server{ - Addr: "localhost:80", + Addr: fmt.Sprintf("localhost:%d", acmeChallengePort), Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { host, _, err := net.SplitHostPort(r.Host) if err != nil { @@ -247,7 +242,7 @@ func (s *naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) } }), } - l, err := net.Listen("tcp", ":80") + l, err := net.Listen("tcp", fmt.Sprintf(":%d", acmeChallengePort)) if err != nil { return err } @@ -257,8 +252,8 @@ func (s *naiveHTTPSolver) Present(ctx context.Context, challenge acme.Challenge) } func (s naiveHTTPSolver) CleanUp(ctx context.Context, challenge acme.Challenge) error { + smallstepacme.InsecurePortHTTP01 = 0 s.logger.Info("cleanup", zap.Any("challenge", challenge)) - smallstepacme.InsecurePortHTTP01 = 80 if s.srv != nil { s.srv.Close() } From a55e7232b52786c51ee18112c602c01f9afc5cb2 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Mon, 5 Feb 2024 17:29:31 +0300 Subject: [PATCH 13/13] Apply suggestions from code review Co-authored-by: Matt Holt --- modules/caddypki/acmeserver/challenges.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/caddypki/acmeserver/challenges.go b/modules/caddypki/acmeserver/challenges.go index eff1d7bb422..728a7492813 100644 --- a/modules/caddypki/acmeserver/challenges.go +++ b/modules/caddypki/acmeserver/challenges.go @@ -8,7 +8,7 @@ import ( "github.com/smallstep/certificates/authority/provisioner" ) -// an opaque string type to represent the supported ACME challenges +// ACMEChallenge is an opaque string that represents supported ACME challenges. type ACMEChallenge string const ( @@ -44,7 +44,7 @@ func (c ACMEChallenge) String() string { return strings.ToLower(string(c)) } -// container for a collective of ACME challenges +// ACMEChallenges is a list of ACME challenges. type ACMEChallenges []ACMEChallenge // validate checks if the given challenges are supported.