From 387209dfb5bc5a907c17d8d158760811b1d32e71 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 24 Aug 2017 15:58:31 -0400 Subject: [PATCH 01/12] Update `google/safebrowsing` lib to tip of master. (#3006) This commit updates the `github.com/google/safebrowsing` dependency to commit f387af, the tip of master at the time of writing. Unit tests were confirmed to pass per CONTRIBUTING.md: ``` $ go test ./... ok github.com/google/safebrowsing 2.500s ? github.com/google/safebrowsing/cmd/sblookup [no test files] ? github.com/google/safebrowsing/cmd/sbserver [no test files] ? github.com/google/safebrowsing/cmd/sbserver/statik [no test files] ? github.com/google/safebrowsing/internal/safebrowsing_proto [no test files] ``` --- Godeps/Godeps.json | 4 +- .../google/safebrowsing/database.go | 25 ++++-- .../google/safebrowsing/safebrowser.go | 78 +++++++++---------- 3 files changed, 59 insertions(+), 48 deletions(-) diff --git a/Godeps/Godeps.json b/Godeps/Godeps.json index 78c14371f..974b63dfc 100644 --- a/Godeps/Godeps.json +++ b/Godeps/Godeps.json @@ -148,11 +148,11 @@ }, { "ImportPath": "github.com/google/safebrowsing", - "Rev": "a8c029efb52bae66853e05241150ab338e98fbc7" + "Rev": "f387afacc9e702b5ed3e90100d3375871e724c08" }, { "ImportPath": "github.com/google/safebrowsing/internal/safebrowsing_proto", - "Rev": "a8c029efb52bae66853e05241150ab338e98fbc7" + "Rev": "f387afacc9e702b5ed3e90100d3375871e724c08" }, { "ImportPath": "github.com/grpc-ecosystem/go-grpc-prometheus", diff --git a/vendor/github.com/google/safebrowsing/database.go b/vendor/github.com/google/safebrowsing/database.go index 025cb7bd6..36434e56a 100644 --- a/vendor/github.com/google/safebrowsing/database.go +++ b/vendor/github.com/google/safebrowsing/database.go @@ -117,16 +117,16 @@ func (db *database) Init(config *Config, logger *log.Logger) bool { db.setError(err) return false } - - // Validate that the database threat list stored on disk is at least a - // superset of the specified configuration. - if db.config.now().Sub(dbf.Time) > (db.config.UpdatePeriod + jitter) { + // Validate that the database threat list stored on disk is not too stale. + if db.isStale(dbf.Time) { db.log.Printf("database loaded is stale") db.ml.Lock() defer db.ml.Unlock() db.setStale() return false } + // Validate that the database threat list stored on disk is at least a + // superset of the specified configuration. tfuNew := make(threatsForUpdate) for _, td := range db.config.ThreatLists { if row, ok := dbf.Table[td]; ok { @@ -142,8 +142,9 @@ func (db *database) Init(config *Config, logger *log.Logger) bool { return true } -// Status reports the health of the database. If in a faulted state, the db -// may repair itself on the next Update. +// Status reports the health of the database. The database is considered faulted +// if there was an error during update or if the last update has gone stale. If +// in a faulted state, the db may repair itself on the next Update. func (db *database) Status() error { db.ml.RLock() defer db.ml.RUnlock() @@ -151,7 +152,7 @@ func (db *database) Status() error { if db.err != nil { return db.err } - if db.config.now().Sub(db.last) > (db.config.UpdatePeriod + jitter) { + if db.isStale(db.last) { db.setStale() return db.err } @@ -306,6 +307,16 @@ func (db *database) setError(err error) { db.ml.Unlock() } +// isStale checks whether the last successful update should be considered stale. +// Staleness is defined as being older than two of the configured update periods +// plus jitter. +func (db *database) isStale(lastUpdate time.Time) bool { + if db.config.now().Sub(lastUpdate) > 2*(db.config.UpdatePeriod+jitter) { + return true + } + return false +} + // setStale sets the error state to a stale message, without clearing // the database state. // diff --git a/vendor/github.com/google/safebrowsing/safebrowser.go b/vendor/github.com/google/safebrowsing/safebrowser.go index f0cdfdb6f..50433a36e 100644 --- a/vendor/github.com/google/safebrowsing/safebrowser.go +++ b/vendor/github.com/google/safebrowsing/safebrowser.go @@ -402,33 +402,34 @@ func (sb *SafeBrowser) LookupURLs(urls []string) (threats [][]URLThreat, err err return threats, err } - // TODO: There are some optimizations to be made here: - // 1.) We could force a database update if it is in error. - // However, we must ensure that we perform some form of rate-limiting. - // 2.) We should batch all of the partial hashes together such that we - // call api.HashLookup only once. + hashes := make(map[hashPrefix]string) + hash2idx := make(map[hashPrefix]int) + + // Construct the follow-up request being made to the server. + // In the request, we only ask for partial hashes for privacy reasons. + req := &pb.FindFullHashesRequest{ + Client: &pb.ClientInfo{ + ClientId: sb.config.ID, + ClientVersion: sb.config.Version, + }, + ThreatInfo: &pb.ThreatInfo{}, + } + ttm := make(map[pb.ThreatType]bool) + ptm := make(map[pb.PlatformType]bool) + tetm := make(map[pb.ThreatEntryType]bool) for i, url := range urls { - hashes, err := generateHashes(url) + urlhashes, err := generateHashes(url) if err != nil { - sb.log.Printf("error generating hashes: %v", err) + sb.log.Printf("error generating urlhashes: %v", err) atomic.AddInt64(&sb.stats.QueriesFail, int64(len(urls)-i)) return threats, err } - // Construct the follow-up request being made to the server. - // In the request, we only ask for partial hashes for privacy reasons. - req := &pb.FindFullHashesRequest{ - Client: &pb.ClientInfo{ - ClientId: sb.config.ID, - ClientVersion: sb.config.Version, - }, - ThreatInfo: &pb.ThreatInfo{}, - } - ttm := make(map[pb.ThreatType]bool) - ptm := make(map[pb.PlatformType]bool) - tetm := make(map[pb.ThreatEntryType]bool) - for fullHash, pattern := range hashes { + for fullHash, pattern := range urlhashes { + hashes[fullHash] = pattern + hash2idx[fullHash] = i + // Lookup in database according to threat list. partialHash, unsureThreats := sb.db.Lookup(fullHash) if len(unsureThreats) == 0 { @@ -451,6 +452,7 @@ func (sb *SafeBrowser) LookupURLs(urls []string) (threats [][]URLThreat, err err }) } } + atomic.AddInt64(&sb.stats.QueriesByCache, 1) case negativeCacheHit: // This is cached as a non-threat. atomic.AddInt64(&sb.stats.QueriesByCache, 1) @@ -467,27 +469,23 @@ func (sb *SafeBrowser) LookupURLs(urls []string) (threats [][]URLThreat, err err &pb.ThreatEntry{Hash: []byte(partialHash)}) } } - for tt := range ttm { - req.ThreatInfo.ThreatTypes = append(req.ThreatInfo.ThreatTypes, tt) - } - for pt := range ptm { - req.ThreatInfo.PlatformTypes = append(req.ThreatInfo.PlatformTypes, pt) - } - for tet := range tetm { - req.ThreatInfo.ThreatEntryTypes = append(req.ThreatInfo.ThreatEntryTypes, tet) - } + } + for tt := range ttm { + req.ThreatInfo.ThreatTypes = append(req.ThreatInfo.ThreatTypes, tt) + } + for pt := range ptm { + req.ThreatInfo.PlatformTypes = append(req.ThreatInfo.PlatformTypes, pt) + } + for tet := range tetm { + req.ThreatInfo.ThreatEntryTypes = append(req.ThreatInfo.ThreatEntryTypes, tet) + } - // All results are known, so just continue. - if len(req.ThreatInfo.ThreatEntries) == 0 { - atomic.AddInt64(&sb.stats.QueriesByCache, 1) - continue - } - - // Actually query the Safe Browsing API for exact full hash matches. + // Actually query the Safe Browsing API for exact full hash matches. + if len(req.ThreatInfo.ThreatEntries) != 0 { resp, err := sb.api.HashLookup(req) if err != nil { sb.log.Printf("HashLookup failure: %v", err) - atomic.AddInt64(&sb.stats.QueriesFail, int64(len(urls)-i)) + atomic.AddInt64(&sb.stats.QueriesFail, 1) return threats, err } @@ -500,7 +498,9 @@ func (sb *SafeBrowser) LookupURLs(urls []string) (threats [][]URLThreat, err err if !fullHash.IsFull() { continue } - if pattern, ok := hashes[fullHash]; ok { + pattern, ok := hashes[fullHash] + idx, findidx := hash2idx[fullHash] + if findidx && ok { td := ThreatDescriptor{ ThreatType: ThreatType(tm.ThreatType), PlatformType: PlatformType(tm.PlatformType), @@ -509,7 +509,7 @@ func (sb *SafeBrowser) LookupURLs(urls []string) (threats [][]URLThreat, err err if !sb.lists[td] { continue } - threats[i] = append(threats[i], URLThreat{ + threats[idx] = append(threats[idx], URLThreat{ Pattern: pattern, ThreatDescriptor: td, }) From d9b0f98f7505266403784891fe0cb50a1dc20469 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Fri, 25 Aug 2017 15:31:32 -0400 Subject: [PATCH 02/12] Use "account" not "registration" throughout WFE2. (#3008) The ACME specification no longer describes "registrations" since this is a fairly overloaded term. Instead the term used is "account". This commit updates the WFE2 & tests throughout to replace occurrences of "reg" and "registration" to use "acct" and "account". NOTE: This change is strictly limited to the wfe2 package. E.g. the RA/SA and many core objects still refer to registrations. Resolves #2986 --- wfe2/verify.go | 2 +- wfe2/verify_test.go | 8 +- wfe2/wfe.go | 199 ++++++++++++++++++++++------------------ wfe2/wfe_test.go | 217 ++++++++++++++++++++++---------------------- 4 files changed, 222 insertions(+), 204 deletions(-) diff --git a/wfe2/verify.go b/wfe2/verify.go index 36cd8f831..11720525a 100644 --- a/wfe2/verify.go +++ b/wfe2/verify.go @@ -375,7 +375,7 @@ func (wfe *WebFrontEndImpl) lookupJWK( header := jws.Signatures[0].Header accountURL := header.KeyID - prefix := wfe.relativeEndpoint(request, regPath) + prefix := wfe.relativeEndpoint(request, acctPath) accountIDStr := strings.TrimPrefix(accountURL, prefix) // Convert the account ID string to an int64 for use with the SA's // GetRegistration RPC diff --git a/wfe2/verify_test.go b/wfe2/verify_test.go index c546ca472..c147cb755 100644 --- a/wfe2/verify_test.go +++ b/wfe2/verify_test.go @@ -133,7 +133,7 @@ func signRequestKeyID( jwk := &jose.JSONWebKey{ Key: privateKey, Algorithm: keyAlgForKey(t, privateKey), - KeyID: fmt.Sprintf("http://localhost/acme/reg/%d", keyID), + KeyID: fmt.Sprintf("http://localhost/acme/acct/%d", keyID), } signerKey := jose.SigningKey{ @@ -994,7 +994,7 @@ func TestLookupJWK(t *testing.T) { Request: makePostRequestWithPath("test-path", errorIDJWSBody), ExpectedProblem: &probs.ProblemDetails{ Type: probs.ServerInternalProblem, - Detail: "Error retreiving account \"http://localhost/acme/reg/100\"", + Detail: "Error retreiving account \"http://localhost/acme/acct/100\"", HTTPStatus: http.StatusInternalServerError, }, ErrorStatType: "JWSKeyIDLookupFailed", @@ -1005,7 +1005,7 @@ func TestLookupJWK(t *testing.T) { Request: makePostRequestWithPath("test-path", missingIDJWSBody), ExpectedProblem: &probs.ProblemDetails{ Type: probs.AccountDoesNotExistProblem, - Detail: "Account \"http://localhost/acme/reg/102\" not found", + Detail: "Account \"http://localhost/acme/acct/102\" not found", HTTPStatus: http.StatusBadRequest, }, ErrorStatType: "JWSKeyIDNotFound", @@ -1234,7 +1234,7 @@ func TestValidPOSTForAccount(t *testing.T) { Request: makePostRequestWithPath("test", missingJWSBody), ExpectedProblem: &probs.ProblemDetails{ Type: probs.AccountDoesNotExistProblem, - Detail: "Account \"http://localhost/acme/reg/102\" not found", + Detail: "Account \"http://localhost/acme/acct/102\" not found", HTTPStatus: http.StatusBadRequest, }, ErrorStatType: "JWSKeyIDNotFound", diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 7b51fb0f9..7594a7f79 100644 --- a/wfe2/wfe.go +++ b/wfe2/wfe.go @@ -35,8 +35,8 @@ import ( // measured_http. const ( directoryPath = "/directory" - newRegPath = "/acme/new-reg" - regPath = "/acme/reg/" + newAcctPath = "/acme/new-acct" + acctPath = "/acme/acct/" authzPath = "/acme/authz/" challengePath = "/acme/challenge/" certPath = "/acme/cert/" @@ -298,8 +298,8 @@ func (wfe *WebFrontEndImpl) relativeDirectory(request *http.Request, directory m func (wfe *WebFrontEndImpl) Handler() http.Handler { m := http.NewServeMux() wfe.HandleFunc(m, directoryPath, wfe.Directory, "GET") - wfe.HandleFunc(m, newRegPath, wfe.NewRegistration, "POST") - wfe.HandleFunc(m, regPath, wfe.Registration, "POST") + wfe.HandleFunc(m, newAcctPath, wfe.NewAccount, "POST") + wfe.HandleFunc(m, acctPath, wfe.Account, "POST") wfe.HandleFunc(m, authzPath, wfe.Authorization, "GET", "POST") wfe.HandleFunc(m, challengePath, wfe.Challenge, "GET", "POST") wfe.HandleFunc(m, certPath, wfe.Certificate, "GET") @@ -368,7 +368,7 @@ func addRequesterHeader(w http.ResponseWriter, requester int64) { // using the `request.Host` of the HTTP request. func (wfe *WebFrontEndImpl) Directory(ctx context.Context, logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { directoryEndpoints := map[string]interface{}{ - "new-reg": newRegPath, + "new-account": newAcctPath, "revoke-cert": revokeCertPath, } @@ -440,10 +440,14 @@ func link(url, relation string) string { return fmt.Sprintf("<%s>;rel=\"%s\"", url, relation) } -// NewRegistration is used by clients to submit a new registration/account -func (wfe *WebFrontEndImpl) NewRegistration(ctx context.Context, logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { +// NewAccount is used by clients to submit a new account +func (wfe *WebFrontEndImpl) NewAccount( + ctx context.Context, + logEvent *requestEvent, + response http.ResponseWriter, + request *http.Request) { - // NewRegistration uses `validSelfAuthenticatedPOST` instead of + // NewAccount uses `validSelfAuthenticatedPOST` instead of // `validPOSTforAccount` because there is no account to authenticate against // until after it is created! body, key, prob := wfe.validSelfAuthenticatedPOST(request, logEvent) @@ -453,10 +457,11 @@ func (wfe *WebFrontEndImpl) NewRegistration(ctx context.Context, logEvent *reque return } - if existingReg, err := wfe.SA.GetRegistrationByKey(ctx, key); err == nil { - response.Header().Set("Location", wfe.relativeEndpoint(request, fmt.Sprintf("%s%d", regPath, existingReg.ID))) - // TODO(#595): check for missing registration err - wfe.sendError(response, logEvent, probs.Conflict("Registration key is already in use"), err) + if existingAcct, err := wfe.SA.GetRegistrationByKey(ctx, key); err == nil { + response.Header().Set("Location", + wfe.relativeEndpoint(request, fmt.Sprintf("%s%d", acctPath, existingAcct.ID))) + // TODO(#595): check for missing account err + wfe.sendError(response, logEvent, probs.Conflict("Account key is already in use"), err) return } @@ -484,37 +489,36 @@ func (wfe *WebFrontEndImpl) NewRegistration(ctx context.Context, logEvent *reque } } - reg, err := wfe.RA.NewRegistration(ctx, init) + acct, err := wfe.RA.NewRegistration(ctx, init) if err != nil { - logEvent.AddError("unable to create new registration: %s", err) - wfe.sendError(response, logEvent, problemDetailsForError(err, "Error creating new registration"), err) + logEvent.AddError("unable to create new account: %s", err) + wfe.sendError(response, logEvent, + problemDetailsForError(err, "Error creating new account"), err) return } - logEvent.Requester = reg.ID - addRequesterHeader(response, reg.ID) - logEvent.Contacts = reg.Contact + logEvent.Requester = acct.ID + addRequesterHeader(response, acct.ID) + logEvent.Contacts = acct.Contact - // Use an explicitly typed variable. Otherwise `go vet' incorrectly complains - // that reg.ID is a string being passed to %d. - regURL := wfe.relativeEndpoint(request, fmt.Sprintf("%s%d", regPath, reg.ID)) + acctURL := wfe.relativeEndpoint(request, fmt.Sprintf("%s%d", acctPath, acct.ID)) - response.Header().Add("Location", regURL) + response.Header().Add("Location", acctURL) if len(wfe.SubscriberAgreementURL) > 0 { response.Header().Add("Link", link(wfe.SubscriberAgreementURL, "terms-of-service")) } - err = wfe.writeJsonResponse(response, logEvent, http.StatusCreated, reg) + err = wfe.writeJsonResponse(response, logEvent, http.StatusCreated, acct) if err != nil { - // ServerInternal because we just created this registration, and it + // ServerInternal because we just created this account, and it // should be OK. - logEvent.AddError("unable to marshal registration: %s", err) - wfe.sendError(response, logEvent, probs.ServerInternal("Error marshaling registration"), err) + logEvent.AddError("unable to marshal account: %s", err) + wfe.sendError(response, logEvent, probs.ServerInternal("Error marshaling account"), err) return } } -func (wfe *WebFrontEndImpl) regHoldsAuthorizations(ctx context.Context, regID int64, names []string) (bool, error) { - authz, err := wfe.SA.GetValidAuthorizations(ctx, regID, names, wfe.clk.Now()) +func (wfe *WebFrontEndImpl) acctHoldsAuthorizations(ctx context.Context, acctID int64, names []string) (bool, error) { + authz, err := wfe.SA.GetValidAuthorizations(ctx, acctID, names, wfe.clk.Now()) if err != nil { return false, err } @@ -539,7 +543,7 @@ func (wfe *WebFrontEndImpl) RevokeCertificate(ctx context.Context, logEvent *req return } -func (wfe *WebFrontEndImpl) logCsr(request *http.Request, cr core.CertificateRequest, registration core.Registration) { +func (wfe *WebFrontEndImpl) logCsr(request *http.Request, cr core.CertificateRequest, account core.Registration) { var csrLog = struct { ClientAddr string CSR string @@ -547,7 +551,7 @@ func (wfe *WebFrontEndImpl) logCsr(request *http.Request, cr core.CertificateReq }{ ClientAddr: getClientAddr(request), CSR: hex.EncodeToString(cr.Bytes), - Registration: registration, + Registration: account, } wfe.log.AuditObject("Certificate request", csrLog) } @@ -672,7 +676,7 @@ func (wfe *WebFrontEndImpl) postChallenge( authz core.Authorization, challengeIndex int, logEvent *requestEvent) { - body, _, currReg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) + body, _, currAcct, prob := wfe.validPOSTForAccount(request, ctx, logEvent) addRequesterHeader(response, logEvent.Requester) if prob != nil { // validPOSTForAccount handles its own setting of logEvent.Errors @@ -680,20 +684,21 @@ func (wfe *WebFrontEndImpl) postChallenge( return } // Any version of the agreement is acceptable here. Version match is enforced in - // wfe.Registration when agreeing the first time. Agreement updates happen - // by mailing subscribers and don't require a registration update. - if currReg.Agreement == "" { - wfe.sendError(response, logEvent, probs.Unauthorized("Registration didn't agree to subscriber agreement before any further actions"), nil) + // wfe.Account when agreeing the first time. Agreement updates happen + // by mailing subscribers and don't require an account update. + if currAcct.Agreement == "" { + wfe.sendError(response, logEvent, + probs.Unauthorized("Account must agree to subscriber agreement before any further actions"), nil) return } - // Check that the registration ID matching the key used matches - // the registration ID on the authz object - if currReg.ID != authz.RegistrationID { - logEvent.AddError("User registration id: %d != Authorization registration id: %v", currReg.ID, authz.RegistrationID) + // Check that the account ID matching the key used matches + // the account ID on the authz object + if currAcct.ID != authz.RegistrationID { + logEvent.AddError("User account id: %d != Authorization account id: %v", currAcct.ID, authz.RegistrationID) wfe.sendError(response, logEvent, - probs.Unauthorized("User registration ID doesn't match registration ID in authorization"), + probs.Unauthorized("User account ID doesn't match account ID in authorization"), nil, ) return @@ -731,13 +736,13 @@ func (wfe *WebFrontEndImpl) postChallenge( } } -// Registration is used by a client to submit an update to their registration. -func (wfe *WebFrontEndImpl) Registration( +// Account is used by a client to submit an update to their account. +func (wfe *WebFrontEndImpl) Account( ctx context.Context, logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { - body, _, currReg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) + body, _, currAcct, prob := wfe.validPOSTForAccount(request, ctx, logEvent) addRequesterHeader(response, logEvent.Requester) if prob != nil { // validPOSTForAccount handles its own setting of logEvent.Errors @@ -746,33 +751,34 @@ func (wfe *WebFrontEndImpl) Registration( } // Requests to this handler should have a path that leads to a known - // registration + // account idStr := request.URL.Path id, err := strconv.ParseInt(idStr, 10, 64) if err != nil { - logEvent.AddError("registration ID must be an integer, was %#v", idStr) - wfe.sendError(response, logEvent, probs.Malformed("Registration ID must be an integer"), err) + logEvent.AddError("account ID must be an integer, was %#v", idStr) + wfe.sendError(response, logEvent, probs.Malformed("Account ID must be an integer"), err) return } else if id <= 0 { - msg := fmt.Sprintf("Registration ID must be a positive non-zero integer, was %d", id) + msg := fmt.Sprintf("Account ID must be a positive non-zero integer, was %d", id) logEvent.AddError(msg) wfe.sendError(response, logEvent, probs.Malformed(msg), nil) return - } else if id != currReg.ID { - logEvent.AddError("Request signing key did not match registration key: %d != %d", id, currReg.ID) - wfe.sendError(response, logEvent, probs.Unauthorized("Request signing key did not match registration key"), nil) + } else if id != currAcct.ID { + logEvent.AddError("Request signing key did not match account key: %d != %d", id, currAcct.ID) + wfe.sendError(response, logEvent, + probs.Unauthorized("Request signing key did not match account key"), nil) return } var update core.Registration err = json.Unmarshal(body, &update) if err != nil { - logEvent.AddError("unable to JSON parse registration: %s", err) - wfe.sendError(response, logEvent, probs.Malformed("Error unmarshaling registration"), err) + logEvent.AddError("unable to JSON parse account: %s", err) + wfe.sendError(response, logEvent, probs.Malformed("Error unmarshaling account"), err) return } - // People *will* POST their full registrations to this endpoint, including + // People *will* POST their full accounts to this endpoint, including // the 'valid' status, to avoid always failing out when that happens only // attempt to deactivate if the provided status is different from their current // status. @@ -780,42 +786,44 @@ func (wfe *WebFrontEndImpl) Registration( // If a user tries to send both a deactivation request and an update to their // contacts or subscriber agreement URL the deactivation will take place and // return before an update would be performed. - if update.Status != "" && update.Status != currReg.Status { + if update.Status != "" && update.Status != currAcct.Status { if update.Status != core.StatusDeactivated { wfe.sendError(response, logEvent, probs.Malformed("Invalid value provided for status field"), nil) return } - wfe.deactivateRegistration(ctx, *currReg, response, request, logEvent) + wfe.deactivateAccount(ctx, *currAcct, response, request, logEvent) return } - // If a user POSTs their registration object including a previously valid + // If a user POSTs their account object including a previously valid // agreement URL but that URL has since changed we will fail out here // since the update agreement URL doesn't match the current URL. To fix that we // only fail if the sent URL doesn't match the currently valid agreement URL - // and it doesn't match the URL currently stored in the registration + // and it doesn't match the URL currently stored in the account // in the database. The RA understands the user isn't actually trying to // update the agreement but since we do an early check here in order to prevent // extraneous requests to the RA we have to add this bypass. - if len(update.Agreement) > 0 && update.Agreement != currReg.Agreement && + if len(update.Agreement) > 0 && update.Agreement != currAcct.Agreement && update.Agreement != wfe.SubscriberAgreementURL { - msg := fmt.Sprintf("Provided agreement URL [%s] does not match current agreement URL [%s]", update.Agreement, wfe.SubscriberAgreementURL) + msg := fmt.Sprintf("Provided agreement URL [%s] does not match current agreement URL [%s]", + update.Agreement, wfe.SubscriberAgreementURL) logEvent.AddError(msg) wfe.sendError(response, logEvent, probs.Malformed(msg), nil) return } - // Registration objects contain a JWK object which are merged in UpdateRegistration - // if it is different from the existing registration key. Since this isn't how you + // Account objects contain a JWK object which are merged in UpdateRegistration + // if it is different from the existing account key. Since this isn't how you // update the key we just copy the existing one into the update object here. This // ensures the key isn't changed and that we can cleanly serialize the update as // JSON to send via RPC to the RA. - update.Key = currReg.Key + update.Key = currAcct.Key - updatedReg, err := wfe.RA.UpdateRegistration(ctx, *currReg, update) + updatedAcct, err := wfe.RA.UpdateRegistration(ctx, *currAcct, update) if err != nil { - logEvent.AddError("unable to update registration: %s", err) - wfe.sendError(response, logEvent, problemDetailsForError(err, "Unable to update registration"), err) + logEvent.AddError("unable to update account: %s", err) + wfe.sendError(response, logEvent, + problemDetailsForError(err, "Unable to update account"), err) return } @@ -823,11 +831,12 @@ func (wfe *WebFrontEndImpl) Registration( response.Header().Add("Link", link(wfe.SubscriberAgreementURL, "terms-of-service")) } - err = wfe.writeJsonResponse(response, logEvent, http.StatusAccepted, updatedReg) + err = wfe.writeJsonResponse(response, logEvent, http.StatusAccepted, updatedAcct) if err != nil { - // ServerInternal because we just generated the reg, it should be OK - logEvent.AddError("unable to marshal updated registration: %s", err) - wfe.sendError(response, logEvent, probs.ServerInternal("Failed to marshal registration"), err) + // ServerInternal because we just generated the account, it should be OK + logEvent.AddError("unable to marshal updated account: %s", err) + wfe.sendError(response, logEvent, + probs.ServerInternal("Failed to marshal account"), err) return } } @@ -838,15 +847,16 @@ func (wfe *WebFrontEndImpl) deactivateAuthorization( logEvent *requestEvent, response http.ResponseWriter, request *http.Request) bool { - body, _, reg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) + body, _, acct, prob := wfe.validPOSTForAccount(request, ctx, logEvent) addRequesterHeader(response, logEvent.Requester) if prob != nil { wfe.sendError(response, logEvent, prob, nil) return false } - if reg.ID != authz.RegistrationID { - logEvent.AddError("registration ID doesn't match ID for authorization") - wfe.sendError(response, logEvent, probs.Unauthorized("Registration ID doesn't match ID for authorization"), nil) + if acct.ID != authz.RegistrationID { + logEvent.AddError("account ID doesn't match ID for authorization") + wfe.sendError(response, logEvent, + probs.Unauthorized("Account ID doesn't match ID for authorization"), nil) return false } var req struct { @@ -1097,7 +1107,7 @@ func (wfe *WebFrontEndImpl) KeyRollover( // Check that the new key isn't the same as the old key. This would fail as // part of the subsequent `wfe.SA.GetRegistrationByKey` check since the new key - // will find the old registration if its equal to the old registration key. We + // will find the old account if its equal to the old account key. We // check new key against old key explicitly to save an RPC round trip and a DB // query for this easy rejection case keysEqual, err := core.PublicKeysEqual(newKey.Key, acct.Key.Key) @@ -1115,8 +1125,10 @@ func (wfe *WebFrontEndImpl) KeyRollover( // Check that the new key isn't already being used for an existing account if existingAcct, err := wfe.SA.GetRegistrationByKey(ctx, &newKey); err != nil { - response.Header().Set("Location", wfe.relativeEndpoint(request, fmt.Sprintf("%s%d", regPath, existingAcct.ID))) - wfe.sendError(response, logEvent, probs.Conflict("New key is already in use for a different account"), err) + response.Header().Set("Location", + wfe.relativeEndpoint(request, fmt.Sprintf("%s%d", acctPath, existingAcct.ID))) + wfe.sendError(response, logEvent, + probs.Conflict("New key is already in use for a different account"), err) return } @@ -1135,20 +1147,27 @@ func (wfe *WebFrontEndImpl) KeyRollover( } } -func (wfe *WebFrontEndImpl) deactivateRegistration(ctx context.Context, reg core.Registration, response http.ResponseWriter, request *http.Request, logEvent *requestEvent) { - err := wfe.RA.DeactivateRegistration(ctx, reg) +func (wfe *WebFrontEndImpl) deactivateAccount( + ctx context.Context, + acct core.Registration, + response http.ResponseWriter, + request *http.Request, + logEvent *requestEvent) { + err := wfe.RA.DeactivateRegistration(ctx, acct) if err != nil { - logEvent.AddError("unable to deactivate registration", err) - wfe.sendError(response, logEvent, problemDetailsForError(err, "Error deactivating registration"), err) + logEvent.AddError("unable to deactivate account", err) + wfe.sendError(response, logEvent, + problemDetailsForError(err, "Error deactivating account"), err) return } - reg.Status = core.StatusDeactivated + acct.Status = core.StatusDeactivated - err = wfe.writeJsonResponse(response, logEvent, http.StatusOK, reg) + err = wfe.writeJsonResponse(response, logEvent, http.StatusOK, acct) if err != nil { - // ServerInternal because registration is from DB and should be fine - logEvent.AddError("unable to marshal updated registration: %s", err) - wfe.sendError(response, logEvent, probs.ServerInternal("Failed to marshal registration"), err) + // ServerInternal because account is from DB and should be fine + logEvent.AddError("unable to marshal updated account: %s", err) + wfe.sendError(response, logEvent, + probs.ServerInternal("Failed to marshal account"), err) return } } @@ -1176,8 +1195,12 @@ type orderJSON struct { } // NewOrder is used by clients to create a new order object from a CSR -func (wfe *WebFrontEndImpl) NewOrder(ctx context.Context, logEvent *requestEvent, response http.ResponseWriter, request *http.Request) { - body, _, reg, prob := wfe.validPOSTForAccount(request, ctx, logEvent) +func (wfe *WebFrontEndImpl) NewOrder( + ctx context.Context, + logEvent *requestEvent, + response http.ResponseWriter, + request *http.Request) { + body, _, acct, prob := wfe.validPOSTForAccount(request, ctx, logEvent) addRequesterHeader(response, logEvent.Requester) if prob != nil { // validPOSTForAccount handles its own setting of logEvent.Errors @@ -1221,7 +1244,7 @@ func (wfe *WebFrontEndImpl) NewOrder(ctx context.Context, logEvent *requestEvent } order, err := wfe.RA.NewOrder(ctx, &rapb.NewOrderRequest{ - RegistrationID: ®.ID, + RegistrationID: &acct.ID, Csr: rawCSR.CSR, }) if err != nil { diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 7e61ad635..7a40864d9 100644 --- a/wfe2/wfe_test.go +++ b/wfe2/wfe_test.go @@ -206,26 +206,26 @@ type MockRegistrationAuthority struct { lastRevocationReason revocation.Reason } -func (ra *MockRegistrationAuthority) NewRegistration(ctx context.Context, reg core.Registration) (core.Registration, error) { - return reg, nil +func (ra *MockRegistrationAuthority) NewRegistration(ctx context.Context, acct core.Registration) (core.Registration, error) { + return acct, nil } -func (ra *MockRegistrationAuthority) NewAuthorization(ctx context.Context, authz core.Authorization, regID int64) (core.Authorization, error) { - authz.RegistrationID = regID +func (ra *MockRegistrationAuthority) NewAuthorization(ctx context.Context, authz core.Authorization, acctID int64) (core.Authorization, error) { + authz.RegistrationID = acctID authz.ID = "bkrPh2u0JUf18-rVBZtOOWWb3GuIiliypL-hBM9Ak1Q" return authz, nil } -func (ra *MockRegistrationAuthority) NewCertificate(ctx context.Context, req core.CertificateRequest, regID int64) (core.Certificate, error) { +func (ra *MockRegistrationAuthority) NewCertificate(ctx context.Context, req core.CertificateRequest, acctID int64) (core.Certificate, error) { return core.Certificate{}, nil } -func (ra *MockRegistrationAuthority) UpdateRegistration(ctx context.Context, reg core.Registration, updated core.Registration) (core.Registration, error) { - keysMatch, _ := core.PublicKeysEqual(reg.Key.Key, updated.Key.Key) +func (ra *MockRegistrationAuthority) UpdateRegistration(ctx context.Context, acct core.Registration, updated core.Registration) (core.Registration, error) { + keysMatch, _ := core.PublicKeysEqual(acct.Key.Key, updated.Key.Key) if !keysMatch { - reg.Key = updated.Key + acct.Key = updated.Key } - return reg, nil + return acct, nil } func (ra *MockRegistrationAuthority) UpdateAuthorization(ctx context.Context, authz core.Authorization, foo int, challenge core.Challenge) (core.Authorization, error) { @@ -660,7 +660,7 @@ func TestDirectory(t *testing.T) { "meta": { "terms-of-service": "http://example.invalid/terms" }, - "new-reg": "http://localhost:4300/acme/new-reg", + "new-account": "http://localhost:4300/acme/new-acct", "revoke-cert": "http://localhost:4300/acme/revoke-cert", "AAAAAAAAAAA": "https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417" }` @@ -699,15 +699,15 @@ func TestRelativeDirectory(t *testing.T) { result string }{ // Test '' (No host header) with no proto header - {"", "", `{"key-change":"http://localhost/acme/key-change","new-reg":"http://localhost/acme/new-reg","revoke-cert":"http://localhost/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, + {"", "", `{"key-change":"http://localhost/acme/key-change","new-account":"http://localhost/acme/new-acct","revoke-cert":"http://localhost/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, // Test localhost:4300 with no proto header - {"localhost:4300", "", `{"key-change":"http://localhost:4300/acme/key-change","new-reg":"http://localhost:4300/acme/new-reg","revoke-cert":"http://localhost:4300/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, + {"localhost:4300", "", `{"key-change":"http://localhost:4300/acme/key-change","new-account":"http://localhost:4300/acme/new-acct","revoke-cert":"http://localhost:4300/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, // Test 127.0.0.1:4300 with no proto header - {"127.0.0.1:4300", "", `{"key-change":"http://127.0.0.1:4300/acme/key-change","new-reg":"http://127.0.0.1:4300/acme/new-reg","revoke-cert":"http://127.0.0.1:4300/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, + {"127.0.0.1:4300", "", `{"key-change":"http://127.0.0.1:4300/acme/key-change","new-account":"http://127.0.0.1:4300/acme/new-acct","revoke-cert":"http://127.0.0.1:4300/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, // Test localhost:4300 with HTTP proto header - {"localhost:4300", "http", `{"key-change":"http://localhost:4300/acme/key-change","new-reg":"http://localhost:4300/acme/new-reg","revoke-cert":"http://localhost:4300/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, + {"localhost:4300", "http", `{"key-change":"http://localhost:4300/acme/key-change","new-account":"http://localhost:4300/acme/new-acct","revoke-cert":"http://localhost:4300/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, // Test localhost:4300 with HTTPS proto header - {"localhost:4300", "https", `{"key-change":"https://localhost:4300/acme/key-change","new-reg":"https://localhost:4300/acme/new-reg","revoke-cert":"https://localhost:4300/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, + {"localhost:4300", "https", `{"key-change":"https://localhost:4300/acme/key-change","new-account":"https://localhost:4300/acme/new-acct","revoke-cert":"https://localhost:4300/acme/revoke-cert","AAAAAAAAAAA":"https://community.letsencrypt.org/t/adding-random-entries-to-the-directory/33417","meta":{"terms-of-service": "http://example.invalid/terms"}}`}, } for _, tt := range dirTests { @@ -758,18 +758,13 @@ func TestHTTPMethods(t *testing.T) { Allowed: getOnly, }, { - Name: "NewReg path should be POST only", - Path: newRegPath, + Name: "NewAcct path should be POST only", + Path: newAcctPath, Allowed: postOnly, }, { - Name: "NewReg path should be POST only", - Path: newRegPath, - Allowed: postOnly, - }, - { - Name: "Reg path should be POST only", - Path: regPath, + Name: "Acct path should be POST only", + Path: acctPath, Allowed: postOnly, }, { @@ -988,12 +983,12 @@ func TestBadNonce(t *testing.T) { responseWriter := httptest.NewRecorder() result, err := signer.Sign([]byte(`{"contact":["mailto:person@mail.com"],"agreement":"` + agreementURL + `"}`)) test.AssertNotError(t, err, "Failed to sign body") - wfe.NewRegistration(ctx, newRequestEvent(), responseWriter, + wfe.NewAccount(ctx, newRequestEvent(), responseWriter, makePostRequestWithPath("nonce", result.FullSerialize())) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{"type":"urn:acme:error:badNonce","detail":"JWS has no anti-replay nonce","status":400}`) } -func TestNewECDSARegistration(t *testing.T) { +func TestNewECDSAAccount(t *testing.T) { wfe, _ := setupWFE(t) // E1 always exists; E2 never exists @@ -1001,25 +996,25 @@ func TestNewECDSARegistration(t *testing.T) { _, ok := key.(*ecdsa.PrivateKey) test.Assert(t, ok, "Couldn't load ECDSA key") - payload := `{"resource":"new-reg","contact":["mailto:person@mail.com"],"agreement":"` + agreementURL + `"}` - path := "new-reg" - signedURL := fmt.Sprintf("http://localhost/%s", path) + payload := `{"contact":["mailto:person@mail.com"],"agreement":"` + agreementURL + `"}` + path := newAcctPath + signedURL := fmt.Sprintf("http://localhost%s", path) _, _, body := signRequestEmbed(t, key, signedURL, payload, wfe.nonceService) request := makePostRequestWithPath(path, body) responseWriter := httptest.NewRecorder() - wfe.NewRegistration(ctx, newRequestEvent(), responseWriter, request) + wfe.NewAccount(ctx, newRequestEvent(), responseWriter, request) - var reg core.Registration + var acct core.Registration responseBody := responseWriter.Body.String() - err := json.Unmarshal([]byte(responseBody), ®) - test.AssertNotError(t, err, "Couldn't unmarshal returned registration object") - test.Assert(t, len(*reg.Contact) >= 1, "No contact field in registration") - test.AssertEquals(t, (*reg.Contact)[0], "mailto:person@mail.com") - test.AssertEquals(t, reg.Agreement, "http://example.invalid/terms") - test.AssertEquals(t, reg.InitialIP.String(), "1.1.1.1") + err := json.Unmarshal([]byte(responseBody), &acct) + test.AssertNotError(t, err, "Couldn't unmarshal returned account object") + test.Assert(t, len(*acct.Contact) >= 1, "No contact field in account") + test.AssertEquals(t, (*acct.Contact)[0], "mailto:person@mail.com") + test.AssertEquals(t, acct.Agreement, "http://example.invalid/terms") + test.AssertEquals(t, acct.InitialIP.String(), "1.1.1.1") - test.AssertEquals(t, responseWriter.Header().Get("Location"), "http://localhost/acme/reg/0") + test.AssertEquals(t, responseWriter.Header().Get("Location"), "http://localhost/acme/acct/0") key = loadKey(t, []byte(testE1KeyPrivatePEM)) _, ok = key.(*ecdsa.PrivateKey) @@ -1031,23 +1026,23 @@ func TestNewECDSARegistration(t *testing.T) { // Reset the body and status code responseWriter = httptest.NewRecorder() // POST, Valid JSON, Key already in use - wfe.NewRegistration(ctx, newRequestEvent(), responseWriter, request) + wfe.NewAccount(ctx, newRequestEvent(), responseWriter, request) responseBody = responseWriter.Body.String() - test.AssertUnmarshaledEquals(t, responseBody, `{"type":"urn:acme:error:malformed","detail":"Registration key is already in use","status":409}`) - test.AssertEquals(t, responseWriter.Header().Get("Location"), "http://localhost/acme/reg/3") + test.AssertUnmarshaledEquals(t, responseBody, `{"type":"urn:acme:error:malformed","detail":"Account key is already in use","status":409}`) + test.AssertEquals(t, responseWriter.Header().Get("Location"), "http://localhost/acme/acct/3") test.AssertEquals(t, responseWriter.Code, 409) } // Test that the WFE handling of the "empty update" POST is correct. The ACME // spec describes how when clients wish to query the server for information -// about a registration an empty registration update should be sent, and -// a populated reg object will be returned. -func TestEmptyRegistration(t *testing.T) { +// about an account an empty account update should be sent, and +// a populated acct object will be returned. +func TestEmptyAccount(t *testing.T) { wfe, _ := setupWFE(t) responseWriter := httptest.NewRecorder() // Test Key 1 is mocked in the mock StorageAuthority used in setupWFE to - // return a populated registration for GetRegistrationByKey when test key 1 is + // return a populated account for GetRegistrationByKey when test key 1 is // used. key := loadKey(t, []byte(test1KeyPrivatePEM)) _, ok := key.(*rsa.PrivateKey) @@ -1059,8 +1054,8 @@ func TestEmptyRegistration(t *testing.T) { _, _, body := signRequestKeyID(t, 1, key, signedURL, payload, wfe.nonceService) request := makePostRequestWithPath(path, body) - // Send a registration update with the trivial body - wfe.Registration( + // Send an account update with the trivial body + wfe.Account( ctx, newRequestEvent(), responseWriter, @@ -1070,44 +1065,44 @@ func TestEmptyRegistration(t *testing.T) { // There should be no error test.AssertNotContains(t, responseBody, "urn:acme:error") - // We should get back a populated Registration - var reg core.Registration - err := json.Unmarshal([]byte(responseBody), ®) - test.AssertNotError(t, err, "Couldn't unmarshal returned registration object") - test.Assert(t, len(*reg.Contact) >= 1, "No contact field in registration") - test.AssertEquals(t, (*reg.Contact)[0], "mailto:person@mail.com") - test.AssertEquals(t, reg.Agreement, "http://example.invalid/terms") + // We should get back a populated Account + var acct core.Registration + err := json.Unmarshal([]byte(responseBody), &acct) + test.AssertNotError(t, err, "Couldn't unmarshal returned account object") + test.Assert(t, len(*acct.Contact) >= 1, "No contact field in account") + test.AssertEquals(t, (*acct.Contact)[0], "mailto:person@mail.com") + test.AssertEquals(t, acct.Agreement, "http://example.invalid/terms") responseWriter.Body.Reset() } -func TestNewRegistration(t *testing.T) { +func TestNewAccount(t *testing.T) { wfe, _ := setupWFE(t) mux := wfe.Handler() key := loadKey(t, []byte(test2KeyPrivatePEM)) _, ok := key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load test2 key") - path := newRegPath - signedURL := fmt.Sprintf("http://localhost%s", newRegPath) + path := newAcctPath + signedURL := fmt.Sprintf("http://localhost%s", path) - wrongAgreementReg := `{"contact":["mailto:person@mail.com"],"agreement":"https://letsencrypt.org/im-bad"}` - // A reg with the wrong agreement URL - _, _, wrongAgreementBody := signRequestEmbed(t, key, signedURL, wrongAgreementReg, wfe.nonceService) + wrongAgreementAcct := `{"contact":["mailto:person@mail.com"],"agreement":"https://letsencrypt.org/im-bad"}` + // An acct with the wrong agreement URL + _, _, wrongAgreementBody := signRequestEmbed(t, key, signedURL, wrongAgreementAcct, wfe.nonceService) // A non-JSON payload _, _, fooBody := signRequestEmbed(t, key, signedURL, `foo`, wfe.nonceService) - type newRegErrorTest struct { + type newAcctErrorTest struct { r *http.Request respBody string } - regErrTests := []newRegErrorTest{ + acctErrTests := []newAcctErrorTest{ // POST, but no body. { &http.Request{ Method: "POST", - URL: mustParseURL(newRegPath), + URL: mustParseURL(newAcctPath), Header: map[string][]string{ "Content-Length": {"0"}, }, @@ -1117,29 +1112,29 @@ func TestNewRegistration(t *testing.T) { // POST, but body that isn't valid JWS { - makePostRequestWithPath(newRegPath, "hi"), + makePostRequestWithPath(newAcctPath, "hi"), `{"type":"urn:acme:error:malformed","detail":"Parse error reading JWS","status":400}`, }, // POST, Properly JWS-signed, but payload is "foo", not base64-encoded JSON. { - makePostRequestWithPath(newRegPath, fooBody), + makePostRequestWithPath(newAcctPath, fooBody), `{"type":"urn:acme:error:malformed","detail":"Request payload did not parse as JSON","status":400}`, }, // Same signed body, but payload modified by one byte, breaking signature. // should fail JWS verification. { - makePostRequestWithPath(newRegPath, + makePostRequestWithPath(newAcctPath, `{"payload":"Zm9x","protected":"eyJhbGciOiJSUzI1NiIsImp3ayI6eyJrdHkiOiJSU0EiLCJuIjoicW5BUkxyVDdYejRnUmNLeUxkeWRtQ3ItZXk5T3VQSW1YNFg0MHRoazNvbjI2RmtNem5SM2ZSanM2NmVMSzdtbVBjQlo2dU9Kc2VVUlU2d0FhWk5tZW1vWXgxZE12cXZXV0l5aVFsZUhTRDdROHZCcmhSNnVJb080akF6SlpSLUNoelp1U0R0N2lITi0zeFVWc3B1NVhHd1hVX01WSlpzaFR3cDRUYUZ4NWVsSElUX09iblR2VE9VM1hoaXNoMDdBYmdaS21Xc1ZiWGg1cy1DcklpY1U0T2V4SlBndW5XWl9ZSkp1ZU9LbVR2bkxsVFY0TXpLUjJvWmxCS1oyN1MwLVNmZFZfUUR4X3lkbGU1b01BeUtWdGxBVjM1Y3lQTUlzWU53Z1VHQkNkWV8yVXppNWVYMGxUYzdNUFJ3ejZxUjFraXAtaTU5VmNHY1VRZ3FIVjZGeXF3IiwiZSI6IkFRQUIifSwia2lkIjoiIiwibm9uY2UiOiJyNHpuenZQQUVwMDlDN1JwZUtYVHhvNkx3SGwxZVBVdmpGeXhOSE1hQnVvIiwidXJsIjoiaHR0cDovL2xvY2FsaG9zdC9hY21lL25ldy1yZWcifQ","signature":"jcTdxSygm_cvD7KbXqsxgnoPApCTSkV4jolToSOd2ciRkg5W7Yl0ZKEEKwOc-dYIbQiwGiDzisyPCicwWsOUA1WSqHylKvZ3nxSMc6KtwJCW2DaOqcf0EEjy5VjiZJUrOt2c-r6b07tbn8sfOJKwlF2lsOeGi4s-rtvvkeQpAU-AWauzl9G4bv2nDUeCviAZjHx_PoUC-f9GmZhYrbDzAvXZ859ktM6RmMeD0OqPN7bhAeju2j9Gl0lnryZMtq2m0J2m1ucenQBL1g4ZkP1JiJvzd2cAz5G7Ftl2YeJJyWhqNd3qq0GVOt1P11s8PTGNaSoM0iR9QfUxT9A6jxARtg"}`), `{"type":"urn:acme:error:malformed","detail":"JWS verification error","status":400}`, }, { - makePostRequestWithPath(newRegPath, wrongAgreementBody), + makePostRequestWithPath(newAcctPath, wrongAgreementBody), `{"type":"urn:acme:error:malformed","detail":"Provided agreement URL [https://letsencrypt.org/im-bad] does not match current agreement URL [` + agreementURL + `]","status":400}`, }, } - for _, rt := range regErrTests { + for _, rt := range acctErrTests { responseWriter := httptest.NewRecorder() mux.ServeHTTP(responseWriter, rt.r) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), rt.respBody) @@ -1151,20 +1146,20 @@ func TestNewRegistration(t *testing.T) { _, _, body := signRequestEmbed(t, key, signedURL, payload, wfe.nonceService) request := makePostRequestWithPath(path, body) - wfe.NewRegistration(ctx, newRequestEvent(), responseWriter, request) + wfe.NewAccount(ctx, newRequestEvent(), responseWriter, request) - var reg core.Registration + var acct core.Registration responseBody := responseWriter.Body.String() - err := json.Unmarshal([]byte(responseBody), ®) - test.AssertNotError(t, err, "Couldn't unmarshal returned registration object") - test.Assert(t, len(*reg.Contact) >= 1, "No contact field in registration") - test.AssertEquals(t, (*reg.Contact)[0], "mailto:person@mail.com") - test.AssertEquals(t, reg.Agreement, "http://example.invalid/terms") - test.AssertEquals(t, reg.InitialIP.String(), "1.1.1.1") + err := json.Unmarshal([]byte(responseBody), &acct) + test.AssertNotError(t, err, "Couldn't unmarshal returned account object") + test.Assert(t, len(*acct.Contact) >= 1, "No contact field in account") + test.AssertEquals(t, (*acct.Contact)[0], "mailto:person@mail.com") + test.AssertEquals(t, acct.Agreement, "http://example.invalid/terms") + test.AssertEquals(t, acct.InitialIP.String(), "1.1.1.1") test.AssertEquals( t, responseWriter.Header().Get("Location"), - "http://localhost/acme/reg/0") + "http://localhost/acme/acct/0") links := responseWriter.Header()["Link"] test.AssertEquals(t, contains(links, "<"+agreementURL+">;rel=\"terms-of-service\""), true) @@ -1179,13 +1174,13 @@ func TestNewRegistration(t *testing.T) { _, _, body = signRequestEmbed(t, key, signedURL, payload, wfe.nonceService) request = makePostRequestWithPath(path, body) - wfe.NewRegistration(ctx, newRequestEvent(), responseWriter, request) + wfe.NewAccount(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), - `{"type":"urn:acme:error:malformed","detail":"Registration key is already in use","status":409}`) + `{"type":"urn:acme:error:malformed","detail":"Account key is already in use","status":409}`) test.AssertEquals( t, responseWriter.Header().Get("Location"), - "http://localhost/acme/reg/1") + "http://localhost/acme/acct/1") test.AssertEquals(t, responseWriter.Code, 409) } @@ -1247,7 +1242,7 @@ func contains(s []string, e string) bool { return false } -func TestRegistration(t *testing.T) { +func TestAccount(t *testing.T) { wfe, _ := setupWFE(t) mux := wfe.Handler() responseWriter := httptest.NewRecorder() @@ -1255,7 +1250,7 @@ func TestRegistration(t *testing.T) { // Test GET proper entry returns 405 mux.ServeHTTP(responseWriter, &http.Request{ Method: "GET", - URL: mustParseURL(regPath), + URL: mustParseURL(acctPath), }) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), @@ -1263,7 +1258,7 @@ func TestRegistration(t *testing.T) { responseWriter.Body.Reset() // Test POST invalid JSON - wfe.Registration(ctx, newRequestEvent(), responseWriter, makePostRequestWithPath("2", "invalid")) + wfe.Account(ctx, newRequestEvent(), responseWriter, makePostRequestWithPath("2", "invalid")) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{"type":"urn:acme:error:malformed","detail":"Parse error reading JWS","status":400}`) @@ -1273,68 +1268,68 @@ func TestRegistration(t *testing.T) { _, ok := key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load RSA key") - signedURL := fmt.Sprintf("http://localhost%s%d", regPath, 102) - path := fmt.Sprintf("%s%d", regPath, 102) - payload := `{"resource":"reg","agreement":"` + agreementURL + `"}` - // ID 102 is used by the mock for missing reg + signedURL := fmt.Sprintf("http://localhost%s%d", acctPath, 102) + path := fmt.Sprintf("%s%d", acctPath, 102) + payload := `{"agreement":"` + agreementURL + `"}` + // ID 102 is used by the mock for missing acct _, _, body := signRequestKeyID(t, 102, nil, signedURL, payload, wfe.nonceService) request := makePostRequestWithPath(path, body) // Test POST valid JSON but key is not registered - wfe.Registration(ctx, newRequestEvent(), responseWriter, request) + wfe.Account(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), - `{"type":"urn:ietf:params:acme:error:accountDoesNotExist","detail":"Account \"http://localhost/acme/reg/102\" not found","status":400}`) + `{"type":"urn:ietf:params:acme:error:accountDoesNotExist","detail":"Account \"http://localhost/acme/acct/102\" not found","status":400}`) responseWriter.Body.Reset() key = loadKey(t, []byte(test1KeyPrivatePEM)) _, ok = key.(*rsa.PrivateKey) test.Assert(t, ok, "Couldn't load RSA key") - // Test POST valid JSON with registration up in the mock (with incorrect agreement URL) + // Test POST valid JSON with account up in the mock (with incorrect agreement URL) payload = `{"agreement":"https://letsencrypt.org/im-bad"}` path = "1" signedURL = "http://localhost/1" _, _, body = signRequestKeyID(t, 1, nil, signedURL, payload, wfe.nonceService) request = makePostRequestWithPath(path, body) - wfe.Registration(ctx, newRequestEvent(), responseWriter, request) + wfe.Account(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{"type":"urn:acme:error:malformed","detail":"Provided agreement URL [https://letsencrypt.org/im-bad] does not match current agreement URL [`+agreementURL+`]","status":400}`) responseWriter.Body.Reset() - // Test POST valid JSON with registration up in the mock (with correct agreement URL) - payload = `{"resource":"reg","agreement":"` + agreementURL + `"}` + // Test POST valid JSON with account up in the mock (with correct agreement URL) + payload = `{"agreement":"` + agreementURL + `"}` _, _, body = signRequestKeyID(t, 1, nil, signedURL, payload, wfe.nonceService) request = makePostRequestWithPath(path, body) - wfe.Registration(ctx, newRequestEvent(), responseWriter, request) + wfe.Account(ctx, newRequestEvent(), responseWriter, request) test.AssertNotContains(t, responseWriter.Body.String(), "urn:acme:error") links := responseWriter.Header()["Link"] test.AssertEquals(t, contains(links, "<"+agreementURL+">;rel=\"terms-of-service\""), true) responseWriter.Body.Reset() - // Test POST valid JSON with garbage in URL but valid registration ID - payload = `{"resource":"reg","agreement":"` + agreementURL + `"}` + // Test POST valid JSON with garbage in URL but valid account ID + payload = `{"agreement":"` + agreementURL + `"}` signedURL = "http://localhost/a/bunch/of/garbage/1" _, _, body = signRequestKeyID(t, 1, nil, signedURL, payload, wfe.nonceService) request = makePostRequestWithPath("/a/bunch/of/garbage/1", body) - wfe.Registration(ctx, newRequestEvent(), responseWriter, request) + wfe.Account(ctx, newRequestEvent(), responseWriter, request) test.AssertContains(t, responseWriter.Body.String(), "400") test.AssertContains(t, responseWriter.Body.String(), "urn:acme:error:malformed") responseWriter.Body.Reset() - // Test POST valid JSON with registration up in the mock (with old agreement URL) + // Test POST valid JSON with account up in the mock (with old agreement URL) responseWriter.HeaderMap = http.Header{} wfe.SubscriberAgreementURL = "http://example.invalid/new-terms" - payload = `{"resource":"reg","agreement":"` + agreementURL + `"}` + payload = `{"agreement":"` + agreementURL + `"}` signedURL = "http://localhost/1" _, _, body = signRequestKeyID(t, 1, nil, signedURL, payload, wfe.nonceService) request = makePostRequestWithPath(path, body) - wfe.Registration(ctx, newRequestEvent(), responseWriter, request) + wfe.Account(ctx, newRequestEvent(), responseWriter, request) test.AssertNotContains(t, responseWriter.Body.String(), "urn:acme:error") links = responseWriter.Header()["Link"] test.AssertEquals(t, contains(links, ";rel=\"terms-of-service\""), true) @@ -1530,7 +1525,7 @@ func TestHeaderBoulderRequester(t *testing.T) { test.Assert(t, ok, "Failed to load test 1 RSA key") payload := `{"agreement":"` + agreementURL + `"}` - path := fmt.Sprintf("%s%d", regPath, 1) + path := fmt.Sprintf("%s%d", acctPath, 1) signedURL := fmt.Sprintf("http://localhost%s", path) _, _, body := signRequestKeyID(t, 1, nil, signedURL, payload, wfe.nonceService) request := makePostRequestWithPath(path, body) @@ -1588,7 +1583,7 @@ func TestDeactivateAuthorization(t *testing.T) { }`) } -func TestDeactivateRegistration(t *testing.T) { +func TestDeactivateAccount(t *testing.T) { responseWriter := httptest.NewRecorder() wfe, _ := setupWFE(t) @@ -1599,7 +1594,7 @@ func TestDeactivateRegistration(t *testing.T) { _, _, body := signRequestKeyID(t, 1, nil, signedURL, payload, wfe.nonceService) request := makePostRequestWithPath(path, body) - wfe.Registration(ctx, newRequestEvent(), responseWriter, request) + wfe.Account(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{"type": "urn:acme:error:malformed","detail": "Invalid value provided for status field","status": 400}`) @@ -1609,7 +1604,7 @@ func TestDeactivateRegistration(t *testing.T) { _, _, body = signRequestKeyID(t, 1, nil, signedURL, payload, wfe.nonceService) request = makePostRequestWithPath(path, body) - wfe.Registration(ctx, newRequestEvent(), responseWriter, request) + wfe.Account(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{ @@ -1632,7 +1627,7 @@ func TestDeactivateRegistration(t *testing.T) { payload = `{"status":"deactivated", "contact":[]}` _, _, body = signRequestKeyID(t, 1, nil, signedURL, payload, wfe.nonceService) request = makePostRequestWithPath(path, body) - wfe.Registration(ctx, newRequestEvent(), responseWriter, request) + wfe.Account(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), `{ @@ -1662,7 +1657,7 @@ func TestDeactivateRegistration(t *testing.T) { _, _, body = signRequestKeyID(t, 3, key, signedURL, payload, wfe.nonceService) request = makePostRequestWithPath(path, body) - wfe.Registration(ctx, newRequestEvent(), responseWriter, request) + wfe.Account(ctx, newRequestEvent(), responseWriter, request) test.AssertUnmarshaledEquals(t, responseWriter.Body.String(), @@ -1787,7 +1782,7 @@ func TestKeyRollover(t *testing.T) { Payload: `{"newKey":` + string(newJWKJSON) + `}`, ExpectedResponse: `{ "type": "urn:acme:error:malformed", - "detail": "Inner key rollover request specified Account \"\", but outer JWS has Key ID \"http://localhost/acme/reg/1\"", + "detail": "Inner key rollover request specified Account \"\", but outer JWS has Key ID \"http://localhost/acme/acct/1\"", "status": 400 }`, NewKey: newKey, @@ -1795,7 +1790,7 @@ func TestKeyRollover(t *testing.T) { }, { Name: "Missing new key from inner payload", - Payload: `{"account":"http://localhost/acme/reg/1"}`, + Payload: `{"account":"http://localhost/acme/acct/1"}`, ExpectedResponse: `{ "type": "urn:acme:error:malformed", "detail": "Inner JWS does not verify with specified new key", @@ -1805,7 +1800,7 @@ func TestKeyRollover(t *testing.T) { }, { Name: "New key is the same as the old key", - Payload: `{"newKey":{"kty":"RSA","n":"yNWVhtYEKJR21y9xsHV-PD_bYwbXSeNuFal46xYxVfRL5mqha7vttvjB_vc7Xg2RvgCxHPCqoxgMPTzHrZT75LjCwIW2K_klBYN8oYvTwwmeSkAz6ut7ZxPv-nZaT5TJhGk0NT2kh_zSpdriEJ_3vW-mqxYbbBmpvHqsa1_zx9fSuHYctAZJWzxzUZXykbWMWQZpEiE0J4ajj51fInEzVn7VxV-mzfMyboQjujPh7aNJxAWSq4oQEJJDgWwSh9leyoJoPpONHxh5nEE5AjE01FkGICSxjpZsF-w8hOTI3XXohUdu29Se26k2B0PolDSuj0GIQU6-W9TdLXSjBb2SpQ","e":"AQAB"},"account":"http://localhost/acme/reg/1"}`, + Payload: `{"newKey":{"kty":"RSA","n":"yNWVhtYEKJR21y9xsHV-PD_bYwbXSeNuFal46xYxVfRL5mqha7vttvjB_vc7Xg2RvgCxHPCqoxgMPTzHrZT75LjCwIW2K_klBYN8oYvTwwmeSkAz6ut7ZxPv-nZaT5TJhGk0NT2kh_zSpdriEJ_3vW-mqxYbbBmpvHqsa1_zx9fSuHYctAZJWzxzUZXykbWMWQZpEiE0J4ajj51fInEzVn7VxV-mzfMyboQjujPh7aNJxAWSq4oQEJJDgWwSh9leyoJoPpONHxh5nEE5AjE01FkGICSxjpZsF-w8hOTI3XXohUdu29Se26k2B0PolDSuj0GIQU6-W9TdLXSjBb2SpQ","e":"AQAB"},"account":"http://localhost/acme/acct/1"}`, ExpectedResponse: `{ "type": "urn:acme:error:malformed", "detail": "New key specified by rollover request is the same as the old key", @@ -1815,7 +1810,7 @@ func TestKeyRollover(t *testing.T) { }, { Name: "Inner JWS signed by the wrong key", - Payload: `{"newKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/reg/1"}`, + Payload: `{"newKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/acct/1"}`, ExpectedResponse: `{ "type": "urn:acme:error:malformed", "detail": "Inner JWS does not verify with specified new key", @@ -1825,7 +1820,7 @@ func TestKeyRollover(t *testing.T) { }, { Name: "Valid key rollover request", - Payload: `{"newKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/reg/1"}`, + Payload: `{"newKey":` + string(newJWKJSON) + `,"account":"http://localhost/acme/acct/1"}`, ExpectedResponse: `{ "id": 1, "key": ` + string(newJWKJSON) + `, From 9026f6cbf80505a76ed215f9f8332a01c7856d3e Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 25 Aug 2017 16:55:38 -0700 Subject: [PATCH 03/12] Remove global state from VA test (#3009) The VA test had a global: `var ident = core.AcmeIdentifier{Type: core.IdentifierDNS, Value: "localhost"}` Evidently this was meant as a convenience to avoid having to retype this common value, but it wound up being mutated independently by different tests. This PR replaces it with a convenience function `dnsi()` that generates a DNS-type identifier with the given hostname. Makes the VA test much more reliable locally. --- va/va_test.go | 125 ++++++++++++++++++++++---------------------------- 1 file changed, 54 insertions(+), 71 deletions(-) diff --git a/va/va_test.go b/va/va_test.go index 06f1aff0f..51b4bb8c6 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -68,7 +68,10 @@ var TheKey = rsa.PrivateKey{ var accountKey = &jose.JSONWebKey{Key: TheKey.Public()} -var ident = core.AcmeIdentifier{Type: core.IdentifierDNS, Value: "localhost"} +// Return an ACME DNS identifier for the given hostname +func dnsi(hostname string) core.AcmeIdentifier { + return core.AcmeIdentifier{Type: core.IdentifierDNS, Value: hostname} +} var ctx = context.Background() @@ -238,7 +241,7 @@ func TestHTTPBadPort(t *testing.T) { badPort := 40000 + mrand.Intn(25000) va.httpPort = badPort - _, prob := va.validateHTTP01(ctx, ident, chall) + _, prob := va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob == nil { t.Fatalf("Server's down; expected refusal. Where did we connect?") } @@ -266,7 +269,7 @@ func TestHTTP(t *testing.T) { log.Clear() t.Logf("Trying to validate: %+v\n", chall) - _, prob := va.validateHTTP01(ctx, ident, chall) + _, prob := va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob != nil { t.Errorf("Unexpected failure in HTTP validation: %s", prob) } @@ -274,7 +277,7 @@ func TestHTTP(t *testing.T) { log.Clear() setChallengeToken(&chall, path404) - _, prob = va.validateHTTP01(ctx, ident, chall) + _, prob = va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob == nil { t.Fatalf("Should have found a 404 for the challenge.") } @@ -285,7 +288,7 @@ func TestHTTP(t *testing.T) { setChallengeToken(&chall, pathWrongToken) // The "wrong token" will actually be the expectedToken. It's wrong // because it doesn't match pathWrongToken. - _, prob = va.validateHTTP01(ctx, ident, chall) + _, prob = va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob == nil { t.Fatalf("Should have found the wrong token value.") } @@ -294,7 +297,7 @@ func TestHTTP(t *testing.T) { log.Clear() setChallengeToken(&chall, pathMoved) - _, prob = va.validateHTTP01(ctx, ident, chall) + _, prob = va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob != nil { t.Fatalf("Failed to follow 301 redirect") } @@ -302,7 +305,7 @@ func TestHTTP(t *testing.T) { log.Clear() setChallengeToken(&chall, pathFound) - _, prob = va.validateHTTP01(ctx, ident, chall) + _, prob = va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob != nil { t.Fatalf("Failed to follow 302 redirect") } @@ -334,7 +337,7 @@ func TestHTTPTimeout(t *testing.T) { setChallengeToken(&chall, pathWaitLong) started := time.Now() - _, prob := va.validateHTTP01(ctx, ident, chall) + _, prob := va.validateHTTP01(ctx, dnsi("localhost"), chall) took := time.Since(started) // Check that the HTTP connection times out after 5 seconds and doesn't block for 10 seconds test.Assert(t, (took > (time.Second * 5)), "HTTP timed out before 5 seconds") @@ -360,7 +363,7 @@ func TestHTTPRedirectLookup(t *testing.T) { va, log := setup(hs, 0) setChallengeToken(&chall, pathMoved) - _, prob := va.validateHTTP01(ctx, ident, chall) + _, prob := va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob != nil { t.Fatalf("Unexpected failure in redirect (%s): %s", pathMoved, prob) } @@ -369,7 +372,7 @@ func TestHTTPRedirectLookup(t *testing.T) { log.Clear() setChallengeToken(&chall, pathFound) - _, prob = va.validateHTTP01(ctx, ident, chall) + _, prob = va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob != nil { t.Fatalf("Unexpected failure in redirect (%s): %s", pathFound, prob) } @@ -379,14 +382,14 @@ func TestHTTPRedirectLookup(t *testing.T) { log.Clear() setChallengeToken(&chall, pathReLookupInvalid) - _, err := va.validateHTTP01(ctx, ident, chall) + _, err := va.validateHTTP01(ctx, dnsi("localhost"), chall) test.AssertError(t, err, chall.Token) test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost \[using 127.0.0.1\]: \[127.0.0.1\]`)), 1) test.AssertEquals(t, len(log.GetAllMatching(`No valid IP addresses found for invalid.invalid`)), 1) log.Clear() setChallengeToken(&chall, pathReLookup) - _, prob = va.validateHTTP01(ctx, ident, chall) + _, prob = va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob != nil { t.Fatalf("Unexpected error in redirect (%s): %s", pathReLookup, prob) } @@ -396,7 +399,7 @@ func TestHTTPRedirectLookup(t *testing.T) { log.Clear() setChallengeToken(&chall, pathRedirectPort) - _, err = va.validateHTTP01(ctx, ident, chall) + _, err = va.validateHTTP01(ctx, dnsi("localhost"), chall) test.AssertError(t, err, chall.Token) test.AssertEquals(t, len(log.GetAllMatching(`redirect from ".*/port-redirect" to ".*other.valid:8080/path"`)), 1) test.AssertEquals(t, len(log.GetAllMatching(`Resolved addresses for localhost \[using 127.0.0.1\]: \[127.0.0.1\]`)), 1) @@ -407,7 +410,7 @@ func TestHTTPRedirectLookup(t *testing.T) { // is referencing the redirected to host, instead of the original host. log.Clear() setChallengeToken(&chall, pathRedirectToFailingURL) - _, prob = va.validateHTTP01(ctx, ident, chall) + _, prob = va.validateHTTP01(ctx, dnsi("localhost"), chall) test.AssertNotNil(t, prob, "Problem Details should not be nil") test.AssertEquals(t, prob.Detail, "Fetching http://other.valid/500: Connection refused") } @@ -420,7 +423,7 @@ func TestHTTPRedirectLoop(t *testing.T) { defer hs.Close() va, _ := setup(hs, 0) - _, prob := va.validateHTTP01(ctx, ident, chall) + _, prob := va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob == nil { t.Fatalf("Challenge should have failed for %s", chall.Token) } @@ -436,13 +439,13 @@ func TestHTTPRedirectUserAgent(t *testing.T) { va.userAgent = rejectUserAgent setChallengeToken(&chall, pathMoved) - _, prob := va.validateHTTP01(ctx, ident, chall) + _, prob := va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob == nil { t.Fatalf("Challenge with rejectUserAgent should have failed (%s).", pathMoved) } setChallengeToken(&chall, pathFound) - _, prob = va.validateHTTP01(ctx, ident, chall) + _, prob = va.validateHTTP01(ctx, dnsi("localhost"), chall) if prob == nil { t.Fatalf("Challenge with rejectUserAgent should have failed (%s).", pathFound) } @@ -471,7 +474,7 @@ func TestTLSSNI01(t *testing.T) { va, log := setup(hs, 0) - _, prob := va.validateTLSSNI01(ctx, ident, chall) + _, prob := va.validateTLSSNI01(ctx, dnsi("localhost"), chall) if prob != nil { t.Fatalf("Unexpected failure in validate TLS-SNI-01: %s", prob) } @@ -505,7 +508,7 @@ func TestTLSSNI01(t *testing.T) { log.Clear() started := time.Now() - _, prob = va.validateTLSSNI01(ctx, ident, chall) + _, prob = va.validateTLSSNI01(ctx, dnsi("localhost"), chall) took := time.Since(started) if prob == nil { t.Fatalf("Validation should've failed") @@ -518,7 +521,7 @@ func TestTLSSNI01(t *testing.T) { // Take down validation server and check that validation fails. hs.Close() - _, err := va.validateTLSSNI01(ctx, ident, chall) + _, err := va.validateTLSSNI01(ctx, dnsi("localhost"), chall) if err == nil { t.Fatalf("Server's down; expected refusal. Where did we connect?") } @@ -528,7 +531,7 @@ func TestTLSSNI01(t *testing.T) { va.tlsPort = getPort(httpOnly) log.Clear() - _, err = va.validateTLSSNI01(ctx, ident, chall) + _, err = va.validateTLSSNI01(ctx, dnsi("localhost"), chall) test.AssertError(t, err, "TLS-SNI-01 validation passed when talking to a HTTP-only server") test.Assert(t, strings.HasSuffix( err.Error(), @@ -543,7 +546,7 @@ func TestTLSSNI02(t *testing.T) { va, log := setup(hs, 0) - _, prob := va.validateTLSSNI02(ctx, ident, chall) + _, prob := va.validateTLSSNI02(ctx, dnsi("localhost"), chall) if prob != nil { t.Fatalf("Unexpected failure in validate TLS-SNI-02: %s", prob) } @@ -577,7 +580,7 @@ func TestTLSSNI02(t *testing.T) { log.Clear() started := time.Now() - _, prob = va.validateTLSSNI02(ctx, ident, chall) + _, prob = va.validateTLSSNI02(ctx, dnsi("localhost"), chall) took := time.Since(started) if prob == nil { t.Fatalf("Validation should have failed") @@ -590,7 +593,7 @@ func TestTLSSNI02(t *testing.T) { // Take down validation server and check that validation fails. hs.Close() - _, err := va.validateTLSSNI02(ctx, ident, chall) + _, err := va.validateTLSSNI02(ctx, dnsi("localhost"), chall) if err == nil { t.Fatalf("Server's down; expected refusal. Where did we connect?") } @@ -601,7 +604,7 @@ func TestTLSSNI02(t *testing.T) { va.tlsPort = getPort(httpOnly) log.Clear() - _, err = va.validateTLSSNI02(ctx, ident, chall) + _, err = va.validateTLSSNI02(ctx, dnsi("localhost"), chall) test.AssertError(t, err, "TLS-SNI-02 validation passed when talking to a HTTP-only server") test.Assert(t, strings.HasSuffix( err.Error(), @@ -626,7 +629,7 @@ func TestTLSError(t *testing.T) { va, _ := setup(hs, 0) - _, prob := va.validateTLSSNI01(ctx, ident, chall) + _, prob := va.validateTLSSNI01(ctx, dnsi("localhost"), chall) if prob == nil { t.Fatalf("TLS validation should have failed: What cert was used?") } @@ -711,7 +714,7 @@ func TestSNIErrInvalidChain(t *testing.T) { va, _ := setup(hs, 0) // Validate the SNI challenge with the test server, expecting it to fail - _, prob := va.validateTLSSNI01(ctx, ident, chall) + _, prob := va.validateTLSSNI01(ctx, dnsi("localhost"), chall) if prob == nil { t.Fatalf("TLS validation should have failed") } @@ -733,7 +736,7 @@ func TestValidateHTTP(t *testing.T) { va, _ := setup(hs, 0) - _, prob := va.validateChallenge(ctx, ident, chall) + _, prob := va.validateChallenge(ctx, dnsi("localhost"), chall) test.Assert(t, prob == nil, "validation failed") } @@ -764,7 +767,7 @@ func TestValidateTLSSNI01(t *testing.T) { va, _ := setup(hs, 0) - _, prob := va.validateChallenge(ctx, ident, chall) + _, prob := va.validateChallenge(ctx, dnsi("localhost"), chall) test.Assert(t, prob == nil, "validation failed") } @@ -776,7 +779,7 @@ func TestValidateTLSSNI01NotSane(t *testing.T) { chall.Token = "not sane" - _, prob := va.validateChallenge(ctx, ident, chall) + _, prob := va.validateChallenge(ctx, dnsi("localhost"), chall) test.AssertEquals(t, prob.Type, probs.MalformedProblem) } @@ -916,7 +919,7 @@ func TestDNSValidationFailure(t *testing.T) { chalDNS := createChallenge(core.ChallengeTypeDNS01) - _, prob := va.validateChallenge(ctx, ident, chalDNS) + _, prob := va.validateChallenge(ctx, dnsi("localhost"), chalDNS) test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) } @@ -952,12 +955,12 @@ func TestDNSValidationNotSane(t *testing.T) { var authz = core.Authorization{ ID: core.NewToken(), RegistrationID: 1, - Identifier: ident, + Identifier: dnsi("localhost"), Challenges: []core.Challenge{chal0, chal1, chal2}, } for i := 0; i < len(authz.Challenges); i++ { - _, prob := va.validateChallenge(ctx, ident, authz.Challenges[i]) + _, prob := va.validateChallenge(ctx, dnsi("localhost"), authz.Challenges[i]) if prob.Type != probs.MalformedProblem { t.Errorf("Got wrong error type for %d: expected %s, got %s", i, prob.Type, probs.MalformedProblem) @@ -973,11 +976,7 @@ func TestDNSValidationServFail(t *testing.T) { chalDNS := createChallenge(core.ChallengeTypeDNS01) - badIdent := core.AcmeIdentifier{ - Type: core.IdentifierDNS, - Value: "servfail.com", - } - _, prob := va.validateChallenge(ctx, badIdent, chalDNS) + _, prob := va.validateChallenge(ctx, dnsi("servfail.com"), chalDNS) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) } @@ -993,7 +992,7 @@ func TestDNSValidationNoServer(t *testing.T) { chalDNS := createChallenge(core.ChallengeTypeDNS01) - _, prob := va.validateChallenge(ctx, ident, chalDNS) + _, prob := va.validateChallenge(ctx, dnsi("localhost"), chalDNS) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) } @@ -1006,12 +1005,7 @@ func TestDNSValidationOK(t *testing.T) { chalDNS.Token = expectedToken chalDNS.ProvidedKeyAuthorization = expectedKeyAuthorization - goodIdent := core.AcmeIdentifier{ - Type: core.IdentifierDNS, - Value: "good-dns01.com", - } - - _, prob := va.validateChallenge(ctx, goodIdent, chalDNS) + _, prob := va.validateChallenge(ctx, dnsi("good-dns01.com"), chalDNS) test.Assert(t, prob == nil, "Should be valid.") } @@ -1025,12 +1019,7 @@ func TestDNSValidationNoAuthorityOK(t *testing.T) { chalDNS.ProvidedKeyAuthorization = expectedKeyAuthorization - goodIdent := core.AcmeIdentifier{ - Type: core.IdentifierDNS, - Value: "no-authority-dns01.com", - } - - _, prob := va.validateChallenge(ctx, goodIdent, chalDNS) + _, prob := va.validateChallenge(ctx, dnsi("no-authority-dns01.com"), chalDNS) test.Assert(t, prob == nil, "Should be valid.") } @@ -1042,8 +1031,7 @@ func TestCAAFailure(t *testing.T) { va, _ := setup(hs, 0) - ident.Value = "reserved.com" - _, prob := va.validateChallengeAndCAA(ctx, ident, chall) + _, prob := va.validateChallengeAndCAA(ctx, dnsi("reserved.com"), chall) test.AssertEquals(t, prob.Type, probs.ConnectionProblem) } @@ -1051,12 +1039,11 @@ func TestLimitedReader(t *testing.T) { chall := core.HTTPChallenge01() setChallengeToken(&chall, core.NewToken()) - ident.Value = "localhost" hs := httpSrv(t, "01234567890123456789012345678901234567890123456789012345678901234567890123456789") va, _ := setup(hs, 0) defer hs.Close() - _, prob := va.validateChallenge(ctx, ident, chall) + _, prob := va.validateChallenge(ctx, dnsi("localhost"), chall) test.AssertEquals(t, prob.Type, probs.UnauthorizedProblem) test.Assert(t, strings.HasPrefix(prob.Detail, "Invalid response from "), @@ -1250,8 +1237,7 @@ func TestFallbackDialer(t *testing.T) { // Since the IPv6First feature flag is not enabled we expect that the IPv4 // address will be used and validation will succeed using the httpSrv we // created earlier. - host := "ipv4.and.ipv6.localhost" - ident = core.AcmeIdentifier{Type: core.IdentifierDNS, Value: host} + ident := dnsi("ipv4.and.ipv6.localhost") records, prob := va.validateChallenge(ctx, ident, chall) test.Assert(t, prob == nil, "validation failed for an dual homed host with IPv6First disabled") // We expect one validation record to be present @@ -1303,8 +1289,7 @@ func TestFallbackTLS(t *testing.T) { // Since the IPv6First feature flag is not enabled we expect that the IPv4 // address will be used and validation will succeed using the httpSrv we // created earlier. - host := "ipv4.and.ipv6.localhost" - ident = core.AcmeIdentifier{Type: core.IdentifierDNS, Value: host} + ident := dnsi("ipv4.and.ipv6.localhost") records, prob := va.validateChallenge(ctx, ident, chall) test.Assert(t, prob == nil, "validation failed for a dual-homed address with an IPv4 server") // We expect one validation record to be present @@ -1342,8 +1327,7 @@ func TestFallbackTLS(t *testing.T) { // Now try a validation for an IPv6 only host. E.g. one without an IPv4 // address. The IPv6 will fail without a server and we expect the overall // validation to fail since there is no IPv4 address/listener to fall back to. - host = "ipv6.localhost" - ident = core.AcmeIdentifier{Type: core.IdentifierDNS, Value: host} + ident = dnsi("ipv6.localhost") va.stats = metrics.NewNoopScope() records, prob = va.validateChallenge(ctx, ident, chall) @@ -1420,8 +1404,7 @@ func TestPerformRemoteValidation(t *testing.T) { // Both remotes working, should succeed probCh := make(chan *probs.ProblemDetails, 1) - ident := core.AcmeIdentifier{Type: core.IdentifierDNS, Value: "localhost"} - localVA.performRemoteValidation(context.Background(), ident.Value, chall, core.Authorization{}, probCh) + localVA.performRemoteValidation(context.Background(), "localhost", chall, core.Authorization{}, probCh) prob := <-probCh if prob != nil { t.Errorf("performRemoteValidation failed: %s", prob) @@ -1431,7 +1414,7 @@ func TestPerformRemoteValidation(t *testing.T) { ms.mu.Lock() delete(ms.allowedUAs, "remote 1") ms.mu.Unlock() - localVA.performRemoteValidation(context.Background(), ident.Value, chall, core.Authorization{}, probCh) + localVA.performRemoteValidation(context.Background(), "localhost", chall, core.Authorization{}, probCh) prob = <-probCh if prob == nil { t.Error("performRemoteValidation didn't fail when one 'remote' validation failed") @@ -1444,7 +1427,7 @@ func TestPerformRemoteValidation(t *testing.T) { ms.mu.Unlock() // Both local and remotes working, should succeed - _, err := localVA.PerformValidation(context.Background(), ident.Value, chall, core.Authorization{}) + _, err := localVA.PerformValidation(context.Background(), "localhost", chall, core.Authorization{}) if err != nil { t.Errorf("PerformValidation failed: %s", err) } @@ -1453,7 +1436,7 @@ func TestPerformRemoteValidation(t *testing.T) { ms.mu.Lock() delete(ms.allowedUAs, "local") ms.mu.Unlock() - _, err = localVA.PerformValidation(context.Background(), ident.Value, chall, core.Authorization{}) + _, err = localVA.PerformValidation(context.Background(), "localhost", chall, core.Authorization{}) if err == nil { t.Error("PerformValidation didn't fail when local validation failed") } @@ -1463,7 +1446,7 @@ func TestPerformRemoteValidation(t *testing.T) { ms.allowedUAs["local"] = struct{}{} delete(ms.allowedUAs, "remote 1") ms.mu.Unlock() - _, err = localVA.PerformValidation(context.Background(), ident.Value, chall, core.Authorization{}) + _, err = localVA.PerformValidation(context.Background(), "localhost", chall, core.Authorization{}) if err == nil { t.Error("PerformValidation didn't fail when one 'remote' validation failed") } @@ -1475,7 +1458,7 @@ func TestPerformRemoteValidation(t *testing.T) { {remoteVA1, "remote 1"}, {remoteVA2, "remote 2"}, } - _, err = localVA.PerformValidation(context.Background(), ident.Value, chall, core.Authorization{}) + _, err = localVA.PerformValidation(context.Background(), "localhost", chall, core.Authorization{}) if err != nil { t.Errorf("PerformValidation failed when one 'remote' validation failed but maxRemoteFailures is 1: %s", err) } @@ -1484,7 +1467,7 @@ func TestPerformRemoteValidation(t *testing.T) { ms.mu.Lock() delete(ms.allowedUAs, "remote 2") ms.mu.Unlock() - _, err = localVA.PerformValidation(context.Background(), ident.Value, chall, core.Authorization{}) + _, err = localVA.PerformValidation(context.Background(), "localhost", chall, core.Authorization{}) if err == nil { t.Error("PerformValidation didn't fail when both 'remote' validations failed") } @@ -1495,7 +1478,7 @@ func TestPerformRemoteValidation(t *testing.T) { ms.mu.Unlock() remoteVA2.userAgent = "slow remote" s := time.Now() - _, err = localVA.PerformValidation(context.Background(), ident.Value, chall, core.Authorization{}) + _, err = localVA.PerformValidation(context.Background(), "localhost", chall, core.Authorization{}) if err != nil { t.Errorf("PerformValidation failed when one 'remote' validation failed but maxRemoteFailures is 1: %s", err) } @@ -1515,7 +1498,7 @@ func TestPerformRemoteValidation(t *testing.T) { {remoteVA2, "remote 2"}, } s = time.Now() - _, err = localVA.PerformValidation(context.Background(), ident.Value, chall, core.Authorization{}) + _, err = localVA.PerformValidation(context.Background(), "localhost", chall, core.Authorization{}) if err == nil { t.Error("PerformValidation didn't fail when two validations failed") } From d1249525ab92a57f6d23ee16b048d255ec7bbb92 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Sat, 26 Aug 2017 10:04:24 -0700 Subject: [PATCH 04/12] Increase timeouts for dns-test-srv. (#3011) They used to be a millisecond, which remarkably worked most of the time. However, some fraction of DNS requests would fail and need to be retried. Even successful integration test runs had a number of such failures, but retries generally saved them. However, sometimes all of the retries for a given lookup would fail, leading to a failure of the overall lookup. This typically manifested as an error looking up CAA, because our integration tests look up CAA much more frequently than other record types. This appears to fix our integration test flakiness. --- test/dns-test-srv/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/dns-test-srv/main.go b/test/dns-test-srv/main.go index 7f11303df..52e8d953f 100644 --- a/test/dns-test-srv/main.go +++ b/test/dns-test-srv/main.go @@ -149,8 +149,8 @@ func (ts *testSrv) serveTestResolver() { dnsServer := &dns.Server{ Addr: "0.0.0.0:8053", Net: "tcp", - ReadTimeout: time.Millisecond, - WriteTimeout: time.Millisecond, + ReadTimeout: time.Second, + WriteTimeout: time.Second, } go func() { err := dnsServer.ListenAndServe() From 0d69b24fccc3a691b60380b89579e284201360c1 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 28 Aug 2017 11:24:03 -0700 Subject: [PATCH 05/12] Move VA's CAA code into separate file (#3010) va.go is quite a large file. This splits out the CAA-related code and tests into its own file for simplicity. This is a simple move; no code has been changed, and there is no package split. --- va/caa.go | 202 +++++++++++++++++++++++++++++++++++++++++++++++++ va/caa_test.go | 119 +++++++++++++++++++++++++++++ va/va.go | 192 ---------------------------------------------- va/va_test.go | 109 -------------------------- 4 files changed, 321 insertions(+), 301 deletions(-) create mode 100644 va/caa.go create mode 100644 va/caa_test.go diff --git a/va/caa.go b/va/caa.go new file mode 100644 index 000000000..240f10b28 --- /dev/null +++ b/va/caa.go @@ -0,0 +1,202 @@ +package va + +import ( + "fmt" + "strings" + "sync" + + "github.com/letsencrypt/boulder/core" + "github.com/letsencrypt/boulder/probs" + "github.com/miekg/dns" + "golang.org/x/net/context" +) + +func (va *ValidationAuthorityImpl) checkCAA(ctx context.Context, identifier core.AcmeIdentifier) *probs.ProblemDetails { + present, valid, err := va.checkCAARecords(ctx, identifier) + if err != nil { + return probs.ConnectionFailure(err.Error()) + } + va.log.AuditInfo(fmt.Sprintf( + "Checked CAA records for %s, [Present: %t, Valid for issuance: %t]", + identifier.Value, + present, + valid, + )) + if !valid { + return probs.ConnectionFailure(fmt.Sprintf("CAA record for %s prevents issuance", identifier.Value)) + } + return nil +} + +// CAASet consists of filtered CAA records +type CAASet struct { + Issue []*dns.CAA + Issuewild []*dns.CAA + Iodef []*dns.CAA + Unknown []*dns.CAA +} + +// returns true if any CAA records have unknown tag properties and are flagged critical. +func (caaSet CAASet) criticalUnknown() bool { + if len(caaSet.Unknown) > 0 { + for _, caaRecord := range caaSet.Unknown { + // The critical flag is the bit with significance 128. However, many CAA + // record users have misinterpreted the RFC and concluded that the bit + // with significance 1 is the critical bit. This is sufficiently + // widespread that that bit must reasonably be considered an alias for + // the critical bit. The remaining bits are 0/ignore as proscribed by the + // RFC. + if (caaRecord.Flag & (128 | 1)) != 0 { + return true + } + } + } + + return false +} + +// Filter CAA records by property +func newCAASet(CAAs []*dns.CAA) *CAASet { + var filtered CAASet + + for _, caaRecord := range CAAs { + switch caaRecord.Tag { + case "issue": + filtered.Issue = append(filtered.Issue, caaRecord) + case "issuewild": + filtered.Issuewild = append(filtered.Issuewild, caaRecord) + case "iodef": + filtered.Iodef = append(filtered.Iodef, caaRecord) + default: + filtered.Unknown = append(filtered.Unknown, caaRecord) + } + } + + return &filtered +} + +type caaResult struct { + records []*dns.CAA + err error +} + +func parseResults(results []caaResult) (*CAASet, error) { + // Return first result + for _, res := range results { + if res.err != nil { + return nil, res.err + } + if len(res.records) > 0 { + return newCAASet(res.records), nil + } + } + return nil, nil +} + +func (va *ValidationAuthorityImpl) parallelCAALookup(ctx context.Context, name string, lookuper func(context.Context, string) ([]*dns.CAA, error)) []caaResult { + labels := strings.Split(name, ".") + results := make([]caaResult, len(labels)) + var wg sync.WaitGroup + + for i := 0; i < len(labels); i++ { + // Start the concurrent DNS lookup. + wg.Add(1) + go func(name string, r *caaResult) { + r.records, r.err = lookuper(ctx, name) + wg.Done() + }(strings.Join(labels[i:], "."), &results[i]) + } + + wg.Wait() + return results +} + +func (va *ValidationAuthorityImpl) getCAASet(ctx context.Context, hostname string) (*CAASet, error) { + hostname = strings.TrimRight(hostname, ".") + + // See RFC 6844 "Certification Authority Processing" for pseudocode. + // Essentially: check CAA records for the FDQN to be issued, and all + // parent domains. + // + // The lookups are performed in parallel in order to avoid timing out + // the RPC call. + // + // We depend on our resolver to snap CNAME and DNAME records. + results := va.parallelCAALookup(ctx, hostname, va.dnsClient.LookupCAA) + return parseResults(results) +} + +func (va *ValidationAuthorityImpl) checkCAARecords(ctx context.Context, identifier core.AcmeIdentifier) (present, valid bool, err error) { + hostname := strings.ToLower(identifier.Value) + caaSet, err := va.getCAASet(ctx, hostname) + if err != nil { + return false, false, err + } + present, valid = va.validateCAASet(caaSet) + return present, valid, nil +} + +func (va *ValidationAuthorityImpl) validateCAASet(caaSet *CAASet) (present, valid bool) { + if caaSet == nil { + // No CAA records found, can issue + va.stats.Inc("CAA.None", 1) + return false, true + } + + // Record stats on directives not currently processed. + if len(caaSet.Iodef) > 0 { + va.stats.Inc("CAA.WithIodef", 1) + } + + if caaSet.criticalUnknown() { + // Contains unknown critical directives. + va.stats.Inc("CAA.UnknownCritical", 1) + return true, false + } + + if len(caaSet.Unknown) > 0 { + va.stats.Inc("CAA.WithUnknownNoncritical", 1) + } + + if len(caaSet.Issue) == 0 { + // Although CAA records exist, none of them pertain to issuance in this case. + // (e.g. there is only an issuewild directive, but we are checking for a + // non-wildcard identifier, or there is only an iodef or non-critical unknown + // directive.) + va.stats.Inc("CAA.NoneRelevant", 1) + return true, true + } + + // There are CAA records pertaining to issuance in our case. Note that this + // includes the case of the unsatisfiable CAA record value ";", used to + // prevent issuance by any CA under any circumstance. + // + // Our CAA identity must be found in the chosen checkSet. + for _, caa := range caaSet.Issue { + if extractIssuerDomain(caa) == va.issuerDomain { + va.stats.Inc("CAA.Authorized", 1) + return true, true + } + } + + // The list of authorized issuers is non-empty, but we are not in it. Fail. + va.stats.Inc("CAA.Unauthorized", 1) + return true, false +} + +// Given a CAA record, assume that the Value is in the issue/issuewild format, +// that is, a domain name with zero or more additional key-value parameters. +// Returns the domain name, which may be "" (unsatisfiable). +func extractIssuerDomain(caa *dns.CAA) string { + v := caa.Value + v = strings.Trim(v, " \t") // Value can start and end with whitespace. + idx := strings.IndexByte(v, ';') + if idx < 0 { + return v // no parameters; domain only + } + + // Currently, ignore parameters. Unfortunately, the RFC makes no statement on + // whether any parameters are critical. Treat unknown parameters as + // non-critical. + return strings.Trim(v[0:idx], " \t") +} diff --git a/va/caa_test.go b/va/caa_test.go new file mode 100644 index 000000000..cbc43d0c0 --- /dev/null +++ b/va/caa_test.go @@ -0,0 +1,119 @@ +package va + +import ( + "errors" + "testing" + + "github.com/miekg/dns" + + "github.com/letsencrypt/boulder/core" + "github.com/letsencrypt/boulder/probs" + "github.com/letsencrypt/boulder/test" +) + +func TestCAATimeout(t *testing.T) { + va, _ := setup(nil, 0) + err := va.checkCAA(ctx, core.AcmeIdentifier{Type: core.IdentifierDNS, Value: "caa-timeout.com"}) + if err.Type != probs.ConnectionProblem { + t.Errorf("Expected timeout error type %s, got %s", probs.ConnectionProblem, err.Type) + } + expected := "DNS problem: query timed out looking up CAA for always.timeout" + if err.Detail != expected { + t.Errorf("checkCAA: got %#v, expected %#v", err.Detail, expected) + } +} + +func TestCAAChecking(t *testing.T) { + type CAATest struct { + Domain string + Present bool + Valid bool + } + tests := []CAATest{ + // Reserved + {"reserved.com", true, false}, + // Critical + {"critical.com", true, false}, + {"nx.critical.com", true, false}, + // Good (absent) + {"absent.com", false, true}, + {"example.co.uk", false, true}, + // Good (present) + {"present.com", true, true}, + {"present.servfail.com", true, true}, + // Good (multiple critical, one matching) + {"multi-crit-present.com", true, true}, + // Bad (unknown critical) + {"unknown-critical.com", true, false}, + {"unknown-critical2.com", true, false}, + // Good (unknown noncritical, no issue/issuewild records) + {"unknown-noncritical.com", true, true}, + // Good (issue record with unknown parameters) + {"present-with-parameter.com", true, true}, + // Bad (unsatisfiable issue record) + {"unsatisfiable.com", true, false}, + } + + va, _ := setup(nil, 0) + for _, caaTest := range tests { + present, valid, err := va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: caaTest.Domain}) + if err != nil { + t.Errorf("checkCAARecords error for %s: %s", caaTest.Domain, err) + } + if present != caaTest.Present { + t.Errorf("checkCAARecords presence mismatch for %s: got %t expected %t", caaTest.Domain, present, caaTest.Present) + } + if valid != caaTest.Valid { + t.Errorf("checkCAARecords validity mismatch for %s: got %t expected %t", caaTest.Domain, valid, caaTest.Valid) + } + } + + present, valid, err := va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: "servfail.com"}) + test.AssertError(t, err, "servfail.com") + test.Assert(t, !present, "Present should be false") + test.Assert(t, !valid, "Valid should be false") + + _, _, err = va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: "servfail.com"}) + if err == nil { + t.Errorf("Should have returned error on CAA lookup, but did not: %s", "servfail.com") + } + + present, valid, err = va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: "servfail.present.com"}) + test.AssertError(t, err, "servfail.present.com") + test.Assert(t, !present, "Present should be false") + test.Assert(t, !valid, "Valid should be false") + + _, _, err = va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: "servfail.present.com"}) + if err == nil { + t.Errorf("Should have returned error on CAA lookup, but did not: %s", "servfail.present.com") + } +} + +func TestCAAFailure(t *testing.T) { + chall := createChallenge(core.ChallengeTypeTLSSNI01) + hs := tlssni01Srv(t, chall) + defer hs.Close() + + va, _ := setup(hs, 0) + + _, prob := va.validateChallengeAndCAA(ctx, dnsi("reserved.com"), chall) + test.AssertEquals(t, prob.Type, probs.ConnectionProblem) +} + +func TestParseResults(t *testing.T) { + r := []caaResult{} + s, err := parseResults(r) + test.Assert(t, s == nil, "set is not nil") + test.Assert(t, err == nil, "error is not nil") + test.AssertNotError(t, err, "no error should be returned") + r = []caaResult{{nil, errors.New("")}, {[]*dns.CAA{{Value: "test"}}, nil}} + s, err = parseResults(r) + test.Assert(t, s == nil, "set is not nil") + test.AssertEquals(t, err.Error(), "") + expected := dns.CAA{Value: "other-test"} + r = []caaResult{{[]*dns.CAA{&expected}, nil}, {[]*dns.CAA{{Value: "test"}}, nil}} + s, err = parseResults(r) + test.AssertEquals(t, len(s.Unknown), 1) + test.Assert(t, s.Unknown[0] == &expected, "Incorrect record returned") + test.AssertNotError(t, err, "no error should be returned") +} diff --git a/va/va.go b/va/va.go index 4af85aca1..8dc513381 100644 --- a/va/va.go +++ b/va/va.go @@ -17,12 +17,10 @@ import ( "os" "strconv" "strings" - "sync" "syscall" "time" "github.com/jmhodges/clock" - "github.com/miekg/dns" "github.com/prometheus/client_golang/prometheus" "golang.org/x/net/context" @@ -761,23 +759,6 @@ func (va *ValidationAuthorityImpl) validateDNS01(ctx context.Context, identifier return nil, probs.Unauthorized("Correct value not found for DNS challenge") } -func (va *ValidationAuthorityImpl) checkCAA(ctx context.Context, identifier core.AcmeIdentifier) *probs.ProblemDetails { - present, valid, err := va.checkCAARecords(ctx, identifier) - if err != nil { - return probs.ConnectionFailure(err.Error()) - } - va.log.AuditInfo(fmt.Sprintf( - "Checked CAA records for %s, [Present: %t, Valid for issuance: %t]", - identifier.Value, - present, - valid, - )) - if !valid { - return probs.ConnectionFailure(fmt.Sprintf("CAA record for %s prevents issuance", identifier.Value)) - } - return nil -} - func (va *ValidationAuthorityImpl) validateChallengeAndCAA(ctx context.Context, identifier core.AcmeIdentifier, challenge core.Challenge) ([]core.ValidationRecord, *probs.ProblemDetails) { ch := make(chan *probs.ProblemDetails, 1) go func() { @@ -939,176 +920,3 @@ func (va *ValidationAuthorityImpl) PerformValidation(ctx context.Context, domain return records, prob } - -// CAASet consists of filtered CAA records -type CAASet struct { - Issue []*dns.CAA - Issuewild []*dns.CAA - Iodef []*dns.CAA - Unknown []*dns.CAA -} - -// returns true if any CAA records have unknown tag properties and are flagged critical. -func (caaSet CAASet) criticalUnknown() bool { - if len(caaSet.Unknown) > 0 { - for _, caaRecord := range caaSet.Unknown { - // The critical flag is the bit with significance 128. However, many CAA - // record users have misinterpreted the RFC and concluded that the bit - // with significance 1 is the critical bit. This is sufficiently - // widespread that that bit must reasonably be considered an alias for - // the critical bit. The remaining bits are 0/ignore as proscribed by the - // RFC. - if (caaRecord.Flag & (128 | 1)) != 0 { - return true - } - } - } - - return false -} - -// Filter CAA records by property -func newCAASet(CAAs []*dns.CAA) *CAASet { - var filtered CAASet - - for _, caaRecord := range CAAs { - switch caaRecord.Tag { - case "issue": - filtered.Issue = append(filtered.Issue, caaRecord) - case "issuewild": - filtered.Issuewild = append(filtered.Issuewild, caaRecord) - case "iodef": - filtered.Iodef = append(filtered.Iodef, caaRecord) - default: - filtered.Unknown = append(filtered.Unknown, caaRecord) - } - } - - return &filtered -} - -type caaResult struct { - records []*dns.CAA - err error -} - -func parseResults(results []caaResult) (*CAASet, error) { - // Return first result - for _, res := range results { - if res.err != nil { - return nil, res.err - } - if len(res.records) > 0 { - return newCAASet(res.records), nil - } - } - return nil, nil -} - -func (va *ValidationAuthorityImpl) parallelCAALookup(ctx context.Context, name string, lookuper func(context.Context, string) ([]*dns.CAA, error)) []caaResult { - labels := strings.Split(name, ".") - results := make([]caaResult, len(labels)) - var wg sync.WaitGroup - - for i := 0; i < len(labels); i++ { - // Start the concurrent DNS lookup. - wg.Add(1) - go func(name string, r *caaResult) { - r.records, r.err = lookuper(ctx, name) - wg.Done() - }(strings.Join(labels[i:], "."), &results[i]) - } - - wg.Wait() - return results -} - -func (va *ValidationAuthorityImpl) getCAASet(ctx context.Context, hostname string) (*CAASet, error) { - hostname = strings.TrimRight(hostname, ".") - - // See RFC 6844 "Certification Authority Processing" for pseudocode. - // Essentially: check CAA records for the FDQN to be issued, and all - // parent domains. - // - // The lookups are performed in parallel in order to avoid timing out - // the RPC call. - // - // We depend on our resolver to snap CNAME and DNAME records. - results := va.parallelCAALookup(ctx, hostname, va.dnsClient.LookupCAA) - return parseResults(results) -} - -func (va *ValidationAuthorityImpl) checkCAARecords(ctx context.Context, identifier core.AcmeIdentifier) (present, valid bool, err error) { - hostname := strings.ToLower(identifier.Value) - caaSet, err := va.getCAASet(ctx, hostname) - if err != nil { - return false, false, err - } - present, valid = va.validateCAASet(caaSet) - return present, valid, nil -} - -func (va *ValidationAuthorityImpl) validateCAASet(caaSet *CAASet) (present, valid bool) { - if caaSet == nil { - // No CAA records found, can issue - va.stats.Inc("CAA.None", 1) - return false, true - } - - // Record stats on directives not currently processed. - if len(caaSet.Iodef) > 0 { - va.stats.Inc("CAA.WithIodef", 1) - } - - if caaSet.criticalUnknown() { - // Contains unknown critical directives. - va.stats.Inc("CAA.UnknownCritical", 1) - return true, false - } - - if len(caaSet.Unknown) > 0 { - va.stats.Inc("CAA.WithUnknownNoncritical", 1) - } - - if len(caaSet.Issue) == 0 { - // Although CAA records exist, none of them pertain to issuance in this case. - // (e.g. there is only an issuewild directive, but we are checking for a - // non-wildcard identifier, or there is only an iodef or non-critical unknown - // directive.) - va.stats.Inc("CAA.NoneRelevant", 1) - return true, true - } - - // There are CAA records pertaining to issuance in our case. Note that this - // includes the case of the unsatisfiable CAA record value ";", used to - // prevent issuance by any CA under any circumstance. - // - // Our CAA identity must be found in the chosen checkSet. - for _, caa := range caaSet.Issue { - if extractIssuerDomain(caa) == va.issuerDomain { - va.stats.Inc("CAA.Authorized", 1) - return true, true - } - } - - // The list of authorized issuers is non-empty, but we are not in it. Fail. - va.stats.Inc("CAA.Unauthorized", 1) - return true, false -} - -// Given a CAA record, assume that the Value is in the issue/issuewild format, -// that is, a domain name with zero or more additional key-value parameters. -// Returns the domain name, which may be "" (unsatisfiable). -func extractIssuerDomain(caa *dns.CAA) string { - v := caa.Value - v = strings.Trim(v, " \t") // Value can start and end with whitespace. - idx := strings.IndexByte(v, ';') - if idx < 0 { - return v // no parameters; domain only - } - - // Currently, ignore parameters. Unfortunately, the RFC makes no statement on - // whether any parameters are critical. Treat unknown parameters as - // non-critical. - return strings.Trim(v[0:idx], " \t") -} diff --git a/va/va_test.go b/va/va_test.go index 51b4bb8c6..bcadf5222 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -9,7 +9,6 @@ import ( "crypto/x509/pkix" "encoding/base64" "encoding/hex" - "errors" "fmt" "math/big" mrand "math/rand" @@ -28,7 +27,6 @@ import ( "github.com/golang/mock/gomock" "github.com/jmhodges/clock" - "github.com/miekg/dns" "golang.org/x/net/context" "gopkg.in/square/go-jose.v2" @@ -784,84 +782,6 @@ func TestValidateTLSSNI01NotSane(t *testing.T) { test.AssertEquals(t, prob.Type, probs.MalformedProblem) } -func TestCAATimeout(t *testing.T) { - va, _ := setup(nil, 0) - err := va.checkCAA(ctx, core.AcmeIdentifier{Type: core.IdentifierDNS, Value: "caa-timeout.com"}) - if err.Type != probs.ConnectionProblem { - t.Errorf("Expected timeout error type %s, got %s", probs.ConnectionProblem, err.Type) - } - expected := "DNS problem: query timed out looking up CAA for always.timeout" - if err.Detail != expected { - t.Errorf("checkCAA: got %#v, expected %#v", err.Detail, expected) - } -} - -func TestCAAChecking(t *testing.T) { - type CAATest struct { - Domain string - Present bool - Valid bool - } - tests := []CAATest{ - // Reserved - {"reserved.com", true, false}, - // Critical - {"critical.com", true, false}, - {"nx.critical.com", true, false}, - // Good (absent) - {"absent.com", false, true}, - {"example.co.uk", false, true}, - // Good (present) - {"present.com", true, true}, - {"present.servfail.com", true, true}, - // Good (multiple critical, one matching) - {"multi-crit-present.com", true, true}, - // Bad (unknown critical) - {"unknown-critical.com", true, false}, - {"unknown-critical2.com", true, false}, - // Good (unknown noncritical, no issue/issuewild records) - {"unknown-noncritical.com", true, true}, - // Good (issue record with unknown parameters) - {"present-with-parameter.com", true, true}, - // Bad (unsatisfiable issue record) - {"unsatisfiable.com", true, false}, - } - - va, _ := setup(nil, 0) - for _, caaTest := range tests { - present, valid, err := va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: caaTest.Domain}) - if err != nil { - t.Errorf("checkCAARecords error for %s: %s", caaTest.Domain, err) - } - if present != caaTest.Present { - t.Errorf("checkCAARecords presence mismatch for %s: got %t expected %t", caaTest.Domain, present, caaTest.Present) - } - if valid != caaTest.Valid { - t.Errorf("checkCAARecords validity mismatch for %s: got %t expected %t", caaTest.Domain, valid, caaTest.Valid) - } - } - - present, valid, err := va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: "servfail.com"}) - test.AssertError(t, err, "servfail.com") - test.Assert(t, !present, "Present should be false") - test.Assert(t, !valid, "Valid should be false") - - _, _, err = va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: "servfail.com"}) - if err == nil { - t.Errorf("Should have returned error on CAA lookup, but did not: %s", "servfail.com") - } - - present, valid, err = va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: "servfail.present.com"}) - test.AssertError(t, err, "servfail.present.com") - test.Assert(t, !present, "Present should be false") - test.Assert(t, !valid, "Valid should be false") - - _, _, err = va.checkCAARecords(ctx, core.AcmeIdentifier{Type: "dns", Value: "servfail.present.com"}) - if err == nil { - t.Errorf("Should have returned error on CAA lookup, but did not: %s", "servfail.present.com") - } -} - func TestPerformValidationInvalid(t *testing.T) { va, _ := setup(nil, 0) @@ -1024,17 +944,6 @@ func TestDNSValidationNoAuthorityOK(t *testing.T) { test.Assert(t, prob == nil, "Should be valid.") } -func TestCAAFailure(t *testing.T) { - chall := createChallenge(core.ChallengeTypeTLSSNI01) - hs := tlssni01Srv(t, chall) - defer hs.Close() - - va, _ := setup(hs, 0) - - _, prob := va.validateChallengeAndCAA(ctx, dnsi("reserved.com"), chall) - test.AssertEquals(t, prob.Type, probs.ConnectionProblem) -} - func TestLimitedReader(t *testing.T) { chall := core.HTTPChallenge01() setChallengeToken(&chall, core.NewToken()) @@ -1076,24 +985,6 @@ func setup(srv *httptest.Server, maxRemoteFailures int) (*ValidationAuthorityImp return va, logger } -func TestParseResults(t *testing.T) { - r := []caaResult{} - s, err := parseResults(r) - test.Assert(t, s == nil, "set is not nil") - test.Assert(t, err == nil, "error is not nil") - test.AssertNotError(t, err, "no error should be returned") - r = []caaResult{{nil, errors.New("")}, {[]*dns.CAA{{Value: "test"}}, nil}} - s, err = parseResults(r) - test.Assert(t, s == nil, "set is not nil") - test.AssertEquals(t, err.Error(), "") - expected := dns.CAA{Value: "other-test"} - r = []caaResult{{[]*dns.CAA{&expected}, nil}, {[]*dns.CAA{{Value: "test"}}, nil}} - s, err = parseResults(r) - test.AssertEquals(t, len(s.Unknown), 1) - test.Assert(t, s.Unknown[0] == &expected, "Incorrect record returned") - test.AssertNotError(t, err, "no error should be returned") -} - func TestAvailableAddresses(t *testing.T) { v6a := net.ParseIP("::1") v6b := net.ParseIP("2001:db8::2:1") // 2001:DB8 is reserved for docs (RFC 3849) From a0ec53d1838e25012f0d200e727c49141f3d409d Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 28 Aug 2017 12:23:26 -0700 Subject: [PATCH 06/12] Raise Exceptions rather than strings. (#3015) raise("foo") isn't valid Python, but raise Exception("foo") is. --- test/integration-test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration-test.py b/test/integration-test.py index 0642c975a..92c32d185 100644 --- a/test/integration-test.py +++ b/test/integration-test.py @@ -146,7 +146,7 @@ def test_gsb_lookups(): # The GSB test server tracks hits with a trailing / on the URL hits = hits_map.get(hostname + "/", 0) if hits != 1: - raise("Expected %d Google Safe Browsing lookups for %s, found %d" % (1, url, actual)) + raise Exception("Expected %d Google Safe Browsing lookups for %s, found %d" % (1, url, actual)) def test_ocsp(): cert_file_pem = os.path.join(tempdir, "cert.pem") @@ -222,7 +222,7 @@ def test_expiration_mailer(): resp = urllib2.urlopen("http://localhost:9381/count?to=%s" % email_addr) mailcount = int(resp.read()) if mailcount != 2: - raise("\nExpiry mailer failed: expected 2 emails, got %d" % mailcount) + raise Exception("\nExpiry mailer failed: expected 2 emails, got %d" % mailcount) def test_revoke_by_account(): cert_file_pem = os.path.join(tempdir, "revokeme.pem") From b0c7bc1bee009f83bae9ff327052e316cdd19706 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 28 Aug 2017 16:40:57 -0700 Subject: [PATCH 07/12] Recheck CAA for authorizations older than 8 hours (#3014) Fixes #2889. VA now implements two gRPC services: VA and CAA. These both run on the same port, but this allows implementation of the IsCAAValid RPC to skip using the gRPC wrappers, and makes it easier to potentially separate the service into its own package in the future. RA.NewCertificate now checks the expiration times of authorizations, and will call out to VA to recheck CAA for those authorizations that were not validated recently enough. --- cmd/boulder-ra/main.go | 4 + cmd/boulder-va/main.go | 3 + features/featureflag_string.go | 4 +- features/features.go | 2 + ra/ra.go | 88 ++++++++++++++++-- ra/ra_test.go | 145 ++++++++++++++++++++++++++++-- test/config-next/ra.json | 1 + va/caa.go | 23 +++++ va/proto/va.pb.go | 158 +++++++++++++++++++++++++++------ va/proto/va.proto | 13 +++ wfe/wfe_test.go | 20 ++++- 11 files changed, 418 insertions(+), 43 deletions(-) diff --git a/cmd/boulder-ra/main.go b/cmd/boulder-ra/main.go index ac7662f07..dbc758c66 100644 --- a/cmd/boulder-ra/main.go +++ b/cmd/boulder-ra/main.go @@ -23,6 +23,7 @@ import ( "github.com/letsencrypt/boulder/ra" rapb "github.com/letsencrypt/boulder/ra/proto" sapb "github.com/letsencrypt/boulder/sa/proto" + vaPB "github.com/letsencrypt/boulder/va/proto" ) type config struct { @@ -129,6 +130,8 @@ func main() { cmd.FailOnError(err, "Unable to create VA client") vac := bgrpc.NewValidationAuthorityGRPCClient(vaConn) + caaClient := vaPB.NewCAAClient(vaConn) + caConn, err := bgrpc.ClientSetup(c.RA.CAService, tls, scope) cmd.FailOnError(err, "Unable to create CA client") // Build a CA client that is only capable of issuing certificates, not @@ -174,6 +177,7 @@ func main() { authorizationLifetime, pendingAuthorizationLifetime, pubc, + caaClient, c.RA.OrderLifetime.Duration, ) diff --git a/cmd/boulder-va/main.go b/cmd/boulder-va/main.go index b2e21e458..63ff2a751 100644 --- a/cmd/boulder-va/main.go +++ b/cmd/boulder-va/main.go @@ -13,6 +13,7 @@ import ( "github.com/letsencrypt/boulder/features" bgrpc "github.com/letsencrypt/boulder/grpc" "github.com/letsencrypt/boulder/va" + vaPB "github.com/letsencrypt/boulder/va/proto" ) type config struct { @@ -147,6 +148,8 @@ func main() { cmd.FailOnError(err, "Unable to setup VA gRPC server") err = bgrpc.RegisterValidationAuthorityGRPCServer(grpcSrv, vai) cmd.FailOnError(err, "Unable to register VA gRPC server") + vaPB.RegisterCAAServer(grpcSrv, vai) + cmd.FailOnError(err, "Unable to register CAA gRPC server") go func() { err = grpcSrv.Serve(l) cmd.FailOnError(err, "VA gRPC service failed") diff --git a/features/featureflag_string.go b/features/featureflag_string.go index a98ab00c6..1068d7891 100644 --- a/features/featureflag_string.go +++ b/features/featureflag_string.go @@ -4,9 +4,9 @@ package features import "fmt" -const _FeatureFlag_name = "unusedAllowAccountDeactivationAllowKeyRolloverResubmitMissingSCTsOnlyUseAIAIssuerURLAllowTLS02ChallengesGenerateOCSPEarlyReusePendingAuthzCountCertificatesExactRandomDirectoryEntryIPv6FirstDirectoryMetaAllowRenewalFirstRL" +const _FeatureFlag_name = "unusedAllowAccountDeactivationAllowKeyRolloverResubmitMissingSCTsOnlyUseAIAIssuerURLAllowTLS02ChallengesGenerateOCSPEarlyReusePendingAuthzCountCertificatesExactRandomDirectoryEntryIPv6FirstDirectoryMetaAllowRenewalFirstRLRecheckCAA" -var _FeatureFlag_index = [...]uint8{0, 6, 30, 46, 69, 84, 104, 121, 138, 160, 180, 189, 202, 221} +var _FeatureFlag_index = [...]uint8{0, 6, 30, 46, 69, 84, 104, 121, 138, 160, 180, 189, 202, 221, 231} func (i FeatureFlag) String() string { if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) { diff --git a/features/features.go b/features/features.go index e329fb58d..c1b253008 100644 --- a/features/features.go +++ b/features/features.go @@ -26,6 +26,7 @@ const ( IPv6First DirectoryMeta AllowRenewalFirstRL + RecheckCAA ) // List of features and their default value, protected by fMu @@ -43,6 +44,7 @@ var features = map[FeatureFlag]bool{ IPv6First: false, DirectoryMeta: false, AllowRenewalFirstRL: false, + RecheckCAA: false, } var fMu = new(sync.RWMutex) diff --git a/ra/ra.go b/ra/ra.go index f93ed0c8e..7ecb23732 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -26,7 +26,7 @@ import ( berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/features" "github.com/letsencrypt/boulder/goodkey" - "github.com/letsencrypt/boulder/grpc" + bgrpc "github.com/letsencrypt/boulder/grpc" blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/metrics" "github.com/letsencrypt/boulder/probs" @@ -36,6 +36,7 @@ import ( "github.com/letsencrypt/boulder/revocation" sapb "github.com/letsencrypt/boulder/sa/proto" vaPB "github.com/letsencrypt/boulder/va/proto" + grpc "google.golang.org/grpc" ) // Note: the issuanceExpvar must be a global. If it is a member of the RA, or @@ -44,6 +45,14 @@ import ( // of exported var name:" error from the expvar package. var issuanceExpvar = expvar.NewInt("lastIssuance") +type caaChecker interface { + IsCAAValid( + ctx context.Context, + in *vaPB.IsCAAValidRequest, + opts ...grpc.CallOption, + ) (*vaPB.IsCAAValidResponse, error) +} + // RegistrationAuthorityImpl defines an RA. // // NOTE: All of the fields in RegistrationAuthorityImpl need to be @@ -54,6 +63,7 @@ type RegistrationAuthorityImpl struct { SA core.StorageAuthority PA core.PolicyAuthority publisher core.Publisher + caa caaChecker stats metrics.Scope DNSClient bdns.DNSClient @@ -94,6 +104,7 @@ func NewRegistrationAuthorityImpl( authorizationLifetime time.Duration, pendingAuthorizationLifetime time.Duration, pubc core.Publisher, + caaClient caaChecker, orderLifetime time.Duration, ) *RegistrationAuthorityImpl { ra := &RegistrationAuthorityImpl{ @@ -115,6 +126,7 @@ func NewRegistrationAuthorityImpl( certsForDomainStats: stats.NewScope("RateLimit", "CertificatesForDomain"), totalCertsStats: stats.NewScope("RateLimit", "TotalCertificates"), publisher: pubc, + caa: caaClient, orderLifetime: orderLifetime, } return ra @@ -426,7 +438,7 @@ func (ra *RegistrationAuthorityImpl) checkInvalidAuthorizationLimit(ctx context. // The SA.CountInvalidAuthorizations method is not implemented on the wrapper // interface, because we want to move towards using gRPC interfaces more // directly. So we type-assert the wrapper to a gRPC-specific type. - saGRPC, ok := ra.SA.(*grpc.StorageAuthorityClientWrapper) + saGRPC, ok := ra.SA.(*bgrpc.StorageAuthorityClientWrapper) if !limit.Enabled() || !ok { return nil } @@ -658,13 +670,22 @@ func (ra *RegistrationAuthorityImpl) MatchesCSR(parsedCertificate *x509.Certific // checkAuthorizations checks that each requested name has a valid authorization // that won't expire before the certificate expires. Returns an error otherwise. -func (ra *RegistrationAuthorityImpl) checkAuthorizations(ctx context.Context, names []string, registration *core.Registration) error { +func (ra *RegistrationAuthorityImpl) checkAuthorizations(ctx context.Context, names []string, regID int64) error { now := ra.clk.Now() - var badNames []string + var badNames, recheckNames []string for i := range names { names[i] = strings.ToLower(names[i]) } - auths, err := ra.SA.GetValidAuthorizations(ctx, registration.ID, names, now) + // Per Baseline Requirements, CAA must be checked within 8 hours of issuance. + // CAA is checked when an authorization is validated, so as long as that was + // less than 8 hours ago, we're fine. If it was more than 8 hours ago + // we have to recheck. Since we don't record the validation time for + // authorizations, we instead look at the expiration time and subtract out the + // expected authorization lifetime. Note: If we adjust the authorization + // lifetime in the future we will need to tweak this correspondingly so it + // works correctly during the switchover. + caaRecheckTime := now.Add(ra.authorizationLifetime).Add(-8 * time.Hour) + auths, err := ra.SA.GetValidAuthorizations(ctx, regID, names, now) if err != nil { return err } @@ -676,6 +697,14 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizations(ctx context.Context, na return berrors.InternalServerError("found an authorization with a nil Expires field: id %s", authz.ID) } else if authz.Expires.Before(now) { badNames = append(badNames, name) + } else if authz.Expires.Before(caaRecheckTime) { + recheckNames = append(recheckNames, name) + } + } + + if features.Enabled(features.RecheckCAA) { + if err = ra.recheckCAA(ctx, recheckNames); err != nil { + return err } } @@ -685,6 +714,51 @@ func (ra *RegistrationAuthorityImpl) checkAuthorizations(ctx context.Context, na strings.Join(badNames, ", "), ) } + + return nil +} + +func (ra *RegistrationAuthorityImpl) recheckCAA(ctx context.Context, names []string) error { + ra.stats.Inc("recheck_caa", 1) + ra.stats.Inc("recheck_caa_names", int64(len(names))) + wg := sync.WaitGroup{} + ch := make(chan *probs.ProblemDetails, len(names)) + for _, name := range names { + wg.Add(1) + go func(name string) { + defer wg.Done() + resp, err := ra.caa.IsCAAValid(ctx, &vaPB.IsCAAValidRequest{ + Domain: &name, + }) + if err != nil { + ra.log.AuditErr(fmt.Sprintf("Rechecking CAA: %s", err)) + ch <- probs.ServerInternal("Internal error rechecking CAA for " + name) + } else if resp.Problem != nil { + ch <- &probs.ProblemDetails{ + Type: probs.ProblemType(*resp.Problem.ProblemType), + Detail: *resp.Problem.Detail, + } + } + }(name) + } + wg.Wait() + close(ch) + var fails []*probs.ProblemDetails + for err := range ch { + if err != nil { + fails = append(fails, err) + } + } + if len(fails) > 0 { + message := "Rechecking CAA: " + for i, pd := range fails { + if i > 0 { + message = message + ", " + } + message = message + pd.Detail + } + return berrors.ConnectionFailureError(message) + } return nil } @@ -753,7 +827,7 @@ func (ra *RegistrationAuthorityImpl) NewCertificate(ctx context.Context, req cor return emptyCert, err } - err = ra.checkAuthorizations(ctx, names, ®istration) + err = ra.checkAuthorizations(ctx, names, registration.ID) if err != nil { logEvent.Error = err.Error() return emptyCert, err @@ -1381,7 +1455,7 @@ func (ra *RegistrationAuthorityImpl) NewOrder(ctx context.Context, req *rapb.New if err != nil { return nil, err } - authzPB, err := grpc.AuthzToPB(authz) + authzPB, err := bgrpc.AuthzToPB(authz) if err != nil { return nil, err } diff --git a/ra/ra_test.go b/ra/ra_test.go index 6a6406440..c4b605ce0 100644 --- a/ra/ra_test.go +++ b/ra/ra_test.go @@ -14,14 +14,11 @@ import ( "net/url" "os" "strings" + "sync" "testing" "time" "github.com/jmhodges/clock" - "github.com/weppos/publicsuffix-go/publicsuffix" - "golang.org/x/net/context" - jose "gopkg.in/square/go-jose.v2" - "github.com/letsencrypt/boulder/bdns" "github.com/letsencrypt/boulder/cmd" "github.com/letsencrypt/boulder/core" @@ -40,6 +37,10 @@ import ( "github.com/letsencrypt/boulder/test" "github.com/letsencrypt/boulder/test/vars" vaPB "github.com/letsencrypt/boulder/va/proto" + "github.com/weppos/publicsuffix-go/publicsuffix" + "golang.org/x/net/context" + "google.golang.org/grpc" + jose "gopkg.in/square/go-jose.v2" ) type DummyValidationAuthority struct { @@ -259,7 +260,7 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, *sa.SQLStorageAut ra := NewRegistrationAuthorityImpl(fc, log, stats, - 1, testKeyPolicy, 0, true, false, 300*24*time.Hour, 7*24*time.Hour, nil, 0) + 1, testKeyPolicy, 0, true, false, 300*24*time.Hour, 7*24*time.Hour, nil, noopCAA{}, 0) ra.SA = ssa ra.VA = va ra.CA = ca @@ -1680,6 +1681,140 @@ func TestDeactivateRegistration(t *testing.T) { test.AssertEquals(t, dbReg.Status, core.StatusDeactivated) } +// noopCAA implements caaChecker, always returning nil +type noopCAA struct{} + +func (cr noopCAA) IsCAAValid( + ctx context.Context, + in *vaPB.IsCAAValidRequest, + opts ...grpc.CallOption, +) (*vaPB.IsCAAValidResponse, error) { + return &vaPB.IsCAAValidResponse{}, nil +} + +// caaRecorder implements caaChecker, always returning nil, but recording the +// names it was called for. +type caaRecorder struct { + sync.Mutex + names map[string]bool +} + +func (cr *caaRecorder) IsCAAValid( + ctx context.Context, + in *vaPB.IsCAAValidRequest, + opts ...grpc.CallOption, +) (*vaPB.IsCAAValidResponse, error) { + cr.Lock() + defer cr.Unlock() + cr.names[*in.Domain] = true + return &vaPB.IsCAAValidResponse{}, nil +} + +// A mock SA that returns special authzs for testing rechecking of CAA (in +// TestRecheckCAADates below) +type mockSAWithRecentAndOlder struct { + recent, older time.Time + mocks.StorageAuthority +} + +func (m *mockSAWithRecentAndOlder) GetValidAuthorizations( + ctx context.Context, + registrationID int64, + names []string, + now time.Time) (map[string]*core.Authorization, error) { + return map[string]*core.Authorization{ + "recent.com": &core.Authorization{Expires: &m.recent}, + "older.com": &core.Authorization{Expires: &m.older}, + "older2.com": &core.Authorization{Expires: &m.older}, + }, nil +} + +// Test that the right set of domain names have their CAA rechecked, based on +// expiration time. +func TestRecheckCAADates(t *testing.T) { + _, _, ra, fc, cleanUp := initAuthorities(t) + defer cleanUp() + _ = features.Set(map[string]bool{"RecheckCAA": true}) + defer features.Reset() + recorder := &caaRecorder{names: make(map[string]bool)} + ra.caa = recorder + ra.authorizationLifetime = 15 * time.Hour + ra.SA = &mockSAWithRecentAndOlder{ + recent: fc.Now().Add(15 * time.Hour), + older: fc.Now().Add(5 * time.Hour), + } + names := []string{"recent.com", "older.com", "older2.com"} + err := ra.checkAuthorizations(context.Background(), names, 999) + if err != nil { + t.Errorf("expected nil err, got %s", err) + } + if recorder.names["recent.com"] { + t.Errorf("Rechecked CAA unnecessarily for recent.com") + } + if !recorder.names["older.com"] { + t.Errorf("Failed to recheck CAA for older.com %#v", recorder.names) + } + if !recorder.names["older2.com"] { + t.Errorf("Failed to recheck CAA for older.com") + } +} + +type caaFailer struct{} + +func (cf *caaFailer) IsCAAValid( + ctx context.Context, + in *vaPB.IsCAAValidRequest, + opts ...grpc.CallOption, +) (*vaPB.IsCAAValidResponse, error) { + name := *in.Domain + if name == "a.com" { + return nil, fmt.Errorf("Error checking CAA for a.com") + } else if name == "c.com" { + return nil, fmt.Errorf("Error checking CAA for c.com") + } + return &vaPB.IsCAAValidResponse{}, nil +} + +func TestRecheckCAAEmpty(t *testing.T) { + _, _, ra, _, cleanUp := initAuthorities(t) + defer cleanUp() + _ = features.Set(map[string]bool{"RecheckCAA": true}) + defer features.Reset() + err := ra.recheckCAA(context.Background(), nil) + if err != nil { + t.Errorf("expected nil err, got %s", err) + } +} + +func TestRecheckCAASuccess(t *testing.T) { + _, _, ra, _, cleanUp := initAuthorities(t) + defer cleanUp() + _ = features.Set(map[string]bool{"RecheckCAA": true}) + defer features.Reset() + names := []string{"a.com", "b.com", "c.com"} + err := ra.recheckCAA(context.Background(), names) + if err != nil { + t.Errorf("expected nil err, got %s", err) + } +} + +func TestRecheckCAAFail(t *testing.T) { + _, _, ra, _, cleanUp := initAuthorities(t) + defer cleanUp() + _ = features.Set(map[string]bool{"RecheckCAA": true}) + defer features.Reset() + names := []string{"a.com", "b.com", "c.com"} + ra.caa = &caaFailer{} + err := ra.recheckCAA(context.Background(), names) + if err == nil { + t.Errorf("expected err, got nil") + } else if !strings.Contains(err.Error(), "error rechecking CAA for a.com") { + t.Errorf("expected error to contain error for a.com, got %q", err) + } else if !strings.Contains(err.Error(), "error rechecking CAA for c.com") { + t.Errorf("expected error to contain error for c.com, got %q", err) + } +} + func TestNewOrder(t *testing.T) { // Only run under test/config-next config where 20170731115209_AddOrders.sql // has been applied diff --git a/test/config-next/ra.json b/test/config-next/ra.json index b58814df4..d7332ce1e 100644 --- a/test/config-next/ra.json +++ b/test/config-next/ra.json @@ -45,6 +45,7 @@ "AllowKeyRollover": true, "AllowTLS02Challenges": true, "CountCertificatesExact": true, + "RecheckCAA": true, "ReusePendingAuthz": true } }, diff --git a/va/caa.go b/va/caa.go index 240f10b28..4579dbfe0 100644 --- a/va/caa.go +++ b/va/caa.go @@ -6,11 +6,34 @@ import ( "sync" "github.com/letsencrypt/boulder/core" + corepb "github.com/letsencrypt/boulder/core/proto" "github.com/letsencrypt/boulder/probs" + vapb "github.com/letsencrypt/boulder/va/proto" "github.com/miekg/dns" "golang.org/x/net/context" ) +func (va *ValidationAuthorityImpl) IsCAAValid( + ctx context.Context, + req *vapb.IsCAAValidRequest, +) (*vapb.IsCAAValidResponse, error) { + prob := va.checkCAA(ctx, core.AcmeIdentifier{ + Type: core.IdentifierDNS, + Value: *req.Domain, + }) + + if prob != nil { + typ := string(prob.Type) + return &vapb.IsCAAValidResponse{ + Problem: &corepb.ProblemDetails{ + ProblemType: &typ, + Detail: &prob.Detail, + }, + }, nil + } + return &vapb.IsCAAValidResponse{}, nil +} + func (va *ValidationAuthorityImpl) checkCAA(ctx context.Context, identifier core.AcmeIdentifier) *probs.ProblemDetails { present, valid, err := va.checkCAARecords(ctx, identifier) if err != nil { diff --git a/va/proto/va.pb.go b/va/proto/va.pb.go index 680bbb671..633df3353 100644 --- a/va/proto/va.pb.go +++ b/va/proto/va.pb.go @@ -9,6 +9,8 @@ It is generated from these files: va/proto/va.proto It has these top-level messages: + IsCAAValidRequest + IsCAAValidResponse IsSafeDomainRequest IsDomainSafe PerformValidationRequest @@ -38,6 +40,41 @@ var _ = math.Inf // proto package needs to be updated. const _ = proto1.ProtoPackageIsVersion2 // please upgrade the proto package +type IsCAAValidRequest struct { + Domain *string `protobuf:"bytes,1,opt,name=domain" json:"domain,omitempty"` + XXX_unrecognized []byte `json:"-"` +} + +func (m *IsCAAValidRequest) Reset() { *m = IsCAAValidRequest{} } +func (m *IsCAAValidRequest) String() string { return proto1.CompactTextString(m) } +func (*IsCAAValidRequest) ProtoMessage() {} +func (*IsCAAValidRequest) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{0} } + +func (m *IsCAAValidRequest) GetDomain() string { + if m != nil && m.Domain != nil { + return *m.Domain + } + return "" +} + +// If CAA is valid for the requested domain, the problem will be empty +type IsCAAValidResponse struct { + Problem *core.ProblemDetails `protobuf:"bytes,1,opt,name=problem" json:"problem,omitempty"` + XXX_unrecognized []byte `json:"-"` +} + +func (m *IsCAAValidResponse) Reset() { *m = IsCAAValidResponse{} } +func (m *IsCAAValidResponse) String() string { return proto1.CompactTextString(m) } +func (*IsCAAValidResponse) ProtoMessage() {} +func (*IsCAAValidResponse) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{1} } + +func (m *IsCAAValidResponse) GetProblem() *core.ProblemDetails { + if m != nil { + return m.Problem + } + return nil +} + type IsSafeDomainRequest struct { Domain *string `protobuf:"bytes,1,opt,name=domain" json:"domain,omitempty"` XXX_unrecognized []byte `json:"-"` @@ -46,7 +83,7 @@ type IsSafeDomainRequest struct { func (m *IsSafeDomainRequest) Reset() { *m = IsSafeDomainRequest{} } func (m *IsSafeDomainRequest) String() string { return proto1.CompactTextString(m) } func (*IsSafeDomainRequest) ProtoMessage() {} -func (*IsSafeDomainRequest) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{0} } +func (*IsSafeDomainRequest) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{2} } func (m *IsSafeDomainRequest) GetDomain() string { if m != nil && m.Domain != nil { @@ -63,7 +100,7 @@ type IsDomainSafe struct { func (m *IsDomainSafe) Reset() { *m = IsDomainSafe{} } func (m *IsDomainSafe) String() string { return proto1.CompactTextString(m) } func (*IsDomainSafe) ProtoMessage() {} -func (*IsDomainSafe) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{1} } +func (*IsDomainSafe) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{3} } func (m *IsDomainSafe) GetIsSafe() bool { if m != nil && m.IsSafe != nil { @@ -82,7 +119,7 @@ type PerformValidationRequest struct { func (m *PerformValidationRequest) Reset() { *m = PerformValidationRequest{} } func (m *PerformValidationRequest) String() string { return proto1.CompactTextString(m) } func (*PerformValidationRequest) ProtoMessage() {} -func (*PerformValidationRequest) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{2} } +func (*PerformValidationRequest) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{4} } func (m *PerformValidationRequest) GetDomain() string { if m != nil && m.Domain != nil { @@ -114,7 +151,7 @@ type AuthzMeta struct { func (m *AuthzMeta) Reset() { *m = AuthzMeta{} } func (m *AuthzMeta) String() string { return proto1.CompactTextString(m) } func (*AuthzMeta) ProtoMessage() {} -func (*AuthzMeta) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{3} } +func (*AuthzMeta) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{5} } func (m *AuthzMeta) GetId() string { if m != nil && m.Id != nil { @@ -139,7 +176,7 @@ type ValidationResult struct { func (m *ValidationResult) Reset() { *m = ValidationResult{} } func (m *ValidationResult) String() string { return proto1.CompactTextString(m) } func (*ValidationResult) ProtoMessage() {} -func (*ValidationResult) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{4} } +func (*ValidationResult) Descriptor() ([]byte, []int) { return fileDescriptor0, []int{6} } func (m *ValidationResult) GetRecords() []*core.ValidationRecord { if m != nil { @@ -156,6 +193,8 @@ func (m *ValidationResult) GetProblems() *core.ProblemDetails { } func init() { + proto1.RegisterType((*IsCAAValidRequest)(nil), "va.IsCAAValidRequest") + proto1.RegisterType((*IsCAAValidResponse)(nil), "va.IsCAAValidResponse") proto1.RegisterType((*IsSafeDomainRequest)(nil), "va.IsSafeDomainRequest") proto1.RegisterType((*IsDomainSafe)(nil), "va.IsDomainSafe") proto1.RegisterType((*PerformValidationRequest)(nil), "va.PerformValidationRequest") @@ -268,28 +307,95 @@ var _VA_serviceDesc = grpc.ServiceDesc{ Metadata: "va/proto/va.proto", } +// Client API for CAA service + +type CAAClient interface { + IsCAAValid(ctx context.Context, in *IsCAAValidRequest, opts ...grpc.CallOption) (*IsCAAValidResponse, error) +} + +type cAAClient struct { + cc *grpc.ClientConn +} + +func NewCAAClient(cc *grpc.ClientConn) CAAClient { + return &cAAClient{cc} +} + +func (c *cAAClient) IsCAAValid(ctx context.Context, in *IsCAAValidRequest, opts ...grpc.CallOption) (*IsCAAValidResponse, error) { + out := new(IsCAAValidResponse) + err := grpc.Invoke(ctx, "/va.CAA/IsCAAValid", in, out, c.cc, opts...) + if err != nil { + return nil, err + } + return out, nil +} + +// Server API for CAA service + +type CAAServer interface { + IsCAAValid(context.Context, *IsCAAValidRequest) (*IsCAAValidResponse, error) +} + +func RegisterCAAServer(s *grpc.Server, srv CAAServer) { + s.RegisterService(&_CAA_serviceDesc, srv) +} + +func _CAA_IsCAAValid_Handler(srv interface{}, ctx context.Context, dec func(interface{}) error, interceptor grpc.UnaryServerInterceptor) (interface{}, error) { + in := new(IsCAAValidRequest) + if err := dec(in); err != nil { + return nil, err + } + if interceptor == nil { + return srv.(CAAServer).IsCAAValid(ctx, in) + } + info := &grpc.UnaryServerInfo{ + Server: srv, + FullMethod: "/va.CAA/IsCAAValid", + } + handler := func(ctx context.Context, req interface{}) (interface{}, error) { + return srv.(CAAServer).IsCAAValid(ctx, req.(*IsCAAValidRequest)) + } + return interceptor(ctx, in, info, handler) +} + +var _CAA_serviceDesc = grpc.ServiceDesc{ + ServiceName: "va.CAA", + HandlerType: (*CAAServer)(nil), + Methods: []grpc.MethodDesc{ + { + MethodName: "IsCAAValid", + Handler: _CAA_IsCAAValid_Handler, + }, + }, + Streams: []grpc.StreamDesc{}, + Metadata: "va/proto/va.proto", +} + func init() { proto1.RegisterFile("va/proto/va.proto", fileDescriptor0) } var fileDescriptor0 = []byte{ - // 313 bytes of a gzipped FileDescriptorProto - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x74, 0x91, 0xc1, 0x4f, 0xc2, 0x30, - 0x14, 0xc6, 0xd9, 0x08, 0x02, 0x0f, 0x51, 0xa8, 0xa8, 0x0b, 0x21, 0x86, 0x34, 0x11, 0x39, 0x8d, - 0x84, 0xab, 0x27, 0x94, 0xcb, 0x0e, 0x26, 0x44, 0x13, 0x0e, 0xde, 0x9e, 0xdb, 0x03, 0x96, 0x14, - 0x8a, 0x6d, 0xd9, 0xc1, 0xbf, 0xc1, 0x3f, 0xda, 0xb4, 0x9b, 0x42, 0x54, 0x6e, 0x6f, 0xdf, 0xef, - 0xdb, 0xeb, 0xd7, 0xaf, 0xd0, 0xce, 0x70, 0xb4, 0x55, 0xd2, 0xc8, 0x51, 0x86, 0xa1, 0x1b, 0x98, - 0x9f, 0x61, 0xf7, 0x32, 0x96, 0x8a, 0x0a, 0x60, 0xc7, 0x1c, 0xf1, 0x5b, 0xb8, 0x88, 0xf4, 0x0b, - 0x2e, 0x68, 0x2a, 0xd7, 0x98, 0x6e, 0x9e, 0xe9, 0x7d, 0x47, 0xda, 0xb0, 0x33, 0x38, 0x49, 0x9c, - 0x10, 0x78, 0x7d, 0x6f, 0x58, 0xe7, 0x37, 0x70, 0x1a, 0xe9, 0xdc, 0x62, 0xcd, 0x96, 0xa7, 0xee, - 0x37, 0xc7, 0x6b, 0x5c, 0x40, 0x30, 0x23, 0xb5, 0x90, 0x6a, 0x3d, 0x47, 0x91, 0x26, 0x68, 0x52, - 0x79, 0x6c, 0x17, 0xe3, 0x50, 0x8f, 0x57, 0x28, 0x04, 0x6d, 0x96, 0x14, 0xf8, 0x7d, 0x6f, 0xd8, - 0x18, 0x9f, 0x87, 0x2e, 0xd2, 0xe3, 0xb7, 0xcc, 0x7a, 0x50, 0xc1, 0x9d, 0x59, 0x7d, 0x04, 0x65, - 0xc7, 0x9b, 0x61, 0x86, 0xe1, 0xc4, 0x0a, 0x4f, 0x64, 0x90, 0x0f, 0xa0, 0xfe, 0xf3, 0xc1, 0x00, - 0xfc, 0x34, 0x29, 0x56, 0x37, 0xa1, 0xa2, 0x68, 0x19, 0x4d, 0xdd, 0xda, 0x32, 0x8f, 0xa1, 0x75, - 0x18, 0x47, 0xef, 0x84, 0x61, 0x77, 0x50, 0x55, 0x14, 0x4b, 0x95, 0xe8, 0xc0, 0xeb, 0x97, 0x87, - 0x8d, 0xf1, 0x55, 0x7e, 0xf6, 0xa1, 0xd1, 0x62, 0x36, 0x80, 0xda, 0x56, 0xc9, 0x37, 0x41, 0x6b, - 0x5d, 0xa4, 0xec, 0xe4, 0xce, 0x59, 0xae, 0x4e, 0xc9, 0x60, 0x2a, 0xf4, 0xf8, 0xd3, 0x03, 0x7f, - 0x3e, 0x61, 0xf7, 0xb6, 0xa1, 0x7d, 0x91, 0xec, 0xda, 0x46, 0xfe, 0xa7, 0xda, 0x6e, 0x2b, 0x07, - 0xfb, 0x32, 0x79, 0x89, 0x45, 0xd0, 0xfe, 0x53, 0x1f, 0xeb, 0x59, 0xe3, 0xb1, 0x56, 0xbb, 0x1d, - 0x4b, 0x7f, 0xdf, 0x8e, 0x97, 0x1e, 0xaa, 0xaf, 0x15, 0xf7, 0xb2, 0x5f, 0x01, 0x00, 0x00, 0xff, - 0xff, 0x7d, 0xf6, 0x5c, 0x13, 0x08, 0x02, 0x00, 0x00, + // 368 bytes of a gzipped FileDescriptorProto + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x74, 0x52, 0xc1, 0x8e, 0xda, 0x30, + 0x14, 0x24, 0x89, 0x28, 0xf0, 0x28, 0x2d, 0xbc, 0x02, 0x8d, 0x22, 0x54, 0x21, 0x57, 0x50, 0x4e, + 0x41, 0xca, 0x15, 0xf5, 0x90, 0x92, 0x4b, 0x0e, 0x95, 0x50, 0x2b, 0x71, 0xd8, 0x9b, 0x37, 0x31, + 0x10, 0x29, 0x60, 0xd6, 0x0e, 0x39, 0xec, 0x37, 0xec, 0x47, 0xaf, 0x6c, 0x67, 0x17, 0xc4, 0xc2, + 0xed, 0xe5, 0xcd, 0x64, 0xde, 0x78, 0x34, 0xd0, 0x2b, 0xe9, 0xfc, 0x28, 0x78, 0xc1, 0xe7, 0x25, + 0xf5, 0xf5, 0x80, 0x76, 0x49, 0xbd, 0x41, 0xc2, 0x05, 0xab, 0x00, 0x35, 0x1a, 0x88, 0xfc, 0x84, + 0x5e, 0x2c, 0x97, 0x61, 0xb8, 0xa6, 0x79, 0x96, 0xfe, 0x63, 0x4f, 0x27, 0x26, 0x0b, 0xfc, 0x02, + 0x9f, 0x52, 0xbe, 0xa7, 0xd9, 0xc1, 0xb5, 0xc6, 0xd6, 0xac, 0x45, 0x16, 0x80, 0x97, 0x24, 0x79, + 0xe4, 0x07, 0xc9, 0x70, 0x02, 0x8d, 0xa3, 0xe0, 0x8f, 0x39, 0xdb, 0x6b, 0x5a, 0x3b, 0xe8, 0xfb, + 0x5a, 0x78, 0x65, 0x96, 0x11, 0x2b, 0x68, 0x96, 0x4b, 0x32, 0x81, 0x6f, 0xb1, 0xfc, 0x4f, 0x37, + 0x2c, 0xd2, 0x92, 0xf7, 0x6e, 0xfc, 0x80, 0xcf, 0xb1, 0x34, 0x14, 0x45, 0x56, 0x78, 0xa6, 0x7f, + 0xd3, 0x78, 0x93, 0xe4, 0xe0, 0xae, 0x98, 0xd8, 0x70, 0xb1, 0xd7, 0x2e, 0x68, 0x91, 0xf1, 0x7b, + 0x5a, 0x48, 0xa0, 0x95, 0xec, 0x68, 0x9e, 0xb3, 0xc3, 0x96, 0xb9, 0xb6, 0xf6, 0xf6, 0xd5, 0x78, + 0x5b, 0xbe, 0xad, 0x71, 0x04, 0x75, 0x7a, 0x2a, 0x76, 0xcf, 0xae, 0xa3, 0xf1, 0x8e, 0x5f, 0x52, + 0x3f, 0x54, 0x8b, 0xbf, 0xac, 0xa0, 0x64, 0x0a, 0xad, 0xf7, 0x0f, 0x04, 0xb0, 0xb3, 0xb4, 0x92, + 0xee, 0x40, 0x5d, 0xb0, 0x6d, 0x1c, 0x69, 0x59, 0x87, 0x24, 0xd0, 0xbd, 0xb4, 0x23, 0x4f, 0x79, + 0x81, 0xbf, 0xa0, 0x21, 0x58, 0xc2, 0x45, 0x2a, 0x5d, 0x6b, 0xec, 0xcc, 0xda, 0xc1, 0xd0, 0xdc, + 0xbe, 0x24, 0x2a, 0x18, 0xa7, 0xd0, 0xac, 0x02, 0x94, 0x95, 0xcb, 0x9b, 0x09, 0x06, 0x2f, 0x16, + 0xd8, 0xeb, 0x10, 0x17, 0x2a, 0xa1, 0x73, 0x90, 0xf8, 0x5d, 0x59, 0xbe, 0x11, 0xad, 0xd7, 0x35, + 0xc0, 0x39, 0x4c, 0x52, 0xc3, 0x18, 0x7a, 0x1f, 0xe2, 0xc3, 0x91, 0x22, 0xde, 0x4b, 0xd5, 0xeb, + 0x2b, 0xf4, 0xfa, 0x75, 0xa4, 0x16, 0x44, 0xe0, 0x2c, 0xc3, 0x10, 0x7f, 0x03, 0x9c, 0x4b, 0x81, + 0x03, 0x73, 0xf3, 0xaa, 0x49, 0xde, 0xf0, 0x7a, 0x6d, 0xba, 0x43, 0x6a, 0x7f, 0x1a, 0x0f, 0x75, + 0xdd, 0xc0, 0xd7, 0x00, 0x00, 0x00, 0xff, 0xff, 0x8a, 0x44, 0xc0, 0x1e, 0xb0, 0x02, 0x00, 0x00, } diff --git a/va/proto/va.proto b/va/proto/va.proto index 48aa053ea..e016390e3 100644 --- a/va/proto/va.proto +++ b/va/proto/va.proto @@ -10,6 +10,19 @@ service VA { rpc PerformValidation(PerformValidationRequest) returns (ValidationResult) {} } +service CAA { + rpc IsCAAValid(IsCAAValidRequest) returns (IsCAAValidResponse) {} +} + +message IsCAAValidRequest { + optional string domain = 1; +} + +// If CAA is valid for the requested domain, the problem will be empty +message IsCAAValidResponse { + optional core.ProblemDetails problem = 1; +} + message IsSafeDomainRequest { optional string domain = 1; } diff --git a/wfe/wfe_test.go b/wfe/wfe_test.go index ef57d9337..02c548103 100644 --- a/wfe/wfe_test.go +++ b/wfe/wfe_test.go @@ -21,9 +21,6 @@ import ( "time" "github.com/jmhodges/clock" - "golang.org/x/net/context" - "gopkg.in/square/go-jose.v2" - "github.com/letsencrypt/boulder/core" corepb "github.com/letsencrypt/boulder/core/proto" berrors "github.com/letsencrypt/boulder/errors" @@ -38,6 +35,10 @@ import ( rapb "github.com/letsencrypt/boulder/ra/proto" "github.com/letsencrypt/boulder/revocation" "github.com/letsencrypt/boulder/test" + vaPB "github.com/letsencrypt/boulder/va/proto" + "golang.org/x/net/context" + "google.golang.org/grpc" + "gopkg.in/square/go-jose.v2" ) const ( @@ -785,6 +786,17 @@ func TestRandomDirectoryKey(t *testing.T) { } } +// noopCAA implements RA's caaChecker, always returning nil +type noopCAA struct{} + +func (cr noopCAA) IsCAAValid( + ctx context.Context, + in *vaPB.IsCAAValidRequest, + opts ...grpc.CallOption, +) (*vaPB.IsCAAValidResponse, error) { + return &vaPB.IsCAAValidResponse{}, nil +} + func TestRelativeDirectory(t *testing.T) { _ = features.Set(map[string]bool{"AllowKeyRollover": true}) defer features.Reset() @@ -853,6 +865,7 @@ func TestIssueCertificate(t *testing.T) { // TODO: Use a mock RA so we can test various conditions of authorized, not // authorized, etc. stats := metrics.NewNoopScope() + ra := ra.NewRegistrationAuthorityImpl( fc, wfe.log, @@ -865,6 +878,7 @@ func TestIssueCertificate(t *testing.T) { 300*24*time.Hour, 7*24*time.Hour, nil, + noopCAA{}, 0, ) ra.SA = mocks.NewStorageAuthority(fc) From 4ec662ee59389b482565be1a979da3fd2a26c36c Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Tue, 29 Aug 2017 07:05:09 -0700 Subject: [PATCH 08/12] Fix data race when MatchesCSR fails (#3017) --- ra/ra.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 7ecb23732..99367fae3 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -847,11 +847,11 @@ func (ra *RegistrationAuthorityImpl) NewCertificate(ctx context.Context, req cor } if ra.publisher != nil { - go func() { + go func(der []byte) { // Since we don't want this method to be canceled if the parent context // expires, pass a background context to it and run it in a goroutine. - _ = ra.publisher.SubmitToCT(context.Background(), cert.DER) - }() + _ = ra.publisher.SubmitToCT(context.Background(), der) + }(cert.DER) } parsedCertificate, err := x509.ParseCertificate([]byte(cert.DER)) From 08cd61bcfa982273852568b6011207d6e913def3 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 30 Aug 2017 11:57:08 -0700 Subject: [PATCH 09/12] Consolidate "rpm" Travis task with another task. (#3022) Travis only allows us 5 simultaneous build jobs, so going from 6 to 5 jobs per build should reduce the wall time required to get a CI result on any given branch. --- .travis.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 64dc63181..eb40700cd 100644 --- a/.travis.yml +++ b/.travis.yml @@ -38,14 +38,13 @@ env: - PATH=$HOME/bin:$PATH # protoc gets installed here - GO15VENDOREXPERIMENT=1 matrix: - - RUN="vet fmt migrations integration godep-restore errcheck generate dashlint" + - RUN="vet fmt migrations integration godep-restore errcheck generate dashlint rpm" # Config changes that have landed in master but not yet been applied to # production can be made in boulder-config-next.json. - RUN="integration" BOULDER_CONFIG_DIR="test/config-next" - RUN="unit" - RUN="unit-next" BOULDER_CONFIG_DIR="test/config-next" - RUN="coverage" - - RUN="rpm" install: - ./test/travis-before-install.sh From ba957936284a6e8b2b0aa17b8c31d0e7716c5227 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 30 Aug 2017 12:03:27 -0700 Subject: [PATCH 10/12] Remove all named returns from RA. (#3021) This is a followup from https://github.com/letsencrypt/boulder/pull/3017, in which we identified a data race caused by the use of named returns. This also reverts the change from that PR, which was only a surface level fix. Fixes #3019. --- ra/ra.go | 130 +++++++++++++++++++++++-------------------------------- 1 file changed, 54 insertions(+), 76 deletions(-) diff --git a/ra/ra.go b/ra/ra.go index 99367fae3..6424a2530 100644 --- a/ra/ra.go +++ b/ra/ra.go @@ -334,15 +334,15 @@ func (ra *RegistrationAuthorityImpl) checkRegistrationLimits(ctx context.Context } // NewRegistration constructs a new Registration from a request. -func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, init core.Registration) (reg core.Registration, err error) { - if err = ra.keyPolicy.GoodKey(init.Key.Key); err != nil { +func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, init core.Registration) (core.Registration, error) { + if err := ra.keyPolicy.GoodKey(init.Key.Key); err != nil { return core.Registration{}, berrors.MalformedError("invalid public key: %s", err.Error()) } - if err = ra.checkRegistrationLimits(ctx, init.InitialIP); err != nil { + if err := ra.checkRegistrationLimits(ctx, init.InitialIP); err != nil { return core.Registration{}, err } - reg = core.Registration{ + reg := core.Registration{ Key: init.Key, Status: core.StatusValid, } @@ -352,13 +352,12 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, init c // MergeUpdate. But we need to fill it in for new registrations. reg.InitialIP = init.InitialIP - err = ra.validateContacts(ctx, reg.Contact) - if err != nil { - return + if err := ra.validateContacts(ctx, reg.Contact); err != nil { + return core.Registration{}, err } // Store the authorization object, then return it - reg, err = ra.SA.NewRegistration(ctx, reg) + reg, err := ra.SA.NewRegistration(ctx, reg) if err != nil { // berrors.InternalServerError since the user-data was validated before being // passed to the SA. @@ -366,7 +365,7 @@ func (ra *RegistrationAuthorityImpl) NewRegistration(ctx context.Context, init c } ra.stats.Inc("NewRegistrations", 1) - return + return reg, nil } func (ra *RegistrationAuthorityImpl) validateContacts(ctx context.Context, contacts *[]string) error { @@ -472,21 +471,21 @@ func (ra *RegistrationAuthorityImpl) checkInvalidAuthorizationLimit(ctx context. // NewAuthorization constructs a new Authz from a request. Values (domains) in // request.Identifier will be lowercased before storage. -func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, request core.Authorization, regID int64) (authz core.Authorization, err error) { +func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, request core.Authorization, regID int64) (core.Authorization, error) { identifier := request.Identifier identifier.Value = strings.ToLower(identifier.Value) // Check that the identifier is present and appropriate - if err = ra.PA.WillingToIssue(identifier); err != nil { - return authz, err + if err := ra.PA.WillingToIssue(identifier); err != nil { + return core.Authorization{}, err } - if err = ra.checkPendingAuthorizationLimit(ctx, regID); err != nil { - return authz, err + if err := ra.checkPendingAuthorizationLimit(ctx, regID); err != nil { + return core.Authorization{}, err } - if err = ra.checkInvalidAuthorizationLimit(ctx, regID, identifier.Value); err != nil { - return authz, err + if err := ra.checkInvalidAuthorizationLimit(ctx, regID, identifier.Value); err != nil { + return core.Authorization{}, err } if identifier.Type == core.IdentifierDNS { @@ -494,10 +493,10 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, reque if err != nil { outErr := berrors.InternalServerError("unable to determine if domain was safe") ra.log.Warning(fmt.Sprintf("%s: %s", outErr, err)) - return authz, outErr + return core.Authorization{}, outErr } if !isSafeResp.GetIsSafe() { - return authz, berrors.UnauthorizedError( + return core.Authorization{}, berrors.UnauthorizedError( "%q was considered an unsafe domain by a third-party API", identifier.Value, ) @@ -513,7 +512,7 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, reque identifier.Value, ) ra.log.Warning(outErr.Error()) - return authz, outErr + return core.Authorization{}, outErr } if existingAuthz, ok := auths[identifier.Value]; ok { @@ -527,7 +526,7 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, reque existingAuthz.ID, ) ra.log.Warning(fmt.Sprintf("%s: %s", outErr.Error(), existingAuthz.ID)) - return authz, outErr + return core.Authorization{}, outErr } // The existing authorization must not expire within the next 24 hours for @@ -549,7 +548,7 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, reque ValidUntil: &nowishNano, }) if err != nil && !berrors.Is(err, berrors.NotFound) { - return authz, berrors.InternalServerError( + return core.Authorization{}, berrors.InternalServerError( "unable to get pending authorization for regID: %d, identifier: %s: %s", regID, identifier.Value, @@ -565,18 +564,14 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, reque expires := ra.clk.Now().Add(ra.pendingAuthorizationLifetime) - // Partially-filled object - authz = core.Authorization{ + authz, err := ra.SA.NewPendingAuthorization(ctx, core.Authorization{ Identifier: identifier, RegistrationID: regID, Status: core.StatusPending, Combinations: combinations, Challenges: challenges, Expires: &expires, - } - - // Get a pending Auth first so we can get our ID back, then update with challenges - authz, err = ra.SA.NewPendingAuthorization(ctx, authz) + }) if err != nil { // berrors.InternalServerError since the user-data was validated before being // passed to the SA. @@ -606,7 +601,7 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(ctx context.Context, reque // * IsCA is false // * ExtKeyUsage only contains ExtKeyUsageServerAuth & ExtKeyUsageClientAuth // * Subject only contains CommonName & Names -func (ra *RegistrationAuthorityImpl) MatchesCSR(parsedCertificate *x509.Certificate, csr *x509.CertificateRequest) (err error) { +func (ra *RegistrationAuthorityImpl) MatchesCSR(parsedCertificate *x509.Certificate, csr *x509.CertificateRequest) error { // Check issued certificate matches what was expected from the CSR hostNames := make([]string, len(csr.DNSNames)) copy(hostNames, csr.DNSNames) @@ -616,56 +611,46 @@ func (ra *RegistrationAuthorityImpl) MatchesCSR(parsedCertificate *x509.Certific hostNames = core.UniqueLowerNames(hostNames) if !core.KeyDigestEquals(parsedCertificate.PublicKey, csr.PublicKey) { - err = berrors.InternalServerError("generated certificate public key doesn't match CSR public key") - return + return berrors.InternalServerError("generated certificate public key doesn't match CSR public key") } if !ra.forceCNFromSAN && len(csr.Subject.CommonName) > 0 && parsedCertificate.Subject.CommonName != strings.ToLower(csr.Subject.CommonName) { - err = berrors.InternalServerError("generated certificate CommonName doesn't match CSR CommonName") - return + return berrors.InternalServerError("generated certificate CommonName doesn't match CSR CommonName") } // Sort both slices of names before comparison. parsedNames := parsedCertificate.DNSNames sort.Strings(parsedNames) sort.Strings(hostNames) if !reflect.DeepEqual(parsedNames, hostNames) { - err = berrors.InternalServerError("generated certificate DNSNames don't match CSR DNSNames") - return + return berrors.InternalServerError("generated certificate DNSNames don't match CSR DNSNames") } if !reflect.DeepEqual(parsedCertificate.IPAddresses, csr.IPAddresses) { - err = berrors.InternalServerError("generated certificate IPAddresses don't match CSR IPAddresses") - return + return berrors.InternalServerError("generated certificate IPAddresses don't match CSR IPAddresses") } if !reflect.DeepEqual(parsedCertificate.EmailAddresses, csr.EmailAddresses) { - err = berrors.InternalServerError("generated certificate EmailAddresses don't match CSR EmailAddresses") - return + return berrors.InternalServerError("generated certificate EmailAddresses don't match CSR EmailAddresses") } if len(parsedCertificate.Subject.Country) > 0 || len(parsedCertificate.Subject.Organization) > 0 || len(parsedCertificate.Subject.OrganizationalUnit) > 0 || len(parsedCertificate.Subject.Locality) > 0 || len(parsedCertificate.Subject.Province) > 0 || len(parsedCertificate.Subject.StreetAddress) > 0 || len(parsedCertificate.Subject.PostalCode) > 0 { - err = berrors.InternalServerError("generated certificate Subject contains fields other than CommonName, or SerialNumber") - return + return berrors.InternalServerError("generated certificate Subject contains fields other than CommonName, or SerialNumber") } now := ra.clk.Now() if now.Sub(parsedCertificate.NotBefore) > time.Hour*24 { - err = berrors.InternalServerError("generated certificate is back dated %s", now.Sub(parsedCertificate.NotBefore)) - return + return berrors.InternalServerError("generated certificate is back dated %s", now.Sub(parsedCertificate.NotBefore)) } if !parsedCertificate.BasicConstraintsValid { - err = berrors.InternalServerError("generated certificate doesn't have basic constraints set") - return + return berrors.InternalServerError("generated certificate doesn't have basic constraints set") } if parsedCertificate.IsCA { - err = berrors.InternalServerError("generated certificate can sign other certificates") - return + return berrors.InternalServerError("generated certificate can sign other certificates") } if !reflect.DeepEqual(parsedCertificate.ExtKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}) { - err = berrors.InternalServerError("generated certificate doesn't have correct key usage extensions") - return + return berrors.InternalServerError("generated certificate doesn't have correct key usage extensions") } - return + return nil } // checkAuthorizations checks that each requested name has a valid authorization @@ -763,7 +748,7 @@ func (ra *RegistrationAuthorityImpl) recheckCAA(ctx context.Context, names []str } // NewCertificate requests the issuance of a certificate. -func (ra *RegistrationAuthorityImpl) NewCertificate(ctx context.Context, req core.CertificateRequest, regID int64) (cert core.Certificate, err error) { +func (ra *RegistrationAuthorityImpl) NewCertificate(ctx context.Context, req core.CertificateRequest, regID int64) (core.Certificate, error) { emptyCert := core.Certificate{} var logEventResult string @@ -784,8 +769,7 @@ func (ra *RegistrationAuthorityImpl) NewCertificate(ctx context.Context, req cor }() if regID <= 0 { - err = berrors.MalformedError("invalid registration ID: %d", regID) - return emptyCert, err + return emptyCert, berrors.MalformedError("invalid registration ID: %d", regID) } registration, err := ra.SA.GetRegistration(ctx, regID) @@ -841,17 +825,18 @@ func (ra *RegistrationAuthorityImpl) NewCertificate(ctx context.Context, req cor Csr: csr.Raw, RegistrationID: ®ID, } - if cert, err = ra.CA.IssueCertificate(ctx, issueReq); err != nil { + cert, err := ra.CA.IssueCertificate(ctx, issueReq) + if err != nil { logEvent.Error = err.Error() return emptyCert, err } if ra.publisher != nil { - go func(der []byte) { + go func() { // Since we don't want this method to be canceled if the parent context // expires, pass a background context to it and run it in a goroutine. - _ = ra.publisher.SubmitToCT(context.Background(), der) - }(cert.DER) + _ = ra.publisher.SubmitToCT(context.Background(), cert.DER) + }() } parsedCertificate, err := x509.ParseCertificate([]byte(cert.DER)) @@ -1171,17 +1156,15 @@ func mergeUpdate(r *core.Registration, input core.Registration) bool { } // UpdateAuthorization updates an authorization with new values. -func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, base core.Authorization, challengeIndex int, response core.Challenge) (authz core.Authorization, err error) { +func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, base core.Authorization, challengeIndex int, response core.Challenge) (core.Authorization, error) { // Refuse to update expired authorizations if base.Expires == nil || base.Expires.Before(ra.clk.Now()) { - err = berrors.MalformedError("expired authorization") - return + return core.Authorization{}, berrors.MalformedError("expired authorization") } - authz = base + authz := base if challengeIndex >= len(authz.Challenges) { - err = berrors.MalformedError("invalid challenge index '%d'", challengeIndex) - return + return core.Authorization{}, berrors.MalformedError("invalid challenge index '%d'", challengeIndex) } ch := &authz.Challenges[challengeIndex] @@ -1203,26 +1186,23 @@ func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, ba // case and return early. if ra.reuseValidAuthz && authz.Status == core.StatusValid { ra.stats.Inc("ReusedValidAuthzChallenge", 1) - return + return authz, nil } // Look up the account key for this authorization reg, err := ra.SA.GetRegistration(ctx, authz.RegistrationID) if err != nil { - err = berrors.InternalServerError(err.Error()) - return + return core.Authorization{}, berrors.InternalServerError(err.Error()) } // Recompute the key authorization field provided by the client and // check it against the value provided expectedKeyAuthorization, err := ch.ExpectedKeyAuthorization(reg.Key) if err != nil { - err = berrors.InternalServerError("could not compute expected key authorization value") - return + return core.Authorization{}, berrors.InternalServerError("could not compute expected key authorization value") } if expectedKeyAuthorization != response.ProvidedKeyAuthorization { - err = berrors.MalformedError("provided key authorization was incorrect") - return + return core.Authorization{}, berrors.MalformedError("provided key authorization was incorrect") } // Copy information over that the client is allowed to supply @@ -1230,16 +1210,14 @@ func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, ba // Double check before sending to VA if cErr := ch.CheckConsistencyForValidation(); cErr != nil { - err = berrors.MalformedError(cErr.Error()) - return + return core.Authorization{}, berrors.MalformedError(cErr.Error()) } // Store the updated version if err = ra.SA.UpdatePendingAuthorization(ctx, authz); err != nil { ra.log.Warning(fmt.Sprintf( "Error calling ra.SA.UpdatePendingAuthorization: %s\n", err.Error())) - err = berrors.InternalServerError("could not update pending authorization") - return + return core.Authorization{}, berrors.InternalServerError("could not update pending authorization") } ra.stats.Inc("NewPendingAuthorizations", 1) @@ -1280,7 +1258,7 @@ func (ra *RegistrationAuthorityImpl) UpdateAuthorization(ctx context.Context, ba } }() ra.stats.Inc("UpdatedPendingAuthorizations", 1) - return + return authz, nil } func revokeEvent(state, serial, cn string, names []string, revocationCode revocation.Reason) string { @@ -1295,9 +1273,9 @@ func revokeEvent(state, serial, cn string, names []string, revocationCode revoca } // RevokeCertificateWithReg terminates trust in the certificate provided. -func (ra *RegistrationAuthorityImpl) RevokeCertificateWithReg(ctx context.Context, cert x509.Certificate, revocationCode revocation.Reason, regID int64) (err error) { +func (ra *RegistrationAuthorityImpl) RevokeCertificateWithReg(ctx context.Context, cert x509.Certificate, revocationCode revocation.Reason, regID int64) error { serialString := core.SerialToString(cert.SerialNumber) - err = ra.SA.MarkCertificateRevoked(ctx, serialString, revocationCode) + err := ra.SA.MarkCertificateRevoked(ctx, serialString, revocationCode) state := "Failure" defer func() { From 2549c3c80eadb50fcdf9bde2f6ee165d0c8937b8 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 31 Aug 2017 09:22:50 -0700 Subject: [PATCH 11/12] Recommend avoiding named returns. (#3027) --- CONTRIBUTING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7e1c5b063..dcfe4f80f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -23,6 +23,7 @@ Thanks for helping us build Boulder! This page contains requirements and guideli # Patch Guidelines * Please include helpful comments. No need to gratuitously comment clear code, but make sure it's clear why things are being done. * Include information in your pull request about what you're trying to accomplish with your patch. +* Avoid named return values. See [#3017](https://github.com/letsencrypt/boulder/pull/3017) for an example of a subtle problem they can cause. * Do not include `XXX`s or naked `TODO`s. Use the formats: ``` // TODO(): Hoverboard + Time-machine unsupported until upstream patch. From 99eeb019849989e2ee82757491d8f96cd73297d2 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Thu, 31 Aug 2017 14:44:25 -0400 Subject: [PATCH 12/12] Only wrap error given to `Rollback` when `tx.Rollback()` fails. (#3025) Prior to this commit the `Rollback` function always wrapped the provided error in a `sa.RollbackError`. This makes it difficult for callers to test the type of the original error. This commit updates the `Rollback` function to only return a `sa.RollbackError` when the call to `tx.Rollback()` produces an error. --- sa/rollback.go | 18 +++++++++--------- sa/rollback_test.go | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) create mode 100644 sa/rollback_test.go diff --git a/sa/rollback.go b/sa/rollback.go index d62105708..f19ced04d 100644 --- a/sa/rollback.go +++ b/sa/rollback.go @@ -26,15 +26,15 @@ func (re *RollbackError) Error() string { return fmt.Sprintf("%s (also, while rolling back: %s)", re.Err, re.RollbackErr) } -// Rollback rolls back the provided transaction (if err is non-nil) and wraps -// the error, if any, of the rollback into a RollbackError. -// -// The err parameter must be non-nil. -// -// err = sa.Rollback(tx, err) +// Rollback rolls back the provided transaction. If the rollback fails for any +// reason a `RollbackError` error is returned wrapping the original error. If no +// rollback error occurs then the original error is returned. func Rollback(tx *gorp.Transaction, err error) error { - return &RollbackError{ - Err: err, - RollbackErr: tx.Rollback(), + if txErr := tx.Rollback(); txErr != nil { + return &RollbackError{ + Err: err, + RollbackErr: txErr, + } } + return err } diff --git a/sa/rollback_test.go b/sa/rollback_test.go new file mode 100644 index 000000000..9ffc7a666 --- /dev/null +++ b/sa/rollback_test.go @@ -0,0 +1,36 @@ +package sa + +import ( + "testing" + + berrors "github.com/letsencrypt/boulder/errors" + "github.com/letsencrypt/boulder/test" +) + +func TestRollback(t *testing.T) { + sa, _, cleanUp := initSA(t) + defer cleanUp() + + tx, _ := sa.dbMap.Begin() + // Commit the transaction so that a subsequent Rollback will always fail. + _ = tx.Commit() + + innerErr := berrors.NotFoundError("Gone, gone, gone") + result := Rollback(tx, innerErr) + + // Since the tx.Rollback will fail we expect the result to be a wrapped error + test.AssertNotEquals(t, result, innerErr) + if rbErr, ok := result.(*RollbackError); !ok { + t.Fatal("Result was not a RollbackError") + test.AssertEquals(t, rbErr.Err, innerErr) + test.AssertNotNil(t, rbErr.RollbackErr, "RollbackErr was nil") + } + + // Create a new transaction and don't commit it this time. The rollback should + // succeed. + tx, _ = sa.dbMap.Begin() + result = Rollback(tx, innerErr) + + // We expect that the err is returned unwrapped. + test.AssertEquals(t, result, innerErr) +}