Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/acme/autocert: Registration Update Fails #26251

Closed
blitzrk opened this issue Jul 6, 2018 · 5 comments
Closed

x/crypto/acme/autocert: Registration Update Fails #26251

blitzrk opened this issue Jul 6, 2018 · 5 comments

Comments

@blitzrk
Copy link

blitzrk commented Jul 6, 2018

What did you do?

I created an HTTP server like log.Fatal(http.Serve(autocert.NewListener("subdomain.example.com"), handler)) where example.com also has a certificate from Let's Encrypt.

What did you expect to see?

TLS handshakes should succeed.

What did you see instead?

On the first cURL of https://subdomain.example.com, I get curl: (35) error:14077438:SSL routines:SSL23_GET_SERVER_HELLO:tlsv1 alert internal error and see in the server logs:

2018/07/06 16:44:20 http: TLS handshake error from 172.17.0.1:41496: 202 : {
  "id": 37902554,
  "key": {
    "kty": "EC",
    "crv": "P-256",
    "x": "XXX",
    "y": "XXX"
  },
  "contact": [],
  "agreement": "https://letsencrypt.org/documents/LE-SA-v1.2-November-15-2017.pdf",
  "initialIp": "xxx.xxx.xxx.xxx",
  "createdAt": "2018-07-06T16:38:46Z",
  "status": "valid"
}

On subsequent cURLs, the handshake continues to fail and I see errors like

2018/07/06 16:45:55 http: TLS handshake error from 172.17.0.1:41500: acme/autocert: unable to authorize "subdomain.example.com"
2018/07/06 16:46:55 http: TLS handshake error from 172.17.0.1:41506: acme/autocert: missing certificate

What I Think Is Happening

The error logged is a 202 and takes the form of a registration update response. Autocert initializes the ACME client and in the process calls client.Register here. Then the ACME client creates a new registration and if the Terms of Service need to be accepted, calls UpdateReg here. This eventually leads to a call like this

res, err := c.post(ctx, c.Key, url, req, wantStatus(
	http.StatusOK,      // updates and deletes
	http.StatusCreated, // new account creation
))

which errors on any result other than a 200 or 201. This could explain the logged 202 response. Checking Let's Encrypt's Boulder project quickly, I found registration updates are returned with a 202 status here: https://github.com/letsencrypt/boulder/blob/release-2018-07-03/wfe/wfe.go#L1190

How I Got It Working Locally

There was no obvious way to pre-accept the terms and conditions, so I decided to test my theory by simply modifying 202 responses for registration update requests to 200s. This worked. The code required to get TLS handshakes working properly looked like this:

func getCertificate(fqdn string) (*tls.Certificate, error) {
        m := &autocert.Manager{
                Client: &acme.Client{
                        HTTPClient: &http.Client{
                                Transport: modRegUpdateRoundTripper{},
                        },
                        DirectoryURL: acme.LetsEncryptURL,
                },
                Prompt:     autocert.AcceptTOS,
                HostPolicy: autocert.HostWhitelist(fqdn),
        }

        // Redirect HTTP to HTTPS and fulfill HTTP-01 challenges
        go http.ListenAndServe(":80", m.HTTPHandler(nil))

        // Listen and serve with TLS
        go http.Serve(m.Listener(), nil)

        // Get TLS cert
        return m.GetCertificate(&tls.ClientHelloInfo{ServerName: fqdn})
}

// Transport that changes registration update 202 responses to 200
type modRegUpdateRoundTripper struct{}

func (modRegUpdateRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
        r, err := http.DefaultTransport.RoundTrip(req)
        if err != nil {
                return nil, err
        }
        // Intercept registration updates
        uri := req.URL.String()
        if strings.HasPrefix(uri, "https://acme-v01.api.letsencrypt.org/acme/reg/") {
                // Change 202 to 200
                if r.StatusCode == http.StatusAccepted {
                        r.StatusCode = http.StatusOK
                }
        }
        return r, nil
}
@gopherbot gopherbot added this to the Unreleased milestone Jul 6, 2018
@blitzrk
Copy link
Author

blitzrk commented Jul 6, 2018

I was looking through the ACME RFC to see if the 202 was a deviation by Let's Encrypt. Section 7.3.2 appears to imply that it is indeed a deviation, if my understanding is correct and accounts are analogous to registrations in Let's Encrypt:

If the client wishes to update this information in the future, it
sends a POST request with updated information to the account URL.
The server MUST ignore any updates to the "orders" field or any other
fields it does not recognize.  If the server accepts the update, it
MUST return a response with a 200 (OK) status code and the resulting
account object.

