Skip to content

WIP: Bugfix 76345 acme client sending jwk to kid expected endpoints #329

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.

Sign up for GitHub

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
+152 −29
Draft

WIP: Bugfix 76345 acme client sending jwk to kid expected endpoints #329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e5217a8
Port changes from cert-manager
Nov 18, 2025
2eccb4f
Fix broken build & tests
Nov 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions acme/acme.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
))
Expand Down
15 changes: 10 additions & 5 deletions acme/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion acme/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
32 changes: 20 additions & 12 deletions acme/rfc8555.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
))
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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{
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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.
Expand Down
118 changes: 114 additions & 4 deletions acme/rfc8555_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
}
}
Expand Down Expand Up @@ -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", `<https://example.com/crt/2>;rel="alternate"`)
Expand Down Expand Up @@ -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)
}
})
}
}