Skip to content

[RFC] Implement ACME RFC 8555, challenge retries #181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed

[RFC] Implement ACME RFC 8555, challenge retries #181

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
00a8ffa
Implement acme RFC 8555, challenge retries
wesgraham Feb 7, 2020
0c2592d
Add automated challenge retries, RFC 8555
wesgraham Feb 14, 2020
995db67
Merge branch 'master' into wesley/Retry
wesgraham Feb 14, 2020
e44c962
Polish retry conditions
wesgraham Feb 19, 2020
72d1b50
Add retry support for TLSALPN challenge
wesgraham Feb 19, 2020
e03f5aa
Resolve merge conflicts with master
wesgraham Feb 19, 2020
4575049
Implement standard backoff strategy
wesgraham Feb 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions acme/api/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -181,13 +180,16 @@ 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.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
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.
Expand Down
38 changes: 37 additions & 1 deletion acme/api/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
}
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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("<https://ca.smallstep.com/acme/authz/%s>;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)
}
})
}
Expand Down
38 changes: 26 additions & 12 deletions acme/authority.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"crypto/tls"
"crypto/x509"
"encoding/base64"
"math"
"math/rand"
"net"
"net/http"
"net/url"
Expand Down Expand Up @@ -263,21 +265,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),
}
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++ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to loop here until the challenge's status lands on a terminal value or while the retry.Active field is not true rather than counting to 10? The challenge's respective validate funcs already count the number of retries and then declare the challenge invalid after the attempts has expired. Not using a fixed 10 would also allow for certain types of challenges to define longer retry periods. However it means a faulty challenge implementation would hang this loop forever. Another option would be to pull the logic out of the challenge validation funcs and just centralize it here, updating the status to invalid only after 10 tries. What were your thoughts while implementing this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My initial thought was similar - I was trying to avoid a scenario where multiple calls to ValidateChallenge() would loop forever. I think "while retry.active" should get the job done, especially with the lock on that loop. The for range(10) was implemented just in case the retry object state was being modified by any other objects, but I'm seeing that is highly unlikely.

Copy link
Author

@wesgraham wesgraham Feb 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented with while retry.Active

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)
}
Expand Down
3 changes: 1 addition & 2 deletions acme/authority_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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)
}
}
Expand Down
86 changes: 74 additions & 12 deletions acme/challenge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"net"
"net/http"
"strings"
"sync"
"time"

"github.com/pkg/errors"
Expand All @@ -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)
Expand Down Expand Up @@ -75,6 +84,7 @@ type challenge interface {
getID() string
getAuthzID() string
getToken() string
getRetry() *Retry
clone() *baseChallenge
getAccountID() string
getValidated() time.Time
Expand All @@ -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) {
Expand All @@ -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
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still happening?

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)
Expand Down Expand Up @@ -338,19 +374,31 @@ 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)
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of zeroing this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Called" to me represented how many times the challenge validation retry was performed on a specific client call. Resetting to 0 was designed to signal that the current process retrying validation had terminated, and the next retry process should start. Called is also used in computing the retry-after header (alongside the backoffs count).

Copy link
Author

@wesgraham wesgraham Feb 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented an updated version of this in the latest commit - let me know of any thoughts

}
if err = upd.save(db, hc); err != nil {
return nil, err
}
return hc, nil
return hc, err
}

// Update and store the challenge.
upd := &http01Challenge{hc.baseChallenge.clone()}
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
Expand Down Expand Up @@ -559,9 +607,20 @@ 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)
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
}
return dc, nil
Expand All @@ -581,19 +640,22 @@ 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
upd.Error = nil
upd.Validated = time.Now().UTC()
upd.Retry.Active = false

if err := upd.save(db, dc); err != nil {
return nil, err
Expand Down
Loading