From 20f8d950c473071ad410f981213c7d67e98260a1 Mon Sep 17 00:00:00 2001 From: max furman Date: Fri, 18 Dec 2020 11:17:12 -0500 Subject: [PATCH 1/3] Fix broken ValidateChallenge test --- acme/authority_test.go | 42 ++++++++++++++++++++++++++++++++++++------ acme/challenge.go | 5 ++++- acme/challenge_test.go | 16 ++++++++++++++++ 3 files changed, 56 insertions(+), 7 deletions(-) diff --git a/acme/authority_test.go b/acme/authority_test.go index d411ca06..961805fa 100644 --- a/acme/authority_test.go +++ b/acme/authority_test.go @@ -2,9 +2,14 @@ package acme import ( "context" + "crypto" + "encoding/base64" "encoding/json" "fmt" + "net/http" + "net/http/httptest" "net/url" + "strings" "testing" "time" @@ -1331,11 +1336,14 @@ func TestAuthorityValidateChallenge(t *testing.T) { prov := newProv() ctx := context.WithValue(context.Background(), ProvisionerContextKey, prov) ctx = context.WithValue(ctx, BaseURLContextKey, "https://test.ca.smallstep.com:8080") + type test struct { auth *Authority id, accID string err *Error ch challenge + jwk *jose.JSONWebKey + server *httptest.Server } tests := map[string]func(t *testing.T) test{ "fail/getChallenge-error": func(t *testing.T) test { @@ -1375,8 +1383,24 @@ func TestAuthorityValidateChallenge(t *testing.T) { } }, "fail/validate-error": func(t *testing.T) test { - ch, err := newHTTPCh() + keyauth := "temp" + keyauthp := &keyauth + // Create test server that returns challenge auth + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, "%s\r\n", *keyauthp) + })) + + ch, err := newHTTPChWithServer(strings.TrimPrefix(ts.URL, "http://")) assert.FatalError(t, err) + + jwk, _, err := jose.GenerateDefaultKeyPair([]byte("pass")) + assert.FatalError(t, err) + + thumbprint, err := jwk.Thumbprint(crypto.SHA256) + assert.FatalError(t, err) + encPrint := base64.RawURLEncoding.EncodeToString(thumbprint) + *keyauthp = fmt.Sprintf("%s.%s", ch.getToken(), encPrint) + b, err := json.Marshal(ch) assert.FatalError(t, err) auth, err := NewAuthority(&db.MockNoSQLDB{ @@ -1393,10 +1417,12 @@ func TestAuthorityValidateChallenge(t *testing.T) { }, "ca.smallstep.com", "acme", nil) assert.FatalError(t, err) return test{ - auth: auth, - id: ch.getID(), - accID: ch.getAccountID(), - err: ServerInternalErr(errors.New("error attempting challenge validation: error saving acme challenge: force")), + auth: auth, + id: ch.getID(), + accID: ch.getAccountID(), + jwk: jwk, + server: ts, + err: ServerInternalErr(errors.New("error attempting challenge validation: error saving acme challenge: force")), } }, "ok": func(t *testing.T) test { @@ -1427,7 +1453,11 @@ func TestAuthorityValidateChallenge(t *testing.T) { for name, run := range tests { t.Run(name, func(t *testing.T) { tc := run(t) - if acmeCh, err := tc.auth.ValidateChallenge(ctx, tc.accID, tc.id, nil); err != nil { + + if tc.server != nil { + defer tc.server.Close() + } + if acmeCh, err := tc.auth.ValidateChallenge(ctx, tc.accID, tc.id, tc.jwk); err != nil { if assert.NotNil(t, tc.err) { ae, ok := err.(*Error) assert.True(t, ok) diff --git a/acme/challenge.go b/acme/challenge.go index a032bc00..6d2d13d1 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -238,7 +238,10 @@ func (bc *baseChallenge) validate(db nosql.DB, jwk *jose.JSONWebKey, vo validate func (bc *baseChallenge) storeError(db nosql.DB, err *Error) error { clone := bc.clone() clone.Error = err.ToACME() - return clone.save(db, bc) + if err := clone.save(db, bc); err != nil { + return ServerInternalErr(errors.Wrap(err, "failure saving error to acme challenge")) + } + return nil } // unmarshalChallenge unmarshals a challenge type into the correct sub-type. diff --git a/acme/challenge_test.go b/acme/challenge_test.go index c3d97f9f..87ec0c4c 100644 --- a/acme/challenge_test.go +++ b/acme/challenge_test.go @@ -69,6 +69,22 @@ func newHTTPCh() (challenge, error) { return newHTTP01Challenge(mockdb, testOps) } +func newHTTPChWithServer(host string) (challenge, error) { + mockdb := &db.MockNoSQLDB{ + MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { + return []byte("foo"), true, nil + }, + } + return newHTTP01Challenge(mockdb, ChallengeOptions{ + AccountID: "accID", + AuthzID: "authzID", + Identifier: Identifier{ + Type: "", // will get set correctly depending on the "new.." method. + Value: host, + }, + }) +} + func TestNewHTTP01Challenge(t *testing.T) { ops := ChallengeOptions{ AccountID: "accID", From 1f9aa65d666433b3a30428c652a82c6db51b131d Mon Sep 17 00:00:00 2001 From: max furman Date: Fri, 18 Dec 2020 17:05:25 -0500 Subject: [PATCH 2/3] Add test case --- acme/authority_test.go | 63 +++++++++++++++++++++++++++++++++++------- 1 file changed, 53 insertions(+), 10 deletions(-) diff --git a/acme/authority_test.go b/acme/authority_test.go index 961805fa..04cf90b0 100644 --- a/acme/authority_test.go +++ b/acme/authority_test.go @@ -1389,6 +1389,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, "%s\r\n", *keyauthp) })) + t.Cleanup(func() { ts.Close() }) ch, err := newHTTPChWithServer(strings.TrimPrefix(ts.URL, "http://")) assert.FatalError(t, err) @@ -1425,7 +1426,7 @@ func TestAuthorityValidateChallenge(t *testing.T) { err: ServerInternalErr(errors.New("error attempting challenge validation: 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) @@ -1449,14 +1450,53 @@ func TestAuthorityValidateChallenge(t *testing.T) { ch: ch, } }, + "ok": func(t *testing.T) test { + keyauth := "temp" + keyauthp := &keyauth + // Create test server that returns challenge auth + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, "%s\r\n", *keyauthp) + })) + t.Cleanup(func() { ts.Close() }) + + ch, err := newHTTPChWithServer(strings.TrimPrefix(ts.URL, "http://")) + assert.FatalError(t, err) + + jwk, _, err := jose.GenerateDefaultKeyPair([]byte("pass")) + assert.FatalError(t, err) + + thumbprint, err := jwk.Thumbprint(crypto.SHA256) + assert.FatalError(t, err) + encPrint := base64.RawURLEncoding.EncodeToString(thumbprint) + *keyauthp = fmt.Sprintf("%s.%s", ch.getToken(), encPrint) + + b, err := json.Marshal(ch) + assert.FatalError(t, err) + auth, err := NewAuthority(&db.MockNoSQLDB{ + MGet: func(bucket, key []byte) ([]byte, error) { + assert.Equals(t, bucket, challengeTable) + assert.Equals(t, key, []byte(ch.getID())) + return b, nil + }, + MCmpAndSwap: func(bucket, key, old, newval []byte) ([]byte, bool, error) { + assert.Equals(t, bucket, challengeTable) + assert.Equals(t, key, []byte(ch.getID())) + return nil, true, nil + }, + }, "ca.smallstep.com", "acme", nil) + assert.FatalError(t, err) + return test{ + auth: auth, + id: ch.getID(), + accID: ch.getAccountID(), + jwk: jwk, + server: ts, + } + }, } for name, run := range tests { t.Run(name, func(t *testing.T) { tc := run(t) - - if tc.server != nil { - defer tc.server.Close() - } if acmeCh, err := tc.auth.ValidateChallenge(ctx, tc.accID, tc.id, tc.jwk); err != nil { if assert.NotNil(t, tc.err) { ae, ok := err.(*Error) @@ -1467,15 +1507,18 @@ func TestAuthorityValidateChallenge(t *testing.T) { } } else { if assert.Nil(t, tc.err) { + fmt.Printf("acmeCh = %+v\n", acmeCh) gotb, err := json.Marshal(acmeCh) assert.FatalError(t, err) - acmeExp, err := tc.ch.toACME(ctx, nil, tc.auth.dir) - assert.FatalError(t, err) - expb, err := json.Marshal(acmeExp) - assert.FatalError(t, err) + if tc.ch != nil { + acmeExp, err := tc.ch.toACME(ctx, nil, tc.auth.dir) + assert.FatalError(t, err) + expb, err := json.Marshal(acmeExp) + assert.FatalError(t, err) - assert.Equals(t, expb, gotb) + assert.Equals(t, expb, gotb) + } } } }) From 265d49dbf8e9dabd2759fdd394cf9d16fa968341 Mon Sep 17 00:00:00 2001 From: max furman Date: Fri, 18 Dec 2020 18:17:55 -0500 Subject: [PATCH 3/3] Remove debug statement --- acme/authority_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/acme/authority_test.go b/acme/authority_test.go index 04cf90b0..8861c15e 100644 --- a/acme/authority_test.go +++ b/acme/authority_test.go @@ -1507,7 +1507,6 @@ func TestAuthorityValidateChallenge(t *testing.T) { } } else { if assert.Nil(t, tc.err) { - fmt.Printf("acmeCh = %+v\n", acmeCh) gotb, err := json.Marshal(acmeCh) assert.FatalError(t, err)