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

@@ -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,27 +614,55 @@ 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))
+	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 {
-
-				cerRes.CertStableURL = resp.Header.Get("Content-Location")
-				cerRes.AccountRef = c.user.GetRegistration().URI
+			certRes.CertStableURL = resp.Header.Get("Content-Location")
+			certRes.AccountRef = c.user.GetRegistration().URI
 
 			issuedCert := pemEncode(derCertificateBytes(cert))
 
@@ -640,7 +672,7 @@ func (c *Client) requestCertificateForCsr(authz []authorizationResource, bundle
 			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)
+				logf("[WARNING][%s] acme: Could not bundle issuer certificate: %v", certRes.Domain, err)
 			} else {
 				issuerCert = pemEncode(derCertificateBytes(issuerCert))
 
@@ -651,10 +683,10 @@ func (c *Client) requestCertificateForCsr(authz []authorizationResource, bundle
 				}
 			}
 
-				cerRes.Certificate = issuedCert
-				cerRes.IssuerCertificate = issuerCert
-				logf("[INFO][%s] Server responded with a certificate.", commonName.Domain)
-				return cerRes, nil
+			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.
@@ -662,21 +694,15 @@ func (c *Client) requestCertificateForCsr(authz []authorizationResource, bundle
 		ra := resp.Header.Get("Retry-After")
 		retryAfter, err := strconv.Atoi(ra)
 		if err != nil {
-				return CertificateResource{}, err
+			return false, err
 		}
 
-			logf("[INFO][%s] acme: Server responded with status 202; retrying after %ds", commonName.Domain, retryAfter)
+		logf("[INFO][%s] acme: Server responded with status 202; retrying after %ds", certRes.Domain, retryAfter)
 		time.Sleep(time.Duration(retryAfter) * time.Second)
 
-			break
+		return false, nil
 	default:
-			return CertificateResource{}, handleHTTPError(resp)
-		}
-
-		resp, err = httpGet(cerRes.CertURL)
-		if err != nil {
-			return CertificateResource{}, err
-		}
+		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
 	}

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)
 	}
 

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 {