From f5d538caab6dc0c167d4e32990c79bbf9eff578c Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Sun, 15 Jan 2017 08:54:49 -0700 Subject: [PATCH] Close response body in error case and close first one (#341) * Close response body in error case * Ensure the body of both responses is closed when polling for cert Also make a new const of maxBodySize, and cap the number of polls to a maximum of 1000. * More correct placement for polling limit * Move const to the top --- acme/client.go | 150 +++++++++++++++++++++++++++++-------------------- acme/error.go | 15 ++--- acme/jws.go | 4 +- 3 files changed, 96 insertions(+), 73 deletions(-) diff --git a/acme/client.go b/acme/client.go index e758d24f..e824f508 100644 --- a/acme/client.go +++ b/acme/client.go @@ -11,6 +11,7 @@ import ( "io/ioutil" "log" "net" + "net/http" "regexp" "strconv" "strings" @@ -22,6 +23,9 @@ var ( Logger *log.Logger ) +// maxBodySize is the maximum size of body that we will read. +const maxBodySize = 1024 * 1024 + // logf writes a log entry. It uses Logger if not // nil, otherwise it uses the default log.Logger. func logf(format string, args ...interface{}) { @@ -610,73 +614,95 @@ func (c *Client) requestCertificateForCsr(authz []authorizationResource, bundle return CertificateResource{}, err } - cerRes := CertificateResource{ + certRes := CertificateResource{ Domain: commonName.Domain, CertURL: resp.Header.Get("Location"), - PrivateKey: privateKeyPem} + PrivateKey: privateKeyPem, + } - for { - switch resp.StatusCode { - case 201, 202: - cert, err := ioutil.ReadAll(limitReader(resp.Body, 1024*1024)) - resp.Body.Close() - if err != nil { - return CertificateResource{}, err - } - - // The server returns a body with a length of zero if the - // certificate was not ready at the time this request completed. - // Otherwise the body is the certificate. - if len(cert) > 0 { - - cerRes.CertStableURL = resp.Header.Get("Content-Location") - cerRes.AccountRef = c.user.GetRegistration().URI - - issuedCert := pemEncode(derCertificateBytes(cert)) - - // The issuer certificate link is always supplied via an "up" link - // in the response headers of a new certificate. - links := parseLinks(resp.Header["Link"]) - issuerCert, err := c.getIssuerCertificate(links["up"]) - if err != nil { - // If we fail to acquire the issuer cert, return the issued certificate - do not fail. - logf("[WARNING][%s] acme: Could not bundle issuer certificate: %v", commonName.Domain, err) - } else { - issuerCert = pemEncode(derCertificateBytes(issuerCert)) - - // If bundle is true, we want to return a certificate bundle. - // To do this, we append the issuer cert to the issued cert. - if bundle { - issuedCert = append(issuedCert, issuerCert...) - } - } - - cerRes.Certificate = issuedCert - cerRes.IssuerCertificate = issuerCert - logf("[INFO][%s] Server responded with a certificate.", commonName.Domain) - return cerRes, nil - } - - // The certificate was granted but is not yet issued. - // Check retry-after and loop. - ra := resp.Header.Get("Retry-After") - retryAfter, err := strconv.Atoi(ra) - if err != nil { - return CertificateResource{}, err - } - - logf("[INFO][%s] acme: Server responded with status 202; retrying after %ds", commonName.Domain, retryAfter) - time.Sleep(time.Duration(retryAfter) * time.Second) - - break - default: - return CertificateResource{}, handleHTTPError(resp) - } - - resp, err = httpGet(cerRes.CertURL) + maxChecks := 1000 + for i := 0; i < maxChecks; i++ { + done, err := c.checkCertResponse(resp, &certRes, bundle) + resp.Body.Close() if err != nil { return CertificateResource{}, err } + if done { + break + } + if i == maxChecks-1 { + return CertificateResource{}, fmt.Errorf("polled for certificate %d times; giving up", i) + } + resp, err = httpGet(certRes.CertURL) + if err != nil { + return CertificateResource{}, err + } + } + + return certRes, nil +} + +// checkCertResponse checks resp to see if a certificate is contained in the +// response, and if so, loads it into certRes and returns true. If the cert +// is not yet ready, it returns false. This function honors the waiting period +// required by the Retry-After header of the response, if specified. This +// function may read from resp.Body but does NOT close it. The certRes input +// should already have the Domain (common name) field populated. If bundle is +// true, the certificate will be bundled with the issuer's cert. +func (c *Client) checkCertResponse(resp *http.Response, certRes *CertificateResource, bundle bool) (bool, error) { + switch resp.StatusCode { + case 201, 202: + cert, err := ioutil.ReadAll(limitReader(resp.Body, maxBodySize)) + if err != nil { + return false, err + } + + // The server returns a body with a length of zero if the + // certificate was not ready at the time this request completed. + // Otherwise the body is the certificate. + if len(cert) > 0 { + certRes.CertStableURL = resp.Header.Get("Content-Location") + certRes.AccountRef = c.user.GetRegistration().URI + + issuedCert := pemEncode(derCertificateBytes(cert)) + + // The issuer certificate link is always supplied via an "up" link + // in the response headers of a new certificate. + links := parseLinks(resp.Header["Link"]) + issuerCert, err := c.getIssuerCertificate(links["up"]) + if err != nil { + // If we fail to acquire the issuer cert, return the issued certificate - do not fail. + logf("[WARNING][%s] acme: Could not bundle issuer certificate: %v", certRes.Domain, err) + } else { + issuerCert = pemEncode(derCertificateBytes(issuerCert)) + + // If bundle is true, we want to return a certificate bundle. + // To do this, we append the issuer cert to the issued cert. + if bundle { + issuedCert = append(issuedCert, issuerCert...) + } + } + + certRes.Certificate = issuedCert + certRes.IssuerCertificate = issuerCert + logf("[INFO][%s] Server responded with a certificate.", certRes.Domain) + return true, nil + } + + // The certificate was granted but is not yet issued. + // Check retry-after and loop. + ra := resp.Header.Get("Retry-After") + retryAfter, err := strconv.Atoi(ra) + if err != nil { + return false, err + } + + logf("[INFO][%s] acme: Server responded with status 202; retrying after %ds", certRes.Domain, retryAfter) + time.Sleep(time.Duration(retryAfter) * time.Second) + + return false, nil + default: + return false, handleHTTPError(resp) } } @@ -689,7 +715,7 @@ func (c *Client) getIssuerCertificate(url string) ([]byte, error) { } defer resp.Body.Close() - issuerBytes, err := ioutil.ReadAll(limitReader(resp.Body, 1024*1024)) + issuerBytes, err := ioutil.ReadAll(limitReader(resp.Body, maxBodySize)) if err != nil { return nil, err } diff --git a/acme/error.go b/acme/error.go index 2aa690b3..6d7013cf 100644 --- a/acme/error.go +++ b/acme/error.go @@ -8,9 +8,7 @@ import ( "strings" ) -const ( - tosAgreementError = "Must agree to subscriber agreement before any further actions" -) +const tosAgreementError = "Must agree to subscriber agreement before any further actions" // RemoteError is the base type for all errors specific to the ACME protocol. type RemoteError struct { @@ -54,20 +52,17 @@ func (c challengeError) Error() string { func handleHTTPError(resp *http.Response) error { var errorDetail RemoteError - contenType := resp.Header.Get("Content-Type") - // try to decode the content as JSON - if contenType == "application/json" || contenType == "application/problem+json" { - decoder := json.NewDecoder(resp.Body) - err := decoder.Decode(&errorDetail) + contentType := resp.Header.Get("Content-Type") + if contentType == "application/json" || contentType == "application/problem+json" { + err := json.NewDecoder(resp.Body).Decode(&errorDetail) if err != nil { return err } } else { - detailBytes, err := ioutil.ReadAll(limitReader(resp.Body, 1024*1024)) + detailBytes, err := ioutil.ReadAll(limitReader(resp.Body, maxBodySize)) if err != nil { return err } - errorDetail.Detail = string(detailBytes) } diff --git a/acme/jws.go b/acme/jws.go index 80296081..2a1fc244 100644 --- a/acme/jws.go +++ b/acme/jws.go @@ -32,7 +32,9 @@ func keyAsJWK(key interface{}) *jose.JsonWebKey { } } -// Posts a JWS signed message to the specified URL +// Posts a JWS signed message to the specified URL. +// It does NOT close the response body, so the caller must +// do that if no error was returned. func (j *jws) post(url string, content []byte) (*http.Response, error) { signedContent, err := j.signContent(content) if err != nil {