diff --git a/pkg/acme/client/interfaces.go b/pkg/acme/client/interfaces.go index 5a3f87e54..4045a0afc 100644 --- a/pkg/acme/client/interfaces.go +++ b/pkg/acme/client/interfaces.go @@ -36,13 +36,24 @@ type Interface interface { ListCertAlternates(ctx context.Context, url string) ([]string, error) WaitOrder(ctx context.Context, url string) (*acme.Order, error) CreateOrderCert(ctx context.Context, finalizeURL string, csr []byte, bundle bool) (der [][]byte, certURL string, err error) + // Accept will (in success cases) be called once per a Challenge once it + // has passed self-check and is ready to be verified by the ACME server. Accept(ctx context.Context, chal *acme.Challenge) (*acme.Challenge, error) GetChallenge(ctx context.Context, url string) (*acme.Challenge, error) + // GetAuthorization will be called once for each required authorization + // for an Order. Additionally it will be called most likely once when a + // Challenge has been scheduled for processing to retrieve its status. GetAuthorization(ctx context.Context, url string) (*acme.Authorization, error) + // WaitAuthorization will, in success cases, be called once per + // Challenge after it has been accepted. WaitAuthorization(ctx context.Context, url string) (*acme.Authorization, error) Register(ctx context.Context, acct *acme.Account, prompt func(tosURL string) bool) (*acme.Account, error) GetReg(ctx context.Context, url string) (*acme.Account, error) + // HTTP01ChallengeResponse will be called once when an cert-manager.io + // Challenge for an http-01 challenge type is being created. HTTP01ChallengeResponse(token string) (string, error) + // DNS01ChallengeResponse will be called once when an cert-manager.io + // Challenge for an http-01 challenge type is being created. DNS01ChallengeRecord(token string) (string, error) Discover(ctx context.Context) (acme.Directory, error) UpdateReg(ctx context.Context, a *acme.Account) (*acme.Account, error) diff --git a/pkg/controller/acmeorders/sync.go b/pkg/controller/acmeorders/sync.go index 6cde88af7..9ab74020d 100644 --- a/pkg/controller/acmeorders/sync.go +++ b/pkg/controller/acmeorders/sync.go @@ -121,7 +121,7 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { } dbg.Info("Computing list of Challenge resources that need to exist to complete this Order") - requiredChallenges, err := buildRequiredChallenges(ctx, cl, genericIssuer, o) + requiredChallenges, err := buildPartialRequiredChallenges(ctx, genericIssuer, o) if err != nil { log.Error(err, "Failed to determine the list of Challenge resources needed for the Order") c.recorder.Eventf(o, corev1.EventTypeWarning, reasonSolver, "Failed to determine a valid solver configuration for the set of domains on the Order: %v", err) @@ -142,6 +142,10 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { switch { case needToCreateChallenges: log.V(logf.DebugLevel).Info("Creating additional Challenge resources to complete Order") + requiredChallenges, err = ensureKeysForChallenges(cl, requiredChallenges) + if err != nil { + return err + } return c.createRequiredChallenges(ctx, o, requiredChallenges) case needToDeleteChallenges: log.V(logf.DebugLevel).Info("Deleting leftover Challenge resources no longer required by Order") @@ -161,6 +165,14 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { return c.finalizeOrder(ctx, cl, o, genericIssuer) } + // At this point, if no Challenges have failed or reached a final state, + // we can return without taking any action. This controller will resync + // the Order on any owned Challenge events. + if !anyChallengesFailed(challenges) && !allChallengesFinal(challenges) { + log.V(logf.DebugLevel).Info("No action taken") + return nil + } + // Note: each of the following code paths uses the ACME Order retrieved // here. Be mindful when adding new code below this call to ACME server- // if the new code does not need this ACME order, try to place it above @@ -181,9 +193,14 @@ func (c *controller) Sync(ctx context.Context, o *cmacme.Order) (err error) { switch { case anyChallengesFailed(challenges): - // TODO (@munnerz): instead of waiting for the ACME server to mark this - // Order as failed, we could just mark the Order as failed as there is - // no way that we will attempt and continue the order anyway. + // TODO (@munnerz): instead of waiting for the ACME server to + // mark this Order as failed, we could just mark the Order as + // failed as there is no way that we will attempt and continue + // the order anyway. This might, however, be a breaking change + // in edge cases where the status of the order resource in ACME + // server cannot be determined from challenge resource statuses + // correctly. Do not change this unless there is a real need for + // it. log.V(logf.DebugLevel).Info("Update Order status as at least one Challenge has failed") _, err := c.updateOrderStatusFromACMEOrder(ctx, cl, o, acmeOrder) if acmeErr, ok := err.(*acmeapi.Error); ok { @@ -386,7 +403,7 @@ func (c *controller) fetchMetadataForAuthorizations(ctx context.Context, o *cmac return nil } -func (c *controller) anyRequiredChallengesDoNotExist(requiredChallenges []cmacme.Challenge) (bool, error) { +func (c *controller) anyRequiredChallengesDoNotExist(requiredChallenges []*cmacme.Challenge) (bool, error) { for _, ch := range requiredChallenges { _, err := c.challengeLister.Challenges(ch.Namespace).Get(ch.Name) if apierrors.IsNotFound(err) { @@ -399,9 +416,9 @@ func (c *controller) anyRequiredChallengesDoNotExist(requiredChallenges []cmacme return false, nil } -func (c *controller) createRequiredChallenges(ctx context.Context, o *cmacme.Order, requiredChallenges []cmacme.Challenge) error { +func (c *controller) createRequiredChallenges(ctx context.Context, o *cmacme.Order, requiredChallenges []*cmacme.Challenge) error { for _, ch := range requiredChallenges { - _, err := c.cmClient.AcmeV1().Challenges(ch.Namespace).Create(ctx, &ch, metav1.CreateOptions{}) + _, err := c.cmClient.AcmeV1().Challenges(ch.Namespace).Create(ctx, ch, metav1.CreateOptions{}) if apierrors.IsAlreadyExists(err) { continue } @@ -413,7 +430,7 @@ func (c *controller) createRequiredChallenges(ctx context.Context, o *cmacme.Ord return nil } -func (c *controller) anyLeftoverChallengesExist(o *cmacme.Order, requiredChallenges []cmacme.Challenge) (bool, error) { +func (c *controller) anyLeftoverChallengesExist(o *cmacme.Order, requiredChallenges []*cmacme.Challenge) (bool, error) { leftoverChallenges, err := c.determineLeftoverChallenges(o, requiredChallenges) if err != nil { return false, err @@ -422,7 +439,7 @@ func (c *controller) anyLeftoverChallengesExist(o *cmacme.Order, requiredChallen return len(leftoverChallenges) > 0, nil } -func (c *controller) deleteLeftoverChallenges(ctx context.Context, o *cmacme.Order, requiredChallenges []cmacme.Challenge) error { +func (c *controller) deleteLeftoverChallenges(ctx context.Context, o *cmacme.Order, requiredChallenges []*cmacme.Challenge) error { leftover, err := c.determineLeftoverChallenges(o, requiredChallenges) if err != nil { return err @@ -452,7 +469,7 @@ func (c *controller) deleteAllChallenges(ctx context.Context, o *cmacme.Order) e return nil } -func (c *controller) determineLeftoverChallenges(o *cmacme.Order, requiredChallenges []cmacme.Challenge) ([]*cmacme.Challenge, error) { +func (c *controller) determineLeftoverChallenges(o *cmacme.Order, requiredChallenges []*cmacme.Challenge) ([]*cmacme.Challenge, error) { requiredNames := map[string]struct{}{} for _, ch := range requiredChallenges { requiredNames[ch.Name] = struct{}{} diff --git a/pkg/controller/acmeorders/sync_test.go b/pkg/controller/acmeorders/sync_test.go index a04fe2521..c55b9723f 100644 --- a/pkg/controller/acmeorders/sync_test.go +++ b/pkg/controller/acmeorders/sync_test.go @@ -206,10 +206,16 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt return "key", nil }, } - testAuthorizationChallenge, err := buildChallenge(context.TODO(), fakeHTTP01ACMECl, testIssuerHTTP01TestCom, testOrderPending, testOrderPending.Status.Authorizations[0]) + testAuthorizationChallenge, err := buildPartialChallenge(context.TODO(), testIssuerHTTP01TestCom, testOrderPending, testOrderPending.Status.Authorizations[0]) + if err != nil { t.Fatalf("error building Challenge resource test fixture: %v", err) } + key, err := fakeHTTP01ACMECl.FakeHTTP01ChallengeResponse(testAuthorizationChallenge.Spec.Token) + if err != nil { + t.Fatalf("error building Challenge resource test fixture: %v", err) + } + testAuthorizationChallenge.Spec.Key = key testAuthorizationChallengeValid := testAuthorizationChallenge.DeepCopy() testAuthorizationChallengeValid.Status.State = cmacme.Valid testAuthorizationChallengeInvalid := testAuthorizationChallenge.DeepCopy() @@ -338,7 +344,7 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt testpkg.NewAction(coretesting.NewCreateAction(cmacme.SchemeGroupVersion.WithResource("challenges"), testAuthorizationChallenge.Namespace, testAuthorizationChallenge)), }, ExpectedEvents: []string{ - `Normal Created Created Challenge resource "testorder-2179654896" for domain "test.com"`, + `Normal Created Created Challenge resource "testorder-756011405" for domain "test.com"`, }, }, acmeClient: &acmecl.FakeACME{ @@ -490,9 +496,6 @@ rUCGwbCUDI0mxadJ3Bz4WxR6fyNpBK2yAinWEsikxqEt ExpectedActions: []testpkg.Action{}, }, acmeClient: &acmecl.FakeACME{ - FakeGetOrder: func(_ context.Context, url string) (*acmeapi.Order, error) { - return testACMEOrderPending, nil - }, FakeHTTP01ChallengeResponse: func(s string) (string, error) { // TODO: assert s = "token" return "key", nil diff --git a/pkg/controller/acmeorders/util.go b/pkg/controller/acmeorders/util.go index 4952c3be0..f4985c91b 100644 --- a/pkg/controller/acmeorders/util.go +++ b/pkg/controller/acmeorders/util.go @@ -35,8 +35,11 @@ var ( orderGvk = cmacme.SchemeGroupVersion.WithKind("Order") ) -func buildRequiredChallenges(ctx context.Context, cl acmecl.Interface, issuer cmapi.GenericIssuer, o *cmacme.Order) ([]cmacme.Challenge, error) { - chs := make([]cmacme.Challenge, 0) +// buildPartialRequiredChallenges builds partial required ACME challenges by +// looking at authorization on order spec and related issuer. It does not call +// ACME. ensureKeysForChallenge must be called before creating the Challenge. +func buildPartialRequiredChallenges(ctx context.Context, issuer cmapi.GenericIssuer, o *cmacme.Order) ([]*cmacme.Challenge, error) { + chs := make([]*cmacme.Challenge, 0) for _, a := range o.Status.Authorizations { if a.InitialState == cmacme.Valid { wc := false @@ -46,17 +49,20 @@ func buildRequiredChallenges(ctx context.Context, cl acmecl.Interface, issuer cm logf.FromContext(ctx).V(logf.DebugLevel).Info("Authorization already valid, not creating Challenge resource", "identifier", a.Identifier, "is_wildcard", wc) continue } - ch, err := buildChallenge(ctx, cl, issuer, o, a) + ch, err := buildPartialChallenge(ctx, issuer, o, a) if err != nil { return nil, err } - chs = append(chs, *ch) + chs = append(chs, ch) } return chs, nil } -func buildChallenge(ctx context.Context, cl acmecl.Interface, issuer cmapi.GenericIssuer, o *cmacme.Order, authz cmacme.ACMEAuthorization) (*cmacme.Challenge, error) { - chSpec, err := challengeSpecForAuthorization(ctx, cl, issuer, o, authz) +// buildPartialChallenge builds a challenge for the required ACME Authorization. +// The spec will be populated with fields that can be determined by looking at +// the ACME Authorization object returned in Order. +func buildPartialChallenge(ctx context.Context, issuer cmapi.GenericIssuer, o *cmacme.Order, authz cmacme.ACMEAuthorization) (*cmacme.Challenge, error) { + chSpec, err := partialChallengeSpecForAuthorization(ctx, issuer, o, authz) if err != nil { // TODO: in this case, we should probably not return the error as it's // unlikely we can make it succeed by retrying. @@ -78,7 +84,10 @@ func buildChallenge(ctx context.Context, cl acmecl.Interface, issuer cmapi.Gener }, nil } -func challengeSpecForAuthorization(ctx context.Context, cl acmecl.Interface, issuer cmapi.GenericIssuer, o *cmacme.Order, authz cmacme.ACMEAuthorization) (*cmacme.ChallengeSpec, error) { +// partialChallengeSpecForAuthorization builds a partial challenge spec by +// looking at the ACME authorization object and issuer. It does not make any +// ACME calls. +func partialChallengeSpecForAuthorization(ctx context.Context, issuer cmapi.GenericIssuer, o *cmacme.Order, authz cmacme.ACMEAuthorization) (*cmacme.ChallengeSpec, error) { log := logf.FromContext(ctx, "challengeSpecForAuthorization") dbg := log.V(logf.DebugLevel) @@ -258,11 +267,6 @@ func challengeSpecForAuthorization(ctx context.Context, cl acmecl.Interface, iss return nil, err } - key, err := keyForChallenge(cl, selectedChallenge.Token, chType) - if err != nil { - return nil, err - } - // 4. handle overriding the HTTP01 ingress class and name fields using the // ACMECertificateHTTP01IngressNameOverride & Class annotations if err := applyIngressParameterAnnotationOverrides(o, selectedSolver); err != nil { @@ -276,7 +280,6 @@ func challengeSpecForAuthorization(ctx context.Context, cl acmecl.Interface, iss URL: selectedChallenge.URL, DNSName: authz.Identifier, Token: selectedChallenge.Token, - Key: key, // selectedSolver cannot be nil due to the check above. Solver: *selectedSolver, Wildcard: wc, @@ -321,15 +324,26 @@ func applyIngressParameterAnnotationOverrides(o *cmacme.Order, s *cmacme.ACMECha return nil } -func keyForChallenge(cl acmecl.Interface, token string, chType cmacme.ACMEChallengeType) (string, error) { - switch chType { - case cmacme.ACMEChallengeTypeHTTP01: - return cl.HTTP01ChallengeResponse(token) - case cmacme.ACMEChallengeTypeDNS01: - return cl.DNS01ChallengeRecord(token) - default: - return "", fmt.Errorf("unsupported challenge type: %v", chType) +func ensureKeysForChallenges(cl acmecl.Interface, challenges []*cmacme.Challenge) ([]*cmacme.Challenge, error) { + for _, ch := range challenges { + var ( + key string + err error + ) + switch ch.Spec.Type { + case cmacme.ACMEChallengeTypeHTTP01: + key, err = cl.HTTP01ChallengeResponse(ch.Spec.Token) + case cmacme.ACMEChallengeTypeDNS01: + key, err = cl.DNS01ChallengeRecord(ch.Spec.Token) + default: + return nil, fmt.Errorf("challenge %s has unsupported challenge type: %s", ch.Name, ch.Spec.Type) + } + if err != nil { + return nil, err + } + ch.Spec.Key = key } + return challenges, nil } func anyChallengesFailed(chs []*cmacme.Challenge) bool { diff --git a/pkg/controller/acmeorders/util_test.go b/pkg/controller/acmeorders/util_test.go index eda53bbcf..ba5d8dce5 100644 --- a/pkg/controller/acmeorders/util_test.go +++ b/pkg/controller/acmeorders/util_test.go @@ -18,6 +18,7 @@ package acmeorders import ( "context" + "fmt" "reflect" "testing" @@ -27,7 +28,8 @@ import ( acmecl "github.com/cert-manager/cert-manager/pkg/acme/client" cmacme "github.com/cert-manager/cert-manager/pkg/apis/acme/v1" - v1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + "github.com/cert-manager/cert-manager/test/unit/gen" ) func TestChallengeSpecForAuthorization(t *testing.T) { @@ -91,7 +93,7 @@ func TestChallengeSpecForAuthorization(t *testing.T) { tests := map[string]struct { acmeClient acmecl.Interface - issuer v1.GenericIssuer + issuer cmapi.GenericIssuer order *cmacme.Order authz *cmacme.ACMEAuthorization @@ -100,9 +102,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }{ "should override the ingress name to edit if override annotation is specified": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{emptySelectorSolverHTTP01}, }, @@ -127,7 +129,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: cmacme.ACMEChallengeSolver{ HTTP01: &cmacme.ACMEChallengeSolverHTTP01{ Ingress: &cmacme.ACMEChallengeSolverHTTP01Ingress{ @@ -139,9 +140,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "should override the ingress class to edit if override annotation is specified": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{emptySelectorSolverHTTP01}, }, @@ -166,7 +167,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: cmacme.ACMEChallengeSolver{ HTTP01: &cmacme.ACMEChallengeSolverHTTP01{ Ingress: &cmacme.ACMEChallengeSolverHTTP01Ingress{ @@ -178,9 +178,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "should return an error if both ingress class and name override annotations are set": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{emptySelectorSolverHTTP01}, }, @@ -206,9 +206,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "should ignore HTTP01 override annotations if DNS01 solver is chosen": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{emptySelectorSolverDNS01}, }, @@ -233,15 +233,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeDNS01, DNSName: "example.com", Token: acmeChallengeDNS01.Token, - Key: "dns01", Solver: emptySelectorSolverDNS01, }, }, "should use configured default solver when no others are present": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{emptySelectorSolverHTTP01}, }, @@ -261,15 +260,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: emptySelectorSolverHTTP01, }, }, "should use configured default solver when no others are present but selector is non-nil": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ { @@ -298,7 +296,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{}, HTTP01: &cmacme.ACMEChallengeSolverHTTP01{ @@ -311,9 +308,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "should use configured default solver when others do not match": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ emptySelectorSolverHTTP01, @@ -336,15 +333,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: emptySelectorSolverHTTP01, }, }, "should use DNS01 solver over HTTP01 if challenge is of type DNS01": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ emptySelectorSolverHTTP01, @@ -367,15 +363,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeDNS01, DNSName: "example.com", Token: acmeChallengeDNS01.Token, - Key: "dns01", Solver: emptySelectorSolverDNS01, }, }, "should return an error if none match": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ nonMatchingSelectorSolver, @@ -397,9 +392,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "uses correct solver when selector explicitly names dnsName": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ emptySelectorSolverHTTP01, @@ -422,15 +417,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: exampleComDNSNameSelectorSolver, }, }, "uses default solver if dnsName does not match": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ emptySelectorSolverHTTP01, @@ -453,15 +447,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "notexample.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: emptySelectorSolverHTTP01, }, }, "if two solvers specify the same dnsName, the one with the most labels should be chosen": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ exampleComDNSNameSelectorSolver, @@ -501,7 +494,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{ MatchLabels: map[string]string{ @@ -519,9 +511,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "if one solver matches with dnsNames, and the other solver matches with labels, the dnsName solver should be chosen": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ exampleComDNSNameSelectorSolver, @@ -560,7 +552,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: exampleComDNSNameSelectorSolver, }, }, @@ -568,9 +559,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { // order to ensure that this behaviour isn't just incidental "if one solver matches with dnsNames, and the other solver matches with labels, the dnsName solver should be chosen (solvers listed in reverse order)": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ { @@ -609,15 +600,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: exampleComDNSNameSelectorSolver, }, }, "if one solver matches with dnsNames, and the other solver matches with 2 labels, the dnsName solver should be chosen": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ exampleComDNSNameSelectorSolver, @@ -658,15 +648,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: exampleComDNSNameSelectorSolver, }, }, "should choose the solver with the most labels matching if multiple match": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ { @@ -718,7 +707,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{ MatchLabels: map[string]string{ @@ -736,9 +724,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "should match wildcard dnsName solver if authorization has Wildcard=true": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ emptySelectorSolverDNS01, @@ -772,7 +760,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { DNSName: "example.com", Wildcard: true, Token: acmeChallengeDNS01.Token, - Key: "dns01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{ DNSNames: []string{"*.example.com"}, @@ -787,9 +774,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "dnsName selectors should take precedence over dnsZone selectors": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ exampleComDNSNameSelectorSolver, @@ -821,15 +808,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: exampleComDNSNameSelectorSolver, }, }, "dnsName selectors should take precedence over dnsZone selectors (reversed order)": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ { @@ -861,15 +847,14 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: exampleComDNSNameSelectorSolver, }, }, "should allow matching with dnsZones": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ emptySelectorSolverDNS01, @@ -903,7 +888,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { DNSName: "www.example.com", Wildcard: true, Token: acmeChallengeDNS01.Token, - Key: "dns01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{ DNSZones: []string{"example.com"}, @@ -918,9 +902,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "most specific dnsZone should be selected if multiple match": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ { @@ -963,7 +947,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { DNSName: "www.prod.example.com", Wildcard: true, Token: acmeChallengeDNS01.Token, - Key: "dns01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{ DNSZones: []string{"prod.example.com"}, @@ -978,9 +961,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "most specific dnsZone should be selected if multiple match (reversed)": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ { @@ -1023,7 +1006,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { DNSName: "www.prod.example.com", Wildcard: true, Token: acmeChallengeDNS01.Token, - Key: "dns01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{ DNSZones: []string{"prod.example.com"}, @@ -1038,9 +1020,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "if two solvers specify the same dnsZone, the one with the most labels should be chosen": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ { @@ -1089,7 +1071,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "www.example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{ MatchLabels: map[string]string{ @@ -1107,9 +1088,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "if both solvers match dnsNames, and one also matches dnsZones, choose the one that matches dnsZones": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ { @@ -1151,7 +1132,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "www.example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{ DNSZones: []string{"example.com"}, @@ -1167,9 +1147,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "if both solvers match dnsNames, and one also matches dnsZones, choose the one that matches dnsZones (reversed)": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ { @@ -1211,7 +1191,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "www.example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: cmacme.ACMEChallengeSolver{ Selector: &cmacme.CertificateDNSNameSelector{ DNSZones: []string{"example.com"}, @@ -1227,9 +1206,9 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }, "uses correct solver when selector explicitly names dnsName (reversed)": { acmeClient: basicACMEClient, - issuer: &v1.Issuer{ - Spec: v1.IssuerSpec{ - IssuerConfig: v1.IssuerConfig{ + issuer: &cmapi.Issuer{ + Spec: cmapi.IssuerSpec{ + IssuerConfig: cmapi.IssuerConfig{ ACME: &cmacme.ACMEIssuer{ Solvers: []cmacme.ACMEChallengeSolver{ exampleComDNSNameSelectorSolver, @@ -1252,7 +1231,6 @@ func TestChallengeSpecForAuthorization(t *testing.T) { Type: cmacme.ACMEChallengeTypeHTTP01, DNSName: "example.com", Token: acmeChallengeHTTP01.Token, - Key: "http01", Solver: exampleComDNSNameSelectorSolver, }, }, @@ -1260,7 +1238,7 @@ func TestChallengeSpecForAuthorization(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { ctx := context.Background() - cs, err := challengeSpecForAuthorization(ctx, test.acmeClient, test.issuer, test.order, *test.authz) + cs, err := partialChallengeSpecForAuthorization(ctx, test.issuer, test.order, *test.authz) if err != nil && !test.expectedError { t.Errorf("expected to not get an error, but got: %v", err) t.Fail() @@ -1274,3 +1252,74 @@ func TestChallengeSpecForAuthorization(t *testing.T) { }) } } + +func Test_ensureKeysForChallenges(t *testing.T) { + basicACMEClient := &acmecl.FakeACME{ + FakeHTTP01ChallengeResponse: func(token string) (string, error) { + switch token { + case "fooToken": + return "fooKeyHTTP01", nil + case "barToken": + return "barKeyHTTP01", nil + } + return "", fmt.Errorf("internal error: unexpected token value %s", token) + }, + FakeDNS01ChallengeRecord: func(token string) (string, error) { + switch token { + case "fooToken": + return "fooKeyDNS01", nil + case "barToken": + return "barKeyDNS01", nil + } + return "", fmt.Errorf("internal error: unexpected token value %s", token) + }, + } + fooChallenge := gen.Challenge("foo", gen.SetChallengeToken("fooToken")) + barChallenge := gen.Challenge("bar", gen.SetChallengeToken("barToken")) + tests := map[string]struct { + acmeClient acmecl.Interface + partialChallenges []*cmacme.Challenge + want []*cmacme.Challenge + wantErr bool + }{ + "happy path with some http-01 challenges": { + acmeClient: basicACMEClient, + partialChallenges: []*cmacme.Challenge{ + gen.ChallengeFrom(fooChallenge, gen.SetChallengeType(cmacme.ACMEChallengeTypeHTTP01)), + gen.ChallengeFrom(barChallenge, gen.SetChallengeType(cmacme.ACMEChallengeTypeHTTP01))}, + want: []*cmacme.Challenge{ + gen.ChallengeFrom(fooChallenge, gen.SetChallengeType(cmacme.ACMEChallengeTypeHTTP01), + gen.SetChallengeKey("fooKeyHTTP01")), + gen.ChallengeFrom(barChallenge, gen.SetChallengeType(cmacme.ACMEChallengeTypeHTTP01), + gen.SetChallengeKey("barKeyHTTP01"))}, + }, + "happy path with some dns-01 challenges": { + acmeClient: basicACMEClient, + partialChallenges: []*cmacme.Challenge{ + gen.ChallengeFrom(fooChallenge, gen.SetChallengeType(cmacme.ACMEChallengeTypeDNS01)), + gen.ChallengeFrom(barChallenge, gen.SetChallengeType(cmacme.ACMEChallengeTypeDNS01))}, + want: []*cmacme.Challenge{ + gen.ChallengeFrom(fooChallenge, gen.SetChallengeType(cmacme.ACMEChallengeTypeDNS01), + gen.SetChallengeKey("fooKeyDNS01")), + gen.ChallengeFrom(barChallenge, gen.SetChallengeType(cmacme.ACMEChallengeTypeDNS01), + gen.SetChallengeKey("barKeyDNS01"))}, + }, + "unhappy path with an unknown challenge type": { + acmeClient: basicACMEClient, + partialChallenges: []*cmacme.Challenge{gen.ChallengeFrom(fooChallenge, gen.SetChallengeType(cmacme.ACMEChallengeType("foo")))}, + wantErr: true, + }, + } + for name, scenario := range tests { + t.Run(name, func(t *testing.T) { + got, err := ensureKeysForChallenges(scenario.acmeClient, scenario.partialChallenges) + if (err != nil) != scenario.wantErr { + t.Errorf("ensureKeysForChallenges() error = %v, wantErr %v", err, scenario.wantErr) + return + } + if !reflect.DeepEqual(got, scenario.want) { + t.Errorf("ensureKeysForChallenges() = %v, want %v", got, scenario.want) + } + }) + } +} diff --git a/test/unit/gen/challenge.go b/test/unit/gen/challenge.go index 408c51655..64a5fc952 100644 --- a/test/unit/gen/challenge.go +++ b/test/unit/gen/challenge.go @@ -60,6 +60,12 @@ func SetChallengeToken(t string) ChallengeModifier { } } +func SetChallengeKey(k string) ChallengeModifier { + return func(ch *cmacme.Challenge) { + ch.Spec.Key = k + } +} + // SetIssuer sets the challenge.spec.issuerRef field func SetChallengeIssuer(o cmmeta.ObjectReference) ChallengeModifier { return func(c *cmacme.Challenge) {