It seems that the autocert and ACME libraries make some exceptions for Let's Encrypt deviations in other parts of the code, so perhaps http.StatusAccepted could be added to the list of acceptable response codes for registration update POST requests. This would be a change to x/crypto/acme.

@x1ddos
Copy link

x1ddos commented Jul 21, 2018

It is indeed a Let's Encrypt's implementation divergence from the spec, even though I couldn't find it in https://github.com/letsencrypt/boulder/blob/master/docs/acme-divergences.md.

@x1ddos
Copy link

x1ddos commented Jul 21, 2018

I'll fix it.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/125415 mentions this issue: acme: expect 202 Accepted from Let's Encrypt

@ghost
Copy link

ghost commented Aug 31, 2018 via email

@golang golang locked and limited conversation to collaborators Aug 31, 2019
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
ACME draft specifies the CA servers should respond
with 201 Created status code but Let's Encrypt
responds with 202 Accepted when creating a new account.

This change adds 202 Accepted as a valid response.
Otherwise, the Client hangs while doing retries,
discarding 202 responses as invalid.

Tests are not updated intentionally
due to this being non-conformant with the spec.

Fixes golang/go#26251

Change-Id: I2918fce3873592c02e96f4118c4d1ecb42da3c4f
Reviewed-on: https://go-review.googlesource.com/125415
Reviewed-by: Brad Fitzpatrick <[email protected]>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
ACME draft specifies the CA servers should respond
with 201 Created status code but Let's Encrypt
responds with 202 Accepted when creating a new account.

This change adds 202 Accepted as a valid response.
Otherwise, the Client hangs while doing retries,
discarding 202 responses as invalid.

Tests are not updated intentionally
due to this being non-conformant with the spec.

Fixes golang/go#26251

Change-Id: I2918fce3873592c02e96f4118c4d1ecb42da3c4f
Reviewed-on: https://go-review.googlesource.com/125415
Reviewed-by: Brad Fitzpatrick <[email protected]>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
ACME draft specifies the CA servers should respond
with 201 Created status code but Let's Encrypt
responds with 202 Accepted when creating a new account.

This change adds 202 Accepted as a valid response.
Otherwise, the Client hangs while doing retries,
discarding 202 responses as invalid.

Tests are not updated intentionally
due to this being non-conformant with the spec.

Fixes golang/go#26251

Change-Id: I2918fce3873592c02e96f4118c4d1ecb42da3c4f
Reviewed-on: https://go-review.googlesource.com/125415
Reviewed-by: Brad Fitzpatrick <[email protected]>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
ACME draft specifies the CA servers should respond
with 201 Created status code but Let's Encrypt
responds with 202 Accepted when creating a new account.

This change adds 202 Accepted as a valid response.
Otherwise, the Client hangs while doing retries,
discarding 202 responses as invalid.

Tests are not updated intentionally
due to this being non-conformant with the spec.

Fixes golang/go#26251

Change-Id: I2918fce3873592c02e96f4118c4d1ecb42da3c4f
Reviewed-on: https://go-review.googlesource.com/125415
Reviewed-by: Brad Fitzpatrick <[email protected]>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
ACME draft specifies the CA servers should respond
with 201 Created status code but Let's Encrypt
responds with 202 Accepted when creating a new account.

This change adds 202 Accepted as a valid response.
Otherwise, the Client hangs while doing retries,
discarding 202 responses as invalid.

Tests are not updated intentionally
due to this being non-conformant with the spec.

Fixes golang/go#26251

Change-Id: I2918fce3873592c02e96f4118c4d1ecb42da3c4f
Reviewed-on: https://go-review.googlesource.com/125415
Reviewed-by: Brad Fitzpatrick <[email protected]>
desdeel2d0m added a commit to desdeel2d0m/crypto that referenced this issue Jul 1, 2024
ACME draft specifies the CA servers should respond
with 201 Created status code but Let's Encrypt
responds with 202 Accepted when creating a new account.

This change adds 202 Accepted as a valid response.
Otherwise, the Client hangs while doing retries,
discarding 202 responses as invalid.

Tests are not updated intentionally
due to this being non-conformant with the spec.

Fixes golang/go#26251

Change-Id: I2918fce3873592c02e96f4118c4d1ecb42da3c4f
Reviewed-on: https://go-review.googlesource.com/125415
Reviewed-by: Brad Fitzpatrick <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants