From 00a8ffae6097b696786371336c90c2a943fc5e8a Mon Sep 17 00:00:00 2001 From: Wesley Graham Date: Fri, 7 Feb 2020 15:28:10 -0800 Subject: [PATCH 1/4] 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 f0180f64a..f3dfd14fc 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -324,9 +324,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 @@ -393,9 +396,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 @@ -415,14 +421,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 720321e5a..1e25eedfa 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -735,6 +735,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) @@ -908,6 +909,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) @@ -1004,6 +1006,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 0c2592d5e8bd477364b1112b0546b3a538ce4e13 Mon Sep 17 00:00:00 2001 From: Wesley Graham Date: Fri, 14 Feb 2020 13:17:52 -0800 Subject: [PATCH 2/4] Add automated challenge retries, RFC 8555 --- acme/api/handler.go | 5 ++-- acme/api/handler_test.go | 38 +++++++++++++++++++++++++- acme/authority.go | 32 +++++++++++++++++----- acme/authority_test.go | 3 +-- acme/challenge.go | 58 ++++++++++++++++++++++++++++++++++++++-- acme/challenge_test.go | 2 +- 6 files changed, 124 insertions(+), 14 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 3757882f3..22d172e44 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 286a7218a..d00a60dcd 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -4,6 +4,8 @@ import ( "crypto" "crypto/x509" "encoding/base64" + "math" + "math/rand" "net" "net/http" "net/url" @@ -262,15 +264,33 @@ 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), } - ch, err = ch.validate(a.db, jwk, validateOptions{ - httpGet: client.Get, - lookupTxt: net.LookupTXT, - }) - 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, + }) + 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 f3c47966f..574dc3e38 100644 --- a/acme/authority_test.go +++ b/acme/authority_test.go @@ -1260,6 +1260,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{ @@ -1293,12 +1294,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 f3dfd14fc..613bcbd7e 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -9,6 +9,7 @@ import ( "io/ioutil" "net/http" "strings" + "sync" "time" "github.com/pkg/errors" @@ -26,10 +27,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) @@ -68,6 +77,7 @@ type challenge interface { getID() string getAuthzID() string getToken() string + getRetry() *Retry clone() *baseChallenge getAccountID() string getValidated() time.Time @@ -94,6 +104,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) { @@ -113,6 +124,7 @@ func newBaseChallenge(accountID, authzID string) (*baseChallenge, error) { Status: StatusPending, Token: token, Created: clock.Now(), + Retry: &Retry{Called:0, Backoffs:1, Active:false}, }, nil } @@ -151,6 +163,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 @@ -183,6 +200,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 } @@ -234,6 +254,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 { @@ -289,7 +314,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) @@ -329,10 +365,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. @@ -340,6 +384,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 @@ -401,6 +446,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 } @@ -436,6 +489,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 1e25eedfa..5672dc1ed 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -649,7 +649,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() @@ -861,6 +860,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 e44c962c3f4d5b84c378430de19261e9b555ee41 Mon Sep 17 00:00:00 2001 From: Wesley Graham Date: Tue, 18 Feb 2020 17:48:53 -0800 Subject: [PATCH 3/4] 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 22d172e44..33e13d159 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 d00a60dcd..5c0e27388 100644 --- a/acme/authority.go +++ b/acme/authority.go @@ -275,7 +275,7 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin Timeout: time.Duration(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 613bcbd7e..d659f5beb 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -254,11 +254,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 { @@ -362,18 +357,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 @@ -443,18 +427,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 @@ -511,3 +484,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 45750490f45e85c9018bc979616faab39340289c Mon Sep 17 00:00:00 2001 From: Wesley Graham Date: Thu, 20 Feb 2020 17:45:23 -0800 Subject: [PATCH 4/4] Implement standard backoff strategy --- acme/api/handler.go | 10 ++++-- acme/authority.go | 28 ++++++++++++++--- acme/challenge.go | 75 ++++++++++++++++++++++++++++++--------------- 3 files changed, 82 insertions(+), 31 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 c8a3c9a12..8ddf8e0a0 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) @@ -275,7 +275,7 @@ func (a *Authority) ValidateChallenge(p provisioner.Interface, accID, chID strin client := http.Client{ Timeout: time.Duration(30 * time.Second), } - + dialer := &net.Dialer{ Timeout: 30 * time.Second, } @@ -297,8 +297,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) } @@ -314,3 +313,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 +} \ No newline at end of file diff --git a/acme/challenge.go b/acme/challenge.go index a85ea33e4..b0f6c4694 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) { @@ -339,13 +365,13 @@ func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo valida resp, err := vo.httpGet(url) if err != nil { - if err = hc.updateRetry(db, ConnectionErr(errors.Wrapf(err, "error doing http GET for url %s", url))); 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 } if resp.StatusCode >= 400 { - if err = hc.updateRetry(db, ConnectionErr(errors.Errorf("error doing http GET for url %s with status code %d", + 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 } @@ -365,7 +391,7 @@ func (hc *http01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo valida return nil, err } if keyAuth != expected { - if err = hc.updateRetry(db, RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ + if err = hc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("keyAuthorization does not match; "+ "expected %s, but got %s", expected, keyAuth))); err != nil { return nil, err } @@ -378,6 +404,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 @@ -430,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.updateRetry(db, ConnectionErr(errors.Wrapf(err, "error doing TLS dial for %s", hostPort))); 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 @@ -441,7 +468,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val certs := cs.PeerCertificates if len(certs) == 0 { - if err = tc.updateRetry(db, RejectedIdentifierErr(errors.Errorf("%s challenge for %s resulted in no certificates", + 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 } @@ -449,7 +476,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val } if !cs.NegotiatedProtocolIsMutual || cs.NegotiatedProtocol != "acme-tls/1" { - if err = tc.updateRetry(db, RejectedIdentifierErr(errors.Errorf("cannot negotiate ALPN acme-tls/1 protocol for "+ + 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 } @@ -459,7 +486,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.updateRetry(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: "+ "leaf certificate must contain a single DNS name, %v", tc.Value))); err != nil { return nil, err } @@ -479,7 +506,7 @@ 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.updateRetry(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 } @@ -490,7 +517,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.updateRetry(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: "+ "malformed acmeValidationV1 extension value"))); err != nil { return nil, err } @@ -498,7 +525,7 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val } if subtle.ConstantTimeCompare(hashedKeyAuth[:], extValue) != 1 { - if err = tc.updateRetry(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: "+ "expected acmeValidationV1 extension value %s for this challenge but got %s", hex.EncodeToString(hashedKeyAuth[:]), hex.EncodeToString(extValue)))); err != nil { return nil, err @@ -510,6 +537,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 @@ -523,14 +551,14 @@ func (tc *tlsALPN01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo val } if foundIDPeAcmeIdentifierV1Obsolete { - if err = tc.updateRetry(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: "+ "obsolete id-pe-acmeIdentifier in acmeValidationV1 extension"))); err != nil { return nil, err } return tc, nil } - if err = tc.updateRetry(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: "+ "missing acmeValidationV1 extension"))); err != nil { return nil, err } @@ -595,7 +623,7 @@ 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.updateRetry(db, DNSErr(errors.Wrapf(err, "error looking up TXT "+ + if err = dc.iterateRetry(db, DNSErr(errors.Wrapf(err, "error looking up TXT "+ "records for domain %s", domain))); err != nil { return nil, err } @@ -616,7 +644,7 @@ func (dc *dns01Challenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validat } } if !found { - if err = dc.updateRetry(db, RejectedIdentifierErr(errors.Errorf("keyAuthorization "+ + if err = dc.iterateRetry(db, RejectedIdentifierErr(errors.Errorf("keyAuthorization "+ "does not match; expected %s, but got %s", expectedKeyAuth, txtRecords))); err != nil { return nil, err } @@ -628,6 +656,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 @@ -650,18 +679,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