From 6fdbd856f7c98ba35752cead9d6e1d5de209d237 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Mon, 27 Apr 2020 18:39:33 -0700 Subject: [PATCH 01/35] git: Ignore *.code-workspace These are visual studio code's workspace configuration files. --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 7cba0d088..f46d43afd 100644 --- a/.gitignore +++ b/.gitignore @@ -19,3 +19,6 @@ coverage.txt vendor output .idea + +# vscode +*.code-workspace From 40d7c42e335e6a0556a8d762179195fd62b66b5e Mon Sep 17 00:00:00 2001 From: Wesley Graham Date: Fri, 7 Feb 2020 15:28:10 -0800 Subject: [PATCH 02/35] Implement acme RFC 8555, challenge retries --- acme/api/handler.go | 17 +++++++++-------- acme/challenge.go | 28 ++++++++++++++++++---------- acme/challenge_test.go | 3 +++ 3 files changed, 30 insertions(+), 18 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 11cd74f22..6f934a3a1 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -2,14 +2,13 @@ package api import ( "fmt" - "net/http" - "github.com/go-chi/chi" "github.com/pkg/errors" "github.com/smallstep/certificates/acme" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority/provisioner" "github.com/smallstep/cli/jose" + "net/http" ) func link(url, typ string) string { @@ -181,13 +180,15 @@ func (h *Handler) GetChallenge(w http.ResponseWriter, r *http.Request) { ch, err = h.Auth.ValidateChallenge(prov, acc.GetID(), chID, acc.GetKey()) if err != nil { api.WriteError(w, err) - return + } else if ch.Status != acme.StatusValid && ch.Status != acme.StatusInvalid { + w.Header().Add("Retry-After", "60") + api.JSON(w, ch) + } else { + getLink := h.Auth.GetLink + w.Header().Add("Link", link(getLink(acme.AuthzLink, acme.URLSafeProvisionerName(prov), true, ch.GetAuthzID()), "up")) + w.Header().Set("Location", getLink(acme.ChallengeLink, acme.URLSafeProvisionerName(prov), true, ch.GetID())) + api.JSON(w, ch) } - - getLink := h.Auth.GetLink - w.Header().Add("Link", link(getLink(acme.AuthzLink, acme.URLSafeProvisionerName(prov), true, ch.GetAuthzID()), "up")) - w.Header().Set("Location", getLink(acme.ChallengeLink, acme.URLSafeProvisionerName(prov), true, ch.GetID())) - api.JSON(w, ch) } // GetCertificate ACME api for retrieving a Certificate. diff --git a/acme/challenge.go b/acme/challenge.go index 1d1cf50bd..ef98f9ea5 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -338,9 +338,12 @@ func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo valida return nil, err } if keyAuth != expected { - if err = hc.storeError(db, - RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ - "expected %s, but got %s", expected, keyAuth))); err != nil { + rejectedErr := RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ + "expected %s, but got %s", expected, keyAuth)) + upd := hc.clone() + upd.Error = rejectedErr.ToACME() + upd.Error.Subproblems = append(upd.Error.Subproblems, rejectedErr) + if err = upd.save(db, hc); err != nil { return nil, err } return hc, nil @@ -559,9 +562,12 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat txtRecords, err := vo.lookupTxt("_acme-challenge." + domain) if err != nil { - if err = dc.storeError(db, - DNSErr(errors.Wrapf(err, "error looking up TXT "+ - "records for domain %s", domain))); err != nil { + dnsErr := DNSErr(errors.Wrapf(err, "error looking up TXT "+ + "records for domain %s", domain)) + upd := dc.clone() + upd.Error = dnsErr.ToACME() + upd.Error.Subproblems = append(upd.Error.Subproblems, dnsErr) + if err = upd.save(db, dc); err != nil { return nil, err } return dc, nil @@ -581,14 +587,16 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat } } if !found { - if err = dc.storeError(db, - RejectedIdentifierErr(errors.Errorf("keyAuthorization "+ - "does not match; expected %s, but got %s", expectedKeyAuth, txtRecords))); err != nil { + rejectedErr := RejectedIdentifierErr(errors.Errorf("keyAuthorization "+ + "does not match; expected %s, but got %s", expectedKeyAuth, txtRecords)) + upd := dc.clone() + upd.Error = rejectedErr.ToACME() + upd.Error.Subproblems = append(upd.Error.Subproblems, rejectedErr) + if err = upd.save(db, dc); err != nil { return nil, err } return dc, nil } - // Update and store the challenge. upd := &dns01Challenge{dc.baseChallenge.clone()} upd.Status = StatusValid diff --git a/acme/challenge_test.go b/acme/challenge_test.go index b2b249cbe..a65f64256 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -858,6 +858,7 @@ func TestHTTP01Validate(t *testing.T) { "expected %s, but got foo", expKeyAuth)) baseClone := ch.clone() baseClone.Error = expErr.ToACME() + baseClone.Error.Subproblems = append(baseClone.Error.Subproblems, expErr) newCh := &http01Challenge{baseClone} newb, err := json.Marshal(newCh) assert.FatalError(t, err) @@ -1746,6 +1747,7 @@ func TestDNS01Validate(t *testing.T) { "domain %s: force", ch.getValue())) baseClone := ch.clone() baseClone.Error = expErr.ToACME() + baseClone.Error.Subproblems = append(baseClone.Error.Subproblems, expErr) newCh := &dns01Challenge{baseClone} newb, err := json.Marshal(newCh) assert.FatalError(t, err) @@ -1842,6 +1844,7 @@ func TestDNS01Validate(t *testing.T) { "expected %s, but got %s", expKeyAuth, []string{"foo", "bar"})) baseClone := ch.clone() baseClone.Error = expErr.ToACME() + baseClone.Error.Subproblems = append(baseClone.Error.Subproblems, expErr) newCh := &http01Challenge{baseClone} newb, err := json.Marshal(newCh) assert.FatalError(t, err) From 66b2c4b1a46d5203341462988fbec9e3b9f74a17 Mon Sep 17 00:00:00 2001 From: Wesley Graham Date: Fri, 14 Feb 2020 13:17:52 -0800 Subject: [PATCH 03/35] Add automated challenge retries, RFC 8555 --- acme/api/handler.go | 5 ++-- acme/api/handler_test.go | 38 +++++++++++++++++++++++++- acme/authority.go | 37 ++++++++++++++++++------- acme/authority_test.go | 3 +-- acme/challenge.go | 58 ++++++++++++++++++++++++++++++++++++++-- acme/challenge_test.go | 2 +- 6 files changed, 126 insertions(+), 17 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 6f934a3a1..33782a68c 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -180,8 +180,9 @@ func (h *Handler) GetChallenge(w http.ResponseWriter, r *http.Request) { ch, err = h.Auth.ValidateChallenge(prov, acc.GetID(), chID, acc.GetKey()) if err != nil { api.WriteError(w, err) - } else if ch.Status != acme.StatusValid && ch.Status != acme.StatusInvalid { - w.Header().Add("Retry-After", "60") + } else if ch.Retry.Active { + retryAfter := int(ch.Retry.Backoffs) * (10 - ch.Retry.Called) + w.Header().Add("Retry-After", string(retryAfter)) api.JSON(w, ch) } else { getLink := h.Auth.GetLink diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index ebafbbb86..928596110 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -589,6 +589,7 @@ func ch() acme.Challenge { URL: "https://ca.smallstep.com/acme/challenge/chID", ID: "chID", AuthzID: "authzID", + Retry: &acme.Retry{Called:0, Backoffs:1, Active:false}, } } @@ -733,6 +734,37 @@ func TestHandlerGetChallenge(t *testing.T) { ch: ch, } }, + "ok/retry-after": func(t *testing.T) test { + key, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + assert.FatalError(t, err) + acc := &acme.Account{ID: "accID", Key: key} + ctx := context.WithValue(context.Background(), provisionerContextKey, prov) + ctx = context.WithValue(ctx, accContextKey, acc) + // TODO: Add correct key such that challenge object is already "active" + chiCtxInactive := chi.NewRouteContext() + chiCtxInactive.URLParams.Add("chID", "chID") + //chiCtxInactive.URLParams.Add("Active", "true") + ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtxInactive) + ch := ch() + ch.Retry.Active = true + chJSON, err := json.Marshal(ch) + assert.FatalError(t, err) + ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: chJSON}) + return test{ + auth: &mockAcmeAuthority{ + validateChallenge: func(p provisioner.Interface, accID, id string, jwk *jose.JSONWebKey) (*acme.Challenge, error) { + assert.Equals(t, p, prov) + assert.Equals(t, accID, acc.ID) + assert.Equals(t, id, ch.ID) + assert.Equals(t, jwk.KeyID, key.KeyID) + return &ch, nil + }, + }, + ctx: ctx, + statusCode: 200, + ch: ch, + } + }, } for name, run := range tests { tc := run(t) @@ -760,13 +792,17 @@ func TestHandlerGetChallenge(t *testing.T) { assert.Equals(t, ae.Identifier, prob.Identifier) assert.Equals(t, ae.Subproblems, prob.Subproblems) assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"}) - } else { + } else if res.StatusCode >= 200 && assert.True(t,res.Header["Retry-After"] == nil){ expB, err := json.Marshal(tc.ch) assert.FatalError(t, err) assert.Equals(t, bytes.TrimSpace(body), expB) assert.Equals(t, res.Header["Link"], []string{fmt.Sprintf(";rel=\"up\"", tc.ch.AuthzID)}) assert.Equals(t, res.Header["Location"], []string{url}) assert.Equals(t, res.Header["Content-Type"], []string{"application/json"}) + } else { + expB, err := json.Marshal(tc.ch) + assert.FatalError(t, err) + assert.Equals(t, bytes.TrimSpace(body), expB) } }) } diff --git a/acme/authority.go b/acme/authority.go index fe51ea9bb..2ebf8e49a 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -5,6 +5,8 @@ import ( "crypto/tls" "crypto/x509" "encoding/base64" + "math" + "math/rand" "net" "net/http" "net/url" @@ -263,21 +265,38 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin if accID != ch.getAccountID() { return nil, UnauthorizedErr(errors.New("account does not own challenge")) } + retry := ch.getRetry() + if retry.Active { + return ch.toACME(a.db, a.dir, p) + } + retry.Mux.Lock() + defer retry.Mux.Unlock() + client := http.Client{ Timeout: time.Duration(30 * time.Second), } dialer := &net.Dialer{ Timeout: 30 * time.Second, } - ch, err = ch.validate(a.db, jwk, validateOptions{ - httpGet: client.Get, - lookupTxt: net.LookupTXT, - tlsDial: func(network, addr string, config *tls.Config) (*tls.Conn, error) { - return tls.DialWithDialer(dialer, network, addr, config) - }, - }) - if err != nil { - return nil, Wrap(err, "error attempting challenge validation") + for i:=0; i < 10; i++ { + ch, err = ch.validate(a.db, jwk, validateOptions{ + httpGet: client.Get, + lookupTxt: net.LookupTXT, + tlsDial: func(network, addr string, config *tls.Config) (*tls.Conn, error) { + return tls.DialWithDialer(dialer, network, addr, config) + }, + }) + if err != nil { + return nil, Wrap(err, "error attempting challenge validation") + } + if ch.getStatus() == StatusValid { + break + } + if ch.getStatus() == StatusInvalid { + return ch.toACME(a.db, a.dir, p) + } + duration := time.Duration(ch.getRetry().Backoffs + math.Mod(rand.Float64(), 5)) + time.Sleep(duration*time.Second) } return ch.toACME(a.db, a.dir, p) } diff --git a/acme/authority_test.go b/acme/authority_test.go index 525a61b9f..0e1b49681 100644 --- a/acme/authority_test.go +++ b/acme/authority_test.go @@ -1276,6 +1276,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { assert.Fatal(t, ok) _ch.baseChallenge.Status = StatusValid _ch.baseChallenge.Validated = clock.Now() + _ch.baseChallenge.Retry.Called = 0 b, err := json.Marshal(ch) assert.FatalError(t, err) auth, err := NewAuthority(&db.MockNoSQLDB{ @@ -1309,12 +1310,10 @@ func TestAuthorityValidateChallenge(t *testing.T) { if assert.Nil(t, tc.err) { gotb, err := json.Marshal(acmeCh) assert.FatalError(t, err) - acmeExp, err := tc.ch.toACME(nil, tc.auth.dir, prov) assert.FatalError(t, err) expb, err := json.Marshal(acmeExp) assert.FatalError(t, err) - assert.Equals(t, expb, gotb) } } diff --git a/acme/challenge.go b/acme/challenge.go index ef98f9ea5..8f0002771 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -14,6 +14,7 @@ import ( "net" "net/http" "strings" + "sync" "time" "github.com/pkg/errors" @@ -31,10 +32,18 @@ type Challenge struct { Validated string `json:"validated,omitempty"` URL string `json:"url"` Error *AError `json:"error,omitempty"` + Retry *Retry `json:"retry"` ID string `json:"-"` AuthzID string `json:"-"` } +type Retry struct { + Called int `json:"id"` + Backoffs float64 `json:"backoffs"` + Active bool `json:"active"` + Mux sync.Mutex `json:"mux"` +} + // ToLog enables response logging. func (c *Challenge) ToLog() (interface{}, error) { b, err := json.Marshal(c) @@ -75,6 +84,7 @@ type challenge interface { getID() string getAuthzID() string getToken() string + getRetry() *Retry clone() *baseChallenge getAccountID() string getValidated() time.Time @@ -101,6 +111,7 @@ type baseChallenge struct { Validated time.Time `json:"validated"` Created time.Time `json:"created"` Error *AError `json:"error"` + Retry *Retry `json:"retry"` } func newBaseChallenge(accountID, authzID string) (*baseChallenge, error) { @@ -120,6 +131,7 @@ func newBaseChallenge(accountID, authzID string) (*baseChallenge, error) { Status: StatusPending, Token: token, Created: clock.Now(), + Retry: &Retry{Called:0, Backoffs:1, Active:false}, }, nil } @@ -158,6 +170,11 @@ func (bc *baseChallenge) getToken() string { return bc.Token } +// getRetry returns the retry state of the baseChallenge +func (bc *baseChallenge) getRetry() *Retry { + return bc.Retry +} + // getValidated returns the validated time of the baseChallenge. func (bc *baseChallenge) getValidated() time.Time { return bc.Validated @@ -190,6 +207,9 @@ func (bc *baseChallenge) toACME(db nosql.DB, dir *directory, p provisioner.Inter if bc.Error != nil { ac.Error = bc.Error } + if bc.Retry != nil { + ac.Retry = bc.Retry + } return ac, nil } @@ -241,6 +261,11 @@ func (bc *baseChallenge) storeError(db nosql.DB, err *Error) error { return clone.save(db, bc) } +func (bc *baseChallenge) storeRetry(db nosql.DB, retry *Retry) error { + clone := bc.clone() + return clone.save(db, bc) +} + // unmarshalChallenge unmarshals a challenge type into the correct sub-type. func unmarshalChallenge(data []byte) (challenge, error) { var getType struct { @@ -303,7 +328,18 @@ func newHTTP01Challenge(db nosql.DB, ops ChallengeOptions) (challenge, error) { // updated. func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validateOptions) (challenge, error) { // If already valid or invalid then return without performing validation. - if hc.getStatus() == StatusValid || hc.getStatus() == StatusInvalid { + if hc.getStatus() == StatusValid { + return hc, nil + } + if hc.getStatus() == StatusInvalid { + // TODO: Resolve segfault on upd.save + upd := hc.clone() + upd.Status = StatusPending + if err := upd.save(db, hc); err != nil { + fmt.Printf("Error in save: %s\n\n\n", err) + return nil, err + } + fmt.Print("Through Save\n\n") return hc, nil } url := fmt.Sprintf("http://%s/.well-known/acme-challenge/%s", hc.Value, hc.Token) @@ -343,10 +379,18 @@ func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo valida upd := hc.clone() upd.Error = rejectedErr.ToACME() upd.Error.Subproblems = append(upd.Error.Subproblems, rejectedErr) + upd.Retry.Called ++ + upd.Retry.Active = true + if upd.Retry.Called >= 10 { + upd.Status = StatusInvalid + upd.Retry.Backoffs *= 2 + upd.Retry.Active = false + upd.Retry.Called = 0 + } if err = upd.save(db, hc); err != nil { return nil, err } - return hc, nil + return hc, err } // Update and store the challenge. @@ -354,6 +398,7 @@ func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo valida upd.Status = StatusValid upd.Error = nil upd.Validated = clock.Now() + upd.Retry.Active = false if err := upd.save(db, hc); err != nil { return nil, err @@ -567,6 +612,14 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat upd := dc.clone() upd.Error = dnsErr.ToACME() upd.Error.Subproblems = append(upd.Error.Subproblems, dnsErr) + upd.Retry.Called ++ + upd.Retry.Active = true + if upd.Retry.Called >= 10 { + upd.Status = StatusInvalid + upd.Retry.Backoffs *= 2 + upd.Retry.Active = false + upd.Retry.Called = 0 + } if err = upd.save(db, dc); err != nil { return nil, err } @@ -602,6 +655,7 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat upd.Status = StatusValid upd.Error = nil upd.Validated = time.Now().UTC() + upd.Retry.Active = false if err := upd.save(db, dc); err != nil { return nil, err diff --git a/acme/challenge_test.go b/acme/challenge_test.go index a65f64256..ba927c99c 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -772,7 +772,6 @@ func TestHTTP01Validate(t *testing.T) { assert.FatalError(t, err) oldb, err := json.Marshal(ch) assert.FatalError(t, err) - expErr := ConnectionErr(errors.Errorf("error doing http GET for url "+ "http://zap.internal/.well-known/acme-challenge/%s with status code 400", ch.getToken())) baseClone := ch.clone() @@ -984,6 +983,7 @@ func TestHTTP01Validate(t *testing.T) { assert.Equals(t, tc.res.getCreated(), ch.getCreated()) assert.Equals(t, tc.res.getValidated(), ch.getValidated()) assert.Equals(t, tc.res.getError(), ch.getError()) + assert.Equals(t, tc.res.getRetry(), ch.getRetry()) } } }) From f9779d0bed0fef38744f8333351922f60973b698 Mon Sep 17 00:00:00 2001 From: Wesley Graham Date: Tue, 18 Feb 2020 17:48:53 -0800 Subject: [PATCH 04/35] Polish retry conditions --- acme/api/handler.go | 2 +- acme/api/handler_test.go | 6 +++-- acme/authority.go | 2 +- acme/challenge.go | 50 +++++++++++++++++----------------------- api/utils.go | 5 ++++ 5 files changed, 32 insertions(+), 33 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 33782a68c..0aab42f91 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -183,7 +183,7 @@ func (h *Handler) GetChallenge(w http.ResponseWriter, r *http.Request) { } else if ch.Retry.Active { retryAfter := int(ch.Retry.Backoffs) * (10 - ch.Retry.Called) w.Header().Add("Retry-After", string(retryAfter)) - api.JSON(w, ch) + api.WriteProcessing(w, ch) } else { getLink := h.Auth.GetLink w.Header().Add("Link", link(getLink(acme.AuthzLink, acme.URLSafeProvisionerName(prov), true, ch.GetAuthzID()), "up")) diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index 928596110..67c4f3669 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -792,17 +792,19 @@ func TestHandlerGetChallenge(t *testing.T) { assert.Equals(t, ae.Identifier, prob.Identifier) assert.Equals(t, ae.Subproblems, prob.Subproblems) assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"}) - } else if res.StatusCode >= 200 && assert.True(t,res.Header["Retry-After"] == nil){ + } else if res.StatusCode >= 200 { expB, err := json.Marshal(tc.ch) assert.FatalError(t, err) assert.Equals(t, bytes.TrimSpace(body), expB) assert.Equals(t, res.Header["Link"], []string{fmt.Sprintf(";rel=\"up\"", tc.ch.AuthzID)}) assert.Equals(t, res.Header["Location"], []string{url}) assert.Equals(t, res.Header["Content-Type"], []string{"application/json"}) - } else { + } else if res.StatusCode >= 100 { expB, err := json.Marshal(tc.ch) assert.FatalError(t, err) assert.Equals(t, bytes.TrimSpace(body), expB) + assert.True(t, res.Header["Retry-After"] != nil) + assert.Equals(t, res.Header["Content-Type"], []string{"application/json"}) } }) } diff --git a/acme/authority.go b/acme/authority.go index 2ebf8e49a..ff387892b 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -278,7 +278,7 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin dialer := &net.Dialer{ Timeout: 30 * time.Second, } - for i:=0; i < 10; i++ { + for ch.getRetry().Active { ch, err = ch.validate(a.db, jwk, validateOptions{ httpGet: client.Get, lookupTxt: net.LookupTXT, diff --git a/acme/challenge.go b/acme/challenge.go index 8f0002771..00a7b9ccb 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -261,11 +261,6 @@ func (bc *baseChallenge) storeError(db nosql.DB, err *Error) error { return clone.save(db, bc) } -func (bc *baseChallenge) storeRetry(db nosql.DB, retry *Retry) error { - clone := bc.clone() - return clone.save(db, bc) -} - // unmarshalChallenge unmarshals a challenge type into the correct sub-type. func unmarshalChallenge(data []byte) (challenge, error) { var getType struct { @@ -376,18 +371,7 @@ func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo valida if keyAuth != expected { rejectedErr := RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ "expected %s, but got %s", expected, keyAuth)) - upd := hc.clone() - upd.Error = rejectedErr.ToACME() - upd.Error.Subproblems = append(upd.Error.Subproblems, rejectedErr) - upd.Retry.Called ++ - upd.Retry.Active = true - if upd.Retry.Called >= 10 { - upd.Status = StatusInvalid - upd.Retry.Backoffs *= 2 - upd.Retry.Active = false - upd.Retry.Called = 0 - } - if err = upd.save(db, hc); err != nil { + if err = hc.updateRetry(db, rejectedErr); err != nil { return nil, err } return hc, err @@ -609,18 +593,7 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat if err != nil { dnsErr := DNSErr(errors.Wrapf(err, "error looking up TXT "+ "records for domain %s", domain)) - upd := dc.clone() - upd.Error = dnsErr.ToACME() - upd.Error.Subproblems = append(upd.Error.Subproblems, dnsErr) - upd.Retry.Called ++ - upd.Retry.Active = true - if upd.Retry.Called >= 10 { - upd.Status = StatusInvalid - upd.Retry.Backoffs *= 2 - upd.Retry.Active = false - upd.Retry.Called = 0 - } - if err = upd.save(db, dc); err != nil { + if err = dc.updateRetry(db, dnsErr); err != nil { return nil, err } return dc, nil @@ -677,3 +650,22 @@ func getChallenge(db nosql.DB, id string) (challenge, error) { } return ch, nil } + +// updateRetry updates a challenge's retry and error objects upon a failed validation attempt +func (bc *baseChallenge) updateRetry(db nosql.DB, error *Error) error { + upd := bc.clone() + upd.Error = error.ToACME() + upd.Error.Subproblems = append(upd.Error.Subproblems, error) + upd.Retry.Called ++ + upd.Retry.Active = true + if upd.Retry.Called >= 10 { + upd.Status = StatusInvalid + upd.Retry.Backoffs *= 2 + upd.Retry.Active = false + upd.Retry.Called = 0 + } + if err := upd.save(db, bc); err != nil { + return err + } + return nil +} diff --git a/api/utils.go b/api/utils.go index 0d87a0651..154023d06 100644 --- a/api/utils.go +++ b/api/utils.go @@ -52,6 +52,11 @@ func JSON(w http.ResponseWriter, v interface{}) { JSONStatus(w, v, http.StatusOK) } +// JSON writes the passed value into the http.ResponseWriter. +func WriteProcessing(w http.ResponseWriter, v interface{}) { + JSONStatus(w, v, http.StatusProcessing) +} + // JSONStatus writes the given value into the http.ResponseWriter and the // given status is written as the status code of the response. func JSONStatus(w http.ResponseWriter, v interface{}, status int) { From 8d4356733e717d313f6d480e145b41956c2c4213 Mon Sep 17 00:00:00 2001 From: Wesley Graham Date: Thu, 20 Feb 2020 17:45:23 -0800 Subject: [PATCH 05/35] Implement standard backoff strategy --- acme/api/handler.go | 10 +++-- acme/authority.go | 26 +++++++++++-- acme/challenge.go | 92 +++++++++++++++++++++++++++++---------------- 3 files changed, 90 insertions(+), 38 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 0aab42f91..31e19b7be 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -181,9 +181,13 @@ func (h *Handler) GetChallenge(w http.ResponseWriter, r *http.Request) { if err != nil { api.WriteError(w, err) } else if ch.Retry.Active { - retryAfter := int(ch.Retry.Backoffs) * (10 - ch.Retry.Called) - w.Header().Add("Retry-After", string(retryAfter)) - api.WriteProcessing(w, ch) + retryAfter, err := h.Auth.BackoffChallenge(prov, acc.GetID(), chID, acc.GetKey()) + if err != nil { + api.WriteError(w, err) + } else { + w.Header().Add("Retry-After", retryAfter.String()) + api.WriteProcessing(w, ch) + } } else { getLink := h.Auth.GetLink w.Header().Add("Link", link(getLink(acme.AuthzLink, acme.URLSafeProvisionerName(prov), true, ch.GetAuthzID()), "up")) diff --git a/acme/authority.go b/acme/authority.go index ff387892b..b1f94c3ea 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -6,7 +6,6 @@ import ( "crypto/x509" "encoding/base64" "math" - "math/rand" "net" "net/http" "net/url" @@ -21,6 +20,7 @@ import ( // Interface is the acme authority interface. type Interface interface { + BackoffChallenge(provisioner.Interface, string, string, *jose.JSONWebKey) (time.Duration, error) DeactivateAccount(provisioner.Interface, string) (*Account, error) FinalizeOrder(provisioner.Interface, string, string, *x509.CertificateRequest) (*Order, error) GetAccount(provisioner.Interface, string) (*Account, error) @@ -295,8 +295,7 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin if ch.getStatus() == StatusInvalid { return ch.toACME(a.db, a.dir, p) } - duration := time.Duration(ch.getRetry().Backoffs + math.Mod(rand.Float64(), 5)) - time.Sleep(duration*time.Second) + time.Sleep(ch.getBackoff()) } return ch.toACME(a.db, a.dir, p) } @@ -312,3 +311,24 @@ func (a *Authority) GetCertificate(accID, certID string) ([]byte, error) { } return cert.toACME(a.db, a.dir) } + +// BackoffChallenge returns the total time to wait until the next sequence of validation attempts completes +func (a *Authority) BackoffChallenge(p provisioner.Interface, accID, chID string, jwk *jose.JSONWebKey) (time.Duration, error) { + ch, err := getChallenge(a.db, chID) + if err != nil { + return -1, err + } + if accID != ch.getAccountID() { + return -1, UnauthorizedErr(errors.New("account does not own challenge")) + } + + remCalls := ch.getRetry().MaxAttempts - math.Mod(ch.getRetry().Called, ch.getRetry().MaxAttempts) + totBackoff := 0*time.Second + for i:=0; i < int(remCalls); i++ { + clone := ch.clone() + clone.Retry.Called += float64(i) + totBackoff += clone.getBackoff() + } + + return totBackoff, nil +} diff --git a/acme/challenge.go b/acme/challenge.go index 00a7b9ccb..9b47c7a6e 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -11,6 +11,8 @@ import ( "encoding/json" "fmt" "io/ioutil" + "math" + "math/rand" "net" "net/http" "strings" @@ -38,10 +40,14 @@ type Challenge struct { } type Retry struct { - Called int `json:"id"` - Backoffs float64 `json:"backoffs"` - Active bool `json:"active"` - Mux sync.Mutex `json:"mux"` + Called float64 `json:"id"` + BaseDelay time.Duration `json:"basedelay"` + MaxDelay time.Duration `json:"maxdelay"` + Multiplier float64 `json:"multiplier"` + Jitter float64 `json:"jitter"` + MaxAttempts float64 `json:"maxattempts"` + Active bool `json:"active"` + Mux sync.Mutex `json:"mux"` } // ToLog enables response logging. @@ -85,6 +91,7 @@ type challenge interface { getAuthzID() string getToken() string getRetry() *Retry + getBackoff() time.Duration clone() *baseChallenge getAccountID() string getValidated() time.Time @@ -131,7 +138,7 @@ func newBaseChallenge(accountID, authzID string) (*baseChallenge, error) { Status: StatusPending, Token: token, Created: clock.Now(), - Retry: &Retry{Called:0, Backoffs:1, Active:false}, + Retry: &Retry{Called:0, BaseDelay:1, MaxDelay: 30, Jitter: 1, MaxAttempts:10, Active:false}, }, nil } @@ -190,6 +197,25 @@ func (bc *baseChallenge) getError() *AError { return bc.Error } +// getBackoff returns the backoff time of the baseChallenge. +func (bc *baseChallenge) getBackoff() time.Duration { + if bc.Retry.Called == 0 { + return bc.Retry.BaseDelay + } + + backoff, max := float64(bc.Retry.BaseDelay), float64(bc.Retry.MaxDelay) + backoff *= math.Pow(bc.Retry.Multiplier, bc.Retry.Called) + if backoff > max { + backoff = max + } + // Introduce Jitter to ensure that clustered requests wont operate in unison + backoff *= 1 + bc.Retry.Jitter*(rand.Float64()*2-1) + if backoff < 0 { + return 0 + } + return time.Duration(backoff) +} + // toACME converts the internal Challenge type into the public acmeChallenge // type for presentation in the ACME protocol. func (bc *baseChallenge) toACME(db nosql.DB, dir *directory, p provisioner.Interface) (*Challenge, error) { @@ -341,14 +367,14 @@ func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo valida resp, err := vo.httpGet(url) if err != nil { - if err = hc.storeError(db, ConnectionErr(errors.Wrapf(err, + if err = hc.iterateRetry(db, ConnectionErr(errors.Wrapf(err, "error doing http GET for url %s", url))); err != nil { return nil, err } return hc, nil } if resp.StatusCode >= 400 { - if err = hc.storeError(db, + if err = hc.iterateRetry(db, ConnectionErr(errors.Errorf("error doing http GET for url %s with status code %d", url, resp.StatusCode))); err != nil { return nil, err @@ -366,12 +392,16 @@ func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo valida expected, err := KeyAuthorization(hc.Token, jwk) if err != nil { + // TODO retry? return nil, err } if keyAuth != expected { - rejectedErr := RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ - "expected %s, but got %s", expected, keyAuth)) - if err = hc.updateRetry(db, rejectedErr); err != nil { + // TODO should we just bail here, this is a "successful" failure. + if err = hc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ + "expected %s, but got %s", expected, keyAuth))); err != nil { + // TODO what's the difference between returning nil with an error, and returning a retry- + // incremented challenge? I think we should probably only hard error if there are no more + // retries left. return nil, err } return hc, err @@ -383,6 +413,7 @@ func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo valida upd.Error = nil upd.Validated = clock.Now() upd.Retry.Active = false + upd.Retry.Called = 0 if err := upd.save(db, hc); err != nil { return nil, err @@ -426,7 +457,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val conn, err := vo.tlsDial("tcp", hostPort, config) if err != nil { - if err = tc.storeError(db, + if err = tc.iterateRetry(db, ConnectionErr(errors.Wrapf(err, "error doing TLS dial for %s", hostPort))); err != nil { return nil, err } @@ -438,7 +469,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val certs := cs.PeerCertificates if len(certs) == 0 { - if err = tc.storeError(db, + if err = tc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("%s challenge for %s resulted in no certificates", tc.Type, tc.Value))); err != nil { return nil, err @@ -447,9 +478,8 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val } if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != "acme-tls/1" { - if err = tc.storeError(db, - RejectedIdentifierErr(errors.Errorf("cannot negotiate ALPN acme-tls/1 protocol for "+ - "tls-alpn-01 challenge"))); err != nil { + if err = tc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("cannot negotiate ALPN acme-tls/1 protocol for "+ + "tls-alpn-01 challenge"))); err != nil { return nil, err } return tc, nil @@ -458,7 +488,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val leafCert := certs[0] if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], tc.Value) { - if err = tc.storeError(db, + if err = tc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ "leaf certificate must contain a single DNS name, %v", tc.Value))); err != nil { return nil, err @@ -479,8 +509,8 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val for _, ext := range leafCert.Extensions { if idPeAcmeIdentifier.Equal(ext.Id) { if !ext.Critical { - if err = tc.storeError(db, - RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ + if err = tc.iterateRetry(db, + RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + "acmeValidationV1 extension not critical"))); err != nil { return nil, err } @@ -491,7 +521,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val rest, err := asn1.Unmarshal(ext.Value, &extValue) if err != nil || len(rest) > 0 || len(hashedKeyAuth) != len(extValue) { - if err = tc.storeError(db, + if err = tc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ "malformed acmeValidationV1 extension value"))); err != nil { return nil, err @@ -500,7 +530,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val } if subtle.ConstantTimeCompare(hashedKeyAuth[:], extValue) != 1 { - if err = tc.storeError(db, + if err = tc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ "expected acmeValidationV1 extension value %s for this challenge but got %s", hex.EncodeToString(hashedKeyAuth[:]), hex.EncodeToString(extValue)))); err != nil { @@ -513,6 +543,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val upd.Status = StatusValid upd.Error = nil upd.Validated = clock.Now() + upd.Retry.Active = false if err := upd.save(db, tc); err != nil { return nil, err @@ -526,7 +557,8 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val } if foundIDPeAcmeIdentifierV1Obsolete { - if err = tc.storeError(db, + // TODO this isn't likely going to change without user action, this is probably a good case for a hard failure... + if err = tc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ "obsolete id-pe-acmeIdentifier in acmeValidationV1 extension"))); err != nil { return nil, err @@ -534,7 +566,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val return tc, nil } - if err = tc.storeError(db, + if err = tc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ "missing acmeValidationV1 extension"))); err != nil { return nil, err @@ -593,7 +625,7 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat if err != nil { dnsErr := DNSErr(errors.Wrapf(err, "error looking up TXT "+ "records for domain %s", domain)) - if err = dc.updateRetry(db, dnsErr); err != nil { + if err = dc.iterateRetry(db, dnsErr); err != nil { return nil, err } return dc, nil @@ -615,10 +647,7 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat if !found { rejectedErr := RejectedIdentifierErr(errors.Errorf("keyAuthorization "+ "does not match; expected %s, but got %s", expectedKeyAuth, txtRecords)) - upd := dc.clone() - upd.Error = rejectedErr.ToACME() - upd.Error.Subproblems = append(upd.Error.Subproblems, rejectedErr) - if err = upd.save(db, dc); err != nil { + if err = dc.iterateRetry(db, rejectedErr); err != nil { return nil, err } return dc, nil @@ -629,6 +658,7 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat upd.Error = nil upd.Validated = time.Now().UTC() upd.Retry.Active = false + upd.Retry.Called = 0 if err := upd.save(db, dc); err != nil { return nil, err @@ -651,18 +681,16 @@ func getChallenge(db nosql.DB, id string) (challenge, error) { return ch, nil } -// updateRetry updates a challenge's retry and error objects upon a failed validation attempt -func (bc *baseChallenge) updateRetry(db nosql.DB, error *Error) error { +// iterateRetry iterates a challenge's retry and error objects upon a failed validation attempt +func (bc *baseChallenge) iterateRetry(db nosql.DB, error *Error) error { upd := bc.clone() upd.Error = error.ToACME() upd.Error.Subproblems = append(upd.Error.Subproblems, error) upd.Retry.Called ++ upd.Retry.Active = true - if upd.Retry.Called >= 10 { + if math.Mod(upd.Retry.Called , upd.Retry.MaxAttempts) == 0 { upd.Status = StatusInvalid - upd.Retry.Backoffs *= 2 upd.Retry.Active = false - upd.Retry.Called = 0 } if err := upd.save(db, bc); err != nil { return err From 8fb558da100615d3459bb7845911ad1d28302112 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Thu, 30 Apr 2020 04:40:56 -0700 Subject: [PATCH 06/35] handler_test: Remove unused field "Backoffs" --- acme/api/handler_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index 67c4f3669..1054de374 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -589,7 +589,7 @@ func ch() acme.Challenge { URL: "https://ca.smallstep.com/acme/challenge/chID", ID: "chID", AuthzID: "authzID", - Retry: &acme.Retry{Called:0, Backoffs:1, Active:false}, + Retry: &acme.Retry{Called: 0, Active: false}, } } From f56c449ea47a2b45dae605ca6af5fd8d13715ce2 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Thu, 30 Apr 2020 04:41:49 -0700 Subject: [PATCH 07/35] handler_test: Add BackoffChallenge The mock acme authority needs to in order to conform to the updated acme authority interface. --- acme/api/handler_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index 1054de374..00226ad6a 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -41,6 +41,7 @@ type mockAcmeAuthority struct { updateAccount func(provisioner.Interface, string, []string) (*acme.Account, error) useNonce func(string) error validateChallenge func(p provisioner.Interface, accID string, id string, jwk *jose.JSONWebKey) (*acme.Challenge, error) + backoffChallenge func(p provisioner.Interface, accID, chID string, jwk *jose.JSONWebKey) (time.Duration, error) ret1 interface{} err error } @@ -203,6 +204,17 @@ func (m *mockAcmeAuthority) ValidateChallenge(p provisioner.Interface, accID str } } +func (m *mockAcmeAuthority) BackoffChallenge(p provisioner.Interface, accID, chID string, jwk *jose.JSONWebKey) (time.Duration, error) { + switch { + case m.backoffChallenge != nil: + return m.backoffChallenge(p, accID, chID, jwk) + case m.err != nil: + return -1, m.err + default: + return m.ret1.(time.Duration), m.err + } +} + func TestHandlerGetNonce(t *testing.T) { tests := []struct { name string From 5e6a020da53faef00bf12ea8306c259708ab8dd4 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Thu, 30 Apr 2020 04:44:36 -0700 Subject: [PATCH 08/35] acme/authority: Add space around `*` Makes the line more readable. --- acme/authority.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/acme/authority.go b/acme/authority.go index b1f94c3ea..b58dc1473 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -323,8 +323,8 @@ func (a *Authority) BackoffChallenge(p provisioner.Interface, accID, chID string } remCalls := ch.getRetry().MaxAttempts - math.Mod(ch.getRetry().Called, ch.getRetry().MaxAttempts) - totBackoff := 0*time.Second - for i:=0; i < int(remCalls); i++ { + totBackoff := 0 * time.Second + for i := 0; i < int(remCalls); i++ { clone := ch.clone() clone.Retry.Called += float64(i) totBackoff += clone.getBackoff() From 9af4dd369278aa67c3ae4cdd7725e526ef6d6ba5 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 6 May 2020 07:39:13 -0700 Subject: [PATCH 09/35] acme: Retry challenge validation attempts Section 8.2 of RFC 8555 explains how retries apply to the validation process. However, much is left up to the implementer. Add retries every 12 seconds for 2 minutes after a client requests a validation. The challenge status remains "processing" indefinitely until a distinct conclusion is reached. This allows a client to continually re-request a validation by sending a post-get to the challenge resource until the process fails or succeeds. Challenges in the processing state include information about why a validation did not complete in the error field. The server also includes a Retry-After header to help clients and servers coordinate. Retries are inherently stateful because they're part of the public API. When running step-ca in a highly available setup with replicas, care must be taken to maintain a persistent identifier for each instance "slot". In kubernetes, this implies a *stateful set*. --- acme/api/handler.go | 75 ++++++--- acme/authority.go | 231 +++++++++++++++++++++------ acme/authz.go | 2 +- acme/challenge.go | 378 ++++++++++++++++++-------------------------- acme/common.go | 2 + api/utils.go | 5 - 6 files changed, 401 insertions(+), 292 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 31e19b7be..728244d00 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -148,51 +148,90 @@ func (h *Handler) GetAuthz(w http.ResponseWriter, r *http.Request) { api.JSON(w, authz) } -// GetChallenge ACME api for retrieving a Challenge. +// ACME api for retrieving the Challenge resource. +// +// Potential Challenges are requested by the client when creating an order. +// Once the client knows the appropriate validation resources are provisioned, +// it makes a POST-as-GET request to this endpoint in order to initiate the +// validation flow. +// +// The validation state machine describes the flow for a challenge. +// +// https://tools.ietf.org/html/rfc8555#section-7.1.6 +// +// Once a validation attempt has completed without error, the challenge's +// status is updated depending on the result (valid|invalid) of the server's +// validation attempt. Once this is the case, a challenge cannot be reset. +// +// If a challenge cannot be completed because no suitable data can be +// acquired the server (whilst communicating retry information) and the +// client (whilst respecting the information from the server) may request +// retries of the validation. +// +// https://tools.ietf.org/html/rfc8555#section-8.2 +// +// Retry status is communicated using the error field and by sending a +// Retry-After header back to the client. +// +// The request body is challenge-specific. The current challenges (http-01, +// dns-01, tls-alpn-01) simply expect an empty object ("{}") in the payload +// of the JWT sent by the client. We don't gain anything by stricly enforcing +// nonexistence of unknown attributes, or, in these three cases, enforcing +// an empty payload. And the spec also says to just ignore it: +// +// > The server MUST ignore any fields in the response object +// > that are not specified as response fields for this type of challenge. +// +// https://tools.ietf.org/html/rfc8555#section-7.5.1 +// func (h *Handler) GetChallenge(w http.ResponseWriter, r *http.Request) { prov, err := provisionerFromContext(r) if err != nil { api.WriteError(w, err) return } + acc, err := accountFromContext(r) if err != nil { api.WriteError(w, err) return } - // Just verify that the payload was set, since we're not strictly adhering - // to ACME V2 spec for reasons specified below. + + // Just verify that the payload was set since the client is required + // to send _something_. _, err = payloadFromContext(r) if err != nil { api.WriteError(w, err) return } - // NOTE: We should be checking that the request is either a POST-as-GET, or - // that the payload is an empty JSON block ({}). However, older ACME clients - // still send a vestigial body (rather than an empty JSON block) and - // strict enforcement would render these clients broken. For the time being - // we'll just ignore the body. var ( - ch *acme.Challenge + ch *acme.Challenge chID = chi.URLParam(r, "chID") ) ch, err = h.Auth.ValidateChallenge(prov, acc.GetID(), chID, acc.GetKey()) if err != nil { api.WriteError(w, err) - } else if ch.Retry.Active { - retryAfter, err := h.Auth.BackoffChallenge(prov, acc.GetID(), chID, acc.GetKey()) - if err != nil { - api.WriteError(w, err) - } else { - w.Header().Add("Retry-After", retryAfter.String()) - api.WriteProcessing(w, ch) - } - } else { + } + + switch ch.Status { + case acme.StatusPending: + panic("validation attempt did not move challenge to the processing state") + // When a transient error occurs, the challenge will not be progressed to the `invalid` state. + // Add a Retry-After header to indicate that the client should check again in the future. + case acme.StatusProcessing: + w.Header().Add("Retry-After", ch.RetryAfter) + w.Header().Add("Cache-Control", "no-cache") + api.JSON(w, ch) + case acme.StatusInvalid: + api.JSON(w, ch) + case acme.StatusValid: getLink := h.Auth.GetLink w.Header().Add("Link", link(getLink(acme.AuthzLink, acme.URLSafeProvisionerName(prov), true, ch.GetAuthzID()), "up")) w.Header().Set("Location", getLink(acme.ChallengeLink, acme.URLSafeProvisionerName(prov), true, ch.GetID())) api.JSON(w, ch) + default: + panic("unexpected challenge state" + ch.Status) } } diff --git a/acme/authority.go b/acme/authority.go index b58dc1473..63998acfd 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -5,10 +5,12 @@ import ( "crypto/tls" "crypto/x509" "encoding/base64" - "math" + "log" "net" "net/http" "net/url" + "os" + "strconv" "time" "github.com/pkg/errors" @@ -20,7 +22,6 @@ import ( // Interface is the acme authority interface. type Interface interface { - BackoffChallenge(provisioner.Interface, string, string, *jose.JSONWebKey) (time.Duration, error) DeactivateAccount(provisioner.Interface, string) (*Account, error) FinalizeOrder(provisioner.Interface, string, string, *x509.CertificateRequest) (*Order, error) GetAccount(provisioner.Interface, string) (*Account, error) @@ -56,8 +57,24 @@ var ( orderTable = []byte("acme_orders") ordersByAccountIDTable = []byte("acme_account_orders_index") certTable = []byte("acme_certs") + ordinal int ) +// Ordinal is used during challenge retries to indicate ownership. +func init() { + ordstr := os.Getenv("STEP_CA_ORDINAL"); + if ordstr == "" { + ordinal = 0 + } else { + ord, err := strconv.Atoi(ordstr) + if err != nil { + log.Fatal("Unrecognized ordinal ingeter value.") + panic(nil) + } + ordinal = ord + } +} + // NewAuthority returns a new Authority that implements the ACME interface. func NewAuthority(db nosql.DB, dns, prefix string, signAuth SignAuthority) (*Authority, error) { if _, ok := db.(*database.SimpleDB); !ok { @@ -256,50 +273,192 @@ func (a *Authority) GetAuthz(p provisioner.Interface, accID, authzID string) (*A return az.toACME(a.db, a.dir, p) } -// ValidateChallenge attempts to validate the challenge. +// The challenge validation state machine looks like: +// +// * https://tools.ietf.org/html/rfc8555#section-7.1.6 +// +// While in the processing state, the server may retry as it sees fit. The challenge validation strategy +// needs to be rather specific in order for retries to work in a replicated, crash-proof deployment. +// In general, the goal is to allow requests to hit arbitrary instances of step-ca while managing retry +// responsibility such that multiple instances agree on an owner. Additionally, when a deployment of the +// CA is in progress, the ownership should be carried forward and new, updated (or in general, restarted), +// instances should pick back up where the crashed instance left off. +// +// The steps are: +// +// 1. Upon incoming request to the challenge endpoint, take ownership of the retry responsibility. +// (a) Set Retry.Owner to this instance's ordinal (STEP_CA_ORDINAL). +// (b) Set Retry.NumAttempts to 0 and Retry.MaxAttempts to the desired max. +// (c) Set Challenge.Status to "processing" +// (d) Set retry_after to a time (retryInterval) in the future. +// 2. Perform the validation attempt. +// 3. If the validation attempt results in a challenge that is still processing, schedule a retry. +// +// It's possible that another request to re-attempt the challenge comes in while a retry attempt is +// pending from a previous request. In general, these old attempts will see that Retry.NextAttempt +// is in the future and drop their task. But this also might have happened on another instance, etc. +// +// 4. When the retry timer fires, check to make sure the retry should still process. +// (a) Refresh the challenge from the DB. +// (a) Check that Retry.Owner is equal to this instance's ordinal. +// (b) Check that Retry.NextAttempt is in the past. +// 5. If the retry will commence, immediately update Retry.NextAttempt and save the challenge. +// +// Finally, if this instance is terminated, retries need to be reschedule when the instance restarts. This +// is handled in the acme provisioner (authority/provisioner/acme.go) initialization. +// +// Note: the default ordinal does not need to be changed unless step-ca is running in a replicated scenario. +// func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID string, jwk *jose.JSONWebKey) (*Challenge, error) { ch, err := getChallenge(a.db, chID) + + // Validate the challenge belongs to the account owned by the requester. if err != nil { return nil, err } if accID != ch.getAccountID() { return nil, UnauthorizedErr(errors.New("account does not own challenge")) } - retry := ch.getRetry() - if retry.Active { - return ch.toACME(a.db, a.dir, p) + + // Take ownership of the challenge status and retry state. The values must be reset. + up := ch.clone() + up.Status = StatusProcessing + up.Retry = &Retry { + Owner: ordinal, + ProvisionerID: p.GetID(), + NumAttempts: 0, + MaxAttempts: 10, + NextAttempt: time.Now().Add(retryInterval).UTC().Format(time.RFC3339), + + } + err = up.save(a.db, ch) + if err != nil { + return nil, Wrap(err, "error saving challenge") } - retry.Mux.Lock() - defer retry.Mux.Unlock() + ch = up + v, err := a.validate(ch, jwk) + // An error here is non-recoverable. Recoverable errors are set on the challenge object + // and should not be returned directly. + if err != nil { + return nil, Wrap(err, "error attempting challenge validation") + } + err = v.save(a.db, ch) + if err != nil { + return nil, Wrap(err, "error saving challenge") + } + ch = v + + switch ch.getStatus() { + case StatusValid, StatusInvalid: + break + case StatusProcessing: + if ch.getRetry().Active() { + time.AfterFunc(retryInterval, func() { + a.RetryChallenge(ch.getID()) + }) + } + default: + panic("post-validation challenge in unexpected state" + ch.getStatus()) + } + return ch.toACME(a.dir, p) +} + +// The challenge validation process is specific to the type of challenge (dns-01, http-01, tls-alpn-01). +// But, we still pass generic "options" to the polymorphic validate call. +func (a *Authority) validate(ch challenge, jwk *jose.JSONWebKey) (challenge, error) { client := http.Client{ Timeout: time.Duration(30 * time.Second), } dialer := &net.Dialer{ Timeout: 30 * time.Second, } - for ch.getRetry().Active { - ch, err = ch.validate(a.db, jwk, validateOptions{ - httpGet: client.Get, - lookupTxt: net.LookupTXT, - tlsDial: func(network, addr string, config *tls.Config) (*tls.Conn, error) { - return tls.DialWithDialer(dialer, network, addr, config) - }, - }) - if err != nil { - return nil, Wrap(err, "error attempting challenge validation") - } - if ch.getStatus() == StatusValid { - break - } - if ch.getStatus() == StatusInvalid { - return ch.toACME(a.db, a.dir, p) + return ch.validate(jwk, validateOptions{ + httpGet: client.Get, + lookupTxt: net.LookupTXT, + tlsDial: func(network, addr string, config *tls.Config) (*tls.Conn, error) { + return tls.DialWithDialer(dialer, network, addr, config) + }, + }) +} + + +const retryInterval = 12 * time.Second + +// see: ValidateChallenge +func (a *Authority) RetryChallenge(chID string) { + ch, err := getChallenge(a.db, chID) + if err != nil { + return + } + switch ch.getStatus() { + case StatusPending: + panic("pending challenges must first be moved to the processing state") + case StatusInvalid, StatusValid: + return + case StatusProcessing: + break + default: + panic("unknown challenge state: " + ch.getStatus()) + } + + // When retrying, check to make sure the ordinal has not changed. + // Make sure there are still retries left. + // Then check to make sure Retry.NextAttempt is in the past. + retry := ch.getRetry() + switch { + case retry.Owner != ordinal: + return + case !retry.Active(): + return + } + t, err := time.Parse(time.RFC3339, retry.NextAttempt) + now := time.Now().UTC() + switch { + case err != nil: + return + case t.Before(now): + return + } + + // Update the db so that other retries simply drop when their timer fires. + up := ch.clone() + up.Retry.NextAttempt = now.Add(retryInterval).UTC().Format(time.RFC3339) + up.Retry.NumAttempts += 1 + err = up.save(a.db, ch) + if err != nil { + return + } + ch = up + + p, err := a.LoadProvisionerByID(retry.ProvisionerID) + acc, err := a.GetAccount(p, ch.getAccountID()) + + v, err := a.validate(up, acc.Key) + if err != nil { + return + } + err = v.save(a.db, ch) + if err != nil { + return + } + ch = v + + switch ch.getStatus() { + case StatusValid, StatusInvalid: + break + case StatusProcessing: + if ch.getRetry().Active() { + time.AfterFunc(retryInterval, func() { + a.RetryChallenge(ch.getID()) + }) } - time.Sleep(ch.getBackoff()) + default: + panic("post-validation challenge in unexpected state " + ch.getStatus()) } - return ch.toACME(a.db, a.dir, p) } + // GetCertificate retrieves the Certificate by ID. func (a *Authority) GetCertificate(accID, certID string) ([]byte, error) { cert, err := getCert(a.db, certID) @@ -312,23 +471,3 @@ func (a *Authority) GetCertificate(accID, certID string) ([]byte, error) { return cert.toACME(a.db, a.dir) } -// BackoffChallenge returns the total time to wait until the next sequence of validation attempts completes -func (a *Authority) BackoffChallenge(p provisioner.Interface, accID, chID string, jwk *jose.JSONWebKey) (time.Duration, error) { - ch, err := getChallenge(a.db, chID) - if err != nil { - return -1, err - } - if accID != ch.getAccountID() { - return -1, UnauthorizedErr(errors.New("account does not own challenge")) - } - - remCalls := ch.getRetry().MaxAttempts - math.Mod(ch.getRetry().Called, ch.getRetry().MaxAttempts) - totBackoff := 0 * time.Second - for i := 0; i < int(remCalls); i++ { - clone := ch.clone() - clone.Retry.Called += float64(i) - totBackoff += clone.getBackoff() - } - - return totBackoff, nil -} diff --git a/acme/authz.go b/acme/authz.go index cdcb15e5b..789dcab25 100644 --- a/acme/authz.go +++ b/acme/authz.go @@ -148,7 +148,7 @@ func (ba *baseAuthz) toACME(db nosql.DB, dir *directory, p provisioner.Interface if err != nil { return nil, err } - chs[i], err = ch.toACME(db, dir, p) + chs[i], err = ch.toACME(dir, p) if err != nil { return nil, err } diff --git a/acme/challenge.go b/acme/challenge.go index 9b47c7a6e..031dcc7c7 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -11,12 +11,9 @@ import ( "encoding/json" "fmt" "io/ioutil" - "math" - "math/rand" "net" "net/http" "strings" - "sync" "time" "github.com/pkg/errors" @@ -34,22 +31,11 @@ type Challenge struct { Validated string `json:"validated,omitempty"` URL string `json:"url"` Error *AError `json:"error,omitempty"` - Retry *Retry `json:"retry"` + RetryAfter string `json:"retry_after,omitempty"` ID string `json:"-"` AuthzID string `json:"-"` } -type Retry struct { - Called float64 `json:"id"` - BaseDelay time.Duration `json:"basedelay"` - MaxDelay time.Duration `json:"maxdelay"` - Multiplier float64 `json:"multiplier"` - Jitter float64 `json:"jitter"` - MaxAttempts float64 `json:"maxattempts"` - Active bool `json:"active"` - Mux sync.Mutex `json:"mux"` -} - // ToLog enables response logging. func (c *Challenge) ToLog() (interface{}, error) { b, err := json.Marshal(c) @@ -82,7 +68,7 @@ type validateOptions struct { // challenge is the interface ACME challenege types must implement. type challenge interface { save(db nosql.DB, swap challenge) error - validate(nosql.DB, *jose.JSONWebKey, validateOptions) (challenge, error) + validate(*jose.JSONWebKey, validateOptions) (challenge, error) getType() string getError() *AError getValue() string @@ -91,18 +77,18 @@ type challenge interface { getAuthzID() string getToken() string getRetry() *Retry - getBackoff() time.Duration clone() *baseChallenge getAccountID() string getValidated() time.Time getCreated() time.Time - toACME(nosql.DB, *directory, provisioner.Interface) (*Challenge, error) + toACME(*directory, provisioner.Interface) (*Challenge, error) } // ChallengeOptions is the type used to created a new Challenge. type ChallengeOptions struct { AccountID string AuthzID string + ProvisionerID string Identifier Identifier } @@ -115,10 +101,10 @@ type baseChallenge struct { Status string `json:"status"` Token string `json:"token"` Value string `json:"value"` - Validated time.Time `json:"validated"` Created time.Time `json:"created"` + Validated time.Time `json:"validated"` Error *AError `json:"error"` - Retry *Retry `json:"retry"` + Retry *Retry `json:"retry"` } func newBaseChallenge(accountID, authzID string) (*baseChallenge, error) { @@ -138,7 +124,6 @@ func newBaseChallenge(accountID, authzID string) (*baseChallenge, error) { Status: StatusPending, Token: token, Created: clock.Now(), - Retry: &Retry{Called:0, BaseDelay:1, MaxDelay: 30, Jitter: 1, MaxAttempts:10, Active:false}, }, nil } @@ -197,28 +182,9 @@ func (bc *baseChallenge) getError() *AError { return bc.Error } -// getBackoff returns the backoff time of the baseChallenge. -func (bc *baseChallenge) getBackoff() time.Duration { - if bc.Retry.Called == 0 { - return bc.Retry.BaseDelay - } - - backoff, max := float64(bc.Retry.BaseDelay), float64(bc.Retry.MaxDelay) - backoff *= math.Pow(bc.Retry.Multiplier, bc.Retry.Called) - if backoff > max { - backoff = max - } - // Introduce Jitter to ensure that clustered requests wont operate in unison - backoff *= 1 + bc.Retry.Jitter*(rand.Float64()*2-1) - if backoff < 0 { - return 0 - } - return time.Duration(backoff) -} - // toACME converts the internal Challenge type into the public acmeChallenge // type for presentation in the ACME protocol. -func (bc *baseChallenge) toACME(db nosql.DB, dir *directory, p provisioner.Interface) (*Challenge, error) { +func (bc *baseChallenge) toACME(dir *directory, p provisioner.Interface) (*Challenge, error) { ac := &Challenge{ Type: bc.getType(), Status: bc.getStatus(), @@ -233,8 +199,8 @@ func (bc *baseChallenge) toACME(db nosql.DB, dir *directory, p provisioner.Inter if bc.Error != nil { ac.Error = bc.Error } - if bc.Retry != nil { - ac.Retry = bc.Retry + if bc.Retry != nil && bc.Status == StatusProcessing { + ac.RetryAfter = bc.Retry.NextAttempt } return ac, nil } @@ -274,10 +240,12 @@ func (bc *baseChallenge) save(db nosql.DB, old challenge) error { func (bc *baseChallenge) clone() *baseChallenge { u := *bc + r := *bc.Retry + u.Retry = &r return &u } -func (bc *baseChallenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validateOptions) (challenge, error) { +func (bc *baseChallenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (challenge, error) { return nil, ServerInternalErr(errors.New("unimplemented")) } @@ -323,6 +291,21 @@ func unmarshalChallenge(data []byte) (challenge, error) { } } +// Challenge retry information is internally relevant and needs to be stored in the DB, but should not be part +// of the public challenge API apart from the Retry-After header. +type Retry struct { + Owner int `json:"owner"` + ProvisionerID string `json:"provisionerid"` + NumAttempts int `json:"numattempts"` + MaxAttempts int `json:"maxattempts"` + NextAttempt string `json:"nextattempt"` +} + +func (r *Retry) Active() bool { + return r.NumAttempts < r.MaxAttempts +} + + // http01Challenge represents an http-01 acme challenge. type http01Challenge struct { *baseChallenge @@ -347,78 +330,61 @@ func newHTTP01Challenge(db nosql.DB, ops ChallengeOptions) (challenge, error) { // Validate attempts to validate the challenge. If the challenge has been // satisfactorily validated, the 'status' and 'validated' attributes are // updated. -func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validateOptions) (challenge, error) { +func (hc *http01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (challenge, error) { // If already valid or invalid then return without performing validation. - if hc.getStatus() == StatusValid { - return hc, nil - } - if hc.getStatus() == StatusInvalid { - // TODO: Resolve segfault on upd.save - upd := hc.clone() - upd.Status = StatusPending - if err := upd.save(db, hc); err != nil { - fmt.Printf("Error in save: %s\n\n\n", err) - return nil, err - } - fmt.Print("Through Save\n\n") + switch hc.getStatus() { + case StatusPending: + panic("pending challenges must first be moved to the processing state") + case StatusProcessing: + break + case StatusValid, StatusInvalid: return hc, nil + default: + panic("unknown challenge state: " + hc.getStatus()) } - url := fmt.Sprintf("http://%s/.well-known/acme-challenge/%s", hc.Value, hc.Token) + up := &http01Challenge{hc.baseChallenge.clone()} + + url := fmt.Sprintf("http://%s/.well-known/acme-challenge/%s", hc.Value, hc.Token) resp, err := vo.httpGet(url) if err != nil { - if err = hc.iterateRetry(db, ConnectionErr(errors.Wrapf(err, - "error doing http GET for url %s", url))); err != nil { - return nil, err - } - return hc, nil + e := errors.Wrapf(err, "error doing http GET for url %s", url) + up.Error = ConnectionErr(e).ToACME() + return up, nil } + defer resp.Body.Close() + if resp.StatusCode >= 400 { - if err = hc.iterateRetry(db, - ConnectionErr(errors.Errorf("error doing http GET for url %s with status code %d", - url, resp.StatusCode))); err != nil { - return nil, err - } - return hc, nil + e := errors.Errorf("error doing http GET for url %s with status code %d", url, resp.StatusCode) + up.Error = ConnectionErr(e).ToACME() + return up, nil } - defer resp.Body.Close() body, err := ioutil.ReadAll(resp.Body) if err != nil { - return nil, ServerInternalErr(errors.Wrapf(err, "error reading "+ - "response body for url %s", url)) + e := errors.Wrapf(err, "error reading response body for url %s", url) + up.Error = ServerInternalErr(e).ToACME() + return up, nil } - keyAuth := strings.Trim(string(body), "\r\n") + keyAuth := strings.Trim(string(body), "\r\n") expected, err := KeyAuthorization(hc.Token, jwk) if err != nil { - // TODO retry? return nil, err } if keyAuth != expected { - // TODO should we just bail here, this is a "successful" failure. - if err = hc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ - "expected %s, but got %s", expected, keyAuth))); err != nil { - // TODO what's the difference between returning nil with an error, and returning a retry- - // incremented challenge? I think we should probably only hard error if there are no more - // retries left. - return nil, err - } - return hc, err + // add base challenge fail validation + e := errors.Errorf("keyAuthorization does not match; expected %s, but got %s", expected, keyAuth) + up.Error = RejectedIdentifierErr(e).ToACME() + up.Status = StatusInvalid + return up, nil } - // Update and store the challenge. - upd := &http01Challenge{hc.baseChallenge.clone()} - upd.Status = StatusValid - upd.Error = nil - upd.Validated = clock.Now() - upd.Retry.Active = false - upd.Retry.Called = 0 - - if err := upd.save(db, hc); err != nil { - return nil, err - } - return upd, nil + up.Status = StatusValid + up.Validated = clock.Now() + up.Error = nil + up.Retry = nil + return up, nil } type tlsALPN01Challenge struct { @@ -434,34 +400,39 @@ func newTLSALPN01Challenge(db nosql.DB, ops ChallengeOptions) (challenge, error) bc.Type = "tls-alpn-01" bc.Value = ops.Identifier.Value - hc := &tlsALPN01Challenge{bc} - if err := hc.save(db, nil); err != nil { + tc := &tlsALPN01Challenge{bc} + if err := tc.save(db, nil); err != nil { return nil, err } - return hc, nil + return tc, nil } -func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validateOptions) (challenge, error) { +func (tc *tlsALPN01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (challenge, error) { // If already valid or invalid then return without performing validation. - if tc.getStatus() == StatusValid || tc.getStatus() == StatusInvalid { + switch tc.getStatus() { + case StatusPending: + panic("pending challenges must first be moved to the processing state") + case StatusProcessing: + break + case StatusValid, StatusInvalid: return tc, nil + default: + panic("unknown challenge state: " + tc.getStatus()) } + up := &tlsALPN01Challenge{tc.baseChallenge.clone()} + config := &tls.Config{ NextProtos: []string{"acme-tls/1"}, ServerName: tc.Value, InsecureSkipVerify: true, // we expect a self-signed challenge certificate } - hostPort := net.JoinHostPort(tc.Value, "443") - conn, err := vo.tlsDial("tcp", hostPort, config) if err != nil { - if err = tc.iterateRetry(db, - ConnectionErr(errors.Wrapf(err, "error doing TLS dial for %s", hostPort))); err != nil { - return nil, err - } - return tc, nil + e := errors.Wrapf(err, "error doing TLS dial for %s", hostPort) + up.Error = ConnectionErr(e).ToACME() + return up, nil } defer conn.Close() @@ -469,31 +440,22 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val certs := cs.PeerCertificates if len(certs) == 0 { - if err = tc.iterateRetry(db, - RejectedIdentifierErr(errors.Errorf("%s challenge for %s resulted in no certificates", - tc.Type, tc.Value))); err != nil { - return nil, err - } - return tc, nil + e := errors.Errorf("%s challenge for %s resulted in no certificates", tc.Type, tc.Value) + up.Error = RejectedIdentifierErr(e).ToACME() + return up, nil } - if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != "acme-tls/1" { - if err = tc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("cannot negotiate ALPN acme-tls/1 protocol for "+ - "tls-alpn-01 challenge"))); err != nil { - return nil, err - } - return tc, nil + e := errors.Errorf("cannot negotiate ALPN acme-tls/1 protocol for tls-alpn-01 challenge") + up.Error = RejectedIdentifierErr(e).ToACME() + return up, nil } leafCert := certs[0] - if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], tc.Value) { - if err = tc.iterateRetry(db, - RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ - "leaf certificate must contain a single DNS name, %v", tc.Value))); err != nil { - return nil, err - } - return tc, nil + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + + "leaf certificate must contain a single DNS name, %v", tc.Value) + up.Error = RejectedIdentifierErr(e).ToACME() + return up, nil } idPeAcmeIdentifier := asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 7, 1, 31} @@ -509,46 +471,37 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val for _, ext := range leafCert.Extensions { if idPeAcmeIdentifier.Equal(ext.Id) { if !ext.Critical { - if err = tc.iterateRetry(db, - RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + - "acmeValidationV1 extension not critical"))); err != nil { - return nil, err - } - return tc, nil + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + + "acmeValidationV1 extension not critical") + up.Error = IncorrectResponseErr(e).ToACME() + return up, nil } var extValue []byte rest, err := asn1.Unmarshal(ext.Value, &extValue) if err != nil || len(rest) > 0 || len(hashedKeyAuth) != len(extValue) { - if err = tc.iterateRetry(db, - RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ - "malformed acmeValidationV1 extension value"))); err != nil { - return nil, err - } - return tc, nil + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + + "malformed acmeValidationV1 extension value") + up.Error = IncorrectResponseErr(e).ToACME() + return up, nil } if subtle.ConstantTimeCompare(hashedKeyAuth[:], extValue) != 1 { - if err = tc.iterateRetry(db, - RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + "expected acmeValidationV1 extension value %s for this challenge but got %s", - hex.EncodeToString(hashedKeyAuth[:]), hex.EncodeToString(extValue)))); err != nil { - return nil, err - } - return tc, nil + hex.EncodeToString(hashedKeyAuth[:]), hex.EncodeToString(extValue)) + up.Error = IncorrectResponseErr(e).ToACME() + // There is an appropriate value, but it doesn't match. + up.Status = StatusInvalid + return up, nil } - upd := &tlsALPN01Challenge{tc.baseChallenge.clone()} - upd.Status = StatusValid - upd.Error = nil - upd.Validated = clock.Now() - upd.Retry.Active = false - - if err := upd.save(db, tc); err != nil { - return nil, err - } - return upd, nil + up.Validated = clock.Now() + up.Status = StatusValid + up.Error = nil + up.Retry = nil + return up, nil } if idPeAcmeIdentifierV1Obsolete.Equal(ext.Id) { @@ -557,20 +510,15 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val } if foundIDPeAcmeIdentifierV1Obsolete { - // TODO this isn't likely going to change without user action, this is probably a good case for a hard failure... - if err = tc.iterateRetry(db, - RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ - "obsolete id-pe-acmeIdentifier in acmeValidationV1 extension"))); err != nil { - return nil, err - } - return tc, nil + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + + "obsolete id-pe-acmeIdentifier in acmeValidationV1 extension") + up.Error = IncorrectResponseErr(e).ToACME() + return up, nil } - if err = tc.iterateRetry(db, - RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ - "missing acmeValidationV1 extension"))); err != nil { - return nil, err - } + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ + "missing acmeValidationV1 extension") + up.Error = IncorrectResponseErr(e).ToACME() return tc, nil } @@ -595,39 +543,35 @@ func newDNS01Challenge(db nosql.DB, ops ChallengeOptions) (challenge, error) { return dc, nil } -// KeyAuthorization creates the ACME key authorization value from a token -// and a jwk. -func KeyAuthorization(token string, jwk *jose.JSONWebKey) (string, error) { - thumbprint, err := jwk.Thumbprint(crypto.SHA256) - if err != nil { - return "", ServerInternalErr(errors.Wrap(err, "error generating JWK thumbprint")) - } - encPrint := base64.RawURLEncoding.EncodeToString(thumbprint) - return fmt.Sprintf("%s.%s", token, encPrint), nil -} - // validate attempts to validate the challenge. If the challenge has been // satisfactorily validated, the 'status' and 'validated' attributes are // updated. -func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validateOptions) (challenge, error) { +func (dc *dns01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (challenge, error) { // If already valid or invalid then return without performing validation. - if dc.getStatus() == StatusValid || dc.getStatus() == StatusInvalid { + switch dc.getStatus() { + case StatusPending: + panic("pending challenges must first be moved to the processing state") + case StatusProcessing: + break + case StatusValid, StatusInvalid: return dc, nil + default: + panic("unknown challenge state: " + dc.getStatus()) } + up := &dns01Challenge{dc.baseChallenge.clone()} + // Normalize domain for wildcard DNS names // This is done to avoid making TXT lookups for domains like // _acme-challenge.*.example.com // Instead perform txt lookup for _acme-challenge.example.com domain := strings.TrimPrefix(dc.Value, "*.") + record := "_acme-challenge." + domain - txtRecords, err := vo.lookupTxt("_acme-challenge." + domain) + txtRecords, err := vo.lookupTxt(record) if err != nil { - dnsErr := DNSErr(errors.Wrapf(err, "error looking up TXT "+ - "records for domain %s", domain)) - if err = dc.iterateRetry(db, dnsErr); err != nil { - return nil, err - } + e := errors.Wrapf(err, "error looking up TXT records for domain %s", domain) + up.Error = DNSErr(e).ToACME() return dc, nil } @@ -637,33 +581,39 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat } h := sha256.Sum256([]byte(expectedKeyAuth)) expected := base64.RawURLEncoding.EncodeToString(h[:]) - var found bool + + if len(txtRecords) == 0 { + e := errors.Errorf("no TXT record found at '%s'", record) + up.Error = DNSErr(e).ToACME() + return up, nil + } + for _, r := range txtRecords { if r == expected { - found = true - break - } - } - if !found { - rejectedErr := RejectedIdentifierErr(errors.Errorf("keyAuthorization "+ - "does not match; expected %s, but got %s", expectedKeyAuth, txtRecords)) - if err = dc.iterateRetry(db, rejectedErr); err != nil { - return nil, err + up.Validated = time.Now().UTC() + up.Status = StatusValid + up.Error = nil + up.Retry = nil + return up, nil } - return dc, nil } - // Update and store the challenge. - upd := &dns01Challenge{dc.baseChallenge.clone()} - upd.Status = StatusValid - upd.Error = nil - upd.Validated = time.Now().UTC() - upd.Retry.Active = false - upd.Retry.Called = 0 - if err := upd.save(db, dc); err != nil { - return nil, err + up.Status = StatusInvalid + e := errors.Errorf("keyAuthorization does not match; expected %s, but got %s", + expectedKeyAuth, txtRecords) + up.Error = IncorrectResponseErr(e).ToACME() + return up, nil +} + +// KeyAuthorization creates the ACME key authorization value from a token +// and a jwk. +func KeyAuthorization(token string, jwk *jose.JSONWebKey) (string, *Error) { + thumbprint, err := jwk.Thumbprint(crypto.SHA256) + if err != nil { + return "", ServerInternalErr(errors.Wrap(err, "error generating JWK thumbprint")) } - return upd, nil + encPrint := base64.RawURLEncoding.EncodeToString(thumbprint) + return fmt.Sprintf("%s.%s", token, encPrint), nil } // getChallenge retrieves and unmarshals an ACME challenge type from the database. @@ -681,19 +631,3 @@ func getChallenge(db nosql.DB, id string) (challenge, error) { return ch, nil } -// iterateRetry iterates a challenge's retry and error objects upon a failed validation attempt -func (bc *baseChallenge) iterateRetry(db nosql.DB, error *Error) error { - upd := bc.clone() - upd.Error = error.ToACME() - upd.Error.Subproblems = append(upd.Error.Subproblems, error) - upd.Retry.Called ++ - upd.Retry.Active = true - if math.Mod(upd.Retry.Called , upd.Retry.MaxAttempts) == 0 { - upd.Status = StatusInvalid - upd.Retry.Active = false - } - if err := upd.save(db, bc); err != nil { - return err - } - return nil -} diff --git a/acme/common.go b/acme/common.go index ecdd371a0..8b6b2e82e 100644 --- a/acme/common.go +++ b/acme/common.go @@ -29,6 +29,8 @@ var ( StatusInvalid = "invalid" // StatusPending -- pending; e.g. an Order that is not ready to be finalized. StatusPending = "pending" + // processing -- e.g. a Challenge that is in the process of being validated. + StatusProcessing = "processing" // StatusDeactivated -- deactivated; e.g. for an Account that is not longer valid. StatusDeactivated = "deactivated" // StatusReady -- ready; e.g. for an Order that is ready to be finalized. diff --git a/api/utils.go b/api/utils.go index 154023d06..0d87a0651 100644 --- a/api/utils.go +++ b/api/utils.go @@ -52,11 +52,6 @@ func JSON(w http.ResponseWriter, v interface{}) { JSONStatus(w, v, http.StatusOK) } -// JSON writes the passed value into the http.ResponseWriter. -func WriteProcessing(w http.ResponseWriter, v interface{}) { - JSONStatus(w, v, http.StatusProcessing) -} - // JSONStatus writes the given value into the http.ResponseWriter and the // given status is written as the status code of the response. func JSONStatus(w http.ResponseWriter, v interface{}, status int) { From bdadea8a3701e02c2c88762fcd3c34882519bb6d Mon Sep 17 00:00:00 2001 From: David Cowden Date: Thu, 7 May 2020 09:27:16 -0700 Subject: [PATCH 10/35] acme: go fmt --- acme/api/handler.go | 2 +- acme/authority.go | 18 ++++++--------- acme/challenge.go | 56 ++++++++++++++++++++++----------------------- 3 files changed, 35 insertions(+), 41 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 728244d00..2deefaed6 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -206,7 +206,7 @@ func (h *Handler) GetChallenge(w http.ResponseWriter, r *http.Request) { } var ( - ch *acme.Challenge + ch *acme.Challenge chID = chi.URLParam(r, "chID") ) ch, err = h.Auth.ValidateChallenge(prov, acc.GetID(), chID, acc.GetKey()) diff --git a/acme/authority.go b/acme/authority.go index 63998acfd..edd4e1712 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -57,12 +57,12 @@ var ( orderTable = []byte("acme_orders") ordersByAccountIDTable = []byte("acme_account_orders_index") certTable = []byte("acme_certs") - ordinal int + ordinal int ) // Ordinal is used during challenge retries to indicate ownership. func init() { - ordstr := os.Getenv("STEP_CA_ORDINAL"); + ordstr := os.Getenv("STEP_CA_ORDINAL") if ordstr == "" { ordinal = 0 } else { @@ -323,13 +323,12 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin // Take ownership of the challenge status and retry state. The values must be reset. up := ch.clone() up.Status = StatusProcessing - up.Retry = &Retry { - Owner: ordinal, + up.Retry = &Retry{ + Owner: ordinal, ProvisionerID: p.GetID(), - NumAttempts: 0, - MaxAttempts: 10, - NextAttempt: time.Now().Add(retryInterval).UTC().Format(time.RFC3339), - + NumAttempts: 0, + MaxAttempts: 10, + NextAttempt: time.Now().Add(retryInterval).UTC().Format(time.RFC3339), } err = up.save(a.db, ch) if err != nil { @@ -382,7 +381,6 @@ func (a *Authority) validate(ch challenge, jwk *jose.JSONWebKey) (challenge, err }) } - const retryInterval = 12 * time.Second // see: ValidateChallenge @@ -458,7 +456,6 @@ func (a *Authority) RetryChallenge(chID string) { } } - // GetCertificate retrieves the Certificate by ID. func (a *Authority) GetCertificate(accID, certID string) ([]byte, error) { cert, err := getCert(a.db, certID) @@ -470,4 +467,3 @@ func (a *Authority) GetCertificate(accID, certID string) ([]byte, error) { } return cert.toACME(a.db, a.dir) } - diff --git a/acme/challenge.go b/acme/challenge.go index 031dcc7c7..8af683287 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -25,15 +25,15 @@ import ( // Challenge is a subset of the challenge type containing only those attributes // required for responses in the ACME protocol. type Challenge struct { - Type string `json:"type"` - Status string `json:"status"` - Token string `json:"token"` - Validated string `json:"validated,omitempty"` - URL string `json:"url"` - Error *AError `json:"error,omitempty"` - RetryAfter string `json:"retry_after,omitempty"` - ID string `json:"-"` - AuthzID string `json:"-"` + Type string `json:"type"` + Status string `json:"status"` + Token string `json:"token"` + Validated string `json:"validated,omitempty"` + URL string `json:"url"` + Error *AError `json:"error,omitempty"` + RetryAfter string `json:"retry_after,omitempty"` + ID string `json:"-"` + AuthzID string `json:"-"` } // ToLog enables response logging. @@ -86,10 +86,10 @@ type challenge interface { // ChallengeOptions is the type used to created a new Challenge. type ChallengeOptions struct { - AccountID string - AuthzID string + AccountID string + AuthzID string ProvisionerID string - Identifier Identifier + Identifier Identifier } // baseChallenge is the base Challenge type that others build from. @@ -294,18 +294,17 @@ func unmarshalChallenge(data []byte) (challenge, error) { // Challenge retry information is internally relevant and needs to be stored in the DB, but should not be part // of the public challenge API apart from the Retry-After header. type Retry struct { - Owner int `json:"owner"` + Owner int `json:"owner"` ProvisionerID string `json:"provisionerid"` - NumAttempts int `json:"numattempts"` - MaxAttempts int `json:"maxattempts"` - NextAttempt string `json:"nextattempt"` + NumAttempts int `json:"numattempts"` + MaxAttempts int `json:"maxattempts"` + NextAttempt string `json:"nextattempt"` } func (r *Retry) Active() bool { return r.NumAttempts < r.MaxAttempts } - // http01Challenge represents an http-01 acme challenge. type http01Challenge struct { *baseChallenge @@ -452,8 +451,8 @@ func (tc *tlsALPN01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) leafCert := certs[0] if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], tc.Value) { - e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + - "leaf certificate must contain a single DNS name, %v", tc.Value) + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ + "leaf certificate must contain a single DNS name, %v", tc.Value) up.Error = RejectedIdentifierErr(e).ToACME() return up, nil } @@ -472,7 +471,7 @@ func (tc *tlsALPN01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) if idPeAcmeIdentifier.Equal(ext.Id) { if !ext.Critical { e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + - "acmeValidationV1 extension not critical") + "acmeValidationV1 extension not critical") up.Error = IncorrectResponseErr(e).ToACME() return up, nil } @@ -482,15 +481,15 @@ func (tc *tlsALPN01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) if err != nil || len(rest) > 0 || len(hashedKeyAuth) != len(extValue) { e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + - "malformed acmeValidationV1 extension value") + "malformed acmeValidationV1 extension value") up.Error = IncorrectResponseErr(e).ToACME() return up, nil } if subtle.ConstantTimeCompare(hashedKeyAuth[:], extValue) != 1 { - e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + - "expected acmeValidationV1 extension value %s for this challenge but got %s", - hex.EncodeToString(hashedKeyAuth[:]), hex.EncodeToString(extValue)) + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ + "expected acmeValidationV1 extension value %s for this challenge but got %s", + hex.EncodeToString(hashedKeyAuth[:]), hex.EncodeToString(extValue)) up.Error = IncorrectResponseErr(e).ToACME() // There is an appropriate value, but it doesn't match. up.Status = StatusInvalid @@ -511,13 +510,13 @@ func (tc *tlsALPN01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) if foundIDPeAcmeIdentifierV1Obsolete { e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + - "obsolete id-pe-acmeIdentifier in acmeValidationV1 extension") + "obsolete id-pe-acmeIdentifier in acmeValidationV1 extension") up.Error = IncorrectResponseErr(e).ToACME() return up, nil } - e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ - "missing acmeValidationV1 extension") + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + + "missing acmeValidationV1 extension") up.Error = IncorrectResponseErr(e).ToACME() return tc, nil } @@ -600,7 +599,7 @@ func (dc *dns01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (ch up.Status = StatusInvalid e := errors.Errorf("keyAuthorization does not match; expected %s, but got %s", - expectedKeyAuth, txtRecords) + expectedKeyAuth, txtRecords) up.Error = IncorrectResponseErr(e).ToACME() return up, nil } @@ -630,4 +629,3 @@ func getChallenge(db nosql.DB, id string) (challenge, error) { } return ch, nil } - From 9518ba44b12a37ba63a8f45beb223ea08a46b070 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Mon, 11 May 2020 18:46:15 -0700 Subject: [PATCH 11/35] provisioner/acme: Add TODO for retry restarts The comment in acme/authority directs users to this file so put a TODO in for posterity. --- authority/provisioner/acme.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/authority/provisioner/acme.go b/authority/provisioner/acme.go index e414410b6..6b75aa071 100644 --- a/authority/provisioner/acme.go +++ b/authority/provisioner/acme.go @@ -57,6 +57,8 @@ func (p *ACME) Init(config Config) (err error) { return err } + // TODO: https://github.com/smallstep/certificates/issues/250 + return err } From 8326632f5b785a088d380bfb19700fbfff68f468 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Mon, 11 May 2020 18:47:07 -0700 Subject: [PATCH 12/35] vscode: Ignore vscode binaries It might make sense to check in the vscode workspace file if we can make everything relative to the project directory. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index f46d43afd..958578f8d 100644 --- a/.gitignore +++ b/.gitignore @@ -22,3 +22,4 @@ output # vscode *.code-workspace +*_bin From 2d0a00c4e11e44cf91cf41974f0e3f3a4ec655b5 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Mon, 11 May 2020 21:22:40 -0700 Subject: [PATCH 13/35] acme/api: Add missing return Stop execution when the error happens. This was previously a typo. --- acme/api/handler.go | 1 + 1 file changed, 1 insertion(+) diff --git a/acme/api/handler.go b/acme/api/handler.go index 2deefaed6..1acbcb697 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -212,6 +212,7 @@ func (h *Handler) GetChallenge(w http.ResponseWriter, r *http.Request) { ch, err = h.Auth.ValidateChallenge(prov, acc.GetID(), chID, acc.GetKey()) if err != nil { api.WriteError(w, err) + return } switch ch.Status { From a857c45847f986c17c9c4b7f30b7eec721b5cb45 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Mon, 11 May 2020 21:23:55 -0700 Subject: [PATCH 14/35] acme/authority: Polymorph the challenge type Prior to validation, we must wrap the base challenge in the correct concrete challenge type so that we dispatch the correct validation method. --- acme/authority.go | 2 +- acme/challenge.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/acme/authority.go b/acme/authority.go index edd4e1712..b1d13c5b4 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -372,7 +372,7 @@ func (a *Authority) validate(ch challenge, jwk *jose.JSONWebKey) (challenge, err dialer := &net.Dialer{ Timeout: 30 * time.Second, } - return ch.validate(jwk, validateOptions{ + return ch.clone().morph().validate(jwk, validateOptions{ httpGet: client.Get, lookupTxt: net.LookupTXT, tlsDial: func(network, addr string, config *tls.Config) (*tls.Conn, error) { diff --git a/acme/challenge.go b/acme/challenge.go index 8af683287..e2e9b16dd 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -291,6 +291,19 @@ func unmarshalChallenge(data []byte) (challenge, error) { } } +func (bc *baseChallenge) morph() challenge { + switch bc.getType() { + case "dns-01": + return &dns01Challenge{bc} + case "http-01": + return &http01Challenge{bc} + case "tls-alpn-01": + return &tlsALPN01Challenge{bc} + default: + panic("unrecognized challenge type: " + bc.getType()) + } +} + // Challenge retry information is internally relevant and needs to be stored in the DB, but should not be part // of the public challenge API apart from the Retry-After header. type Retry struct { From 9f1888297340552211a7dc68ca4f6bbbd7af4325 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Mon, 11 May 2020 21:25:31 -0700 Subject: [PATCH 15/35] acme/challenge: Copy retry information on clone When cloning a challenge, deeply clone the retry field if it is not nil. --- acme/challenge.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index e2e9b16dd..bf3986784 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -240,8 +240,10 @@ func (bc *baseChallenge) save(db nosql.DB, old challenge) error { func (bc *baseChallenge) clone() *baseChallenge { u := *bc - r := *bc.Retry - u.Retry = &r + if bc.Retry != nil { + r := *bc.Retry + u.Retry = &r + } return &u } From 089e3aea4fad4400676ea25adf1b57ee0cee857b Mon Sep 17 00:00:00 2001 From: David Cowden Date: Mon, 11 May 2020 21:26:21 -0700 Subject: [PATCH 16/35] acme/challenge: Fix error return type on KeyAuthorization In golang, one should always return error types rather than interfaces that conform to an error protocol. Why? Because of this: https://play.golang.org/p/MVa5vowuNRo Feels ~~like JavaScript~~ bad, man. --- acme/challenge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/challenge.go b/acme/challenge.go index bf3986784..fe3acd041 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -621,7 +621,7 @@ func (dc *dns01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (ch // KeyAuthorization creates the ACME key authorization value from a token // and a jwk. -func KeyAuthorization(token string, jwk *jose.JSONWebKey) (string, *Error) { +func KeyAuthorization(token string, jwk *jose.JSONWebKey) (string, error) { thumbprint, err := jwk.Thumbprint(crypto.SHA256) if err != nil { return "", ServerInternalErr(errors.Wrap(err, "error generating JWK thumbprint")) From 2514b58f584fbfe45da95b8d80058feeb3d88d38 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Tue, 12 May 2020 04:52:44 -0700 Subject: [PATCH 17/35] acme/api: Fixup handler_test Remove superfluous test. Add test checking for the Retry-After header if the challenge's RetryAfter field is set. --- acme/api/handler_test.go | 56 +++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index 00226ad6a..d1e6a7194 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -601,7 +601,6 @@ func ch() acme.Challenge { URL: "https://ca.smallstep.com/acme/challenge/chID", ID: "chID", AuthzID: "authzID", - Retry: &acme.Retry{Called: 0, Active: false}, } } @@ -618,6 +617,7 @@ func TestHandlerGetChallenge(t *testing.T) { ch acme.Challenge problem *acme.Error } + var tests = map[string]func(t *testing.T) test{ "fail/no-provisioner": func(t *testing.T) test { return test{ @@ -626,6 +626,7 @@ func TestHandlerGetChallenge(t *testing.T) { problem: acme.ServerInternalErr(errors.New("provisioner expected in request context")), } }, + "fail/nil-provisioner": func(t *testing.T) test { return test{ ctx: context.WithValue(context.Background(), provisionerContextKey, nil), @@ -633,6 +634,7 @@ func TestHandlerGetChallenge(t *testing.T) { problem: acme.ServerInternalErr(errors.New("provisioner expected in request context")), } }, + "fail/no-account": func(t *testing.T) test { return test{ ctx: context.WithValue(context.Background(), provisionerContextKey, prov), @@ -640,6 +642,7 @@ func TestHandlerGetChallenge(t *testing.T) { problem: acme.AccountDoesNotExistErr(nil), } }, + "fail/nil-account": func(t *testing.T) test { ctx := context.WithValue(context.Background(), provisionerContextKey, prov) ctx = context.WithValue(ctx, accContextKey, nil) @@ -649,6 +652,7 @@ func TestHandlerGetChallenge(t *testing.T) { problem: acme.AccountDoesNotExistErr(nil), } }, + "fail/no-payload": func(t *testing.T) test { acc := &acme.Account{ID: "accID"} ctx := context.WithValue(context.Background(), provisionerContextKey, prov) @@ -659,6 +663,7 @@ func TestHandlerGetChallenge(t *testing.T) { problem: acme.ServerInternalErr(errors.New("payload expected in request context")), } }, + "fail/nil-payload": func(t *testing.T) test { acc := &acme.Account{ID: "accID"} ctx := context.WithValue(context.Background(), provisionerContextKey, prov) @@ -670,6 +675,7 @@ func TestHandlerGetChallenge(t *testing.T) { problem: acme.ServerInternalErr(errors.New("payload expected in request context")), } }, + "fail/validate-challenge-error": func(t *testing.T) test { acc := &acme.Account{ID: "accID"} ctx := context.WithValue(context.Background(), provisionerContextKey, prov) @@ -678,28 +684,14 @@ func TestHandlerGetChallenge(t *testing.T) { ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx) return test{ auth: &mockAcmeAuthority{ - err: acme.UnauthorizedErr(nil), + err: acme.ServerInternalErr(nil), }, ctx: ctx, - statusCode: 401, - problem: acme.UnauthorizedErr(nil), - } - }, - "fail/get-challenge-error": func(t *testing.T) test { - acc := &acme.Account{ID: "accID"} - ctx := context.WithValue(context.Background(), provisionerContextKey, prov) - ctx = context.WithValue(ctx, accContextKey, acc) - ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{isPostAsGet: true}) - ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx) - return test{ - auth: &mockAcmeAuthority{ - err: acme.UnauthorizedErr(nil), - }, - ctx: ctx, - statusCode: 401, - problem: acme.UnauthorizedErr(nil), + statusCode: 500, + problem: acme.ServerInternalErr(nil), } }, + "ok/validate-challenge": func(t *testing.T) test { key, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) @@ -746,19 +738,19 @@ func TestHandlerGetChallenge(t *testing.T) { ch: ch, } }, + "ok/retry-after": func(t *testing.T) test { key, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) acc := &acme.Account{ID: "accID", Key: key} ctx := context.WithValue(context.Background(), provisionerContextKey, prov) ctx = context.WithValue(ctx, accContextKey, acc) - // TODO: Add correct key such that challenge object is already "active" chiCtxInactive := chi.NewRouteContext() chiCtxInactive.URLParams.Add("chID", "chID") - //chiCtxInactive.URLParams.Add("Active", "true") ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtxInactive) ch := ch() - ch.Retry.Active = true + ch.Status = "processing" + ch.RetryAfter = time.Now().Add(1 * time.Minute).UTC().Format(time.RFC3339) chJSON, err := json.Marshal(ch) assert.FatalError(t, err) ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: chJSON}) @@ -778,6 +770,8 @@ func TestHandlerGetChallenge(t *testing.T) { } }, } + + // Run the tests for name, run := range tests { tc := run(t) t.Run(name, func(t *testing.T) { @@ -808,15 +802,17 @@ func TestHandlerGetChallenge(t *testing.T) { expB, err := json.Marshal(tc.ch) assert.FatalError(t, err) assert.Equals(t, bytes.TrimSpace(body), expB) - assert.Equals(t, res.Header["Link"], []string{fmt.Sprintf(";rel=\"up\"", tc.ch.AuthzID)}) - assert.Equals(t, res.Header["Location"], []string{url}) - assert.Equals(t, res.Header["Content-Type"], []string{"application/json"}) - } else if res.StatusCode >= 100 { - expB, err := json.Marshal(tc.ch) - assert.FatalError(t, err) - assert.Equals(t, bytes.TrimSpace(body), expB) - assert.True(t, res.Header["Retry-After"] != nil) assert.Equals(t, res.Header["Content-Type"], []string{"application/json"}) + switch tc.ch.Status { + case "processing": + assert.Equals(t, res.Header["Cache-Control"], []string{"no-cache"}) + assert.Equals(t, res.Header["Retry-After"], []string{tc.ch.RetryAfter}) + case "valid": + assert.Equals(t, res.Header["Location"], []string{url}) + assert.Equals(t, res.Header["Link"], []string{fmt.Sprintf(";rel=\"up\"", tc.ch.AuthzID)}) + } + } else { + assert.Fatal(t, false, "Unexpected Status Code") } }) } From 84af2ad5622a7ddd6824500251d1fd5e3de4789e Mon Sep 17 00:00:00 2001 From: David Cowden Date: Tue, 12 May 2020 08:33:32 -0700 Subject: [PATCH 18/35] acme: Fix test compile * Add toACME test for the "processing" state. --- acme/authority_test.go | 4 +- acme/authz_test.go | 4 +- acme/challenge.go | 2 +- acme/challenge_test.go | 110 ++++++++++++++++++++++++++++++++--------- 4 files changed, 93 insertions(+), 27 deletions(-) diff --git a/acme/authority_test.go b/acme/authority_test.go index 0e1b49681..21f80a06b 100644 --- a/acme/authority_test.go +++ b/acme/authority_test.go @@ -1276,7 +1276,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { assert.Fatal(t, ok) _ch.baseChallenge.Status = StatusValid _ch.baseChallenge.Validated = clock.Now() - _ch.baseChallenge.Retry.Called = 0 + _ch.baseChallenge.Retry = nil b, err := json.Marshal(ch) assert.FatalError(t, err) auth, err := NewAuthority(&db.MockNoSQLDB{ @@ -1310,7 +1310,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { if assert.Nil(t, tc.err) { gotb, err := json.Marshal(acmeCh) assert.FatalError(t, err) - acmeExp, err := tc.ch.toACME(nil, tc.auth.dir, prov) + acmeExp, err := tc.ch.toACME(tc.auth.dir, prov) assert.FatalError(t, err) expb, err := json.Marshal(acmeExp) assert.FatalError(t, err) diff --git a/acme/authz_test.go b/acme/authz_test.go index 05e3c40b5..1cbb939e5 100644 --- a/acme/authz_test.go +++ b/acme/authz_test.go @@ -434,9 +434,9 @@ func TestAuthzToACME(t *testing.T) { assert.Equals(t, acmeAz.Identifier, iden) assert.Equals(t, acmeAz.Status, StatusPending) - acmeCh1, err := ch1.toACME(nil, dir, prov) + acmeCh1, err := ch1.toACME(dir, prov) assert.FatalError(t, err) - acmeCh2, err := ch2.toACME(nil, dir, prov) + acmeCh2, err := ch2.toACME(dir, prov) assert.FatalError(t, err) assert.Equals(t, acmeAz.Challenges[0], acmeCh1) diff --git a/acme/challenge.go b/acme/challenge.go index fe3acd041..925723280 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -306,7 +306,7 @@ func (bc *baseChallenge) morph() challenge { } } -// Challenge retry information is internally relevant and needs to be stored in the DB, but should not be part +// Retry information for challenges is internally relevant and needs to be stored in the DB, but should not be part // of the public challenge API apart from the Retry-After header. type Retry struct { Owner int `json:"owner"` diff --git a/acme/challenge_test.go b/acme/challenge_test.go index ba927c99c..1eaa4ced6 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -259,28 +259,36 @@ func TestNewDNS01Challenge(t *testing.T) { } } -func TestChallengeToACME(t *testing.T) { +func TestChallengeToACME_Valid(t *testing.T) { dir := newDirectory("ca.smallstep.com", "acme") - httpCh, err := newHTTPCh() - assert.FatalError(t, err) - _httpCh, ok := httpCh.(*http01Challenge) - assert.Fatal(t, ok) - _httpCh.baseChallenge.Validated = clock.Now() - dnsCh, err := newDNSCh() - assert.FatalError(t, err) - tlsALPNCh, err := newTLSALPNCh() - assert.FatalError(t, err) + n := clock.Now() + + fns := []func() (challenge, error){ + newDNSCh, + newHTTPCh, + newTLSALPNCh, + } + chs := make([]challenge, 3) + + for i, f := range fns { + ch, err := f() + assert.FatalError(t, err) + b := ch.clone() + b.Validated = n + chs[i] = b.morph() + } prov := newProv() tests := map[string]challenge{ - "dns": dnsCh, - "http": httpCh, - "tls-alpn": tlsALPNCh, + "dns": chs[0], + "http": chs[1], + "tls-alpn": chs[2], } + for name, ch := range tests { t.Run(name, func(t *testing.T) { - ach, err := ch.toACME(nil, dir, prov) + ach, err := ch.toACME(dir, prov) assert.FatalError(t, err) assert.Equals(t, ach.Type, ch.getType()) @@ -292,12 +300,70 @@ func TestChallengeToACME(t *testing.T) { assert.Equals(t, ach.ID, ch.getID()) assert.Equals(t, ach.AuthzID, ch.getAuthzID()) - if ach.Type == "http-01" { - v, err := time.Parse(time.RFC3339, ach.Validated) - assert.FatalError(t, err) - assert.Equals(t, v.String(), _httpCh.baseChallenge.Validated.String()) + v, err := time.Parse(time.RFC3339, ach.Validated) + assert.FatalError(t, err) + assert.Equals(t, v, ch.getValidated()) + + assert.Equals(t, ach.RetryAfter, "") + }) + } +} + +func TestChallengeToACME_Retry(t *testing.T) { + dir := newDirectory("example.com", "acme") + + n := clock.Now() + + fns := []func() (challenge, error){ + newDNSCh, + newHTTPCh, + newTLSALPNCh, + } + states := []*Retry{ + nil, + {NextAttempt: n.Format(time.RFC3339)}, + } + chs := make([]challenge, len(fns)*len(states)) + + for i, s := range states { + for j, f := range fns { + ch, err := f() + assert.FatalError(t, err) + b := ch.clone() + b.Status = "processing" + b.Retry = s + chs[j+i*len(fns)] = b.morph() + } + } + + prov := newProv() + tests := map[string]challenge{ + "dns_no-retry": chs[0+0*len(fns)], + "http_no-retry": chs[1+0*len(fns)], + "tls-alpn_no-retry": chs[2+0*len(fns)], + "dns_retry": chs[0+1*len(fns)], + "http_retry": chs[1+1*len(fns)], + "tls_alpn_retry": chs[2+1*len(fns)], + } + for name, ch := range tests { + t.Run(name, func(t *testing.T) { + ach, err := ch.toACME(dir, prov) + assert.FatalError(t, err) + + assert.Equals(t, ach.Type, ch.getType()) + assert.Equals(t, ach.Status, ch.getStatus()) + assert.Equals(t, ach.Token, ch.getToken()) + assert.Equals(t, ach.URL, + fmt.Sprintf("https://example.com/acme/%s/challenge/%s", + URLSafeProvisionerName(prov), ch.getID())) + assert.Equals(t, ach.ID, ch.getID()) + assert.Equals(t, ach.AuthzID, ch.getAuthzID()) + + assert.Equals(t, ach.Validated, "") + if ch.getRetry() != nil { + assert.Equals(t, ach.RetryAfter, ch.getRetry().NextAttempt) } else { - assert.Equals(t, ach.Validated, "") + assert.Equals(t, ach.RetryAfter, "") } }) } @@ -965,7 +1031,7 @@ func TestHTTP01Validate(t *testing.T) { for name, run := range tests { t.Run(name, func(t *testing.T) { tc := run(t) - if ch, err := tc.ch.validate(tc.db, tc.jwk, tc.vo); err != nil { + if ch, err := tc.ch.validate(tc.jwk, tc.vo); err != nil { if assert.NotNil(t, tc.err) { ae, ok := err.(*Error) assert.True(t, ok) @@ -1589,7 +1655,7 @@ func TestTLSALPN01Validate(t *testing.T) { defer tc.srv.Close() } - if ch, err := tc.ch.validate(tc.db, tc.jwk, tc.vo); err != nil { + if ch, err := tc.ch.validate(tc.jwk, tc.vo); err != nil { if assert.NotNil(t, tc.err) { ae, ok := err.(*Error) assert.True(t, ok) @@ -1950,7 +2016,7 @@ func TestDNS01Validate(t *testing.T) { for name, run := range tests { t.Run(name, func(t *testing.T) { tc := run(t) - if ch, err := tc.ch.validate(tc.db, tc.jwk, tc.vo); err != nil { + if ch, err := tc.ch.validate(tc.jwk, tc.vo); err != nil { if assert.NotNil(t, tc.err) { ae, ok := err.(*Error) assert.True(t, ok) From 8556d45c3ffe68529ceaa403ff1eed03e30a5235 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 04:03:01 -0700 Subject: [PATCH 19/35] acme/authority: Move comment onto correct block The comment appeared too early. --- acme/authority.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/acme/authority.go b/acme/authority.go index b1d13c5b4..61b6b08c0 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -311,11 +311,11 @@ func (a *Authority) GetAuthz(p provisioner.Interface, accID, authzID string) (*A // func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID string, jwk *jose.JSONWebKey) (*Challenge, error) { ch, err := getChallenge(a.db, chID) - - // Validate the challenge belongs to the account owned by the requester. if err != nil { return nil, err } + + // Validate the challenge belongs to the account owned by the requester. if accID != ch.getAccountID() { return nil, UnauthorizedErr(errors.New("account does not own challenge")) } From 794725bcc3db235c4e1dd5a25a07580fe21651dd Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 04:03:47 -0700 Subject: [PATCH 20/35] acme/api: Remove unused BackoffChallenge func The mock has an old func that is no longer used. Remove it. --- acme/api/handler_test.go | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index d1e6a7194..9b8d04910 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -41,7 +41,6 @@ type mockAcmeAuthority struct { updateAccount func(provisioner.Interface, string, []string) (*acme.Account, error) useNonce func(string) error validateChallenge func(p provisioner.Interface, accID string, id string, jwk *jose.JSONWebKey) (*acme.Challenge, error) - backoffChallenge func(p provisioner.Interface, accID, chID string, jwk *jose.JSONWebKey) (time.Duration, error) ret1 interface{} err error } @@ -204,17 +203,6 @@ func (m *mockAcmeAuthority) ValidateChallenge(p provisioner.Interface, accID str } } -func (m *mockAcmeAuthority) BackoffChallenge(p provisioner.Interface, accID, chID string, jwk *jose.JSONWebKey) (time.Duration, error) { - switch { - case m.backoffChallenge != nil: - return m.backoffChallenge(p, accID, chID, jwk) - case m.err != nil: - return -1, m.err - default: - return m.ret1.(time.Duration), m.err - } -} - func TestHandlerGetNonce(t *testing.T) { tests := []struct { name string @@ -807,7 +795,7 @@ func TestHandlerGetChallenge(t *testing.T) { case "processing": assert.Equals(t, res.Header["Cache-Control"], []string{"no-cache"}) assert.Equals(t, res.Header["Retry-After"], []string{tc.ch.RetryAfter}) - case "valid": + case "valid", "invalid": assert.Equals(t, res.Header["Location"], []string{url}) assert.Equals(t, res.Header["Link"], []string{fmt.Sprintf(";rel=\"up\"", tc.ch.AuthzID)}) } From 8ae32f50f28d7d5d8f1968349ee1e28c644ee94f Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 05:04:49 -0700 Subject: [PATCH 21/35] acme: Fix comment style to appease linter The linter likes comments on public functions to start with their name, for some reason... --- acme/authority.go | 10 ++++++++-- acme/challenge.go | 1 + acme/common.go | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/acme/authority.go b/acme/authority.go index 61b6b08c0..cc1a6daad 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -273,6 +273,8 @@ func (a *Authority) GetAuthz(p provisioner.Interface, accID, authzID string) (*A return az.toACME(a.db, a.dir, p) } +// ValidateChallenge ... +// // The challenge validation state machine looks like: // // * https://tools.ietf.org/html/rfc8555#section-7.1.6 @@ -296,7 +298,8 @@ func (a *Authority) GetAuthz(p provisioner.Interface, accID, authzID string) (*A // // It's possible that another request to re-attempt the challenge comes in while a retry attempt is // pending from a previous request. In general, these old attempts will see that Retry.NextAttempt -// is in the future and drop their task. But this also might have happened on another instance, etc. +// is in the future and drop their task. Because another instance may have taken ownership, old attempts +// would also see a different ordinal than their own. // // 4. When the retry timer fires, check to make sure the retry should still process. // (a) Refresh the challenge from the DB. @@ -383,6 +386,9 @@ func (a *Authority) validate(ch challenge, jwk *jose.JSONWebKey) (challenge, err const retryInterval = 12 * time.Second +// RetryChallenge behaves similar to ValidateChallenge, but simply attempts to perform a validation and +// write update the challenge record in the db if the challenge has remaining retry attempts. +// // see: ValidateChallenge func (a *Authority) RetryChallenge(chID string) { ch, err := getChallenge(a.db, chID) @@ -422,7 +428,7 @@ func (a *Authority) RetryChallenge(chID string) { // Update the db so that other retries simply drop when their timer fires. up := ch.clone() up.Retry.NextAttempt = now.Add(retryInterval).UTC().Format(time.RFC3339) - up.Retry.NumAttempts += 1 + up.Retry.NumAttempts++ err = up.save(a.db, ch) if err != nil { return diff --git a/acme/challenge.go b/acme/challenge.go index 925723280..b87f4ee22 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -316,6 +316,7 @@ type Retry struct { NextAttempt string `json:"nextattempt"` } +// Active returns a boolean indicating whether a Retry struct has remaining attempts or not. func (r *Retry) Active() bool { return r.NumAttempts < r.MaxAttempts } diff --git a/acme/common.go b/acme/common.go index 8b6b2e82e..936574e3c 100644 --- a/acme/common.go +++ b/acme/common.go @@ -29,7 +29,7 @@ var ( StatusInvalid = "invalid" // StatusPending -- pending; e.g. an Order that is not ready to be finalized. StatusPending = "pending" - // processing -- e.g. a Challenge that is in the process of being validated. + // StatusProcessing -- processing e.g. a Challenge that is in the process of being validated. StatusProcessing = "processing" // StatusDeactivated -- deactivated; e.g. for an Account that is not longer valid. StatusDeactivated = "deactivated" From 609e1312da7a052eecd94c609ce36d5888b1f08c Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 07:29:12 -0700 Subject: [PATCH 22/35] acme/api: Write headers for invalid challenges Include the "Link" and "Location" headers on invalid challenge resources. An invalid challenge is still a perfectly acceptable response. --- acme/api/handler.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 1acbcb697..7a804918e 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -224,9 +224,7 @@ func (h *Handler) GetChallenge(w http.ResponseWriter, r *http.Request) { w.Header().Add("Retry-After", ch.RetryAfter) w.Header().Add("Cache-Control", "no-cache") api.JSON(w, ch) - case acme.StatusInvalid: - api.JSON(w, ch) - case acme.StatusValid: + case acme.StatusValid, acme.StatusInvalid: getLink := h.Auth.GetLink w.Header().Add("Link", link(getLink(acme.AuthzLink, acme.URLSafeProvisionerName(prov), true, ch.GetAuthzID()), "up")) w.Header().Set("Location", getLink(acme.ChallengeLink, acme.URLSafeProvisionerName(prov), true, ch.GetID())) From b061d0af34a88a23544cb4825878543eb8917d0b Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 07:31:21 -0700 Subject: [PATCH 23/35] acme/authority: Fix error message in test The error message was updated. Make the test should reflect the new changes. --- acme/authority_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/authority_test.go b/acme/authority_test.go index 21f80a06b..f798053b3 100644 --- a/acme/authority_test.go +++ b/acme/authority_test.go @@ -1266,7 +1266,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { auth: auth, id: ch.getID(), accID: ch.getAccountID(), - err: ServerInternalErr(errors.New("error attempting challenge validation: error saving acme challenge: force")), + err: ServerInternalErr(errors.New("error saving challenge: error saving acme challenge: force")), } }, "ok": func(t *testing.T) test { From 976c8f82c67a3cadf7caaf802a7983f9ad050cb9 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 07:55:38 -0700 Subject: [PATCH 24/35] acme/authority: Fix tests Also, return early from ValidateChallenge if the challenge is already valid. Interestingly, we aren't actually testing most of the ValidateChallenge func, just the early error and return conditions. We should add some more coverage here. --- acme/authority.go | 8 ++++++++ acme/authority_test.go | 20 ++++++++++++-------- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/acme/authority.go b/acme/authority.go index cc1a6daad..9a522e8df 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -317,6 +317,14 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin if err != nil { return nil, err } + switch ch.getStatus() { + case StatusPending, StatusProcessing: + break + case StatusInvalid, StatusValid: + return ch.toACME(a.dir, p) + default: + panic("unknown challenge state: " + ch.getStatus()) + } // Validate the challenge belongs to the account owned by the requester. if accID != ch.getAccountID() { diff --git a/acme/authority_test.go b/acme/authority_test.go index f798053b3..ff6cec0a8 100644 --- a/acme/authority_test.go +++ b/acme/authority_test.go @@ -1224,6 +1224,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { err: ServerInternalErr(errors.Errorf("error loading challenge %s: force", id)), } }, + "fail/challenge-not-owned-by-account": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) @@ -1244,6 +1245,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { err: UnauthorizedErr(errors.New("account does not own challenge")), } }, + "fail/validate-error": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) @@ -1269,15 +1271,16 @@ func TestAuthorityValidateChallenge(t *testing.T) { err: ServerInternalErr(errors.New("error saving challenge: error saving acme challenge: force")), } }, - "ok": func(t *testing.T) test { + + "ok/already-valid": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - _ch, ok := ch.(*http01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Status = StatusValid - _ch.baseChallenge.Validated = clock.Now() - _ch.baseChallenge.Retry = nil - b, err := json.Marshal(ch) + bc := ch.clone() + bc.Status = StatusValid + bc.Validated = clock.Now() + bc.Retry = nil + rch := bc.morph() + b, err := json.Marshal(rch) assert.FatalError(t, err) auth, err := NewAuthority(&db.MockNoSQLDB{ MGet: func(bucket, key []byte) ([]byte, error) { @@ -1291,10 +1294,11 @@ func TestAuthorityValidateChallenge(t *testing.T) { auth: auth, id: ch.getID(), accID: ch.getAccountID(), - ch: ch, + ch: rch, } }, } + for name, run := range tests { t.Run(name, func(t *testing.T) { tc := run(t) From 5354906b9c8a20b0dda1e470d8a9758e46d79973 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 10:56:19 -0700 Subject: [PATCH 25/35] acme/api: Add func name to beginning of comment --- acme/api/handler.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 7a804918e..13ce6f99d 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -2,13 +2,14 @@ package api import ( "fmt" + "net/http" + "github.com/go-chi/chi" "github.com/pkg/errors" "github.com/smallstep/certificates/acme" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority/provisioner" "github.com/smallstep/cli/jose" - "net/http" ) func link(url, typ string) string { @@ -148,7 +149,7 @@ func (h *Handler) GetAuthz(w http.ResponseWriter, r *http.Request) { api.JSON(w, authz) } -// ACME api for retrieving the Challenge resource. +// GetChallenge is the ACME api for retrieving a Challenge resource. // // Potential Challenges are requested by the client when creating an order. // Once the client knows the appropriate validation resources are provisioned, From 5e5a76c3b53e0edc1c654f8b334e651e9b9e3b21 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 11:10:14 -0700 Subject: [PATCH 26/35] acme/api: Set Link and Location headers for all 200 On the challenge resource, set "Link" and "Location" headers for all successful requests to the challenge resource. --- acme/api/handler.go | 22 ++++++++-------------- acme/api/handler_test.go | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 13ce6f99d..b204e2564 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -216,23 +216,17 @@ func (h *Handler) GetChallenge(w http.ResponseWriter, r *http.Request) { return } - switch ch.Status { - case acme.StatusPending: - panic("validation attempt did not move challenge to the processing state") - // When a transient error occurs, the challenge will not be progressed to the `invalid` state. - // Add a Retry-After header to indicate that the client should check again in the future. - case acme.StatusProcessing: + getLink := h.Auth.GetLink + w.Header().Add("Link", link(getLink(acme.AuthzLink, acme.URLSafeProvisionerName(prov), true, ch.GetAuthzID()), "up")) + w.Header().Set("Location", getLink(acme.ChallengeLink, acme.URLSafeProvisionerName(prov), true, ch.GetID())) + + if ch.Status == acme.StatusProcessing { w.Header().Add("Retry-After", ch.RetryAfter) + // 200s are cachable. Don't cache this because it will likely change. w.Header().Add("Cache-Control", "no-cache") - api.JSON(w, ch) - case acme.StatusValid, acme.StatusInvalid: - getLink := h.Auth.GetLink - w.Header().Add("Link", link(getLink(acme.AuthzLink, acme.URLSafeProvisionerName(prov), true, ch.GetAuthzID()), "up")) - w.Header().Set("Location", getLink(acme.ChallengeLink, acme.URLSafeProvisionerName(prov), true, ch.GetID())) - api.JSON(w, ch) - default: - panic("unexpected challenge state" + ch.Status) } + + api.JSON(w, ch) } // GetCertificate ACME api for retrieving a Certificate. diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index 9b8d04910..f4ec41b5f 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -742,6 +742,7 @@ func TestHandlerGetChallenge(t *testing.T) { chJSON, err := json.Marshal(ch) assert.FatalError(t, err) ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: chJSON}) + count := 0 return test{ auth: &mockAcmeAuthority{ validateChallenge: func(p provisioner.Interface, accID, id string, jwk *jose.JSONWebKey) (*acme.Challenge, error) { @@ -751,7 +752,27 @@ func TestHandlerGetChallenge(t *testing.T) { assert.Equals(t, jwk.KeyID, key.KeyID) return &ch, nil }, + getLink: func(typ acme.Link, provID string, abs bool, in ...string) string { + var ret string + switch count { + case 0: + assert.Equals(t, typ, acme.AuthzLink) + assert.Equals(t, provID, acme.URLSafeProvisionerName(prov)) + assert.True(t, abs) + assert.Equals(t, in, []string{ch.AuthzID}) + ret = fmt.Sprintf("https://ca.smallstep.com/acme/authz/%s", ch.AuthzID) + case 1: + assert.Equals(t, typ, acme.ChallengeLink) + assert.Equals(t, provID, acme.URLSafeProvisionerName(prov)) + assert.True(t, abs) + assert.Equals(t, in, []string{ch.ID}) + ret = url + } + count++ + return ret + }, }, + ctx: ctx, statusCode: 200, ch: ch, From b8b3ca2ac11a2d611dc9a170130d75a546ac097c Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 11:38:40 -0700 Subject: [PATCH 27/35] acme/authority: Add descriptive intro to ValidateChallenge --- acme/authority.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/acme/authority.go b/acme/authority.go index 9a522e8df..63ad07eb8 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -273,7 +273,8 @@ func (a *Authority) GetAuthz(p provisioner.Interface, accID, authzID string) (*A return az.toACME(a.db, a.dir, p) } -// ValidateChallenge ... +// ValidateChallenge loads a challenge resource and then begins the validation process if the challenge +// is not in one of its terminal states {valid|invalid}. // // The challenge validation state machine looks like: // From c378e0043a1054b6df44522ed21021aea2f8f0af Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 19:22:07 -0700 Subject: [PATCH 28/35] acme: Move ordinal to application The authority now receives the ordinal in its constructor rather than a global variable set at package initialization time. The ordinal is passed via the command line option `--ordinal`. --- acme/api/handler_test.go | 2 +- acme/authority.go | 28 +++---------- acme/authority_test.go | 86 ++++++++++++++++++++-------------------- ca/ca.go | 11 ++++- commands/app.go | 10 ++++- 5 files changed, 68 insertions(+), 69 deletions(-) diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index f4ec41b5f..70c366d73 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -231,7 +231,7 @@ func TestHandlerGetNonce(t *testing.T) { } func TestHandlerGetDirectory(t *testing.T) { - auth, err := acme.NewAuthority(new(db.MockNoSQLDB), "ca.smallstep.com", "acme", nil) + auth, err := acme.NewAuthority(new(db.MockNoSQLDB), "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) prov := newProv() url := fmt.Sprintf("http://ca.smallstep.com/acme/%s/directory", acme.URLSafeProvisionerName(prov)) diff --git a/acme/authority.go b/acme/authority.go index 63ad07eb8..471b1bcaa 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -5,12 +5,9 @@ import ( "crypto/tls" "crypto/x509" "encoding/base64" - "log" "net" "net/http" "net/url" - "os" - "strconv" "time" "github.com/pkg/errors" @@ -46,6 +43,7 @@ type Authority struct { db nosql.DB dir *directory signAuth SignAuthority + ordinal int } var ( @@ -57,26 +55,10 @@ var ( orderTable = []byte("acme_orders") ordersByAccountIDTable = []byte("acme_account_orders_index") certTable = []byte("acme_certs") - ordinal int ) -// Ordinal is used during challenge retries to indicate ownership. -func init() { - ordstr := os.Getenv("STEP_CA_ORDINAL") - if ordstr == "" { - ordinal = 0 - } else { - ord, err := strconv.Atoi(ordstr) - if err != nil { - log.Fatal("Unrecognized ordinal ingeter value.") - panic(nil) - } - ordinal = ord - } -} - // NewAuthority returns a new Authority that implements the ACME interface. -func NewAuthority(db nosql.DB, dns, prefix string, signAuth SignAuthority) (*Authority, error) { +func NewAuthority(db nosql.DB, dns, prefix string, signAuth SignAuthority, ordinal int) (*Authority, error) { if _, ok := db.(*database.SimpleDB); !ok { // If it's not a SimpleDB then go ahead and bootstrap the DB with the // necessary ACME tables. SimpleDB should ONLY be used for testing. @@ -91,7 +73,7 @@ func NewAuthority(db nosql.DB, dns, prefix string, signAuth SignAuthority) (*Aut } } return &Authority{ - db: db, dir: newDirectory(dns, prefix), signAuth: signAuth, + db: db, dir: newDirectory(dns, prefix), signAuth: signAuth, ordinal: ordinal, }, nil } @@ -336,7 +318,7 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin up := ch.clone() up.Status = StatusProcessing up.Retry = &Retry{ - Owner: ordinal, + Owner: a.ordinal, ProvisionerID: p.GetID(), NumAttempts: 0, MaxAttempts: 10, @@ -420,7 +402,7 @@ func (a *Authority) RetryChallenge(chID string) { // Then check to make sure Retry.NextAttempt is in the past. retry := ch.getRetry() switch { - case retry.Owner != ordinal: + case retry.Owner != a.ordinal: return case !retry.Active(): return diff --git a/acme/authority_test.go b/acme/authority_test.go index ff6cec0a8..9171af083 100644 --- a/acme/authority_test.go +++ b/acme/authority_test.go @@ -14,7 +14,7 @@ import ( ) func TestAuthorityGetLink(t *testing.T) { - auth, err := NewAuthority(new(db.MockNoSQLDB), "ca.smallstep.com", "acme", nil) + auth, err := NewAuthority(new(db.MockNoSQLDB), "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) provID := "acme-test-provisioner" type test struct { @@ -70,7 +70,7 @@ func TestAuthorityGetLink(t *testing.T) { } func TestAuthorityGetDirectory(t *testing.T) { - auth, err := NewAuthority(new(db.MockNoSQLDB), "ca.smallstep.com", "acme", nil) + auth, err := NewAuthority(new(db.MockNoSQLDB), "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) prov := newProv() acmeDir := auth.GetDirectory(prov) @@ -94,7 +94,7 @@ func TestAuthorityNewNonce(t *testing.T) { MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { return nil, false, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -110,7 +110,7 @@ func TestAuthorityNewNonce(t *testing.T) { *res = string(key) return nil, true, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -149,7 +149,7 @@ func TestAuthorityUseNonce(t *testing.T) { MUpdate: func(tx *database.Tx) error { return errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -161,7 +161,7 @@ func TestAuthorityUseNonce(t *testing.T) { MUpdate: func(tx *database.Tx) error { return nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -205,7 +205,7 @@ func TestAuthorityNewAccount(t *testing.T) { MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { return nil, false, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -231,7 +231,7 @@ func TestAuthorityNewAccount(t *testing.T) { count++ return nil, true, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -281,7 +281,7 @@ func TestAuthorityGetAccount(t *testing.T) { assert.Equals(t, key, []byte(id)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -298,7 +298,7 @@ func TestAuthorityGetAccount(t *testing.T) { MGet: func(bucket, key []byte) ([]byte, error) { return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -348,7 +348,7 @@ func TestAuthorityGetAccountByKey(t *testing.T) { jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) jwk.Key = "foo" - auth, err := NewAuthority(new(db.MockNoSQLDB), "ca.smallstep.com", "acme", nil) + auth, err := NewAuthority(new(db.MockNoSQLDB), "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -367,7 +367,7 @@ func TestAuthorityGetAccountByKey(t *testing.T) { assert.Equals(t, key, []byte(kid)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -399,7 +399,7 @@ func TestAuthorityGetAccountByKey(t *testing.T) { count++ return ret, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -453,7 +453,7 @@ func TestAuthorityGetOrder(t *testing.T) { assert.Equals(t, key, []byte(id)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -472,7 +472,7 @@ func TestAuthorityGetOrder(t *testing.T) { assert.Equals(t, key, []byte(o.ID)) return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -501,7 +501,7 @@ func TestAuthorityGetOrder(t *testing.T) { return nil, ServerInternalErr(errors.New("force")) } }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -522,7 +522,7 @@ func TestAuthorityGetOrder(t *testing.T) { assert.Equals(t, key, []byte(o.ID)) return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -576,7 +576,7 @@ func TestAuthorityGetCertificate(t *testing.T) { assert.Equals(t, key, []byte(id)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -595,7 +595,7 @@ func TestAuthorityGetCertificate(t *testing.T) { assert.Equals(t, key, []byte(cert.ID)) return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -615,7 +615,7 @@ func TestAuthorityGetCertificate(t *testing.T) { assert.Equals(t, key, []byte(cert.ID)) return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -670,7 +670,7 @@ func TestAuthorityGetAuthz(t *testing.T) { assert.Equals(t, key, []byte(id)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -689,7 +689,7 @@ func TestAuthorityGetAuthz(t *testing.T) { assert.Equals(t, key, []byte(az.getID())) return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -720,7 +720,7 @@ func TestAuthorityGetAuthz(t *testing.T) { count++ return ret, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -812,7 +812,7 @@ func TestAuthorityGetAuthz(t *testing.T) { count++ return ret, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -862,7 +862,7 @@ func TestAuthorityNewOrder(t *testing.T) { MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { return nil, false, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -916,7 +916,7 @@ func TestAuthorityNewOrder(t *testing.T) { MGet: func(bucket, key []byte) ([]byte, error) { return nil, database.ErrNotFound }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -966,7 +966,7 @@ func TestAuthorityGetOrdersByAccount(t *testing.T) { assert.Equals(t, key, []byte(id)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -998,7 +998,7 @@ func TestAuthorityGetOrdersByAccount(t *testing.T) { count++ return ret, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1045,7 +1045,7 @@ func TestAuthorityGetOrdersByAccount(t *testing.T) { count++ return ret, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1094,7 +1094,7 @@ func TestAuthorityFinalizeOrder(t *testing.T) { assert.Equals(t, key, []byte(id)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1113,7 +1113,7 @@ func TestAuthorityFinalizeOrder(t *testing.T) { assert.Equals(t, key, []byte(o.ID)) return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1139,7 +1139,7 @@ func TestAuthorityFinalizeOrder(t *testing.T) { assert.Equals(t, key, []byte(o.ID)) return nil, false, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1161,7 +1161,7 @@ func TestAuthorityFinalizeOrder(t *testing.T) { assert.Equals(t, key, []byte(o.ID)) return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1216,7 +1216,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { assert.Equals(t, key, []byte(id)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1236,7 +1236,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { assert.Equals(t, key, []byte(ch.getID())) return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1262,7 +1262,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { assert.Equals(t, key, []byte(ch.getID())) return nil, false, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1288,7 +1288,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { assert.Equals(t, key, []byte(ch.getID())) return b, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1344,7 +1344,7 @@ func TestAuthorityUpdateAccount(t *testing.T) { assert.Equals(t, key, []byte(id)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1366,7 +1366,7 @@ func TestAuthorityUpdateAccount(t *testing.T) { MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { return nil, false, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1394,7 +1394,7 @@ func TestAuthorityUpdateAccount(t *testing.T) { assert.Equals(t, key, []byte(acc.ID)) return nil, true, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1449,7 +1449,7 @@ func TestAuthorityDeactivateAccount(t *testing.T) { assert.Equals(t, key, []byte(id)) return nil, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1470,7 +1470,7 @@ func TestAuthorityDeactivateAccount(t *testing.T) { MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { return nil, false, errors.New("force") }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, @@ -1498,7 +1498,7 @@ func TestAuthorityDeactivateAccount(t *testing.T) { assert.Equals(t, key, []byte(acc.ID)) return nil, true, nil }, - }, "ca.smallstep.com", "acme", nil) + }, "ca.smallstep.com", "acme", nil, 0) assert.FatalError(t, err) return test{ auth: auth, diff --git a/ca/ca.go b/ca/ca.go index 96bebba47..18b43d927 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -26,6 +26,7 @@ type options struct { configFile string password []byte database db.AuthDB + ordinal int } func (o *options) apply(opts []Option) { @@ -60,6 +61,14 @@ func WithDatabase(db db.AuthDB) Option { } } + +// WithOrdinal sets the server's ordinal identifier (an int). +func WithOrdinal(ordinal int) Option { + return func(o *options) { + o.ordinal = ordinal + } +} + // CA is the type used to build the complete certificate authority. It builds // the HTTP server, set ups the middlewares and the HTTP handlers. type CA struct { @@ -124,7 +133,7 @@ func (ca *CA) Init(config *authority.Config) (*CA, error) { } prefix := "acme" - acmeAuth, err := acme.NewAuthority(auth.GetDatabase().(nosql.DB), dns, prefix, auth) + acmeAuth, err := acme.NewAuthority(auth.GetDatabase().(nosql.DB), dns, prefix, auth, ca.opts.ordinal) if err != nil { return nil, errors.Wrap(err, "error creating ACME authority") } diff --git a/commands/app.go b/commands/app.go index 9b22ac2d3..19b09a475 100644 --- a/commands/app.go +++ b/commands/app.go @@ -34,6 +34,11 @@ intermediate private key.`, Name: "resolver", Usage: "address of a DNS resolver to be used instead of the default.", }, + cli.IntFlag{ + Name: "ordinal", + Usage: `Unique identifying this instance of step-ca in a highly- +available (replicated) deployment.`, + }, }, } @@ -42,6 +47,9 @@ func appAction(ctx *cli.Context) error { passFile := ctx.String("password-file") resolver := ctx.String("resolver") + // grab the ordinal or default to 0 + ordinal := ctx.Int("ordinal") + // If zero cmd line args show help, if >1 cmd line args show error. if ctx.NArg() == 0 { return cli.ShowAppHelp(ctx) @@ -72,7 +80,7 @@ func appAction(ctx *cli.Context) error { } } - srv, err := ca.New(config, ca.WithConfigFile(configFile), ca.WithPassword(password)) + srv, err := ca.New(config, ca.WithConfigFile(configFile), ca.WithPassword(password), ca.WithOrdinal(ordinal)) if err != nil { fatal(err) } From f0228183f5d76bc0d3eeebfbff2828a4102bdbc6 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 19:41:35 -0700 Subject: [PATCH 29/35] project: go mod tidy --- go.mod | 1 - go.sum | 3 --- 2 files changed, 4 deletions(-) diff --git a/go.mod b/go.mod index 9ce422bf7..34810b17d 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,6 @@ require ( github.com/rs/xid v1.2.1 github.com/sirupsen/logrus v1.4.2 github.com/smallstep/assert v0.0.0-20200103212524-b99dc1097b15 - github.com/smallstep/certinfo v1.2.1 // indirect github.com/smallstep/cli v0.14.3-rc.1 github.com/smallstep/nosql v0.3.0 github.com/urfave/cli v1.22.2 diff --git a/go.sum b/go.sum index ebb9945cc..f9de86dc7 100644 --- a/go.sum +++ b/go.sum @@ -459,13 +459,10 @@ github.com/smallstep/assert v0.0.0-20200103212524-b99dc1097b15/go.mod h1:MyOHs9P github.com/smallstep/certificates v0.14.2/go.mod h1:eleWnbKTXDdV9GxWNOtbdjBiitdK5SuO4FCXOvcdLEY= github.com/smallstep/certinfo v1.2.0 h1:XJCH6fLKwGFcUndQ6ARtFdaqoujBCQnGbGRegf6PWcc= github.com/smallstep/certinfo v1.2.0/go.mod h1:1gQJekdPwPvUwFWGTi7bZELmQT09cxC9wJ0VBkBNiwU= -github.com/smallstep/certinfo v1.2.1/go.mod h1:1gQJekdPwPvUwFWGTi7bZELmQT09cxC9wJ0VBkBNiwU= github.com/smallstep/cli v0.14.2 h1:0Z1MtcgJfVS9RstNokWSNqE20xPwdiEhZgNuZxYRWRI= github.com/smallstep/cli v0.14.2/go.mod h1:JOTzEzQ4/l863KUqs9qlAqPagWPOqu6lc3C59S1nYzU= github.com/smallstep/cli v0.14.3-rc.1 h1:u5oUKbm6HL2lD7Xoary+DmIRJ1Ni6uov/DyA78u0CzA= github.com/smallstep/cli v0.14.3-rc.1/go.mod h1:9dsTyViHYZRwU+YjQYMHdRVk9jONeZSioYC5rqA3LoE= -github.com/smallstep/cli v0.14.3-rc.1.0.20200424181552-aac2566c506c h1:768U4DPoD1jm6GNHtbVssjUXNtwj4YNRcEWD4RxdXcM= -github.com/smallstep/cli v0.14.3-rc.1.0.20200424181552-aac2566c506c/go.mod h1:BcnWHhXfZzs7/CAh49cC47NoAA6bUJftjfDNqm6cxDY= github.com/smallstep/nosql v0.2.0/go.mod h1:qyxCqeyGwkuM6bfJSY3sg+aiXEiD0GbQOPzIF8/ZD8Q= github.com/smallstep/nosql v0.3.0 h1:V1X5vfDsDt89499h3jZFUlR4VnnsYYs5tXaQZ0w8z5U= github.com/smallstep/nosql v0.3.0/go.mod h1:QG7gNOpidifn99MjZaiNbm7HPesIyBd97F/OfacNz8Q= From deacbdc3585be4c802739b36e8d10365d2139243 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 13 May 2020 20:06:50 -0700 Subject: [PATCH 30/35] acme: Don't panic on logic errors Since it will ultimately 500 anyway, just return an error. --- acme/authority.go | 18 +++++++++++++----- acme/challenge.go | 20 +++++++++++++------- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/acme/authority.go b/acme/authority.go index 471b1bcaa..1863b48ff 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -5,6 +5,7 @@ import ( "crypto/tls" "crypto/x509" "encoding/base64" + "log" "net" "net/http" "net/url" @@ -306,7 +307,8 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin case StatusInvalid, StatusValid: return ch.toACME(a.dir, p) default: - panic("unknown challenge state: " + ch.getStatus()) + e:= errors.Errorf("unknown challenge state: %s", ch.getStatus()) + return nil, ServerInternalErr(e) } // Validate the challenge belongs to the account owned by the requester. @@ -352,7 +354,8 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin }) } default: - panic("post-validation challenge in unexpected state" + ch.getStatus()) + e := errors.Errorf("post-validation challenge in unexpected state, %s", ch.getStatus()) + return nil, ServerInternalErr(e) } return ch.toACME(a.dir, p) } @@ -388,13 +391,17 @@ func (a *Authority) RetryChallenge(chID string) { } switch ch.getStatus() { case StatusPending: - panic("pending challenges must first be moved to the processing state") + e := errors.New("pending challenges must first be moved to the processing state") + log.Printf("%v", e) + return case StatusInvalid, StatusValid: return case StatusProcessing: break default: - panic("unknown challenge state: " + ch.getStatus()) + e:= errors.Errorf("unknown challenge state: %s", ch.getStatus()) + log.Printf("%v", e) + return } // When retrying, check to make sure the ordinal has not changed. @@ -449,7 +456,8 @@ func (a *Authority) RetryChallenge(chID string) { }) } default: - panic("post-validation challenge in unexpected state " + ch.getStatus()) + e := errors.Errorf("post-validation challenge in unexpected state, %s", ch.getStatus()) + log.Printf("%v", e) } } diff --git a/acme/challenge.go b/acme/challenge.go index b87f4ee22..3ee27af8f 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -302,7 +302,7 @@ func (bc *baseChallenge) morph() challenge { case "tls-alpn-01": return &tlsALPN01Challenge{bc} default: - panic("unrecognized challenge type: " + bc.getType()) + return bc } } @@ -349,13 +349,15 @@ func (hc *http01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (c // If already valid or invalid then return without performing validation. switch hc.getStatus() { case StatusPending: - panic("pending challenges must first be moved to the processing state") + e := errors.New("pending challenges must first be moved to the processing state") + return nil, ServerInternalErr(e) case StatusProcessing: break case StatusValid, StatusInvalid: return hc, nil default: - panic("unknown challenge state: " + hc.getStatus()) + e := errors.Errorf("unknown challenge state: %s", hc.getStatus()) + return nil, ServerInternalErr(e) } up := &http01Challenge{hc.baseChallenge.clone()} @@ -426,13 +428,15 @@ func (tc *tlsALPN01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) // If already valid or invalid then return without performing validation. switch tc.getStatus() { case StatusPending: - panic("pending challenges must first be moved to the processing state") + e := errors.New("pending challenges must first be moved to the processing state") + return nil, ServerInternalErr(e) case StatusProcessing: break case StatusValid, StatusInvalid: return tc, nil default: - panic("unknown challenge state: " + tc.getStatus()) + e := errors.Errorf("unknown challenge state: %s", tc.getStatus()) + return nil, ServerInternalErr(e) } up := &tlsALPN01Challenge{tc.baseChallenge.clone()} @@ -565,13 +569,15 @@ func (dc *dns01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (ch // If already valid or invalid then return without performing validation. switch dc.getStatus() { case StatusPending: - panic("pending challenges must first be moved to the processing state") + e := errors.New("pending challenges must first be moved to the processing state") + return nil, ServerInternalErr(e) case StatusProcessing: break case StatusValid, StatusInvalid: return dc, nil default: - panic("unknown challenge state: " + dc.getStatus()) + e := errors.Errorf("unknown challenge state: %s", dc.getStatus()) + return nil, ServerInternalErr(e) } up := &dns01Challenge{dc.baseChallenge.clone()} From 0f63e43b10ef7d9372d07682b1012cb3e01f0583 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Tue, 19 May 2020 03:38:04 -0700 Subject: [PATCH 31/35] acme: Update http-01 challenge tests Add tests for the starting challenge statuses. Removed unneeded db write test. --- acme/challenge.go | 19 +-- acme/challenge_test.go | 281 ++++++++++++++++++++++++----------------- 2 files changed, 178 insertions(+), 122 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index 07d8c4f12..4baa085d2 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -389,17 +389,20 @@ func (hc *http01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (c if err != nil { return nil, err } - if keyAuth != expected { - // add base challenge fail validation - e := errors.Errorf("keyAuthorization does not match; expected %s, but got %s", expected, keyAuth) - up.Error = RejectedIdentifierErr(e).ToACME() - up.Status = StatusInvalid + + // success + if keyAuth == expected { + up.Validated = clock.Now() + up.Status = StatusValid + up.Error = nil + up.Retry = nil return up, nil } - up.Status = StatusValid - up.Validated = clock.Now() - up.Error = nil + // fail + up.Status = StatusInvalid + e := errors.Errorf("keyAuthorization does not match; expected %s, but got %s", expected, keyAuth) + up.Error = RejectedIdentifierErr(e).ToACME() up.Retry = nil return up, nil } diff --git a/acme/challenge_test.go b/acme/challenge_test.go index b574995f0..889990737 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -790,6 +790,7 @@ func TestHTTP01Validate(t *testing.T) { err *Error } tests := map[string]func(t *testing.T) test{ + "ok/status-already-valid": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) @@ -801,6 +802,7 @@ func TestHTTP01Validate(t *testing.T) { res: ch, } }, + "ok/status-already-invalid": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) @@ -812,77 +814,127 @@ func TestHTTP01Validate(t *testing.T) { res: ch, } }, - "ok/http-get-error": func(t *testing.T) test { + + "error/status-pending": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) + b := ch.clone() + b.Status = StatusPending + e := errors.New("pending challenges must first be moved to the processing state") + return test{ + ch: b.morph(), + err: ServerInternalErr(e), + } + }, + + "error/status-unknown": func(t *testing.T) test { + ch, err := newHTTPCh() assert.FatalError(t, err) + b := ch.clone() + b.Status = "unknown" + e := errors.New("unknown challenge state: unknown") + return test{ + ch: b.morph(), + err: ServerInternalErr(e), + } + }, - expErr := ConnectionErr(errors.Errorf("error doing http GET for url "+ - "http://zap.internal/.well-known/acme-challenge/%s: force", ch.getToken())) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &http01Challenge{baseClone} - newb, err := json.Marshal(newCh) + "ok/http-get-error": func(t *testing.T) test { + ch, err := newHTTPCh() assert.FatalError(t, err) + up := ch.clone() + up.Retry = &Retry{ + Owner: 0, + ProvisionerID: "acme/acme", + NumAttempts: 1, + MaxAttempts: 6, + NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), + } + up.Status = StatusProcessing + ch = up.morph() + chb, err := json.Marshal(ch) + assert.FatalError(t, err) + + rch := ch.clone() + geterr := errors.New("force") + url := fmt.Sprintf("http://%s/.well-known/acme-challenge/%s", ch.getValue(), ch.getToken()) + e := errors.Wrapf(geterr, "error doing http GET for url %s", url) + rch.Error = ConnectionErr(e).ToACME() + return test{ ch: ch, vo: validateOptions{ httpGet: func(url string) (*http.Response, error) { - return nil, errors.New("force") + return nil, geterr }, }, db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, newval, newb) - return nil, true, nil - }, + Ret1: chb, + Ret2: true, }, - res: ch, + res: rch, } }, + "ok/http-get->=400": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) - expErr := ConnectionErr(errors.Errorf("error doing http GET for url "+ - "http://zap.internal/.well-known/acme-challenge/%s with status code 400", ch.getToken())) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &http01Challenge{baseClone} - newb, err := json.Marshal(newCh) + up := ch.clone() + up.Retry = &Retry{ + Owner: 0, + ProvisionerID: "acme/acme", + NumAttempts: 1, + MaxAttempts: 6, + NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), + } + up.Status = StatusProcessing + ch = up.morph() + chb, err := json.Marshal(ch) assert.FatalError(t, err) + + rch := ch.clone() + url := fmt.Sprintf("http://%s/.well-known/acme-challenge/%s", ch.getValue(), ch.getToken()) + e := errors.Errorf("error doing http GET for url %s with status code %d", url, http.StatusBadRequest) + rch.Error = ConnectionErr(e).ToACME() + return test{ ch: ch, vo: validateOptions{ httpGet: func(url string) (*http.Response, error) { return &http.Response{ + Body: ioutil.NopCloser(bytes.NewBufferString("")), StatusCode: http.StatusBadRequest, }, nil }, }, db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, newval, newb) - return nil, true, nil - }, + Ret1: chb, + Ret2: true, }, - res: ch, + res: rch, } }, + "fail/read-body": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + up := ch.clone() + up.Retry = &Retry{ + Owner: 0, + ProvisionerID: "acme/acme", + NumAttempts: 1, + MaxAttempts: 6, + NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), + } + up.Status = StatusProcessing + ch = up.morph() + chb, err := json.Marshal(ch) assert.FatalError(t, err) - jwk.Key = "foo" + + rch := ch.clone() + url := fmt.Sprintf("http://%s/.well-known/acme-challenge/%s", ch.getValue(), ch.getToken()) + e := errors.Wrapf(errors.New("force"), "error reading response body for url %s", url) + rch.Error = ServerInternalErr(e).ToACME() return test{ ch: ch, @@ -893,18 +945,34 @@ func TestHTTP01Validate(t *testing.T) { }, nil }, }, - jwk: jwk, - err: ServerInternalErr(errors.Errorf("error reading response "+ - "body for url http://zap.internal/.well-known/acme-challenge/%s: force", - ch.getToken())), + db: &db.MockNoSQLDB{ + Ret1: chb, + Ret2: true, + }, + res: rch, } }, + "fail/key-authorization-gen-error": func(t *testing.T) test { - ch, err := newHTTPCh() - assert.FatalError(t, err) jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) jwk.Key = "foo" + + ch, err := newHTTPCh() + assert.FatalError(t, err) + b := ch.clone() + b.Retry = &Retry{ + Owner: 0, + ProvisionerID: "acme/acme", + NumAttempts: 1, + MaxAttempts: 6, + NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), + } + b.Status = StatusProcessing + ch = b.morph() + chb, err := json.Marshal(ch) + assert.FatalError(t, err) + return test{ ch: ch, vo: validateOptions{ @@ -914,30 +982,45 @@ func TestHTTP01Validate(t *testing.T) { }, nil }, }, + db: &db.MockNoSQLDB{ + Ret1: chb, + Ret2: true, + }, jwk: jwk, err: ServerInternalErr(errors.New("error generating JWK thumbprint: square/go-jose: unknown key type 'string'")), } }, + "ok/key-auth-mismatch": func(t *testing.T) test { - ch, err := newHTTPCh() - assert.FatalError(t, err) - oldb, err := json.Marshal(ch) + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + ch, err := newHTTPCh() + assert.FatalError(t, err) + b := ch.clone() + b.Retry = &Retry{ + Owner: 0, + ProvisionerID: "acme/acme", + NumAttempts: 1, + MaxAttempts: 6, + NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), + } + b.Status = StatusProcessing + ch = b.morph() + chb, err := json.Marshal(ch) assert.FatalError(t, err) expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) - expErr := RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ - "expected %s, but got foo", expKeyAuth)) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - baseClone.Error.Subproblems = append(baseClone.Error.Subproblems, expErr) - newCh := &http01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + b = ch.clone() + e := errors.Errorf("keyAuthorization does not match; expected %s, but got foo", expKeyAuth) + ae := RejectedIdentifierErr(e) + b.Error = ae.ToACME() + b.Retry = nil + b.Status = StatusInvalid + + rch := b.morph() return test{ ch: ch, @@ -950,67 +1033,44 @@ func TestHTTP01Validate(t *testing.T) { }, jwk: jwk, db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, newval, newb) - return nil, true, nil - }, + Ret1: chb, + Ret2: true, }, - res: ch, + res: rch, } }, - "fail/save-error": func(t *testing.T) test { - ch, err := newHTTPCh() - assert.FatalError(t, err) + "ok": func(t *testing.T) test { jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) - assert.FatalError(t, err) - return test{ - ch: ch, - vo: validateOptions{ - httpGet: func(url string) (*http.Response, error) { - return &http.Response{ - Body: ioutil.NopCloser(bytes.NewBufferString(expKeyAuth)), - }, nil - }, - }, - jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - return nil, false, errors.New("force") - }, - }, - err: ServerInternalErr(errors.New("error saving acme challenge: force")), - } - }, - "ok": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - _ch, ok := ch.(*http01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Error = MalformedErr(nil).ToACME() - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) - - jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + b := ch.clone() + b.Retry = &Retry{ + Owner: 0, + ProvisionerID: "acme/acme", + NumAttempts: 1, + MaxAttempts: 6, + NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), + } + b.Status = StatusProcessing + ch = b.morph() + chb, err := json.Marshal(ch) assert.FatalError(t, err) expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) - baseClone := ch.clone() - baseClone.Status = StatusValid - baseClone.Error = nil - newCh := &http01Challenge{baseClone} + b = ch.clone() + b.Validated = clock.Now() + b.Status = StatusValid + b.Error = nil + b.Retry = nil + rch := b.morph() return test{ - ch: ch, - res: newCh, + ch: ch, vo: validateOptions{ httpGet: func(url string) (*http.Response, error) { return &http.Response{ @@ -1020,25 +1080,14 @@ func TestHTTP01Validate(t *testing.T) { }, jwk: jwk, db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - - httpCh, err := unmarshalChallenge(newval) - assert.FatalError(t, err) - assert.Equals(t, httpCh.getStatus(), StatusValid) - assert.True(t, httpCh.getValidated().Before(time.Now().UTC().Add(time.Minute))) - assert.True(t, httpCh.getValidated().After(time.Now().UTC().Add(-1*time.Second))) - - baseClone.Validated = httpCh.getValidated() - - return nil, true, nil - }, + Ret1: chb, + Ret2: true, }, + res: rch, } }, } + for name, run := range tests { t.Run(name, func(t *testing.T) { tc := run(t) @@ -1058,7 +1107,11 @@ func TestHTTP01Validate(t *testing.T) { assert.Equals(t, tc.res.getStatus(), ch.getStatus()) assert.Equals(t, tc.res.getToken(), ch.getToken()) assert.Equals(t, tc.res.getCreated(), ch.getCreated()) - assert.Equals(t, tc.res.getValidated(), ch.getValidated()) + if tc.res.getValidated() != ch.getValidated() { + assert.True(t, ch.getValidated().After(tc.res.getValidated()), + "validated timestamp should come after challenge creation") + + } assert.Equals(t, tc.res.getError(), ch.getError()) assert.Equals(t, tc.res.getRetry(), ch.getRetry()) } From d54f963b81bf70cb74ad7a9c5e0e5cd28313508a Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 20 May 2020 18:02:58 -0700 Subject: [PATCH 32/35] acme/retry: Update DNS challenge tests --- acme/challenge.go | 6 +- acme/challenge_test.go | 390 ++++++++++++++++------------------------- 2 files changed, 158 insertions(+), 238 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index 4baa085d2..13eb6d0cd 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -402,7 +402,7 @@ func (hc *http01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (c // fail up.Status = StatusInvalid e := errors.Errorf("keyAuthorization does not match; expected %s, but got %s", expected, keyAuth) - up.Error = RejectedIdentifierErr(e).ToACME() + up.Error = IncorrectResponseErr(e).ToACME() up.Retry = nil return up, nil } @@ -596,7 +596,7 @@ func (dc *dns01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (ch if err != nil { e := errors.Wrapf(err, "error looking up TXT records for domain %s", domain) up.Error = DNSErr(e).ToACME() - return dc, nil + return up, nil } expectedKeyAuth, err := KeyAuthorization(dc.Token, jwk) @@ -614,7 +614,7 @@ func (dc *dns01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (ch for _, r := range txtRecords { if r == expected { - up.Validated = time.Now().UTC() + up.Validated = clock.Now() up.Status = StatusValid up.Error = nil up.Retry = nil diff --git a/acme/challenge_test.go b/acme/challenge_test.go index 889990737..4c8530631 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -791,24 +791,24 @@ func TestHTTP01Validate(t *testing.T) { } tests := map[string]func(t *testing.T) test{ - "ok/status-already-valid": func(t *testing.T) test { + "valid/status-noop": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - _ch, ok := ch.(*http01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Status = StatusValid + b := ch.clone() + b.Status = StatusValid + ch = b.morph() return test{ ch: ch, res: ch, } }, - "ok/status-already-invalid": func(t *testing.T) test { + "invalid/status-noop": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - _ch, ok := ch.(*http01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Status = StatusInvalid + b := ch.clone() + b.Status = StatusInvalid + ch = b.morph() return test{ ch: ch, res: ch, @@ -842,18 +842,9 @@ func TestHTTP01Validate(t *testing.T) { "ok/http-get-error": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - up := ch.clone() - up.Retry = &Retry{ - Owner: 0, - ProvisionerID: "acme/acme", - NumAttempts: 1, - MaxAttempts: 6, - NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), - } - up.Status = StatusProcessing - ch = up.morph() - chb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + ch = b.morph() rch := ch.clone() geterr := errors.New("force") @@ -868,29 +859,16 @@ func TestHTTP01Validate(t *testing.T) { return nil, geterr }, }, - db: &db.MockNoSQLDB{ - Ret1: chb, - Ret2: true, - }, res: rch, } }, - "ok/http-get->=400": func(t *testing.T) test { + "processing/http-get->=400": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - up := ch.clone() - up.Retry = &Retry{ - Owner: 0, - ProvisionerID: "acme/acme", - NumAttempts: 1, - MaxAttempts: 6, - NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), - } - up.Status = StatusProcessing - ch = up.morph() - chb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + ch = b.morph() rch := ch.clone() url := fmt.Sprintf("http://%s/.well-known/acme-challenge/%s", ch.getValue(), ch.getToken()) @@ -907,29 +885,16 @@ func TestHTTP01Validate(t *testing.T) { }, nil }, }, - db: &db.MockNoSQLDB{ - Ret1: chb, - Ret2: true, - }, res: rch, } }, - "fail/read-body": func(t *testing.T) test { + "processing/read-body-error": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) - up := ch.clone() - up.Retry = &Retry{ - Owner: 0, - ProvisionerID: "acme/acme", - NumAttempts: 1, - MaxAttempts: 6, - NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), - } - up.Status = StatusProcessing - ch = up.morph() - chb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + ch = b.morph() rch := ch.clone() url := fmt.Sprintf("http://%s/.well-known/acme-challenge/%s", ch.getValue(), ch.getToken()) @@ -945,33 +910,20 @@ func TestHTTP01Validate(t *testing.T) { }, nil }, }, - db: &db.MockNoSQLDB{ - Ret1: chb, - Ret2: true, - }, res: rch, } }, - "fail/key-authorization-gen-error": func(t *testing.T) test { - jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) - assert.FatalError(t, err) - jwk.Key = "foo" - + "error/key-authorization-gen": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) b := ch.clone() - b.Retry = &Retry{ - Owner: 0, - ProvisionerID: "acme/acme", - NumAttempts: 1, - MaxAttempts: 6, - NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), - } b.Status = StatusProcessing ch = b.morph() - chb, err := json.Marshal(ch) + + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) + jwk.Key = "foo" return test{ ch: ch, @@ -982,44 +934,28 @@ func TestHTTP01Validate(t *testing.T) { }, nil }, }, - db: &db.MockNoSQLDB{ - Ret1: chb, - Ret2: true, - }, jwk: jwk, err: ServerInternalErr(errors.New("error generating JWK thumbprint: square/go-jose: unknown key type 'string'")), } }, - "ok/key-auth-mismatch": func(t *testing.T) test { - jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) - assert.FatalError(t, err) - + "invalid/key-auth-mismatch": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) b := ch.clone() - b.Retry = &Retry{ - Owner: 0, - ProvisionerID: "acme/acme", - NumAttempts: 1, - MaxAttempts: 6, - NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), - } b.Status = StatusProcessing ch = b.morph() - chb, err := json.Marshal(ch) - assert.FatalError(t, err) + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + assert.FatalError(t, err) expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) b = ch.clone() e := errors.Errorf("keyAuthorization does not match; expected %s, but got foo", expKeyAuth) - ae := RejectedIdentifierErr(e) - b.Error = ae.ToACME() + b.Error = IncorrectResponseErr(e).ToACME() b.Retry = nil b.Status = StatusInvalid - rch := b.morph() return test{ @@ -1032,33 +968,19 @@ func TestHTTP01Validate(t *testing.T) { }, }, jwk: jwk, - db: &db.MockNoSQLDB{ - Ret1: chb, - Ret2: true, - }, res: rch, } }, - "ok": func(t *testing.T) test { - jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) - assert.FatalError(t, err) - + "valid/normal-http-get": func(t *testing.T) test { ch, err := newHTTPCh() assert.FatalError(t, err) b := ch.clone() - b.Retry = &Retry{ - Owner: 0, - ProvisionerID: "acme/acme", - NumAttempts: 1, - MaxAttempts: 6, - NextAttempt: time.Now().UTC().Add(time.Minute).Format(time.RFC3339), - } b.Status = StatusProcessing ch = b.morph() - chb, err := json.Marshal(ch) - assert.FatalError(t, err) + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + assert.FatalError(t, err) expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) @@ -1079,10 +1001,6 @@ func TestHTTP01Validate(t *testing.T) { }, }, jwk: jwk, - db: &db.MockNoSQLDB{ - Ret1: chb, - Ret2: true, - }, res: rch, } }, @@ -1108,9 +1026,12 @@ func TestHTTP01Validate(t *testing.T) { assert.Equals(t, tc.res.getToken(), ch.getToken()) assert.Equals(t, tc.res.getCreated(), ch.getCreated()) if tc.res.getValidated() != ch.getValidated() { - assert.True(t, ch.getValidated().After(tc.res.getValidated()), - "validated timestamp should come after challenge creation") - + now := clock.Now() + window := now.Sub(tc.res.getValidated()) + assert.True(t, now.Sub(ch.getValidated()) <= window, + "validated timestamp should come before now but after test case setup") + } else { + assert.Equals(t, tc.res.getValidated(), ch.getValidated()) } assert.Equals(t, tc.res.getError(), ch.getError()) assert.Equals(t, tc.res.getRetry(), ch.getRetry()) @@ -1845,42 +1766,67 @@ func TestDNS01Validate(t *testing.T) { err *Error } tests := map[string]func(t *testing.T) test{ - "ok/status-already-valid": func(t *testing.T) test { + + "valid/status-noop": func(t *testing.T) test { ch, err := newDNSCh() assert.FatalError(t, err) - _ch, ok := ch.(*dns01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Status = StatusValid + b := ch.clone() + b.Status = StatusValid + ch = b.morph() return test{ ch: ch, res: ch, } }, - "ok/status-already-invalid": func(t *testing.T) test { + + "invalid/status-noop": func(t *testing.T) test { ch, err := newDNSCh() assert.FatalError(t, err) - _ch, ok := ch.(*dns01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Status = StatusInvalid + b := ch.clone() + b.Status = StatusInvalid + ch = b.morph() return test{ ch: ch, res: ch, } }, - "ok/lookup-txt-error": func(t *testing.T) test { + + "error/status-pending": func(t *testing.T) test { ch, err := newDNSCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) + b := ch.clone() + b.Status = StatusPending + e := errors.New("pending challenges must first be moved to the processing state") + return test{ + ch: b.morph(), + err: ServerInternalErr(e), + } + }, + + "error/status-unknown": func(t *testing.T) test { + ch, err := newDNSCh() assert.FatalError(t, err) + b := ch.clone() + b.Status = "unknown" + e := errors.New("unknown challenge state: unknown") + return test{ + ch: b.morph(), + err: ServerInternalErr(e), + } + }, - expErr := DNSErr(errors.Errorf("error looking up TXT records for "+ - "domain %s: force", ch.getValue())) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - baseClone.Error.Subproblems = append(baseClone.Error.Subproblems, expErr) - newCh := &dns01Challenge{baseClone} - newb, err := json.Marshal(newCh) + "processing/lookup-txt-error": func(t *testing.T) test { + ch, err := newDNSCh() assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + ch = b.morph() + + b = ch.clone() + e := errors.Errorf("error looking up TXT records for domain %s: force", ch.getValue()) + b.Error = DNSErr(e).ToACME() + rch := b.morph() + return test{ ch: ch, vo: validateOptions{ @@ -1888,65 +1834,52 @@ func TestDNS01Validate(t *testing.T) { return nil, errors.New("force") }, }, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, newval, newb) - return nil, true, nil - }, - }, - res: ch, + res: rch, } }, - "ok/lookup-txt-wildcard": func(t *testing.T) test { + + "fail/key-authorization-gen-error": func(t *testing.T) test { ch, err := newDNSCh() assert.FatalError(t, err) - _ch, ok := ch.(*dns01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Value = "*.zap.internal" + b := ch.clone() + b.Status = StatusProcessing + ch = b.morph() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) - assert.FatalError(t, err) - h := sha256.Sum256([]byte(expKeyAuth)) - expected := base64.RawURLEncoding.EncodeToString(h[:]) - - baseClone := ch.clone() - baseClone.Status = StatusValid - baseClone.Error = nil - newCh := &dns01Challenge{baseClone} + jwk.Key = "foo" return test{ - ch: ch, - res: newCh, + ch: ch, vo: validateOptions{ lookupTxt: func(url string) ([]string, error) { - assert.Equals(t, url, "_acme-challenge.zap.internal") - return []string{"foo", expected}, nil + return []string{"foo", "bar"}, nil }, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - dnsCh, err := unmarshalChallenge(newval) - assert.FatalError(t, err) - assert.Equals(t, dnsCh.getStatus(), StatusValid) - baseClone.Validated = dnsCh.getValidated() - return nil, true, nil - }, - }, + err: ServerInternalErr(errors.New("error generating JWK thumbprint: square/go-jose: unknown key type 'string'")), } }, - "fail/key-authorization-gen-error": func(t *testing.T) test { + + "invalid/key-auth-mismatch": func(t *testing.T) test { ch, err := newDNSCh() assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + ch = b.morph() + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - jwk.Key = "foo" + expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) + assert.FatalError(t, err) + + e := errors.Errorf("keyAuthorization does not match; "+ + "expected %s, but got %s", expKeyAuth, []string{"foo", "bar"}) + b = ch.clone() + b.Status = StatusInvalid + b.Error = IncorrectResponseErr(e).ToACME() + rch := b.morph() + return test{ ch: ch, vo: validateOptions{ @@ -1955,61 +1888,58 @@ func TestDNS01Validate(t *testing.T) { }, }, jwk: jwk, - err: ServerInternalErr(errors.New("error generating JWK thumbprint: square/go-jose: unknown key type 'string'")), + res: rch, } }, - "ok/key-auth-mismatch": func(t *testing.T) test { + + "processing/empty-list": func(t *testing.T) test { ch, err := newDNSCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + ch = b.morph() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) - assert.FatalError(t, err) - - expErr := RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ - "expected %s, but got %s", expKeyAuth, []string{"foo", "bar"})) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - baseClone.Error.Subproblems = append(baseClone.Error.Subproblems, expErr) - newCh := &http01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + e := errors.New("no TXT record found at '_acme-challenge.zap.internal'") + b = ch.clone() + b.Error = DNSErr(e).ToACME() + rch := b.morph() return test{ ch: ch, vo: validateOptions{ lookupTxt: func(url string) ([]string, error) { - return []string{"foo", "bar"}, nil + return []string{}, nil }, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, newval, newb) - return nil, true, nil - }, - }, - res: ch, + res: rch, } }, - "fail/save-error": func(t *testing.T) test { + + "valid/lookup-txt-normal": func(t *testing.T) test { ch, err := newDNSCh() assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + ch = b.morph() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) h := sha256.Sum256([]byte(expKeyAuth)) expected := base64.RawURLEncoding.EncodeToString(h[:]) + + b = ch.clone() + b.Validated = clock.Now() + b.Status = StatusValid + b.Error = nil + b.Retry = nil + rch := b.morph() + return test{ ch: ch, vo: validateOptions{ @@ -2018,22 +1948,17 @@ func TestDNS01Validate(t *testing.T) { }, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - return nil, false, errors.New("force") - }, - }, - err: ServerInternalErr(errors.New("error saving acme challenge: force")), + res: rch, } }, - "ok": func(t *testing.T) test { + + "valid/lookup-txt-wildcard": func(t *testing.T) test { ch, err := newDNSCh() assert.FatalError(t, err) - _ch, ok := ch.(*dns01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Error = MalformedErr(nil).ToACME() - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + b.Value = "*.zap.internal" + ch = b.morph() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) @@ -2043,40 +1968,27 @@ func TestDNS01Validate(t *testing.T) { h := sha256.Sum256([]byte(expKeyAuth)) expected := base64.RawURLEncoding.EncodeToString(h[:]) - baseClone := ch.clone() - baseClone.Status = StatusValid - baseClone.Error = nil - newCh := &dns01Challenge{baseClone} + b = ch.clone() + b.Status = StatusValid + b.Validated = clock.Now() + b.Error = nil + b.Retry = nil + rch := b.morph() return test{ - ch: ch, - res: newCh, + ch: ch, vo: validateOptions{ lookupTxt: func(url string) ([]string, error) { + assert.Equals(t, url, "_acme-challenge.zap.internal") return []string{"foo", expected}, nil }, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - - dnsCh, err := unmarshalChallenge(newval) - assert.FatalError(t, err) - assert.Equals(t, dnsCh.getStatus(), StatusValid) - assert.True(t, dnsCh.getValidated().Before(time.Now().UTC())) - assert.True(t, dnsCh.getValidated().After(time.Now().UTC().Add(-1*time.Second))) - - baseClone.Validated = dnsCh.getValidated() - - return nil, true, nil - }, - }, + res: rch, } }, } + for name, run := range tests { t.Run(name, func(t *testing.T) { tc := run(t) @@ -2096,8 +2008,16 @@ func TestDNS01Validate(t *testing.T) { assert.Equals(t, tc.res.getStatus(), ch.getStatus()) assert.Equals(t, tc.res.getToken(), ch.getToken()) assert.Equals(t, tc.res.getCreated(), ch.getCreated()) - assert.Equals(t, tc.res.getValidated(), ch.getValidated()) + if tc.res.getValidated() != ch.getValidated() { + now := clock.Now() + window := now.Sub(tc.res.getValidated()) + assert.True(t, now.Sub(ch.getValidated()) <= window, + "validated timestamp should come before now but after test case setup") + } else { + assert.Equals(t, tc.res.getValidated(), ch.getValidated()) + } assert.Equals(t, tc.res.getError(), ch.getError()) + assert.Equals(t, tc.res.getRetry(), ch.getRetry()) } } }) From 05780554d297da01ec8b414c26c4f81ad413ecc9 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 20 May 2020 20:32:02 -0700 Subject: [PATCH 33/35] acme/retry: Cleanup tls-alpn-01 tests This logic was already in the correct form so it was much easier to update. --- acme/challenge.go | 8 +- acme/challenge_test.go | 477 ++++++++++++++--------------------------- 2 files changed, 165 insertions(+), 320 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index 13eb6d0cd..0063c4530 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -463,12 +463,12 @@ func (tc *tlsALPN01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) if len(certs) == 0 { e := errors.Errorf("%s challenge for %s resulted in no certificates", tc.Type, tc.Value) - up.Error = RejectedIdentifierErr(e).ToACME() + up.Error = TLSErr(e).ToACME() return up, nil } if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != "acme-tls/1" { e := errors.Errorf("cannot negotiate ALPN acme-tls/1 protocol for tls-alpn-01 challenge") - up.Error = RejectedIdentifierErr(e).ToACME() + up.Error = TLSErr(e).ToACME() return up, nil } @@ -476,7 +476,7 @@ func (tc *tlsALPN01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) if len(leafCert.DNSNames) != 1 || !strings.EqualFold(leafCert.DNSNames[0], tc.Value) { e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ "leaf certificate must contain a single DNS name, %v", tc.Value) - up.Error = RejectedIdentifierErr(e).ToACME() + up.Error = TLSErr(e).ToACME() return up, nil } @@ -541,7 +541,7 @@ func (tc *tlsALPN01Challenge) validate(jwk *jose.JSONWebKey, vo validateOptions) e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: " + "missing acmeValidationV1 extension") up.Error = IncorrectResponseErr(e).ToACME() - return tc, nil + return up, nil } // dns01Challenge represents an dns-01 acme challenge. diff --git a/acme/challenge_test.go b/acme/challenge_test.go index 4c8530631..fc6e0323d 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -1052,493 +1052,358 @@ func TestTLSALPN01Validate(t *testing.T) { err *Error } tests := map[string]func(t *testing.T) test{ - "ok/status-already-valid": func(t *testing.T) test { + + "valid/status-noop": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - _ch, ok := ch.(*tlsALPN01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Status = StatusValid - + b := ch.clone() + b.Status = StatusValid + ch = b.morph() return test{ ch: ch, res: ch, } }, - "ok/status-already-invalid": func(t *testing.T) test { + + "invalid/status-noop": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - _ch, ok := ch.(*tlsALPN01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Status = StatusInvalid - + b := ch.clone() + b.Status = StatusInvalid + ch = b.morph() return test{ ch: ch, res: ch, } }, - "ok/tls-dial-error": func(t *testing.T) test { + + "processing/tls-dial-error": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing - expErr := ConnectionErr(errors.Errorf("error doing TLS dial for %v:443: force", ch.getValue())) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + a := b.clone() + e := (errors.Errorf("error doing TLS dial for %v:443: force", ch.getValue())) + a.Error = ConnectionErr(e).ToACME() return test{ - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: func(network, addr string, config *tls.Config) (*tls.Conn, error) { return nil, errors.New("force") }, }, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, newval, newb) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/timeout": func(t *testing.T) test { + + "processing/timeout": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing - expErr := ConnectionErr(errors.Errorf("error doing TLS dial for %v:443: tls: DialWithDialer timed out", ch.getValue())) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + a := b.clone() + e := errors.Errorf("error doing TLS dial for %v:443: tls: DialWithDialer timed out", ch.getValue()) + a.Error = ConnectionErr(e).ToACME() srv, tlsDial := newTestTLSALPNServer(nil) // srv.Start() - do not start server to cause timeout return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: tlsDial, }, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/no-certificates": func(t *testing.T) test { + + "processing/no-certificates": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing - expErr := RejectedIdentifierErr(errors.Errorf("tls-alpn-01 challenge for %v resulted in no certificates", ch.getValue())) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + a := b.clone() + e := errors.Errorf("tls-alpn-01 challenge for %v resulted in no certificates", ch.getValue()) + a.Error = TLSErr(e).ToACME() return test{ - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: func(network, addr string, config *tls.Config) (*tls.Conn, error) { return tls.Client(&noopConn{}, config), nil }, }, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/no-names": func(t *testing.T) test { + + "processing/no-protocol": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) + b := ch.clone() + b.Status = StatusProcessing + + a := b.clone() + e := errors.New("cannot negotiate ALPN acme-tls/1 protocol for tls-alpn-01 challenge") + a.Error = TLSErr(e).ToACME() + + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expErr := RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.getValue())) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) + srv := httptest.NewTLSServer(nil) + + return test{ + srv: srv, + ch: b.morph(), + vo: validateOptions{ + tlsDial: func(network, addr string, config *tls.Config) (*tls.Conn, error) { + return tls.DialWithDialer(&net.Dialer{Timeout: time.Second}, "tcp", srv.Listener.Addr().String(), config) + }, + }, + jwk: jwk, + res: a.morph(), + } + }, + + "processing/no-names": func(t *testing.T) test { + ch, err := newTLSALPNCh() assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + + a := b.clone() + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.getValue()) + a.Error = TLSErr(e).ToACME() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) expKeyAuthHash := sha256.Sum256([]byte(expKeyAuth)) cert, err := newTLSALPNValidationCert(expKeyAuthHash[:], false, true) assert.FatalError(t, err) - srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: tlsDial, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/too-many-names": func(t *testing.T) test { + + "processing/too-many-names": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing - expErr := RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.getValue())) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + a := b.clone() + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.getValue()) + a.Error = TLSErr(e).ToACME() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) expKeyAuthHash := sha256.Sum256([]byte(expKeyAuth)) cert, err := newTLSALPNValidationCert(expKeyAuthHash[:], false, true, ch.getValue(), "other.internal") assert.FatalError(t, err) - srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: tlsDial, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/wrong-name": func(t *testing.T) test { + + "processing/wrong-name": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing - expErr := RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.getValue())) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + a := b.clone() + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: leaf certificate must contain a single DNS name, %v", ch.getValue()) + a.Error = TLSErr(e).ToACME() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) expKeyAuthHash := sha256.Sum256([]byte(expKeyAuth)) cert, err := newTLSALPNValidationCert(expKeyAuthHash[:], false, true, "other.internal") assert.FatalError(t, err) - srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: tlsDial, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/no-extension": func(t *testing.T) test { - ch, err := newTLSALPNCh() - assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) - expErr := RejectedIdentifierErr(errors.New("incorrect certificate for tls-alpn-01 challenge: missing acmeValidationV1 extension")) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) + "processing/no-extension": func(t *testing.T) test { + ch, err := newTLSALPNCh() assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing + a := b.clone() + e := errors.New("incorrect certificate for tls-alpn-01 challenge: missing acmeValidationV1 extension") + a.Error = IncorrectResponseErr(e).ToACME() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) cert, err := newTLSALPNValidationCert(nil, false, true, ch.getValue()) assert.FatalError(t, err) - srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: tlsDial, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/extension-not-critical": func(t *testing.T) test { + + "processing/extension-not-critical": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing - expErr := RejectedIdentifierErr(errors.New("incorrect certificate for tls-alpn-01 challenge: acmeValidationV1 extension not critical")) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + a := b.clone() + e := errors.New("incorrect certificate for tls-alpn-01 challenge: acmeValidationV1 extension not critical") + a.Error = IncorrectResponseErr(e).ToACME() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) expKeyAuthHash := sha256.Sum256([]byte(expKeyAuth)) cert, err := newTLSALPNValidationCert(expKeyAuthHash[:], false, false, ch.getValue()) assert.FatalError(t, err) - srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: tlsDial, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/extension-malformed": func(t *testing.T) test { + + "processing/extension-malformed": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing - expErr := RejectedIdentifierErr(errors.New("incorrect certificate for tls-alpn-01 challenge: malformed acmeValidationV1 extension value")) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + a := b.clone() + e := errors.New("incorrect certificate for tls-alpn-01 challenge: malformed acmeValidationV1 extension value") + a.Error = IncorrectResponseErr(e).ToACME() jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) cert, err := newTLSALPNValidationCert([]byte{1, 2, 3}, false, true, ch.getValue()) assert.FatalError(t, err) - srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: tlsDial, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/no-protocol": func(t *testing.T) test { - ch, err := newTLSALPNCh() - assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) - - jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) - assert.FatalError(t, err) - - expErr := RejectedIdentifierErr(errors.New("cannot negotiate ALPN acme-tls/1 protocol for tls-alpn-01 challenge")) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) - - srv := httptest.NewTLSServer(nil) - return test{ - srv: srv, - ch: ch, - vo: validateOptions{ - tlsDial: func(network, addr string, config *tls.Config) (*tls.Conn, error) { - return tls.DialWithDialer(&net.Dialer{Timeout: time.Second}, "tcp", srv.Listener.Addr().String(), config) - }, - }, - jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, - } - }, - "ok/mismatched-token": func(t *testing.T) test { + "invalid/mismatched-token": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) expKeyAuthHash := sha256.Sum256([]byte(expKeyAuth)) incorrectTokenHash := sha256.Sum256([]byte("mismatched")) - expErr := RejectedIdentifierErr(errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ + a := b.clone() + e := errors.Errorf("incorrect certificate for tls-alpn-01 challenge: "+ "expected acmeValidationV1 extension value %s for this challenge but got %s", - hex.EncodeToString(expKeyAuthHash[:]), hex.EncodeToString(incorrectTokenHash[:]))) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + hex.EncodeToString(expKeyAuthHash[:]), hex.EncodeToString(incorrectTokenHash[:])) + a.Error = IncorrectResponseErr(e).ToACME() + a.Status = StatusInvalid cert, err := newTLSALPNValidationCert(incorrectTokenHash[:], false, true, ch.getValue()) assert.FatalError(t, err) - srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: tlsDial, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok/obsolete-oid": func(t *testing.T) test { + + "processing/obsolete-oid": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) + b := ch.clone() + b.Status = StatusProcessing jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expErr := RejectedIdentifierErr(errors.New("incorrect certificate for tls-alpn-01 challenge: " + - "obsolete id-pe-acmeIdentifier in acmeValidationV1 extension")) - baseClone := ch.clone() - baseClone.Error = expErr.ToACME() - newCh := &tlsALPN01Challenge{baseClone} - newb, err := json.Marshal(newCh) - assert.FatalError(t, err) + a := b.clone() + e := errors.New("incorrect certificate for tls-alpn-01 challenge: " + + "obsolete id-pe-acmeIdentifier in acmeValidationV1 extension") + a.Error = IncorrectResponseErr(e).ToACME() expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) @@ -1546,92 +1411,63 @@ func TestTLSALPN01Validate(t *testing.T) { cert, err := newTLSALPNValidationCert(expKeyAuthHash[:], true, true, ch.getValue()) assert.FatalError(t, err) - srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: tlsDial, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - assert.Equals(t, string(newval), string(newb)) - return nil, true, nil - }, - }, - res: ch, + res: a.morph(), } }, - "ok": func(t *testing.T) test { + + "valid/expected-identifier": func(t *testing.T) test { ch, err := newTLSALPNCh() assert.FatalError(t, err) - _ch, ok := ch.(*tlsALPN01Challenge) - assert.Fatal(t, ok) - _ch.baseChallenge.Error = MalformedErr(nil).ToACME() - oldb, err := json.Marshal(ch) - assert.FatalError(t, err) - - baseClone := ch.clone() - baseClone.Status = StatusValid - baseClone.Error = nil - newCh := &tlsALPN01Challenge{baseClone} + b := ch.clone() + b.Status = StatusProcessing jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) assert.FatalError(t, err) - expKeyAuth, err := KeyAuthorization(ch.getToken(), jwk) assert.FatalError(t, err) expKeyAuthHash := sha256.Sum256([]byte(expKeyAuth)) + a := b.clone() + a.Validated = clock.Now() + a.Status = StatusValid + a.Error = nil + a.Retry = nil + cert, err := newTLSALPNValidationCert(expKeyAuthHash[:], false, true, ch.getValue()) assert.FatalError(t, err) - srv, tlsDial := newTestTLSALPNServer(cert) srv.Start() return test{ srv: srv, - ch: ch, + ch: b.morph(), vo: validateOptions{ tlsDial: func(network, addr string, config *tls.Config) (conn *tls.Conn, err error) { assert.Equals(t, network, "tcp") - assert.Equals(t, addr, net.JoinHostPort(newCh.getValue(), "443")) + assert.Equals(t, addr, net.JoinHostPort(ch.getValue(), "443")) assert.Equals(t, config.NextProtos, []string{"acme-tls/1"}) - assert.Equals(t, config.ServerName, newCh.getValue()) + assert.Equals(t, config.ServerName, ch.getValue()) assert.True(t, config.InsecureSkipVerify) return tlsDial(network, addr, config) }, }, jwk: jwk, - db: &db.MockNoSQLDB{ - MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { - assert.Equals(t, bucket, challengeTable) - assert.Equals(t, key, []byte(ch.getID())) - assert.Equals(t, old, oldb) - - alpnCh, err := unmarshalChallenge(newval) - assert.FatalError(t, err) - assert.Equals(t, alpnCh.getStatus(), StatusValid) - assert.True(t, alpnCh.getValidated().Before(time.Now().UTC().Add(time.Minute))) - assert.True(t, alpnCh.getValidated().After(time.Now().UTC().Add(-1*time.Second))) - - baseClone.Validated = alpnCh.getValidated() - - return nil, true, nil - }, - }, - res: newCh, + res: a.morph(), } }, } + for name, run := range tests { t.Run(name, func(t *testing.T) { tc := run(t) @@ -1657,7 +1493,16 @@ func TestTLSALPN01Validate(t *testing.T) { assert.Equals(t, tc.res.getToken(), ch.getToken()) assert.Equals(t, tc.res.getCreated(), ch.getCreated()) assert.Equals(t, tc.res.getValidated(), ch.getValidated()) + if tc.res.getValidated() != ch.getValidated() { + now := clock.Now() + window := now.Sub(tc.res.getValidated()) + assert.True(t, now.Sub(ch.getValidated()) <= window, + "validated timestamp should come before now but after test case setup") + } else { + assert.Equals(t, tc.res.getValidated(), ch.getValidated()) + } assert.Equals(t, tc.res.getError(), ch.getError()) + assert.Equals(t, tc.res.getRetry(), ch.getRetry()) } } }) From 6c3943900831699406a9645902e613525ee81b6d Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 20 May 2020 20:41:07 -0700 Subject: [PATCH 34/35] acme: make fmt --- acme/api/handler_test.go | 2 +- acme/authority.go | 6 +++--- ca/ca.go | 1 - commands/app.go | 2 +- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/acme/api/handler_test.go b/acme/api/handler_test.go index 8cbed7afb..3bf73a0e6 100644 --- a/acme/api/handler_test.go +++ b/acme/api/handler_test.go @@ -813,7 +813,7 @@ func TestHandlerGetChallenge(t *testing.T) { assert.Equals(t, res.Header["Cache-Control"], []string{"no-cache"}) assert.Equals(t, res.Header["Retry-After"], []string{tc.ch.RetryAfter}) case "valid", "invalid": - // + // } } else { assert.Fatal(t, false, "Unexpected Status Code") diff --git a/acme/authority.go b/acme/authority.go index 8677274ef..6ebff9e17 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -272,7 +272,7 @@ func (a *Authority) GetAuthz(ctx context.Context, accID, authzID string) (*Authz } // ValidateChallenge loads a challenge resource and then begins the validation process if the challenge -// is not in one of its terminal states {valid|invalid}. +// is not in one of its terminal states {valid|invalid}. // // The challenge validation state machine looks like: // @@ -322,7 +322,7 @@ func (a *Authority) ValidateChallenge(ctx context.Context, accID, chID string, j case StatusInvalid, StatusValid: return ch.toACME(ctx, a.dir) default: - e:= errors.Errorf("unknown challenge state: %s", ch.getStatus()) + e := errors.Errorf("unknown challenge state: %s", ch.getStatus()) return nil, ServerInternalErr(e) } @@ -419,7 +419,7 @@ func (a *Authority) RetryChallenge(chID string) { case StatusProcessing: break default: - e:= errors.Errorf("unknown challenge state: %s", ch.getStatus()) + e := errors.Errorf("unknown challenge state: %s", ch.getStatus()) log.Printf("%v", e) return } diff --git a/ca/ca.go b/ca/ca.go index 18b43d927..469c2a255 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -61,7 +61,6 @@ func WithDatabase(db db.AuthDB) Option { } } - // WithOrdinal sets the server's ordinal identifier (an int). func WithOrdinal(ordinal int) Option { return func(o *options) { diff --git a/commands/app.go b/commands/app.go index 19b09a475..9ab367bf3 100644 --- a/commands/app.go +++ b/commands/app.go @@ -35,7 +35,7 @@ intermediate private key.`, Usage: "address of a DNS resolver to be used instead of the default.", }, cli.IntFlag{ - Name: "ordinal", + Name: "ordinal", Usage: `Unique identifying this instance of step-ca in a highly- available (replicated) deployment.`, }, From 112fc59f46160ccaf18d6d0431b46e99fd9ab1d6 Mon Sep 17 00:00:00 2001 From: David Cowden Date: Wed, 20 May 2020 21:10:27 -0700 Subject: [PATCH 35/35] make: Fix lint errors Add `golanglint-ci` to the modules so it's available when running `make lint`. --- acme/authority.go | 6 ++++++ acme/challenge.go | 6 ------ acme/challenge_test.go | 3 --- go.mod | 7 ++++++- go.sum | 13 +++++++++++++ 5 files changed, 25 insertions(+), 10 deletions(-) diff --git a/acme/authority.go b/acme/authority.go index 6ebff9e17..7f8cccac8 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -454,12 +454,18 @@ func (a *Authority) RetryChallenge(chID string) { ch = up p, err := a.LoadProvisionerByID(retry.ProvisionerID) + if err != nil { + return + } if p.GetType() != provisioner.TypeACME { log.Printf("%v", AccountDoesNotExistErr(errors.New("provisioner must be of type ACME"))) return } ctx := context.WithValue(context.Background(), ProvisionerContextKey, p) acc, err := a.GetAccount(ctx, ch.getAccountID()) + if err != nil { + return + } v, err := a.validate(ch, acc.Key) if err != nil { diff --git a/acme/challenge.go b/acme/challenge.go index 0063c4530..32801ab26 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -251,12 +251,6 @@ func (bc *baseChallenge) validate(jwk *jose.JSONWebKey, vo validateOptions) (cha return nil, ServerInternalErr(errors.New("unimplemented")) } -func (bc *baseChallenge) storeError(db nosql.DB, err *Error) error { - clone := bc.clone() - clone.Error = err.ToACME() - return clone.save(db, bc) -} - // unmarshalChallenge unmarshals a challenge type into the correct sub-type. func unmarshalChallenge(data []byte) (challenge, error) { var getType struct { diff --git a/acme/challenge_test.go b/acme/challenge_test.go index fc6e0323d..56d577089 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -786,7 +786,6 @@ func TestHTTP01Validate(t *testing.T) { ch challenge res challenge jwk *jose.JSONWebKey - db nosql.DB err *Error } tests := map[string]func(t *testing.T) test{ @@ -1048,7 +1047,6 @@ func TestTLSALPN01Validate(t *testing.T) { ch challenge res challenge jwk *jose.JSONWebKey - db nosql.DB err *Error } tests := map[string]func(t *testing.T) test{ @@ -1607,7 +1605,6 @@ func TestDNS01Validate(t *testing.T) { ch challenge res challenge jwk *jose.JSONWebKey - db nosql.DB err *Error } tests := map[string]func(t *testing.T) test{ diff --git a/go.mod b/go.mod index 9cbf14189..5dc25df0b 100644 --- a/go.mod +++ b/go.mod @@ -8,21 +8,26 @@ require ( github.com/go-chi/chi v4.0.2+incompatible github.com/googleapis/gax-go/v2 v2.0.5 github.com/juju/ansiterm v0.0.0-20180109212912-720a0952cc2a // indirect + github.com/kr/text v0.2.0 // indirect github.com/lunixbochs/vtclean v1.0.0 // indirect github.com/newrelic/go-agent v2.15.0+incompatible + github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect github.com/pkg/errors v0.8.1 github.com/rs/xid v1.2.1 github.com/sirupsen/logrus v1.4.2 github.com/smallstep/assert v0.0.0-20200103212524-b99dc1097b15 github.com/smallstep/cli v0.14.3 github.com/smallstep/nosql v0.3.0 + github.com/stretchr/testify v1.5.1 // indirect github.com/urfave/cli v1.22.2 golang.org/x/crypto v0.0.0-20200323165209-0ec3e9974c59 - golang.org/x/net v0.0.0-20191209160850-c0dbc17a3553 + golang.org/x/net v0.0.0-20200226121028-0de0cce0169b google.golang.org/api v0.15.0 google.golang.org/genproto v0.0.0-20191230161307-f3c370f40bfb google.golang.org/grpc v1.26.0 + gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f // indirect gopkg.in/square/go-jose.v2 v2.4.0 + gopkg.in/yaml.v2 v2.2.8 // indirect ) //replace github.com/smallstep/cli => ../cli diff --git a/go.sum b/go.sum index 8bb4b7716..0d6f07a22 100644 --- a/go.sum +++ b/go.sum @@ -86,6 +86,7 @@ github.com/cpuguy83/go-md2man/v2 v2.0.0-20190314233015-f79a8a8ca69d/go.mod h1:ma github.com/cpuguy83/go-md2man/v2 v2.0.0 h1:EoUDS0afbrsXAZ9YQ9jdu/mZ2sXgT1/2yyNng4PGlyM= github.com/cpuguy83/go-md2man/v2 v2.0.0/go.mod h1:maD7wRr/U5Z6m/iR4s+kqSMx2CaBsrgA7czyZG/E6dU= github.com/creack/pty v1.1.7/go.mod h1:lj5s0c3V2DBrqTV7llrYr5NG6My20zk30Fl46Y7DoTY= +github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -300,6 +301,8 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/pty v1.1.8/go.mod h1:O1sed60cT9XZ5uDucP5qwvh+TE3NnUj51EiZO/lmSfw= github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= +github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= +github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/letsencrypt/pkcs11key v2.0.1-0.20170608213348-396559074696+incompatible h1:GfzE+uq7odDW7nOmp1QWuilLEK7kJf8i84XcIfk3mKA= github.com/letsencrypt/pkcs11key v2.0.1-0.20170608213348-396559074696+incompatible/go.mod h1:iGYXKqDXt0cpBthCHdr9ZdsQwyGlYFh/+8xa4WzIQ34= @@ -362,6 +365,8 @@ github.com/nbutton23/zxcvbn-go v0.0.0-20180912185939-ae427f1e4c1d h1:AREM5mwr4u1 github.com/nbutton23/zxcvbn-go v0.0.0-20180912185939-ae427f1e4c1d/go.mod h1:o96djdrsSGy3AWPyBgZMAGfxZNfgntdJG+11KU4QvbU= github.com/newrelic/go-agent v2.15.0+incompatible h1:IB0Fy+dClpBq9aEoIrLyQXzU34JyI1xVTanPLB/+jvU= github.com/newrelic/go-agent v2.15.0+incompatible/go.mod h1:a8Fv1b/fYhFSReoTU6HDkTYIMZeSVNffmoS726Y0LzQ= +github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e h1:fD57ERR4JtEqsWbfPhv4DMiApHyliiK5xCTNVSPiaAs= +github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e/go.mod h1:zD1mROLANZcx1PVRCS0qkT7pwLkGfwJo4zjcN/Tysno= github.com/oklog/ulid v1.3.1/go.mod h1:CirwcVhetQ6Lv90oh/F+FBtV6XMibvdAFo93nm5qn4U= github.com/olekukonko/tablewriter v0.0.1/go.mod h1:vsDQFd/mU46D+Z4whnwzcISnGGzXWMclvtLoiIKAKIo= github.com/olekukonko/tablewriter v0.0.4 h1:vHD/YYe1Wolo78koG299f7V/VAS08c6IpCLn+Ejf/w8= @@ -498,6 +503,8 @@ github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXf github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= +github.com/stretchr/testify v1.5.1 h1:nOGnQDM7FYENwehXlg/kFVnos3rEvtKTjRvOWSzb6H4= +github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA= github.com/subosito/gotenv v1.2.0 h1:Slr1R9HxAlEKefgq5jn9U+DnETlIUa6HfgEzj0g5d7s= github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= github.com/timakin/bodyclose v0.0.0-20190721030226-87058b9bfcec/go.mod h1:Qimiffbc6q9tBWlVV6x0P9sat/ao1xEkREYPPj9hphk= @@ -616,6 +623,8 @@ golang.org/x/net v0.0.0-20190923162816-aa69164e4478/go.mod h1:z5CRVTTTmAJ677TzLL golang.org/x/net v0.0.0-20191002035440-2ec189313ef0/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20191209160850-c0dbc17a3553 h1:efeOvDhwQ29Dj3SdAV/MJf8oukgn+8D8WgaCaRMchF8= golang.org/x/net v0.0.0-20191209160850-c0dbc17a3553/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= +golang.org/x/net v0.0.0-20200226121028-0de0cce0169b h1:0mm1VjtFUOIlE1SbDlwjYaDxZVDP2S5ou6y0gSgXHu8= +golang.org/x/net v0.0.0-20200226121028-0de0cce0169b/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= golang.org/x/oauth2 v0.0.0-20190402181905-9f3314589c9a/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw= @@ -749,6 +758,8 @@ gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8 gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15 h1:YR8cESwS4TdDjEe65xsg0ogRM/Nc3DYOhEAlW+xobZo= gopkg.in/check.v1 v1.0.0-20190902080502-41f04d3bba15/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f h1:BLraFXnmrev5lT+xlilqcH8XK9/i0At2xKjWk4p6zsU= +gopkg.in/check.v1 v1.0.0-20200227125254-8fa46927fb4f/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/cheggaaa/pb.v1 v1.0.28 h1:n1tBJnnK2r7g9OW2btFH91V92STTUevLXYFb8gy9EMk= gopkg.in/cheggaaa/pb.v1 v1.0.28/go.mod h1:V/YB90LKu/1FcN3WVnfiiE5oMCibMjukxqG/qStrOgw= gopkg.in/errgo.v2 v2.1.0/go.mod h1:hNsd1EY+bozCKY1Ytp96fpM3vjJbqLJn88ws8XvfDNI= @@ -770,6 +781,8 @@ gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.5/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.7 h1:VUgggvou5XRW9mHwD/yXxIYSMtY0zoKQf/v226p2nyo= gopkg.in/yaml.v2 v2.2.7/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= +gopkg.in/yaml.v2 v2.2.8 h1:obN1ZagJSUGI0Ek/LBmuj4SNLPfIny3KsKFopxRdj10= +gopkg.in/yaml.v2 v2.2.8/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4= honnef.co/go/tools v0.0.0-20190418001031-e561f6794a2a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=