diff --git a/acme/acme.go b/acme/acme.go index b53ea28891..326aea0d57 100644 --- a/acme/acme.go +++ b/acme/acme.go @@ -142,18 +142,18 @@ type Client struct { // // When in pre-RFC mode or when c.getRegRFC responds with an error, accountKID // returns noKeyID. -func (c *Client) accountKID(ctx context.Context) KeyID { +func (c *Client) accountKID(ctx context.Context) (KeyID, error) { c.cacheMu.Lock() defer c.cacheMu.Unlock() if c.KID != noKeyID { - return c.KID + return c.KID, nil } a, err := c.getRegRFC(ctx) if err != nil { - return noKeyID + return noKeyID, err } c.KID = KeyID(a.URI) - return c.KID + return c.KID, nil } var errPreRFC = errors.New("acme: server does not support the RFC 8555 version of ACME") @@ -368,7 +368,7 @@ func (c *Client) authorize(ctx context.Context, typ, val string) (*Authorization Resource: "new-authz", Identifier: authzID{Type: typ, Value: val}, } - res, err := c.post(ctx, nil, c.dir.AuthzURL, req, wantStatus(http.StatusCreated)) + res, err := c.post(ctx, nil, true, c.dir.AuthzURL, req, wantStatus(http.StatusCreated)) if err != nil { return nil, err } @@ -428,7 +428,7 @@ func (c *Client) RevokeAuthorization(ctx context.Context, url string) error { Status: "deactivated", Delete: true, } - res, err := c.post(ctx, nil, url, req, wantStatus(http.StatusOK)) + res, err := c.post(ctx, nil, true, url, req, wantStatus(http.StatusOK)) if err != nil { return err } @@ -521,7 +521,7 @@ func (c *Client) Accept(ctx context.Context, chal *Challenge) (*Challenge, error if len(chal.Payload) != 0 { payload = chal.Payload } - res, err := c.post(ctx, nil, chal.URI, payload, wantStatus( + res, err := c.post(ctx, nil, true, chal.URI, payload, wantStatus( http.StatusOK, // according to the spec http.StatusAccepted, // Let's Encrypt: see https://goo.gl/WsJ7VT (acme-divergences.md) )) diff --git a/acme/http.go b/acme/http.go index 7d1052acd4..520e6296f1 100644 --- a/acme/http.go +++ b/acme/http.go @@ -159,7 +159,8 @@ func (c *Client) get(ctx context.Context, url string, ok resOkay) (*http.Respons // It makes a POST request in KID form with zero JWS payload. // See nopayload doc comments in jws.go. func (c *Client) postAsGet(ctx context.Context, url string, ok resOkay) (*http.Response, error) { - return c.post(ctx, nil, url, noPayload, ok) + // All POST-as-GET requests required KID + return c.post(ctx, nil, true, url, noPayload, ok) } // post issues a signed POST request in JWS format using the provided key @@ -169,10 +170,10 @@ func (c *Client) postAsGet(ctx context.Context, url string, ok resOkay) (*http.R // post retries unsuccessful attempts according to c.RetryBackoff // until the context is done or a non-retriable error is received. // It uses postNoRetry to make individual requests. -func (c *Client) post(ctx context.Context, key crypto.Signer, url string, body interface{}, ok resOkay) (*http.Response, error) { +func (c *Client) post(ctx context.Context, key crypto.Signer, requireKid bool, url string, body interface{}, ok resOkay) (*http.Response, error) { retry := c.retryTimer() for { - res, req, err := c.postNoRetry(ctx, key, url, body) + res, req, err := c.postNoRetry(ctx, key, requireKid, url, body) if err != nil { return nil, err } @@ -211,14 +212,18 @@ func (c *Client) post(ctx context.Context, key crypto.Signer, url string, body i // and JWK is used only when KID is unavailable: new account endpoint and certificate // revocation requests authenticated by a cert key. // See jwsEncodeJSON for other details. -func (c *Client) postNoRetry(ctx context.Context, key crypto.Signer, url string, body interface{}) (*http.Response, *http.Request, error) { +func (c *Client) postNoRetry(ctx context.Context, key crypto.Signer, requireKid bool, url string, body interface{}) (*http.Response, *http.Request, error) { kid := noKeyID + var err error if key == nil { if c.Key == nil { return nil, nil, errors.New("acme: Client.Key must be populated to make POST requests") } key = c.Key - kid = c.accountKID(ctx) + kid, err = c.accountKID(ctx) + if requireKid && (kid == noKeyID || err != nil) { + return nil, nil, fmt.Errorf("acme: the operation requires account KID but the value is not provided and encountered error while retrieving it from the server: %w", err) + } } nonce, err := c.popNonce(ctx, url) if err != nil { diff --git a/acme/http_test.go b/acme/http_test.go index d124e4e219..ed669f79de 100644 --- a/acme/http_test.go +++ b/acme/http_test.go @@ -244,7 +244,7 @@ func TestAccountKidLoop(t *testing.T) { // then Client.Key must be set, otherwise we fall into an // infinite loop (which also causes a deadlock). client := &Client{dir: &Directory{OrderURL: ":)"}} - _, _, err := client.postNoRetry(context.Background(), nil, "", nil) + _, _, err := client.postNoRetry(context.Background(), nil, false, "", nil) if err == nil { t.Fatal("Client.postNoRetry didn't fail with a nil key") } diff --git a/acme/rfc8555.go b/acme/rfc8555.go index 976b27702a..ef9342644c 100644 --- a/acme/rfc8555.go +++ b/acme/rfc8555.go @@ -26,12 +26,15 @@ func (c *Client) DeactivateReg(ctx context.Context) error { if _, err := c.Discover(ctx); err != nil { // required by c.accountKID return err } - url := string(c.accountKID(ctx)) + url, err := c.accountKID(ctx) + if err != nil { + return err + } if url == "" { return ErrNoAccount } req := json.RawMessage(`{"status": "deactivated"}`) - res, err := c.post(ctx, nil, url, req, wantStatus(http.StatusOK)) + res, err := c.post(ctx, nil, true, string(url), req, wantStatus(http.StatusOK)) if err != nil { return err } @@ -65,7 +68,7 @@ func (c *Client) registerRFC(ctx context.Context, acct *Account, prompt func(tos req.ExternalAccountBinding = eabJWS } - res, err := c.post(ctx, c.Key, c.dir.RegURL, req, wantStatus( + res, err := c.post(ctx, c.Key, false, c.dir.RegURL, req, wantStatus( http.StatusOK, // account with this key already registered http.StatusCreated, // new account created )) @@ -100,7 +103,10 @@ func (c *Client) encodeExternalAccountBinding(eab *ExternalAccountBinding) (*jso // updateRegRFC is equivalent to c.UpdateReg but for CAs implementing RFC 8555. // It expects c.Discover to have already been called. func (c *Client) updateRegRFC(ctx context.Context, a *Account) (*Account, error) { - url := string(c.accountKID(ctx)) + url, err := c.accountKID(ctx) + if err != nil { + return nil, err + } if url == "" { return nil, ErrNoAccount } @@ -109,7 +115,7 @@ func (c *Client) updateRegRFC(ctx context.Context, a *Account) (*Account, error) }{ Contact: a.Contact, } - res, err := c.post(ctx, nil, url, req, wantStatus(http.StatusOK)) + res, err := c.post(ctx, nil, false, string(url), req, wantStatus(http.StatusOK)) if err != nil { return nil, err } @@ -121,7 +127,7 @@ func (c *Client) updateRegRFC(ctx context.Context, a *Account) (*Account, error) // It expects c.Discover to have already been called. func (c *Client) getRegRFC(ctx context.Context) (*Account, error) { req := json.RawMessage(`{"onlyReturnExisting": true}`) - res, err := c.post(ctx, c.Key, c.dir.RegURL, req, wantStatus(http.StatusOK)) + res, err := c.post(ctx, c.Key, false, c.dir.RegURL, req, wantStatus(http.StatusOK)) if e, ok := err.(*Error); ok && e.ProblemType == "urn:ietf:params:acme:error:accountDoesNotExist" { return nil, ErrNoAccount } @@ -157,9 +163,9 @@ func (c *Client) accountKeyRollover(ctx context.Context, newKey crypto.Signer) e if err != nil { return err } - kid := c.accountKID(ctx) + kid, err := c.accountKID(ctx) if kid == noKeyID { - return ErrNoAccount + return err } oldKey, err := jwkEncode(c.Key.Public()) if err != nil { @@ -177,7 +183,7 @@ func (c *Client) accountKeyRollover(ctx context.Context, newKey crypto.Signer) e return err } - res, err := c.post(ctx, nil, dir.KeyChangeURL, base64.RawURLEncoding.EncodeToString(inner), wantStatus(http.StatusOK)) + res, err := c.post(ctx, nil, true, dir.KeyChangeURL, base64.RawURLEncoding.EncodeToString(inner), wantStatus(http.StatusOK)) if err != nil { return err } @@ -205,6 +211,7 @@ func (c *Client) AuthorizeOrder(ctx context.Context, id []AuthzID, opt ...OrderO Identifiers []wireAuthzID `json:"identifiers"` NotBefore string `json:"notBefore,omitempty"` NotAfter string `json:"notAfter,omitempty"` + Profile string `json:"profile,omitempty"` }{} for _, v := range id { req.Identifiers = append(req.Identifiers, wireAuthzID{ @@ -224,7 +231,7 @@ func (c *Client) AuthorizeOrder(ctx context.Context, id []AuthzID, opt ...OrderO } } - res, err := c.post(ctx, nil, dir.OrderURL, req, wantStatus(http.StatusCreated)) + res, err := c.post(ctx, nil, true, dir.OrderURL, req, wantStatus(http.StatusCreated)) if err != nil { return nil, err } @@ -350,7 +357,7 @@ func (c *Client) CreateOrderCert(ctx context.Context, url string, csr []byte, bu }{ CSR: base64.RawURLEncoding.EncodeToString(csr), } - res, err := c.post(ctx, nil, url, req, wantStatus(http.StatusOK)) + res, err := c.post(ctx, nil, true, url, req, wantStatus(http.StatusOK)) if err != nil { return nil, "", err } @@ -432,7 +439,8 @@ func (c *Client) revokeCertRFC(ctx context.Context, key crypto.Signer, cert []by Cert: base64.RawURLEncoding.EncodeToString(cert), Reason: int(reason), } - res, err := c.post(ctx, key, c.dir.RevokeURL, req, wantStatus(http.StatusOK)) + // Notice: revoke cert is a special case in RFC 8555 where the KID or JWK form are both allowed + res, err := c.post(ctx, key, false, c.dir.RevokeURL, req, wantStatus(http.StatusOK)) if err != nil { if isAlreadyRevoked(err) { // Assume it is not an error to revoke an already revoked cert. diff --git a/acme/rfc8555_test.go b/acme/rfc8555_test.go index e9cedb5927..2ec105b9f2 100644 --- a/acme/rfc8555_test.go +++ b/acme/rfc8555_test.go @@ -188,7 +188,7 @@ func TestRFC_postKID(t *testing.T) { }, } req := json.RawMessage(`{"msg":"ping"}`) - res, err := cl.post(ctx, nil /* use kid */, ts.URL+"/post", req, wantStatus(http.StatusOK)) + res, err := cl.post(ctx, nil /* use kid */, false, ts.URL+"/post", req, wantStatus(http.StatusOK)) if err != nil { t.Fatal(err) } @@ -346,9 +346,13 @@ func TestRFC_Register(t *testing.T) { if !didPrompt { t.Error("tos prompt wasn't called") } - if v := cl.accountKID(ctx); v != KeyID(okAccount.URI) { + v, err := cl.accountKID(ctx) + if v != KeyID(okAccount.URI) { t.Errorf("account kid = %q; want %q", v, okAccount.URI) } + if err != nil { + t.Errorf("account kid error %+v", err) + } } func TestRFC_RegisterExternalAccountBinding(t *testing.T) { @@ -483,9 +487,13 @@ func TestRFC_RegisterExternalAccountBinding(t *testing.T) { if !didPrompt { t.Error("tos prompt wasn't called") } - if v := cl.accountKID(ctx); v != KeyID(okAccount.URI) { + v, err := cl.accountKID(ctx) + if v != KeyID(okAccount.URI) { t.Errorf("account kid = %q; want %q", v, okAccount.URI) } + if err != nil { + t.Errorf("account kid error %+v", err) + } } func TestRFC_RegisterExisting(t *testing.T) { @@ -504,7 +512,11 @@ func TestRFC_RegisterExisting(t *testing.T) { t.Errorf("err = %v; want %v", err, ErrAccountAlreadyExists) } kid := KeyID(s.url("/accounts/1")) - if v := cl.accountKID(context.Background()); v != kid { + v, err := cl.accountKID(context.Background()) + if err != nil { + t.Errorf("account kid error %+v", err) + } + if v != kid { t.Errorf("account kid = %q; want %q", v, kid) } } @@ -999,6 +1011,11 @@ func TestRFC_AlreadyRevokedCert(t *testing.T) { func TestRFC_ListCertAlternates(t *testing.T) { s := newACMEServer() + s.handle("/acme/new-account", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", s.url("/accounts/1")) + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"status": "valid"}`)) + }) s.handle("/crt", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/pem-certificate-chain") w.Header().Add("Link", `;rel="alternate"`) @@ -1028,3 +1045,96 @@ func TestRFC_ListCertAlternates(t *testing.T) { t.Errorf("ListCertAlternates(/crt2): %v; want nil", crts) } } + +func TestRFC_RequireKidEndpoints(t *testing.T) { + s := newACMEServer() + s.handle("/acme/new-account", func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + var payload struct { + OnlyReturnExisting bool `json:"onlyReturnExisting,omitempty"` + } + decodeJWSRequest(t, &payload, bytes.NewReader(b)) + if payload.OnlyReturnExisting { + // get account req + w.WriteHeader(http.StatusBadRequest) + } else { + // new account req + w.Header().Set("Location", "/account-1") + w.Write([]byte(`{"status":"valid"}`)) + } + }) + s.start() + defer s.close() + + tests := []struct { + name string + op func(client *Client) error + }{ + {"AuthorizeOrder", func(cl *Client) error { + _, err := cl.AuthorizeOrder(context.Background(), DomainIDs("example.org")) + return err + }}, + {"GetOrder", func(cl *Client) error { + _, err := cl.GetOrder(context.Background(), s.url("/orders/1")) + return err + }}, + {"WaitOrder", func(cl *Client) error { + _, err := cl.WaitOrder(context.Background(), s.url("/orders/1")) + return err + }}, + {"CreateOrderCert", func(cl *Client) error { + q := &x509.CertificateRequest{ + Subject: pkix.Name{CommonName: "example.org"}, + } + csr, err := x509.CreateCertificateRequest(rand.Reader, q, testKeyEC) + if err != nil { + t.Fatal(err) + } + + _, _, err = cl.CreateOrderCert(context.Background(), s.url("/pleaseissue"), csr, true) + return err + }}, + {"ListCertAlternates", func(cl *Client) error { + _, err := cl.ListCertAlternates(context.Background(), s.url("/crt")) + return err + }}, + {"Authorize", func(cl *Client) error { + _, err := cl.Authorize(context.Background(), s.url("example.com")) + return err + }}, + {"AuthorizeIP", func(cl *Client) error { + _, err := cl.AuthorizeIP(context.Background(), s.url("1.1.1.1")) + return err + }}, + {"GetAuthorization", func(cl *Client) error { + _, err := cl.GetAuthorization(context.Background(), s.url("/authz/1")) + return err + }}, + {"RevokeAuthorization", func(cl *Client) error { + err := cl.RevokeAuthorization(context.Background(), s.url("/authz/1")) + return err + }}, + {"WaitAuthorization", func(cl *Client) error { + _, err := cl.WaitAuthorization(context.Background(), s.url("/authz/1")) + return err + }}, + {"GetChallenge", func(cl *Client) error { + _, err := cl.GetChallenge(context.Background(), s.url("/chall/1")) + return err + }}, + {"Accept", func(cl *Client) error { + _, err := cl.Accept(context.Background(), &Challenge{Type: "http-01", URI: "/chall/1", Token: "test"}) + return err + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cl := &Client{Key: testKeyEC, DirectoryURL: s.url("/")} + err := tt.op(cl) + expected := "acme: the operation requires account KID but the value is not provided and encountered error while retrieving it from the server: 400 : 400 Bad Request" + if err == nil || err.Error() != expected { + t.Errorf("err: %v; want %v", err, expected) + } + }) + } +}