From 55eeec6ec7d650e5a5cb1d59c96ec70933facfa2 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Wed, 2 Sep 2015 12:58:20 -0700 Subject: [PATCH 1/6] Append rand ID to client queues --- rpc/amqp-rpc.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/rpc/amqp-rpc.go b/rpc/amqp-rpc.go index ba926fa98..6449b6689 100644 --- a/rpc/amqp-rpc.go +++ b/rpc/amqp-rpc.go @@ -486,7 +486,12 @@ func NewAmqpRPCClient(clientQueuePrefix, serverQueue string, channel *amqp.Chann return nil, err } - clientQueue := fmt.Sprintf("%s.%s", clientQueuePrefix, hostname) + randID := make([]byte, 3) + _, err = rand.Read(randID) + if err != nil { + return nil, err + } + clientQueue := fmt.Sprintf("%s.%s.%x", clientQueuePrefix, hostname, randID) rpc = &AmqpRPCCLient{ serverQueue: serverQueue, From ffcd1c866d2464b66e900cc9758a13db6a11d8f7 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 9 Sep 2015 14:40:37 -0400 Subject: [PATCH 2/6] Make challenge URI a display-time property. Challenge URIs should be determined by the WFE at fetch time, rather than stored alongside the challenge in the DB. This simplifies a lot of the logic, and allows to to remove a code path in NewAuthorization where we create an authorization, then immediately save it with modifications to the challenges. This change also gives challenges their own endpoint, which contains the challenge id rather than the challenge's offset within its parent authorization. This is also a first step towards replacing UpdateAuthorization with UpdateChallenge: https://github.com/letsencrypt/boulder/issues/760. --- cmd/boulder-ra/main.go | 2 - core/objects.go | 16 +- ra/registration-authority.go | 33 ++--- ra/registration-authority_test.go | 1 - .../20150828163255_RemoveChallengeURI.sql | 13 ++ sa/model.go | 16 +- sa/storage-authority.go | 17 ++- sa/storage-authority_test.go | 4 +- wfe/web-front-end.go | 140 +++++++++++------- wfe/web-front-end_test.go | 49 +++--- 10 files changed, 164 insertions(+), 127 deletions(-) create mode 100644 sa/_db/migrations/20150828163255_RemoveChallengeURI.sql diff --git a/cmd/boulder-ra/main.go b/cmd/boulder-ra/main.go index da77da9c0..e03fc672b 100644 --- a/cmd/boulder-ra/main.go +++ b/cmd/boulder-ra/main.go @@ -18,7 +18,6 @@ import ( blog "github.com/letsencrypt/boulder/log" "github.com/letsencrypt/boulder/ra" "github.com/letsencrypt/boulder/rpc" - "github.com/letsencrypt/boulder/wfe" ) func main() { @@ -44,7 +43,6 @@ func main() { cmd.FailOnError(err, "Couldn't create PA") rai := ra.NewRegistrationAuthorityImpl(clock.Default(), auditlogger) - rai.AuthzBase = c.Common.BaseURL + wfe.AuthzPath rai.MaxKeySize = c.Common.MaxKeySize rai.PA = pa raDNSTimeout, err := time.ParseDuration(c.Common.DNSTimeout) diff --git a/core/objects.go b/core/objects.go index 1d6456c7d..b3d617e10 100644 --- a/core/objects.go +++ b/core/objects.go @@ -241,6 +241,8 @@ type ValidationRecord struct { // challenge, we just throw all the elements into one bucket, // together with the common metadata elements. type Challenge struct { + ID int64 `json:"id,omitempty"` + // The type of challenge Type string `json:"type"` @@ -255,7 +257,7 @@ type Challenge struct { Validated *time.Time `json:"validated,omitempty"` // A URI to which a response can be POSTed - URI *AcmeURL `json:"uri"` + URI string `json:"uri"` // Used by simpleHttp, dvsni, and dns challenges Token string `json:"token,omitempty"` @@ -431,6 +433,18 @@ type Authorization struct { Combinations [][]int `json:"combinations,omitempty" db:"combinations"` } +// FindChallenge will look for the given challenge inside this authorization. If +// found, it will return the index of that challenge within the Authorization's +// Challenges array. Otherwise it will return -1. +func (authz *Authorization) FindChallenge(challengeID int64) int { + for i, c := range authz.Challenges { + if c.ID == challengeID { + return i + } + } + return -1 +} + // JSONBuffer fields get encoded and decoded JOSE-style, in base64url encoding // with stripped padding. type JSONBuffer []byte diff --git a/ra/registration-authority.go b/ra/registration-authority.go index 1f55eb3bb..3ca578101 100644 --- a/ra/registration-authority.go +++ b/ra/registration-authority.go @@ -10,7 +10,6 @@ import ( "errors" "fmt" "net/mail" - "strconv" "strings" "time" @@ -32,7 +31,6 @@ type RegistrationAuthorityImpl struct { clk clock.Clock log *blog.AuditLogger - AuthzBase string MaxKeySize int } @@ -151,6 +149,11 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization // Create validations, but we have to update them with URIs later challenges, combinations := ra.PA.ChallengesFor(identifier) + for i, _ := range challenges { + // Add the account key used to generate the challenge + challenges[i].AccountKey = ®.Key + } + // Partially-filled object authz = core.Authorization{ Identifier: identifier, @@ -166,33 +169,19 @@ func (ra *RegistrationAuthorityImpl) NewAuthorization(request core.Authorization // InternalServerError since the user-data was validated before being // passed to the SA. err = core.InternalServerError(fmt.Sprintf("Invalid authorization request: %s", err)) - return authz, err + return core.Authorization{}, err } - // Construct all the challenge URIs - for i := range authz.Challenges { - // Ignoring these errors because we construct the URLs to be correct - challengeURI, _ := core.ParseAcmeURL(ra.AuthzBase + authz.ID + "?challenge=" + strconv.Itoa(i)) - authz.Challenges[i].URI = challengeURI - - // Add the account key used to generate the challenge - authz.Challenges[i].AccountKey = ®.Key - - if !authz.Challenges[i].IsSane(false) { + // Check each challenge for sanity. + for _, challenge := range authz.Challenges { + if !challenge.IsSane(false) { // InternalServerError because we generated these challenges, they should // be OK. - err = core.InternalServerError(fmt.Sprintf("Challenge didn't pass sanity check: %+v", authz.Challenges[i])) - return authz, err + err = core.InternalServerError(fmt.Sprintf("Challenge didn't pass sanity check: %+v", challenge)) + return core.Authorization{}, err } } - // Store the authorization object, then return it - err = ra.SA.UpdatePendingAuthorization(authz) - if err != nil { - // InternalServerError because we created the authorization just above, - // and adding Sane challenges should not break it. - err = core.InternalServerError(err.Error()) - } return authz, err } diff --git a/ra/registration-authority_test.go b/ra/registration-authority_test.go index 81fbef207..9ff430876 100644 --- a/ra/registration-authority_test.go +++ b/ra/registration-authority_test.go @@ -215,7 +215,6 @@ func initAuthorities(t *testing.T) (*DummyValidationAuthority, *sa.SQLStorageAut ra.VA = va ra.CA = &ca ra.PA = pa - ra.AuthzBase = "http://acme.invalid/authz/" ra.MaxKeySize = 4096 ra.DNSResolver = &mocks.MockDNS{} diff --git a/sa/_db/migrations/20150828163255_RemoveChallengeURI.sql b/sa/_db/migrations/20150828163255_RemoveChallengeURI.sql new file mode 100644 index 000000000..ed640adf9 --- /dev/null +++ b/sa/_db/migrations/20150828163255_RemoveChallengeURI.sql @@ -0,0 +1,13 @@ + +-- +goose Up +-- SQL in section 'Up' is executed when this migration is applied + +ALTER TABLE `challenges` DROP COLUMN `uri`; + + +-- +goose Down +-- SQL section 'Down' is executed when this migration is rolled back + +ALTER TABLE `challenges` ADD COLUMN ( + `uri` varchar(255) +); diff --git a/sa/model.go b/sa/model.go index e206331ee..0d0216414 100644 --- a/sa/model.go +++ b/sa/model.go @@ -36,7 +36,6 @@ type challModel struct { Status core.AcmeStatus `db:"status"` Error []byte `db:"error"` Validated *time.Time `db:"validated"` - URI string `db:"uri"` Token string `db:"token"` TLS *bool `db:"tls"` Validation []byte `db:"validation"` @@ -85,6 +84,7 @@ func modelToRegistration(rm *regModel) (core.Registration, error) { func challengeToModel(c *core.Challenge, authID string) (*challModel, error) { cm := challModel{ + ID: c.ID, AuthorizationID: authID, Type: c.Type, Status: c.Status, @@ -108,12 +108,6 @@ func challengeToModel(c *core.Challenge, authID string) (*challModel, error) { } cm.Error = errJSON } - if c.URI != nil { - if len(c.URI.String()) > 255 { - return nil, fmt.Errorf("URI is too long to store in the database") - } - cm.URI = c.URI.String() - } if len(c.ValidationRecord) > 0 { vrJSON, err := json.Marshal(c.ValidationRecord) if err != nil { @@ -139,19 +133,13 @@ func challengeToModel(c *core.Challenge, authID string) (*challModel, error) { func modelToChallenge(cm *challModel) (core.Challenge, error) { c := core.Challenge{ + ID: cm.ID, Type: cm.Type, Status: cm.Status, Validated: cm.Validated, Token: cm.Token, TLS: cm.TLS, } - if len(cm.URI) > 0 { - uri, err := core.ParseAcmeURL(cm.URI) - if err != nil { - return core.Challenge{}, err - } - c.URI = uri - } if len(cm.Validation) > 0 { val, err := jose.ParseSigned(string(cm.Validation)) if err != nil { diff --git a/sa/storage-authority.go b/sa/storage-authority.go index 55022e28c..6b3d3a8f2 100644 --- a/sa/storage-authority.go +++ b/sa/storage-authority.go @@ -440,17 +440,28 @@ func (ssa *SQLStorageAuthority) NewPendingAuthorization(authz core.Authorization return } - for _, c := range authz.Challenges { - chall, err := challengeToModel(&c, pendingAuthz.ID) + for i, c := range authz.Challenges { + challModel, err := challengeToModel(&c, pendingAuthz.ID) if err != nil { tx.Rollback() return core.Authorization{}, err } - err = tx.Insert(chall) + // Magic happens here: Gorp will modify challModel, setting challModel.ID + // to the auto-increment primary key. This is important because we want + // the challenge objects inside the Authorization we return to know their + // IDs, so they can have proper URLs. + // See https://godoc.org/github.com/coopernurse/gorp#DbMap.Insert + err = tx.Insert(challModel) if err != nil { tx.Rollback() return core.Authorization{}, err } + challenge, err := modelToChallenge(challModel) + if err != nil { + tx.Rollback() + return core.Authorization{}, err + } + authz.Challenges[i] = challenge } err = tx.Commit() diff --git a/sa/storage-authority_test.go b/sa/storage-authority_test.go index c6b50bfd8..206c73e6b 100644 --- a/sa/storage-authority_test.go +++ b/sa/storage-authority_test.go @@ -177,9 +177,7 @@ func CreateDomainAuthWithRegId(t *testing.T, domainName string, sa *SQLStorageAu test.Assert(t, authz.ID != "", "ID shouldn't be blank") // prepare challenge for auth - u, err := core.ParseAcmeURL(domainName) - test.AssertNotError(t, err, "Couldn't parse domainName "+domainName) - chall := core.Challenge{Type: "simpleHttp", Status: core.StatusValid, URI: u, Token: "THISWOULDNTBEAGOODTOKEN"} + chall := core.Challenge{Type: "simpleHttp", Status: core.StatusValid, URI: domainName, Token: "THISWOULDNTBEAGOODTOKEN"} combos := make([][]int, 1) combos[0] = []int{0, 1} exp := time.Now().AddDate(0, 0, 1) // expire in 1 day diff --git a/wfe/web-front-end.go b/wfe/web-front-end.go index 2a49d647d..35891cb55 100644 --- a/wfe/web-front-end.go +++ b/wfe/web-front-end.go @@ -32,6 +32,7 @@ const ( RegPath = "/acme/reg/" NewAuthzPath = "/acme/new-authz" AuthzPath = "/acme/authz/" + ChallengePath = "/acme/challenge/" NewCertPath = "/acme/new-cert" CertPath = "/acme/cert/" RevokeCertPath = "/acme/revoke-cert" @@ -47,13 +48,14 @@ type WebFrontEndImpl struct { log *blog.AuditLogger // URL configuration parameters - BaseURL string - NewReg string - RegBase string - NewAuthz string - AuthzBase string - NewCert string - CertBase string + BaseURL string + NewReg string + RegBase string + NewAuthz string + AuthzBase string + ChallengeBase string + NewCert string + CertBase string // JSON encoded endpoint directory DirectoryJSON []byte @@ -195,6 +197,7 @@ func (wfe *WebFrontEndImpl) Handler() (http.Handler, error) { wfe.RegBase = wfe.BaseURL + RegPath wfe.NewAuthz = wfe.BaseURL + NewAuthzPath wfe.AuthzBase = wfe.BaseURL + AuthzPath + wfe.ChallengeBase = wfe.BaseURL + ChallengePath wfe.NewCert = wfe.BaseURL + NewCertPath wfe.CertBase = wfe.BaseURL + CertPath @@ -218,7 +221,8 @@ func (wfe *WebFrontEndImpl) Handler() (http.Handler, error) { wfe.HandleFunc(m, NewAuthzPath, wfe.NewAuthorization, "POST") wfe.HandleFunc(m, NewCertPath, wfe.NewCertificate, "POST") wfe.HandleFunc(m, RegPath, wfe.Registration, "POST") - wfe.HandleFunc(m, AuthzPath, wfe.Authorization, "GET", "POST") + wfe.HandleFunc(m, AuthzPath, wfe.Authorization, "GET") + wfe.HandleFunc(m, ChallengePath, wfe.Challenge, "GET", "POST") wfe.HandleFunc(m, CertPath, wfe.Certificate, "GET") wfe.HandleFunc(m, RevokeCertPath, wfe.RevokeCertificate, "POST") wfe.HandleFunc(m, TermsPath, wfe.Terms, "GET") @@ -549,8 +553,7 @@ func (wfe *WebFrontEndImpl) NewAuthorization(response http.ResponseWriter, reque // Make a URL for this authz, then blow away the ID and RegID before serializing authzURL := wfe.AuthzBase + string(authz.ID) - authz.ID = "" - authz.RegistrationID = 0 + wfe.prepAuthorizationForDisplay(&authz) responseBody, err := json.Marshal(authz) if err != nil { logEvent.Error = err.Error() @@ -757,46 +760,93 @@ func (wfe *WebFrontEndImpl) NewCertificate(response http.ResponseWriter, request wfe.Stats.Inc("Certificates", 1, 1.0) } -func (wfe *WebFrontEndImpl) challenge( +func (wfe *WebFrontEndImpl) Challenge( response http.ResponseWriter, - request *http.Request, - authz core.Authorization, - logEvent *requestEvent) { + request *http.Request) { + logEvent := wfe.populateRequestEvent(request) + defer wfe.logRequestDetails(&logEvent) - // Check that the requested challenge exists within the authorization - found := false - var challengeIndex int - var challenge core.Challenge - for i, challenge := range authz.Challenges { - tempURL := challenge.URI - if tempURL.Path == request.URL.Path && tempURL.RawQuery == request.URL.RawQuery { - found = true - challengeIndex = i - break - } + notFound := func() { + wfe.sendError(response, "No such registration", request.URL.Path, http.StatusNotFound) } - if !found { - logEvent.Error = "Unable to find challenge" - wfe.sendError(response, logEvent.Error, request.URL.RawQuery, http.StatusNotFound) + // Challenge URIs are of the form /acme/challenge//. + // Here we parse out the id components. TODO: Use a better tool to parse out + // URL structure: https://github.com/letsencrypt/boulder/issues/437 + slug := strings.Split(request.URL.Path[len(ChallengePath):], "/") + if len(slug) != 2 { + notFound() return } + authorizationID := slug[0] + challengeID, err := strconv.ParseInt(slug[1], 10, 64) + if err != nil { + notFound() + return + } + logEvent.Extra["AuthorizationID"] = authorizationID + logEvent.Extra["ChallengeID"] = challengeID + + authz, err := wfe.SA.GetAuthorization(authorizationID) + if err != nil { + notFound() + return + } + // Check that the requested challenge exists within the authorization + challengeIndex := authz.FindChallenge(challengeID) + if challengeIndex == -1 { + notFound() + return + } + challenge := authz.Challenges[challengeIndex] + + logEvent.Extra["ChallengeType"] = challenge.Type + logEvent.Extra["AuthorizationRegistrationID"] = authz.RegistrationID + logEvent.Extra["AuthorizationIdentifier"] = authz.Identifier + logEvent.Extra["AuthorizationStatus"] = authz.Status + logEvent.Extra["AuthorizationExpires"] = authz.Expires switch request.Method { case "GET": - wfe.getChallenge(response, request, authz, challenge, logEvent) + wfe.getChallenge(response, request, authz, challenge, &logEvent) case "POST": - wfe.postChallenge(response, request, authz, challengeIndex, logEvent) + wfe.postChallenge(response, request, authz, challengeIndex, &logEvent) } } +// prepChallengeForDisplay takes a core.Challenge and prepares it for display to +// the client by filling in its URI field and clearing its AccountKey and ID +// fields. +// TODO: Come up with a cleaner way to do this. +// https://github.com/letsencrypt/boulder/issues/761 +func (wfe *WebFrontEndImpl) prepChallengeForDisplay(authz core.Authorization, challenge *core.Challenge) { + challenge.URI = fmt.Sprintf("%s%s/%d", wfe.ChallengeBase, authz.ID, challenge.ID) + challenge.AccountKey = nil + // 0 is considered "empty" for the purpose of the JSON omitempty tag. + challenge.ID = 0 +} + +// prepAuthorizationForDisplay takes a core.Authorization and prepares it for +// display to the client by clearing its ID and RegistrationID fields, and +// preparing all its challenges. +func (wfe *WebFrontEndImpl) prepAuthorizationForDisplay(authz *core.Authorization) { + for i, _ := range authz.Challenges { + wfe.prepChallengeForDisplay(*authz, &authz.Challenges[i]) + } + authz.ID = "" + authz.RegistrationID = 0 +} + func (wfe *WebFrontEndImpl) getChallenge( response http.ResponseWriter, request *http.Request, authz core.Authorization, challenge core.Challenge, logEvent *requestEvent) { + + wfe.prepChallengeForDisplay(authz, &challenge) + jsonReply, err := json.Marshal(challenge) if err != nil { logEvent.Error = err.Error() @@ -807,7 +857,7 @@ func (wfe *WebFrontEndImpl) getChallenge( } authzURL := wfe.AuthzBase + string(authz.ID) - response.Header().Add("Location", challenge.URI.String()) + response.Header().Add("Location", challenge.URI) response.Header().Set("Content-Type", "application/json") response.Header().Add("Link", link(authzURL, "up")) response.WriteHeader(http.StatusAccepted) @@ -874,6 +924,7 @@ func (wfe *WebFrontEndImpl) postChallenge( // assumption: UpdateAuthorization does not modify order of challenges challenge := updatedAuthorization.Challenges[challengeIndex] + wfe.prepChallengeForDisplay(authz, &challenge) jsonReply, err := json.Marshal(challenge) if err != nil { logEvent.Error = err.Error() @@ -883,7 +934,7 @@ func (wfe *WebFrontEndImpl) postChallenge( } authzURL := wfe.AuthzBase + string(authz.ID) - response.Header().Add("Location", challenge.URI.String()) + response.Header().Add("Location", challenge.URI) response.Header().Set("Content-Type", "application/json") response.Header().Add("Link", link(authzURL, "up")) response.WriteHeader(http.StatusAccepted) @@ -997,31 +1048,8 @@ func (wfe *WebFrontEndImpl) Authorization(response http.ResponseWriter, request logEvent.Extra["AuthorizationStatus"] = authz.Status logEvent.Extra["AuthorizationExpires"] = authz.Expires - // If there is a fragment, then this is actually a request to a challenge URI - if len(request.URL.RawQuery) != 0 { - wfe.challenge(response, request, authz, &logEvent) - } else if request.Method == "GET" { - wfe.GetAuthorization(response, request, authz, &logEvent) - } else { - // For challenges, POST and GET are allowed. For authorizations only GET is - // allowed. - // TODO(jsha): Split challenge updates into a different path so we can use - // the HandleFunc functionality for declaring allowed methods. - // https://github.com/letsencrypt/boulder/issues/638 - logEvent.Error = "Method not allowed" - response.Header().Set("Allow", "GET") - wfe.sendError(response, logEvent.Error, request.Method, http.StatusMethodNotAllowed) - } -} + wfe.prepAuthorizationForDisplay(&authz) -func (wfe *WebFrontEndImpl) GetAuthorization( - response http.ResponseWriter, - request *http.Request, - authz core.Authorization, - logEvent *requestEvent) { - // Blank out ID and regID - authz.ID = "" - authz.RegistrationID = 0 jsonReply, err := json.Marshal(authz) if err != nil { logEvent.Error = err.Error() diff --git a/wfe/web-front-end_test.go b/wfe/web-front-end_test.go index 36e92ead2..d5836f7fd 100644 --- a/wfe/web-front-end_test.go +++ b/wfe/web-front-end_test.go @@ -173,7 +173,20 @@ func (sa *MockSA) GetRegistrationByKey(jwk jose.JsonWebKey) (core.Registration, func (sa *MockSA) GetAuthorization(id string) (core.Authorization, error) { if id == "valid" { exp := time.Now().AddDate(100, 0, 0) - return core.Authorization{Status: core.StatusValid, RegistrationID: 1, Expires: &exp, Identifier: core.AcmeIdentifier{Type: "dns", Value: "not-an-example.com"}}, nil + return core.Authorization{ + ID: "valid", + Status: core.StatusValid, + RegistrationID: 1, + Expires: &exp, + Identifier: core.AcmeIdentifier{Type: "dns", Value: "not-an-example.com"}, + Challenges: []core.Challenge{ + core.Challenge{ + ID: 23, + Type: "dns", + URI: "http://localhost:4300/acme/challenge/valid/23", + }, + }, + }, nil } return core.Authorization{}, nil } @@ -356,6 +369,7 @@ func setupWFE(t *testing.T) WebFrontEndImpl { wfe.RegBase = wfe.BaseURL + RegPath wfe.NewAuthz = wfe.BaseURL + NewAuthzPath wfe.AuthzBase = wfe.BaseURL + AuthzPath + wfe.ChallengeBase = wfe.BaseURL + ChallengePath wfe.NewCert = wfe.BaseURL + NewCertPath wfe.CertBase = wfe.BaseURL + CertPath wfe.SubscriberAgreementURL = agreementURL @@ -470,7 +484,8 @@ func TestStandardHeaders(t *testing.T) { {wfe.NewReg, []string{"POST"}}, {wfe.RegBase, []string{"POST"}}, {wfe.NewAuthz, []string{"POST"}}, - {wfe.AuthzBase, []string{"GET", "POST"}}, + {wfe.AuthzBase, []string{"GET"}}, + {wfe.ChallengeBase, []string{"GET", "POST"}}, {wfe.NewCert, []string{"POST"}}, {wfe.CertBase, []string{"GET"}}, {wfe.SubscriberAgreementURL, []string{"GET"}}, @@ -700,37 +715,21 @@ func TestChallenge(t *testing.T) { `), &key) test.AssertNotError(t, err, "Could not unmarshal testing key") - challengeURL := "/acme/authz/asdf?challenge=foo" - challengeAcme := (*core.AcmeURL)(mustParseURL(challengeURL)) - authz := core.Authorization{ - ID: "asdf", - Identifier: core.AcmeIdentifier{ - Type: "dns", - Value: "letsencrypt.org", - }, - Challenges: []core.Challenge{ - core.Challenge{ - Type: "dns", - URI: challengeAcme, - }, - }, - RegistrationID: 1, - } - - wfe.challenge(responseWriter, + challengeURL := "/acme/challenge/valid/23" + wfe.Challenge(responseWriter, makePostRequestWithPath(challengeURL, - signRequest(t, `{"resource":"challenge"}`, &wfe.nonceService)), - authz, &requestEvent{}) + signRequest(t, `{"resource":"challenge"}`, &wfe.nonceService))) + test.AssertEquals(t, responseWriter.Code, 202) test.AssertEquals( t, responseWriter.Header().Get("Location"), - "/acme/authz/asdf?challenge=foo") + challengeURL) test.AssertEquals( t, responseWriter.Header().Get("Link"), - `;rel="up"`) + `;rel="up"`) test.AssertEquals( t, responseWriter.Body.String(), - `{"type":"dns","uri":"/acme/authz/asdf?challenge=foo"}`) + `{"type":"dns","uri":"/acme/challenge/valid/23"}`) } func TestNewRegistration(t *testing.T) { From 7808f8fc0084ab3ba85d4671c54e6acba493b810 Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Wed, 9 Sep 2015 13:46:07 -0700 Subject: [PATCH 3/6] Set expiration on amqp.Publish calls, fixes #757 --- rpc/amqp-rpc.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rpc/amqp-rpc.go b/rpc/amqp-rpc.go index 6449b6689..538a2c9d2 100644 --- a/rpc/amqp-rpc.go +++ b/rpc/amqp-rpc.go @@ -366,6 +366,7 @@ func (rpc *AmqpRPCServer) processMessage(msg amqp.Delivery) { CorrelationId: msg.CorrelationId, Type: msg.Type, Body: jsonResponse, // XXX-JWS: jws.Sign(privKey, body) + Expiration: "30000", }) } @@ -563,6 +564,7 @@ func (rpc *AmqpRPCCLient) Dispatch(method string, body []byte) chan []byte { ReplyTo: rpc.clientQueue, Type: method, Body: body, // XXX-JWS: jws.Sign(privKey, body) + Expiration: "30000", }) return responseChan From 09c2a05a0158c02500a972be62c6db6e46010449 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Wed, 9 Sep 2015 16:50:54 -0400 Subject: [PATCH 4/6] Fix index method. The HTML reply pointed to the new-reg URL, when it should point to the directory. Also fix https://github.com/letsencrypt/boulder/issues/717 by checking first whether the request path is exactly "/" and giving 404 otherwise. --- wfe/web-front-end.go | 30 +++++++++++++++++++----------- wfe/web-front-end_test.go | 27 ++++++++++++++++++++++++--- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/wfe/web-front-end.go b/wfe/web-front-end.go index 2a49d647d..c31cc40bb 100644 --- a/wfe/web-front-end.go +++ b/wfe/web-front-end.go @@ -11,7 +11,6 @@ import ( "database/sql" "encoding/json" "fmt" - "html/template" "io/ioutil" "net/http" "regexp" @@ -212,7 +211,6 @@ func (wfe *WebFrontEndImpl) Handler() (http.Handler, error) { wfe.DirectoryJSON = directoryJSON m := http.NewServeMux() - wfe.HandleFunc(m, "/", wfe.Index, "GET") wfe.HandleFunc(m, DirectoryPath, wfe.Directory, "GET") wfe.HandleFunc(m, NewRegPath, wfe.NewRegistration, "POST") wfe.HandleFunc(m, NewAuthzPath, wfe.NewAuthorization, "POST") @@ -224,6 +222,10 @@ func (wfe *WebFrontEndImpl) Handler() (http.Handler, error) { wfe.HandleFunc(m, TermsPath, wfe.Terms, "GET") wfe.HandleFunc(m, IssuerPath, wfe.Issuer, "GET") wfe.HandleFunc(m, BuildIDPath, wfe.BuildID, "GET") + // We don't use our special HandleFunc for "/" because it matches everything, + // meaning we can wind up returning 405 when we mean to return 404. See + // https://github.com/letsencrypt/boulder/issues/717 + m.HandleFunc("/", wfe.Index) return m, nil } @@ -243,16 +245,22 @@ func (wfe *WebFrontEndImpl) Index(response http.ResponseWriter, request *http.Re return } - tmpl := template.Must(template.New("body").Parse(` - - This is an ACME - Certificate Authority running Boulder, - New registration is available at {{.NewReg}}. - - -`)) - tmpl.Execute(response, wfe) + if request.Method != "GET" { + logEvent.Error = "Bad method" + response.Header().Set("Allow", "GET") + response.WriteHeader(http.StatusMethodNotAllowed) + return + } + response.Header().Set("Content-Type", "text/html") + response.Write([]byte(fmt.Sprintf(` + + This is an ACME + Certificate Authority running Boulder. + JSON directory is available at %s. + + + `, DirectoryPath, DirectoryPath))) addCacheHeader(response, wfe.IndexCacheDuration.Seconds()) } diff --git a/wfe/web-front-end_test.go b/wfe/web-front-end_test.go index 490f8c7ca..a1bbfa430 100644 --- a/wfe/web-front-end_test.go +++ b/wfe/web-front-end_test.go @@ -466,7 +466,6 @@ func TestStandardHeaders(t *testing.T) { path string allowed []string }{ - {"/", []string{"GET"}}, {wfe.NewReg, []string{"POST"}}, {wfe.RegBase, []string{"POST"}}, {wfe.NewAuthz, []string{"POST"}}, @@ -492,6 +491,28 @@ func TestStandardHeaders(t *testing.T) { } } +func TestIndexPOST(t *testing.T) { + wfe := setupWFE(t) + responseWriter := httptest.NewRecorder() + url, _ := url.Parse("/") + wfe.Index(responseWriter, &http.Request{ + Method: "POST", + URL: url, + }) + test.AssertEquals(t, responseWriter.Code, http.StatusMethodNotAllowed) +} + +func TestPOST404(t *testing.T) { + wfe := setupWFE(t) + responseWriter := httptest.NewRecorder() + url, _ := url.Parse("/foobar") + wfe.Index(responseWriter, &http.Request{ + Method: "POST", + URL: url, + }) + test.AssertEquals(t, responseWriter.Code, http.StatusNotFound) +} + func TestIndex(t *testing.T) { wfe := setupWFE(t) wfe.IndexCacheDuration = time.Second * 10 @@ -505,8 +526,8 @@ func TestIndex(t *testing.T) { }) test.AssertEquals(t, responseWriter.Code, http.StatusOK) test.AssertNotEquals(t, responseWriter.Body.String(), "404 page not found\n") - test.Assert(t, strings.Contains(responseWriter.Body.String(), wfe.NewReg), - "new-reg not found") + test.Assert(t, strings.Contains(responseWriter.Body.String(), DirectoryPath), + "directory path not found") test.AssertEquals(t, responseWriter.Header().Get("Cache-Control"), "public, max-age=10") responseWriter.Body.Reset() From 442e6e28e5141d0c07725bb482242ff4d5d130b6 Mon Sep 17 00:00:00 2001 From: Jeff Hodges Date: Thu, 10 Sep 2015 12:07:32 -0700 Subject: [PATCH 5/6] set /directory's content-type correctly to json Fixes #769 --- wfe/web-front-end.go | 1 + wfe/web-front-end_test.go | 1 + 2 files changed, 2 insertions(+) diff --git a/wfe/web-front-end.go b/wfe/web-front-end.go index c502de3ae..c817ae276 100644 --- a/wfe/web-front-end.go +++ b/wfe/web-front-end.go @@ -265,6 +265,7 @@ func addCacheHeader(w http.ResponseWriter, age float64) { } func (wfe *WebFrontEndImpl) Directory(response http.ResponseWriter, request *http.Request) { + response.Header().Set("Content-Type", "application/json") response.Write(wfe.DirectoryJSON) } diff --git a/wfe/web-front-end_test.go b/wfe/web-front-end_test.go index ce90b53e5..176f456ea 100644 --- a/wfe/web-front-end_test.go +++ b/wfe/web-front-end_test.go @@ -525,6 +525,7 @@ func TestDirectory(t *testing.T) { Method: "GET", URL: url, }) + test.AssertEquals(t, responseWriter.Header().Get("Content-Type"), "application/json") test.AssertEquals(t, responseWriter.Code, http.StatusOK) test.AssertEquals(t, responseWriter.Body.String(), `{"new-authz":"http://localhost:4300/acme/new-authz","new-cert":"http://localhost:4300/acme/new-cert","new-reg":"http://localhost:4300/acme/new-reg","revoke-cert":"http://localhost:4300/acme/revoke-cert"}`) } From ff0bb9a403c44de24e69ee74acbc8ad20d05b3b2 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 10 Sep 2015 15:59:01 -0400 Subject: [PATCH 6/6] Pass pointer to challenge in getChallenge, rather than value. --- wfe/web-front-end.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/wfe/web-front-end.go b/wfe/web-front-end.go index 90b37c36c..73948e68c 100644 --- a/wfe/web-front-end.go +++ b/wfe/web-front-end.go @@ -824,7 +824,7 @@ func (wfe *WebFrontEndImpl) Challenge( switch request.Method { case "GET": - wfe.getChallenge(response, request, authz, challenge, &logEvent) + wfe.getChallenge(response, request, authz, &challenge, &logEvent) case "POST": wfe.postChallenge(response, request, authz, challengeIndex, &logEvent) @@ -858,10 +858,10 @@ func (wfe *WebFrontEndImpl) getChallenge( response http.ResponseWriter, request *http.Request, authz core.Authorization, - challenge core.Challenge, + challenge *core.Challenge, logEvent *requestEvent) { - wfe.prepChallengeForDisplay(authz, &challenge) + wfe.prepChallengeForDisplay(authz, challenge) jsonReply, err := json.Marshal(challenge) if err != nil {