From 3a368eae8a435ea9c697566d02b404b23115fd4d Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Thu, 2 May 2024 16:16:50 -0400 Subject: [PATCH 01/21] Begin implementing draft-ietf-acme-ari-03 --- wfe/wfe.go | 60 +++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 55 insertions(+), 5 deletions(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index 0f47fc26..f9ee60e9 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -53,6 +53,9 @@ const ( keyRolloverPath = "/rollover-account-key" ordersPath = "/list-orderz/" + // Draft or likely-to-change paths + renewalInfoPath = "/draft-ietf-acme-ari-03/renewalInfo/" + // Theses entrypoints are not a part of the standard ACME endpoints, // and are exposed by Pebble as an integration test tool. We export // RootCertPath so that the pebble binary can reference it. @@ -507,6 +510,8 @@ func (wfe *WebFrontEndImpl) Handler() http.Handler { m := http.NewServeMux() // GET & POST handlers wfe.HandleFunc(m, DirectoryPath, wfe.Directory, http.MethodGet, http.MethodPost) + wfe.HandleFunc(m, renewalInfoPath, wfe.RenewalInfo, http.MethodGet, http.MethodPost) + // Note for noncePath: http.MethodGet also implies http.MethodHead wfe.HandleFunc(m, noncePath, wfe.Nonce, http.MethodGet, http.MethodPost) @@ -545,11 +550,12 @@ func (wfe *WebFrontEndImpl) Directory( request *http.Request, ) { directoryEndpoints := map[string]string{ - "newNonce": noncePath, - "newAccount": newAccountPath, - "newOrder": newOrderPath, - "revokeCert": revokeCertPath, - "keyChange": keyRolloverPath, + "newNonce": noncePath, + "newAccount": newAccountPath, + "newOrder": newOrderPath, + "revokeCert": revokeCertPath, + "keyChange": keyRolloverPath, + "renewalInfo": renewalInfoPath, } // RFC 8555 §6.3 says the server's directory endpoint should support @@ -1775,6 +1781,50 @@ func (wfe *WebFrontEndImpl) orderForDisplay( return result } +// RenewalInfo implements ACME Renewal Info (ARI) +func (wfe *WebFrontEndImpl) RenewalInfo(_ context.Context, response http.ResponseWriter, request *http.Request) { + if request.Method == http.MethodPost { + wfe.UpdateRenewal(context.TODO(), response, request) + wfe.sendError(acme.InternalErrorProblem("POSTing to RenewalInfo has not been implemented yet"), response) + return + } + + if len(request.URL.Path) == 0 { + wfe.sendError(acme.NotFoundProblem("Must specify a request path"), response) + return + } + + if request.Method == http.MethodGet { + wfe.UpdateRenewal(context.TODO(), response, request) + wfe.sendError(acme.InternalErrorProblem("GETing RenewalInfo is being implemented"), response) + return + } + + certID, err := parseCertID(request.URL.Path, nil) + fmt.Println(certID) + if err != nil { + wfe.sendError(acme.MalformedProblem(fmt.Sprintf("While parsing ARI CertID an error occurred: %s", err)), response) + return + } +} + +func (wfe *WebFrontEndImpl) UpdateRenewal(_ context.Context, response http.ResponseWriter, request *http.Request) { + // Not yet implemented +} + +// parseCertID parses a unique identifier (certID) as specified in +// draft-ietf-acme-ari-03. It takes the composite string as input returns a +// certID struct with the keyIdentifier and serialNumber extracted and decoded. +// For more details see: +// https://datatracker.ietf.org/doc/html/draft-ietf-acme-ari-02#section-4.1. +func parseCertID(path string, issuer *x509.Certificate) (string, error) { + fmt.Println(path) + if issuer == nil { + return "", fmt.Errorf("issuer was nil") + } + return "", fmt.Errorf("working on it still") +} + // Order retrieves the details of an existing order func (wfe *WebFrontEndImpl) Order( _ context.Context, From 9eb7c0a64cbe6fa2bb10b59e5b703556541e3d72 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 3 May 2024 22:13:16 -0400 Subject: [PATCH 02/21] Initial implementation *cough* copy/paste *cough* of RenewalInfo --- ca/ca.go | 13 +++++++++ core/types.go | 66 ++++++++++++++++++++++++++++++++++++++++++++ go.mod | 2 +- wfe/wfe.go | 76 +++++++++++++++++++++++++++++++++++++++++++-------- 4 files changed, 145 insertions(+), 12 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index c16d7f4a..d859c191 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -438,6 +438,19 @@ func (ca *CAImpl) CompleteOrder(order *core.Order) { order.Unlock() } +func (ca *CAImpl) GetSubjectKeyIDs() core.SubjectKeyIDs { + skis := core.SubjectKeyIDs{} + for _, chain := range ca.chains { + skis = append(skis, chain.root.cert.Cert.SubjectKeyId) + for _, intermediate := range chain.intermediates { + skis = append(skis, intermediate.cert.Cert.SubjectKeyId) + } + } + ca.log.Printf("Found %d SKIs\n", len(skis)) + + return skis +} + func (ca *CAImpl) GetNumberOfRootCerts() int { return len(ca.chains) } diff --git a/core/types.go b/core/types.go index b94d5a77..0d39a1b3 100644 --- a/core/types.go +++ b/core/types.go @@ -8,6 +8,7 @@ import ( "encoding/pem" "errors" "fmt" + "math/big" "sync" "time" @@ -200,3 +201,68 @@ type ValidationRecord struct { Error *acme.ProblemDetails ValidatedAt time.Time } + +// SubjectKeyIDs is a convenience type that holds the Subject Key Identifier +// value for each Pebble generated root and intermediate certificate. +type SubjectKeyIDs [][]byte + +// CertID represents a unique identifier (CertID) for a certificate as per the +// ACME protocol's "renewalInfo" resource, as specified in draft-ietf-acme-ari- +// 03. The CertID is a composite string derived from the base64url-encoded +// keyIdentifier of the certificate's Authority Key Identifier (AKI) and the +// base64url-encoded serial number of the certificate, separated by a period. +// For more details see: +// https://datatracker.ietf.org/doc/html/draft-ietf-acme-ari-02#section-4.1. +type CertID struct { + KeyIdentifier []byte + SerialNumber *big.Int +} + +// SuggestedWindow is a type exposed inside the RenewalInfo resource. +type SuggestedWindow struct { + Start time.Time `json:"start"` + End time.Time `json:"end"` +} + +// IsWithin returns true if the given time is within the suggested window, +// inclusive of the start time and exclusive of the end time. +func (window SuggestedWindow) IsWithin(now time.Time) bool { + return !now.Before(window.Start) && now.Before(window.End) +} + +// RenewalInfo is a type which is exposed to clients which query the renewalInfo +// endpoint specified in draft-aaron-ari. +type RenewalInfo struct { + SuggestedWindow SuggestedWindow `json:"suggestedWindow"` +} + +// RenewalInfoSimple constructs a `RenewalInfo` object and suggested window +// using a very simple renewal calculation: calculate a point 2/3rds of the way +// through the validity period, then give a 2-day window around that. Both the +// `issued` and `expires` timestamps are expected to be UTC. +func RenewalInfoSimple(issued time.Time, expires time.Time) *RenewalInfo { + validity := expires.Add(time.Second).Sub(issued) + renewalOffset := validity / time.Duration(3) + idealRenewal := expires.Add(-renewalOffset) + return &RenewalInfo{ + SuggestedWindow: SuggestedWindow{ + Start: idealRenewal.Add(-24 * time.Hour), + End: idealRenewal.Add(24 * time.Hour), + }, + } +} + +// RenewalInfoImmediate constructs a `RenewalInfo` object with a suggested +// window in the past. Per the draft-ietf-acme-ari-01 spec, clients should +// attempt to renew immediately if the suggested window is in the past. The +// passed `now` is assumed to be a timestamp representing the current moment in +// time. +func RenewalInfoImmediate(now time.Time) *RenewalInfo { + oneHourAgo := now.Add(-1 * time.Hour) + return &RenewalInfo{ + SuggestedWindow: SuggestedWindow{ + Start: oneHourAgo, + End: oneHourAgo.Add(time.Minute * 30), + }, + } +} diff --git a/go.mod b/go.mod index 388cee75..d9b7d22b 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/letsencrypt/pebble/v2 -go 1.21 +go 1.22 require ( github.com/go-jose/go-jose/v4 v4.0.1 diff --git a/wfe/wfe.go b/wfe/wfe.go index f9ee60e9..36076a0f 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1794,20 +1794,48 @@ func (wfe *WebFrontEndImpl) RenewalInfo(_ context.Context, response http.Respons return } - if request.Method == http.MethodGet { - wfe.UpdateRenewal(context.TODO(), response, request) - wfe.sendError(acme.InternalErrorProblem("GETing RenewalInfo is being implemented"), response) + certID, err := wfe.parseCertID(request.URL.Path) + if err != nil { + wfe.sendError(acme.MalformedProblem(fmt.Sprintf("Parsing ARI CertID failed: %s", err)), response) + return + } + + renewalInfo, err := wfe.determineARIWindow(context.TODO(), certID.SerialNumber) + if err != nil { + /* + if errors.Is(err, berrors.NotFound) { + wfe.sendError(response, logEvent, probs.NotFound("Certificate replaced by this order was not found"), nil) + return + } + */ + wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error determining renewal window: %s", err)), response) return } - certID, err := parseCertID(request.URL.Path, nil) - fmt.Println(certID) + response.Header().Set("Retry-After", fmt.Sprintf("%d", int(6*time.Hour/time.Second))) + err = wfe.writeJSONResponse(response, http.StatusOK, renewalInfo) if err != nil { - wfe.sendError(acme.MalformedProblem(fmt.Sprintf("While parsing ARI CertID an error occurred: %s", err)), response) + wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error marshalling renewalInfo: %s", err)), response) return } } +func (wfe *WebFrontEndImpl) determineARIWindow(_ context.Context, serial *big.Int) (*core.RenewalInfo, error) { + // Check if the serial is revoked. + isRevoked := wfe.db.GetRevokedCertificateBySerial(serial) + if isRevoked != nil { + // The existing certificate is revoked, renew immediately. + return core.RenewalInfoImmediate(time.Now().In(time.UTC)), nil + } + + cert := wfe.db.GetCertificateBySerial(serial) + if cert != nil { + return nil, fmt.Errorf("failed to retrieve existing certificate") + } + + return core.RenewalInfoSimple(cert.Cert.NotBefore, cert.Cert.NotAfter), nil +} + func (wfe *WebFrontEndImpl) UpdateRenewal(_ context.Context, response http.ResponseWriter, request *http.Request) { // Not yet implemented } @@ -1817,12 +1845,38 @@ func (wfe *WebFrontEndImpl) UpdateRenewal(_ context.Context, response http.Respo // certID struct with the keyIdentifier and serialNumber extracted and decoded. // For more details see: // https://datatracker.ietf.org/doc/html/draft-ietf-acme-ari-02#section-4.1. -func parseCertID(path string, issuer *x509.Certificate) (string, error) { - fmt.Println(path) - if issuer == nil { - return "", fmt.Errorf("issuer was nil") +func (wfe *WebFrontEndImpl) parseCertID(path string) (*core.CertID, error) { + parts := strings.Split(path, ".") + if len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return nil, acme.MalformedProblem("Invalid path") } - return "", fmt.Errorf("working on it still") + + akid, err := base64.RawURLEncoding.DecodeString(parts[0]) + if err != nil { + return nil, acme.MalformedProblem(fmt.Sprintf("Authority Key Identifier was not base64url-encoded or contained padding: %s", err)) + } + + var found bool + skis := wfe.ca.GetSubjectKeyIDs() + for _, skiBytes := range skis { + if bytes.Equal(skiBytes, akid) { + found = true + break + } + } + if !found { + return nil, acme.MalformedProblem("path contained an Authority Key Identifier that did not match a known issuer") + } + + serialNumber, err := base64.RawURLEncoding.DecodeString(parts[1]) + if err != nil { + return nil, acme.MalformedProblem(fmt.Sprintf("serial number was not base64url-encoded or contained padding: %s", err)) + } + + return &core.CertID{ + KeyIdentifier: akid, + SerialNumber: new(big.Int).SetBytes(serialNumber), + }, nil } // Order retrieves the details of an existing order From 86b303da50daf69e53691ebff321e0445feafb1e Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 10 May 2024 15:45:59 -0400 Subject: [PATCH 03/21] It works now --- acme/common.go | 3 +++ ca/ca.go | 1 - core/types.go | 1 + wfe/wfe.go | 55 ++++++++++++++++++++++++++++++++++++-------------- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/acme/common.go b/acme/common.go index ab926d26..47a58ace 100644 --- a/acme/common.go +++ b/acme/common.go @@ -59,6 +59,9 @@ type Order struct { NotAfter string `json:"notAfter,omitempty"` Authorizations []string `json:"authorizations"` Certificate string `json:"certificate,omitempty"` + + // https://datatracker.ietf.org/doc/html/draft-ietf-acme-ari-03#section-5 + Replaces string `json:"replaces,omitempty"` } // An Authorization is created for each identifier in an order diff --git a/ca/ca.go b/ca/ca.go index d859c191..13afe10e 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -446,7 +446,6 @@ func (ca *CAImpl) GetSubjectKeyIDs() core.SubjectKeyIDs { skis = append(skis, intermediate.cert.Cert.SubjectKeyId) } } - ca.log.Printf("Found %d SKIs\n", len(skis)) return skis } diff --git a/core/types.go b/core/types.go index 0d39a1b3..44450f87 100644 --- a/core/types.go +++ b/core/types.go @@ -28,6 +28,7 @@ type Order struct { AuthorizationObjects []*Authorization BeganProcessing bool CertificateObject *Certificate + Replaces string `json:"replaces,omitempty"` } func (o *Order) GetStatus() (string, error) { diff --git a/wfe/wfe.go b/wfe/wfe.go index 36076a0f..b809c5d9 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1633,6 +1633,30 @@ func (wfe *WebFrontEndImpl) makeChallenges(authz *core.Authorization, request *h return nil } +func (wfe *WebFrontEndImpl) validateReplacementOrder(_ context.Context, replaces string, response http.ResponseWriter) error { + if replaces == "" { + // No replacement indicated + return nil + } + + certID, err := wfe.parseCertID(replaces) + if err != nil { + wfe.sendError(acme.MalformedProblem(fmt.Sprintf("Parsing ARI CertID failed: %s", err)), response) + return nil + } + + replacementOrder := wfe.db.GetOrderByID(certID.SerialNumber.String()) + if replacementOrder == nil { + return fmt.Errorf("could not find order") + } + if replacementOrder.Replaces != "" { + wfe.sendError(acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.SerialNumber)), response) + return nil + } + + return nil +} + // NewOrder creates a new Order request and populates its authorizations func (wfe *WebFrontEndImpl) NewOrder( _ context.Context, @@ -1695,6 +1719,7 @@ func (wfe *WebFrontEndImpl) NewOrder( Identifiers: uniquenames, NotBefore: newOrder.NotBefore, NotAfter: newOrder.NotAfter, + Replaces: newOrder.Replaces, }, ExpiresDate: expires, } @@ -1800,14 +1825,8 @@ func (wfe *WebFrontEndImpl) RenewalInfo(_ context.Context, response http.Respons return } - renewalInfo, err := wfe.determineARIWindow(context.TODO(), certID.SerialNumber) + renewalInfo, err := wfe.determineARIWindow(context.TODO(), certID) if err != nil { - /* - if errors.Is(err, berrors.NotFound) { - wfe.sendError(response, logEvent, probs.NotFound("Certificate replaced by this order was not found"), nil) - return - } - */ wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error determining renewal window: %s", err)), response) return } @@ -1820,17 +1839,20 @@ func (wfe *WebFrontEndImpl) RenewalInfo(_ context.Context, response http.Respons } } -func (wfe *WebFrontEndImpl) determineARIWindow(_ context.Context, serial *big.Int) (*core.RenewalInfo, error) { - // Check if the serial is revoked. - isRevoked := wfe.db.GetRevokedCertificateBySerial(serial) +func (wfe *WebFrontEndImpl) determineARIWindow(_ context.Context, id *core.CertID) (*core.RenewalInfo, error) { + if id == nil { + return nil, fmt.Errorf("CertID was nil") + } + + // Check if the serial is revoked, and if so, renew it immediately. + isRevoked := wfe.db.GetRevokedCertificateBySerial(id.SerialNumber) if isRevoked != nil { - // The existing certificate is revoked, renew immediately. return core.RenewalInfoImmediate(time.Now().In(time.UTC)), nil } - cert := wfe.db.GetCertificateBySerial(serial) - if cert != nil { - return nil, fmt.Errorf("failed to retrieve existing certificate") + cert := wfe.db.GetCertificateBySerial(id.SerialNumber) + if cert == nil { + return nil, fmt.Errorf("failed to retrieve existing certificate serial") } return core.RenewalInfoSimple(cert.Cert.NotBefore, cert.Cert.NotAfter), nil @@ -1844,7 +1866,7 @@ func (wfe *WebFrontEndImpl) UpdateRenewal(_ context.Context, response http.Respo // draft-ietf-acme-ari-03. It takes the composite string as input returns a // certID struct with the keyIdentifier and serialNumber extracted and decoded. // For more details see: -// https://datatracker.ietf.org/doc/html/draft-ietf-acme-ari-02#section-4.1. +// https://datatracker.ietf.org/doc/html/draft-ietf-acme-ari-03#section-4.1. func (wfe *WebFrontEndImpl) parseCertID(path string) (*core.CertID, error) { parts := strings.Split(path, ".") if len(parts) != 2 || parts[0] == "" || parts[1] == "" { @@ -1861,6 +1883,7 @@ func (wfe *WebFrontEndImpl) parseCertID(path string) (*core.CertID, error) { for _, skiBytes := range skis { if bytes.Equal(skiBytes, akid) { found = true + wfe.log.Printf("Found an issuer matching SubjectKeyID %x", skiBytes) break } } @@ -1963,6 +1986,8 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( orderStatus := existingOrder.Status orderExpires := existingOrder.ExpiresDate orderIdentifiers := existingOrder.Identifiers + // TODO(PHIL) + //orderReplaces := existingOrder.Replaces // And then immediately unlock it again - we don't defer() here because // `maybeIssue` will also acquire a read lock and we call that before // returning From 46b2e8bd2ea22f837993c3e44449e9eb9bcd0969 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 10 May 2024 16:09:33 -0400 Subject: [PATCH 04/21] Fix a bunch of golangci-lint issues --- wfe/wfe.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index b809c5d9..8b018908 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -669,7 +669,7 @@ func (wfe *WebFrontEndImpl) parseJWS(body string) (*jose.JSONWebSignature, error Signatures []interface{} } if err := json.Unmarshal([]byte(body), &unprotected); err != nil { - return nil, fmt.Errorf("Parse error reading JWS: %w", err) + return nil, fmt.Errorf("parse error reading JWS: %w", err) } // ACME v2 never uses values from the unprotected JWS header. Reject JWS that @@ -688,11 +688,11 @@ func (wfe *WebFrontEndImpl) parseJWS(body string) (*jose.JSONWebSignature, error parsedJWS, err := jose.ParseSigned(body, goodJWSSignatureAlgorithms) if err != nil { - return nil, fmt.Errorf("Parse error reading JWS: %w", err) + return nil, fmt.Errorf("parse error reading JWS: %w", err) } if len(parsedJWS.Signatures) > 1 { - return nil, errors.New("Too many signatures in POST body") + return nil, errors.New("too many signatures in POST body") } if len(parsedJWS.Signatures) == 0 { @@ -1647,7 +1647,7 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(_ context.Context, replaces replacementOrder := wfe.db.GetOrderByID(certID.SerialNumber.String()) if replacementOrder == nil { - return fmt.Errorf("could not find order") + return errors.New("could not find order") } if replacementOrder.Replaces != "" { wfe.sendError(acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.SerialNumber)), response) @@ -1680,7 +1680,7 @@ func (wfe *WebFrontEndImpl) NewOrder( err := json.Unmarshal(postData.body, &newOrder) if err != nil { wfe.sendError( - acme.MalformedProblem(fmt.Sprintf("Error unmarshalling body JSON: %s", err.Error())), response) + acme.MalformedProblem(fmt.Sprintf("Error unmarshaling body JSON: %s", err.Error())), response) return } @@ -1831,17 +1831,17 @@ func (wfe *WebFrontEndImpl) RenewalInfo(_ context.Context, response http.Respons return } - response.Header().Set("Retry-After", fmt.Sprintf("%d", int(6*time.Hour/time.Second))) + response.Header().Set("Retry-After", strconv.Itoa(int(6*time.Hour/time.Second))) err = wfe.writeJSONResponse(response, http.StatusOK, renewalInfo) if err != nil { - wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error marshalling renewalInfo: %s", err)), response) + wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error marshaling renewalInfo: %s", err)), response) return } } func (wfe *WebFrontEndImpl) determineARIWindow(_ context.Context, id *core.CertID) (*core.RenewalInfo, error) { if id == nil { - return nil, fmt.Errorf("CertID was nil") + return nil, errors.New("CertID was nil") } // Check if the serial is revoked, and if so, renew it immediately. @@ -1852,7 +1852,7 @@ func (wfe *WebFrontEndImpl) determineARIWindow(_ context.Context, id *core.CertI cert := wfe.db.GetCertificateBySerial(id.SerialNumber) if cert == nil { - return nil, fmt.Errorf("failed to retrieve existing certificate serial") + return nil, errors.New("failed to retrieve existing certificate serial") } return core.RenewalInfoSimple(cert.Cert.NotBefore, cert.Cert.NotAfter), nil @@ -1987,7 +1987,7 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( orderExpires := existingOrder.ExpiresDate orderIdentifiers := existingOrder.Identifiers // TODO(PHIL) - //orderReplaces := existingOrder.Replaces + // orderReplaces := existingOrder.Replaces // And then immediately unlock it again - we don't defer() here because // `maybeIssue` will also acquire a read lock and we call that before // returning From a267fa2393ff389517382206cbb1b6bba5a9e5bb Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Mon, 13 May 2024 17:00:07 -0400 Subject: [PATCH 05/21] More work --- wfe/wfe.go | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index 8b018908..9f605552 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1633,27 +1633,27 @@ func (wfe *WebFrontEndImpl) makeChallenges(authz *core.Authorization, request *h return nil } -func (wfe *WebFrontEndImpl) validateReplacementOrder(_ context.Context, replaces string, response http.ResponseWriter) error { +func (wfe *WebFrontEndImpl) validateReplacementOrder(_ context.Context, replaces string, orderID string) *acme.ProblemDetails { if replaces == "" { - // No replacement indicated + wfe.log.Printf("Order %q is not a replacement\n", orderID) return nil } certID, err := wfe.parseCertID(replaces) if err != nil { - wfe.sendError(acme.MalformedProblem(fmt.Sprintf("Parsing ARI CertID failed: %s", err)), response) - return nil + return acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)) } replacementOrder := wfe.db.GetOrderByID(certID.SerialNumber.String()) if replacementOrder == nil { - return errors.New("could not find order") + return acme.NotFoundProblem("could not find order") } + if replacementOrder.Replaces != "" { - wfe.sendError(acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.SerialNumber)), response) - return nil + return acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.SerialNumber)) } + wfe.log.Printf("Order %q is a replacement\n", orderID) return nil } @@ -1730,6 +1730,12 @@ func (wfe *WebFrontEndImpl) NewOrder( return } + prob = wfe.validateReplacementOrder(context.TODO(), order.Replaces, order.ID) + if prob != nil { + wfe.sendError(prob, response) + return + } + // Create the authorizations for the order err = wfe.makeAuthorizations(order, request) if err != nil { @@ -1809,7 +1815,7 @@ func (wfe *WebFrontEndImpl) orderForDisplay( // RenewalInfo implements ACME Renewal Info (ARI) func (wfe *WebFrontEndImpl) RenewalInfo(_ context.Context, response http.ResponseWriter, request *http.Request) { if request.Method == http.MethodPost { - wfe.UpdateRenewal(context.TODO(), response, request) + //wfe.UpdateRenewal(context.TODO(), response, request) wfe.sendError(acme.InternalErrorProblem("POSTing to RenewalInfo has not been implemented yet"), response) return } @@ -1858,9 +1864,21 @@ func (wfe *WebFrontEndImpl) determineARIWindow(_ context.Context, id *core.CertI return core.RenewalInfoSimple(cert.Cert.NotBefore, cert.Cert.NotAfter), nil } +/* func (wfe *WebFrontEndImpl) UpdateRenewal(_ context.Context, response http.ResponseWriter, request *http.Request) { - // Not yet implemented + if len(request.URL.Path) == 0 { + wfe.sendError(acme.NotFoundProblem("Must specify a request path"), response) + return + } + + certID, err := wfe.parseCertID(request.URL.Path) + if err != nil { + wfe.sendError(acme.MalformedProblem(fmt.Sprintf("Parsing ARI CertID failed: %s", err)), response) + return + } + } +*/ // parseCertID parses a unique identifier (certID) as specified in // draft-ietf-acme-ari-03. It takes the composite string as input returns a @@ -1987,7 +2005,7 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( orderExpires := existingOrder.ExpiresDate orderIdentifiers := existingOrder.Identifiers // TODO(PHIL) - // orderReplaces := existingOrder.Replaces + //orderReplaces := existingOrder.Replaces // And then immediately unlock it again - we don't defer() here because // `maybeIssue` will also acquire a read lock and we call that before // returning From 4b586e43001a0a624aec775e01f170d0e13df33c Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Tue, 14 May 2024 16:42:49 -0400 Subject: [PATCH 06/21] ARI support hooray --- core/types.go | 1 - db/memorystore.go | 24 ++++++++++++++++++++ wfe/wfe.go | 58 +++++++++++++++++++++++++++++++++++------------ 3 files changed, 68 insertions(+), 15 deletions(-) diff --git a/core/types.go b/core/types.go index 44450f87..0d39a1b3 100644 --- a/core/types.go +++ b/core/types.go @@ -28,7 +28,6 @@ type Order struct { AuthorizationObjects []*Authorization BeganProcessing bool CertificateObject *Certificate - Replaces string `json:"replaces,omitempty"` } func (o *Order) GetStatus() (string, error) { diff --git a/db/memorystore.go b/db/memorystore.go index bf45796d..275f3300 100644 --- a/db/memorystore.go +++ b/db/memorystore.go @@ -212,6 +212,30 @@ func (m *MemoryStore) GetOrderByID(id string) *core.Order { return nil } +// GetOrderByCertSerial returns the order that resulted in the given certificate +// serial. If no such order exists, an error will be returned. +func (m *MemoryStore) GetOrderByCertSerial(certID *big.Int) (*core.Order, error) { + if certID == nil { + return nil, errors.New("certID was nil") + } + + m.RLock() + defer m.RUnlock() + + for _, order := range m.ordersByID { + order.Lock() + defer order.Unlock() + if order.CertificateObject.Cert == nil { + continue + } + if order.CertificateObject.Cert.SerialNumber.Cmp(certID) == 0 { + return order, nil + } + } + + return nil, errors.New("could not find order resulting in the given certificate serial number") +} + func (m *MemoryStore) GetOrdersByAccountID(accountID string) []*core.Order { m.RLock() defer m.RUnlock() diff --git a/wfe/wfe.go b/wfe/wfe.go index 9f605552..886ab93c 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -20,6 +20,7 @@ import ( "net/mail" "net/url" "os" + "slices" "sort" "strconv" "strings" @@ -1633,27 +1634,59 @@ func (wfe *WebFrontEndImpl) makeChallenges(authz *core.Authorization, request *h return nil } -func (wfe *WebFrontEndImpl) validateReplacementOrder(_ context.Context, replaces string, orderID string) *acme.ProblemDetails { - if replaces == "" { - wfe.log.Printf("Order %q is not a replacement\n", orderID) +// validateReplacementOrder performs several sanity checks on the order to +// determine if the order is a replacement of an existing order. If the order is +// not a replacement or is a valid replacement, no problem will be returned. +// Otherwise the caller will receive a problem. +func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme.ProblemDetails { + if newOrder == nil { + return acme.InternalErrorProblem("Order is nil") + } + + // Lock the order for reading + newOrder.RLock() + defer newOrder.RUnlock() + + if newOrder.Replaces == "" { + wfe.log.Printf("Order %q is not a replacement\n", newOrder.ID) return nil } - certID, err := wfe.parseCertID(replaces) + certID, err := wfe.parseCertID(newOrder.Replaces) if err != nil { return acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)) } - replacementOrder := wfe.db.GetOrderByID(certID.SerialNumber.String()) - if replacementOrder == nil { - return acme.NotFoundProblem("could not find order") + originalOrder, err := wfe.db.GetOrderByCertSerial(certID.SerialNumber) + if err != nil { + return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) } - if replacementOrder.Replaces != "" { + if originalOrder.Replaces != "" { return acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.SerialNumber)) } - wfe.log.Printf("Order %q is a replacement\n", orderID) + // Servers SHOULD check that the identified certificate and the current New + // Order request correspond to the same ACME Account and share a + // preponderance of identifiers, and that the identified certificate has not + // already been marked as replaced by a different finalized Order. Servers + // MAY ignore the replaces field in New Order requests which do not pass + // such checks. + // https://datatracker.ietf.org/doc/html/draft-ietf-acme-ari-03#section-5 + var foundMatchingIdentifier bool + for _, id := range originalOrder.Identifiers { + if slices.Contains(newOrder.Identifiers, id) { + foundMatchingIdentifier = true + break + } + } + + if !foundMatchingIdentifier { + return acme.InternalErrorProblem("at least one identifier in the new order and existing order must match") + } + + wfe.log.Printf("Order %q is a replacement of %q\n", newOrder.ID, originalOrder.ID) + return nil } @@ -1730,9 +1763,8 @@ func (wfe *WebFrontEndImpl) NewOrder( return } - prob = wfe.validateReplacementOrder(context.TODO(), order.Replaces, order.ID) - if prob != nil { - wfe.sendError(prob, response) + if err := wfe.validateReplacementOrder(order); err != nil { + wfe.sendError(err, response) return } @@ -2004,8 +2036,6 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( orderStatus := existingOrder.Status orderExpires := existingOrder.ExpiresDate orderIdentifiers := existingOrder.Identifiers - // TODO(PHIL) - //orderReplaces := existingOrder.Replaces // And then immediately unlock it again - we don't defer() here because // `maybeIssue` will also acquire a read lock and we call that before // returning From 016d9b97f431976ac1408bc88a4221aaa168d88e Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Tue, 14 May 2024 16:45:08 -0400 Subject: [PATCH 07/21] Strip trailing slash on renewalInfo just like in boulder --- wfe/wfe.go | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index 886ab93c..63154a3e 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -551,12 +551,16 @@ func (wfe *WebFrontEndImpl) Directory( request *http.Request, ) { directoryEndpoints := map[string]string{ - "newNonce": noncePath, - "newAccount": newAccountPath, - "newOrder": newOrderPath, - "revokeCert": revokeCertPath, - "keyChange": keyRolloverPath, - "renewalInfo": renewalInfoPath, + "newNonce": noncePath, + "newAccount": newAccountPath, + "newOrder": newOrderPath, + "revokeCert": revokeCertPath, + "keyChange": keyRolloverPath, + // ARI-capable clients are expected to add the trailing slash per the + // draft. We explicitly strip the trailing slash here so that clients + // don't need to add trailing slash handling in their own code, saving + // them minimal amounts of complexity. + "renewalInfo": strings.TrimRight(renewalInfoPath, "/"), } // RFC 8555 §6.3 says the server's directory endpoint should support From 24487ca8b29b737c9d6291eb120cb522e105ddc6 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Tue, 14 May 2024 16:52:29 -0400 Subject: [PATCH 08/21] Begin plumbing for indicating if an order has been replaced --- core/types.go | 2 ++ wfe/wfe.go | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/core/types.go b/core/types.go index 0d39a1b3..cb723930 100644 --- a/core/types.go +++ b/core/types.go @@ -28,6 +28,8 @@ type Order struct { AuthorizationObjects []*Authorization BeganProcessing bool CertificateObject *Certificate + // Indicates if the order has replaced via ARI. + IsReplaced bool } func (o *Order) GetStatus() (string, error) { diff --git a/wfe/wfe.go b/wfe/wfe.go index 63154a3e..d60602bd 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1666,7 +1666,7 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) } - if originalOrder.Replaces != "" { + if originalOrder.Replaces != "" || !originalOrder.IsReplaced { return acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.SerialNumber)) } From ecf01b1c945eb422e2297c99d5275ade5a352aa9 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Wed, 15 May 2024 15:38:53 -0400 Subject: [PATCH 09/21] Indicate order IsReplaced --- core/types.go | 2 +- db/memorystore.go | 26 ++++++++++++++++++++----- wfe/wfe.go | 48 +++++++++++++++++++++++------------------------ 3 files changed, 46 insertions(+), 30 deletions(-) diff --git a/core/types.go b/core/types.go index cb723930..5a8c1c35 100644 --- a/core/types.go +++ b/core/types.go @@ -28,7 +28,7 @@ type Order struct { AuthorizationObjects []*Authorization BeganProcessing bool CertificateObject *Certificate - // Indicates if the order has replaced via ARI. + // Indicates if the finalized order has been successfully replaced via ARI. IsReplaced bool } diff --git a/db/memorystore.go b/db/memorystore.go index 275f3300..465f905d 100644 --- a/db/memorystore.go +++ b/db/memorystore.go @@ -94,6 +94,22 @@ func (m *MemoryStore) GetAccountByKey(key crypto.PublicKey) (*core.Account, erro return m.accountsByKeyID[keyID], nil } +// UpdateReplacedOrder +func (m *MemoryStore) UpdateReplacedOrder(serial *big.Int) error { + originalOrder, err := m.GetOrderByCertSerial(serial) + if err != nil { + return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) + } + + m.RLock() + defer m.RUnlock() + + originalOrder.IsReplaced = true + m.ordersByID[originalOrder.ID] = originalOrder + + return nil +} + // Note that this function should *NOT* be used for key changes. It assumes // the public key associated to the account does not change. Use ChangeAccountKey // to change the account's public key. @@ -204,8 +220,8 @@ func (m *MemoryStore) GetOrderByID(id string) *core.Order { if err != nil { panic(err) } - order.Lock() - defer order.Unlock() + order.RLock() + defer order.RUnlock() order.Status = orderStatus return order } @@ -225,7 +241,7 @@ func (m *MemoryStore) GetOrderByCertSerial(certID *big.Int) (*core.Order, error) for _, order := range m.ordersByID { order.Lock() defer order.Unlock() - if order.CertificateObject.Cert == nil { + if order.CertificateObject == nil || order.CertificateObject.Cert == nil { continue } if order.CertificateObject.Cert.SerialNumber.Cmp(certID) == 0 { @@ -246,8 +262,8 @@ func (m *MemoryStore) GetOrdersByAccountID(accountID string) []*core.Order { if err != nil { panic(err) } - order.Lock() - defer order.Unlock() + order.RLock() + defer order.RUnlock() order.Status = orderStatus } return orders diff --git a/wfe/wfe.go b/wfe/wfe.go index d60602bd..71c5b4b9 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1648,11 +1648,11 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme } // Lock the order for reading - newOrder.RLock() - defer newOrder.RUnlock() + newOrder.Lock() + defer newOrder.Unlock() if newOrder.Replaces == "" { - wfe.log.Printf("Order %q is not a replacement\n", newOrder.ID) + wfe.log.Printf("ARI: order %q is not a replacement\n", newOrder.ID) return nil } @@ -1666,16 +1666,20 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) } - if originalOrder.Replaces != "" || !originalOrder.IsReplaced { + if originalOrder.IsReplaced { return acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.SerialNumber)) } + if originalOrder.AccountID != newOrder.AccountID { + return acme.UnauthorizedProblem("requester account did not request the certificate being replaced by this order") + } + // Servers SHOULD check that the identified certificate and the current New // Order request correspond to the same ACME Account and share a // preponderance of identifiers, and that the identified certificate has not // already been marked as replaced by a different finalized Order. Servers // MAY ignore the replaces field in New Order requests which do not pass - // such checks. + // such checks (but Pebble will not ignore it). // https://datatracker.ietf.org/doc/html/draft-ietf-acme-ari-03#section-5 var foundMatchingIdentifier bool for _, id := range originalOrder.Identifiers { @@ -1684,12 +1688,11 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme break } } - if !foundMatchingIdentifier { return acme.InternalErrorProblem("at least one identifier in the new order and existing order must match") } - wfe.log.Printf("Order %q is a replacement of %q\n", newOrder.ID, originalOrder.ID) + wfe.log.Printf("ARI: order %q is a replacement of %q\n", newOrder.ID, originalOrder.ID) return nil } @@ -1851,7 +1854,6 @@ func (wfe *WebFrontEndImpl) orderForDisplay( // RenewalInfo implements ACME Renewal Info (ARI) func (wfe *WebFrontEndImpl) RenewalInfo(_ context.Context, response http.ResponseWriter, request *http.Request) { if request.Method == http.MethodPost { - //wfe.UpdateRenewal(context.TODO(), response, request) wfe.sendError(acme.InternalErrorProblem("POSTing to RenewalInfo has not been implemented yet"), response) return } @@ -1900,22 +1902,6 @@ func (wfe *WebFrontEndImpl) determineARIWindow(_ context.Context, id *core.CertI return core.RenewalInfoSimple(cert.Cert.NotBefore, cert.Cert.NotAfter), nil } -/* -func (wfe *WebFrontEndImpl) UpdateRenewal(_ context.Context, response http.ResponseWriter, request *http.Request) { - if len(request.URL.Path) == 0 { - wfe.sendError(acme.NotFoundProblem("Must specify a request path"), response) - return - } - - certID, err := wfe.parseCertID(request.URL.Path) - if err != nil { - wfe.sendError(acme.MalformedProblem(fmt.Sprintf("Parsing ARI CertID failed: %s", err)), response) - return - } - -} -*/ - // parseCertID parses a unique identifier (certID) as specified in // draft-ietf-acme-ari-03. It takes the composite string as input returns a // certID struct with the keyIdentifier and serialNumber extracted and decoded. @@ -2040,6 +2026,7 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( orderStatus := existingOrder.Status orderExpires := existingOrder.ExpiresDate orderIdentifiers := existingOrder.Identifiers + orderReplaces := existingOrder.Replaces // And then immediately unlock it again - we don't defer() here because // `maybeIssue` will also acquire a read lock and we call that before // returning @@ -2174,6 +2161,19 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( wfe.log.Printf("Order %s is fully authorized. Processing finalization", orderID) go wfe.ca.CompleteOrder(existingOrder) + wfe.log.Println("here 1") + if orderReplaces != "" { + wfe.log.Println("here 2") + certID, err := wfe.parseCertID(orderReplaces) + if err != nil { + wfe.sendError(acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)), response) + return + } + wfe.log.Println("here 3") + go wfe.db.UpdateReplacedOrder(certID.SerialNumber) + wfe.log.Printf("Order %s has been marked as replaced in the DB", orderID) + } + // Set the existingOrder to processing before displaying to the user existingOrder.Status = acme.StatusProcessing From 66e448c73f2b28b8874b06f461ae4ae12aeda290 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Wed, 15 May 2024 15:44:00 -0400 Subject: [PATCH 10/21] Change locking --- db/memorystore.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/db/memorystore.go b/db/memorystore.go index 465f905d..39223c8d 100644 --- a/db/memorystore.go +++ b/db/memorystore.go @@ -101,8 +101,8 @@ func (m *MemoryStore) UpdateReplacedOrder(serial *big.Int) error { return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) } - m.RLock() - defer m.RUnlock() + m.Lock() + defer m.Unlock() originalOrder.IsReplaced = true m.ordersByID[originalOrder.ID] = originalOrder @@ -220,8 +220,8 @@ func (m *MemoryStore) GetOrderByID(id string) *core.Order { if err != nil { panic(err) } - order.RLock() - defer order.RUnlock() + order.Lock() + defer order.Unlock() order.Status = orderStatus return order } @@ -239,8 +239,8 @@ func (m *MemoryStore) GetOrderByCertSerial(certID *big.Int) (*core.Order, error) defer m.RUnlock() for _, order := range m.ordersByID { - order.Lock() - defer order.Unlock() + order.RLock() + defer order.RUnlock() if order.CertificateObject == nil || order.CertificateObject.Cert == nil { continue } @@ -262,8 +262,8 @@ func (m *MemoryStore) GetOrdersByAccountID(accountID string) []*core.Order { if err != nil { panic(err) } - order.RLock() - defer order.RUnlock() + order.Lock() + defer order.Unlock() order.Status = orderStatus } return orders From 1997f463702424a150d2fc1ee863c6f7959fd169 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Wed, 15 May 2024 15:48:53 -0400 Subject: [PATCH 11/21] Probably don't need this --- wfe/wfe.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index 71c5b4b9..6013df91 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1647,10 +1647,6 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme return acme.InternalErrorProblem("Order is nil") } - // Lock the order for reading - newOrder.Lock() - defer newOrder.Unlock() - if newOrder.Replaces == "" { wfe.log.Printf("ARI: order %q is not a replacement\n", newOrder.ID) return nil From 86812150cb2848142fded2531a0d6280aac225de Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Thu, 16 May 2024 23:07:05 -0400 Subject: [PATCH 12/21] Support replacement orders and error out during duplicate replacement --- ca/ca.go | 10 ++--- core/types.go | 3 +- db/memorystore.go | 111 +++++++++++++++++++++++++--------------------- wfe/wfe.go | 25 +++++++---- 4 files changed, 81 insertions(+), 68 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index 13afe10e..7176a201 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -148,9 +148,7 @@ func (ca *CAImpl) makeRootCert( return nil, err } - hexSerial := hex.EncodeToString(cert.SerialNumber.Bytes()) newCert := &core.Certificate{ - ID: hexSerial, Cert: cert, DER: der, } @@ -180,7 +178,7 @@ func (ca *CAImpl) newRootIssuer(name string) (*issuer, error) { return nil, err } - ca.log.Printf("Generated new root issuer %s with serial %s and SKI %x\n", rc.Cert.Subject, rc.ID, subjectKeyID) + ca.log.Printf("Generated new root issuer %s with serial %s and SKI %x\n", rc.Cert.Subject, rc.Cert.SerialNumber.String(), subjectKeyID) return &issuer{ key: rk, cert: rc, @@ -196,7 +194,7 @@ func (ca *CAImpl) newIntermediateIssuer(root *issuer, intermediateKey crypto.Sig if err != nil { return nil, err } - ca.log.Printf("Generated new intermediate issuer %s with serial %s and SKI %x\n", ic.Cert.Subject, ic.ID, subjectKeyID) + ca.log.Printf("Generated new intermediate issuer %s with serial %s and SKI %x\n", ic.Cert.Subject, ic.Cert.SerialNumber.String(), subjectKeyID) return &issuer{ key: intermediateKey, cert: ic, @@ -327,9 +325,7 @@ func (ca *CAImpl) newCertificate(domains []string, ips []net.IP, key crypto.Publ issuers[i] = issuerChain } - hexSerial := hex.EncodeToString(cert.SerialNumber.Bytes()) newCert := &core.Certificate{ - ID: hexSerial, AccountID: accountID, Cert: cert, DER: der, @@ -430,7 +426,7 @@ func (ca *CAImpl) CompleteOrder(order *core.Order) { ca.log.Printf("Error: unable to issue order: %s", err.Error()) return } - ca.log.Printf("Issued certificate serial %s for order %s\n", cert.ID, order.ID) + ca.log.Printf("Issued certificate serial %s for order %s\n", cert.Cert.SerialNumber.String(), order.ID) // Lock and update the order to store the issued certificate order.Lock() diff --git a/core/types.go b/core/types.go index 5a8c1c35..a1323f80 100644 --- a/core/types.go +++ b/core/types.go @@ -150,7 +150,6 @@ func (ch *Challenge) ExpectedKeyAuthorization(key *jose.JSONWebKey) string { } type Certificate struct { - ID string Cert *x509.Certificate DER []byte IssuerChains [][]*Certificate @@ -166,7 +165,7 @@ func (c Certificate) PEM() []byte { }) if err != nil { panic(fmt.Sprintf("Unable to encode certificate %q to PEM: %s", - c.ID, err.Error())) + c.Cert.SerialNumber.String(), err.Error())) } return buf.Bytes() diff --git a/db/memorystore.go b/db/memorystore.go index 39223c8d..7abda0f3 100644 --- a/db/memorystore.go +++ b/db/memorystore.go @@ -46,15 +46,18 @@ type MemoryStore struct { // key bytes. accountsByKeyID map[string]*core.Account - ordersByID map[string]*core.Order - ordersByAccountID map[string][]*core.Order + // ordersByIssuedSerial indexes the hex encoding of the certificate's + // SerialNumber. + ordersByIssuedSerial map[string]*core.Order + ordersByID map[string]*core.Order + ordersByAccountID map[string][]*core.Order authorizationsByID map[string]*core.Authorization challengesByID map[string]*core.Challenge - certificatesByID map[string]*core.Certificate - revokedCertificatesByID map[string]*core.RevokedCertificate + certificatesByID map[string]*core.Certificate + revokedCertificatesBySerial map[string]*core.RevokedCertificate externalAccountKeysByID map[string][]byte @@ -63,17 +66,18 @@ type MemoryStore struct { func NewMemoryStore() *MemoryStore { return &MemoryStore{ - accountRand: rand.New(rand.NewSource(time.Now().UnixNano())), - accountsByID: make(map[string]*core.Account), - accountsByKeyID: make(map[string]*core.Account), - ordersByID: make(map[string]*core.Order), - ordersByAccountID: make(map[string][]*core.Order), - authorizationsByID: make(map[string]*core.Authorization), - challengesByID: make(map[string]*core.Challenge), - certificatesByID: make(map[string]*core.Certificate), - revokedCertificatesByID: make(map[string]*core.RevokedCertificate), - externalAccountKeysByID: make(map[string][]byte), - blockListByDomain: make([][]string, 0), + accountRand: rand.New(rand.NewSource(time.Now().UnixNano())), + accountsByID: make(map[string]*core.Account), + accountsByKeyID: make(map[string]*core.Account), + ordersByIssuedSerial: make(map[string]*core.Order), + ordersByID: make(map[string]*core.Order), + ordersByAccountID: make(map[string][]*core.Order), + authorizationsByID: make(map[string]*core.Authorization), + challengesByID: make(map[string]*core.Challenge), + certificatesByID: make(map[string]*core.Certificate), + revokedCertificatesBySerial: make(map[string]*core.RevokedCertificate), + externalAccountKeysByID: make(map[string][]byte), + blockListByDomain: make([][]string, 0), } } @@ -95,17 +99,20 @@ func (m *MemoryStore) GetAccountByKey(key crypto.PublicKey) (*core.Account, erro } // UpdateReplacedOrder -func (m *MemoryStore) UpdateReplacedOrder(serial *big.Int) error { - originalOrder, err := m.GetOrderByCertSerial(serial) +// We intentionally don't Lock the database inside this method because the +// inner GetOrderByIssuedSerial which is used elsewhere does an RLock which +// would hang. +// , account *core.Account +func (m *MemoryStore) UpdateReplacedOrder(serial string) error { + if serial == "" { + return acme.InternalErrorProblem("no serial provided") + } + + originalOrder, err := m.GetOrderByIssuedSerial(serial) if err != nil { return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) } - - m.Lock() - defer m.Unlock() - originalOrder.IsReplaced = true - m.ordersByID[originalOrder.ID] = originalOrder return nil } @@ -211,6 +218,20 @@ func (m *MemoryStore) AddOrder(order *core.Order) (int, error) { return len(m.ordersByID), nil } +func (m *MemoryStore) AddOrderByIssuedSerial(order *core.Order) (int, error) { + m.Lock() + defer m.Unlock() + + if order.CertificateObject == nil || order.CertificateObject.Cert == nil { + return len(m.ordersByIssuedSerial), errors.New("order must have non-empty CertificateObject and Cert") + } + + serial := order.CertificateObject.Cert.SerialNumber.String() + m.ordersByIssuedSerial[serial] = order + + return len(m.ordersByIssuedSerial), nil +} + func (m *MemoryStore) GetOrderByID(id string) *core.Order { m.RLock() defer m.RUnlock() @@ -228,28 +249,18 @@ func (m *MemoryStore) GetOrderByID(id string) *core.Order { return nil } -// GetOrderByCertSerial returns the order that resulted in the given certificate +// GetOrderByIssuedSerial returns the order that resulted in the given certificate // serial. If no such order exists, an error will be returned. -func (m *MemoryStore) GetOrderByCertSerial(certID *big.Int) (*core.Order, error) { - if certID == nil { - return nil, errors.New("certID was nil") - } - +func (m *MemoryStore) GetOrderByIssuedSerial(serial string) (*core.Order, error) { m.RLock() defer m.RUnlock() - for _, order := range m.ordersByID { - order.RLock() - defer order.RUnlock() - if order.CertificateObject == nil || order.CertificateObject.Cert == nil { - continue - } - if order.CertificateObject.Cert.SerialNumber.Cmp(certID) == 0 { - return order, nil - } + order, ok := m.ordersByIssuedSerial[serial] + if !ok { + return nil, errors.New("could not find order resulting in the given certificate serial number") } - return nil, errors.New("could not find order resulting in the given certificate serial number") + return order, nil } func (m *MemoryStore) GetOrdersByAccountID(accountID string) []*core.Order { @@ -346,19 +357,19 @@ func (m *MemoryStore) AddCertificate(cert *core.Certificate) (int, error) { m.Lock() defer m.Unlock() - certID := cert.ID - if len(certID) == 0 { - return 0, errors.New("cert must have a non-empty ID to add to MemoryStore") + serial := cert.Cert.SerialNumber.String() + if serial == "" { + return 0, errors.New("invalid serial") } - if _, present := m.certificatesByID[certID]; present { - return 0, fmt.Errorf("cert %q already exists", certID) + if _, present := m.certificatesByID[serial]; present { + return 0, fmt.Errorf("cert %q already exists", serial) } - if _, present := m.revokedCertificatesByID[certID]; present { - return 0, fmt.Errorf("cert %q already exists (and is revoked)", certID) + if _, present := m.revokedCertificatesBySerial[serial]; present { + return 0, fmt.Errorf("cert %q already exists (and is revoked)", serial) } - m.certificatesByID[certID] = cert + m.certificatesByID[serial] = cert return len(m.certificatesByID), nil } @@ -387,7 +398,7 @@ func (m *MemoryStore) GetCertificateByDER(der []byte) *core.Certificate { func (m *MemoryStore) GetRevokedCertificateByDER(der []byte) *core.RevokedCertificate { m.RLock() defer m.RUnlock() - for _, c := range m.revokedCertificatesByID { + for _, c := range m.revokedCertificatesBySerial { if reflect.DeepEqual(c.Certificate.DER, der) { return c } @@ -399,8 +410,8 @@ func (m *MemoryStore) GetRevokedCertificateByDER(der []byte) *core.RevokedCertif func (m *MemoryStore) RevokeCertificate(cert *core.RevokedCertificate) { m.Lock() defer m.Unlock() - m.revokedCertificatesByID[cert.Certificate.ID] = cert - delete(m.certificatesByID, cert.Certificate.ID) + m.revokedCertificatesBySerial[cert.Certificate.Cert.SerialNumber.String()] = cert + delete(m.certificatesByID, cert.Certificate.Cert.SerialNumber.String()) } /* @@ -447,7 +458,7 @@ func (m *MemoryStore) GetCertificateBySerial(serialNumber *big.Int) *core.Certif func (m *MemoryStore) GetRevokedCertificateBySerial(serialNumber *big.Int) *core.RevokedCertificate { m.RLock() defer m.RUnlock() - for _, c := range m.revokedCertificatesByID { + for _, c := range m.revokedCertificatesBySerial { if serialNumber.Cmp(c.Certificate.Cert.SerialNumber) == 0 { return c } diff --git a/wfe/wfe.go b/wfe/wfe.go index 6013df91..f3023095 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1657,7 +1657,7 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme return acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)) } - originalOrder, err := wfe.db.GetOrderByCertSerial(certID.SerialNumber) + originalOrder, err := wfe.db.GetOrderByIssuedSerial(certID.SerialNumber.String()) if err != nil { return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) } @@ -1783,7 +1783,7 @@ func (wfe *WebFrontEndImpl) NewOrder( count, err := wfe.db.AddOrder(order) if err != nil { wfe.sendError( - acme.InternalErrorProblem("Error saving order"), response) + acme.InternalErrorProblem(fmt.Sprintf("Error saving order: %s", err)), response) return } wfe.log.Printf("Added order %q to the db\n", order.ID) @@ -1841,7 +1841,7 @@ func (wfe *WebFrontEndImpl) orderForDisplay( if order.CertificateObject != nil { result.Certificate = wfe.relativeEndpoint( request, - certPath+order.CertificateObject.ID) + certPath+order.CertificateObject.Cert.SerialNumber.String()) } return result @@ -2153,25 +2153,32 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( existingOrder.BeganProcessing = true existingOrder.Unlock() - // Ask the CA to complete the order in a separate goroutine. wfe.log.Printf("Order %s is fully authorized. Processing finalization", orderID) - go wfe.ca.CompleteOrder(existingOrder) + wfe.ca.CompleteOrder(existingOrder) + + // Store the order in this table so we can check if it had previously been replaced. + count, err := wfe.db.AddOrderByIssuedSerial(existingOrder) + if err != nil { + wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error saving order: %s", err)), response) + return + } + wfe.log.Printf("Added order %q to the orderByIssuedSerial DB\n", existingOrder.ID) + wfe.log.Printf("There are now %d orders in the orderByIssuedSerial DB\n", count) - wfe.log.Println("here 1") if orderReplaces != "" { - wfe.log.Println("here 2") certID, err := wfe.parseCertID(orderReplaces) if err != nil { wfe.sendError(acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)), response) return } - wfe.log.Println("here 3") - go wfe.db.UpdateReplacedOrder(certID.SerialNumber) + wfe.db.UpdateReplacedOrder(certID.SerialNumber.String()) wfe.log.Printf("Order %s has been marked as replaced in the DB", orderID) } // Set the existingOrder to processing before displaying to the user + existingOrder.Lock() existingOrder.Status = acme.StatusProcessing + existingOrder.Unlock() addRetryAfterHeader(response, wfe.retryAfterOrder) From 077727d99c6531925d4588dcecd8eb2e09e578c6 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Thu, 16 May 2024 23:52:09 -0400 Subject: [PATCH 13/21] Check error --- wfe/wfe.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index f3023095..cd527800 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -2171,7 +2171,11 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( wfe.sendError(acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)), response) return } - wfe.db.UpdateReplacedOrder(certID.SerialNumber.String()) + err = wfe.db.UpdateReplacedOrder(certID.SerialNumber.String()) + if err != nil { + wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error updating replacement order: %s", err)), response) + return + } wfe.log.Printf("Order %s has been marked as replaced in the DB", orderID) } From 59face71a1c5e7ec2da2a3b4f6c809a1be4f7684 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 17 May 2024 14:25:54 -0400 Subject: [PATCH 14/21] Fixes --- ca/ca.go | 10 +++++--- core/types.go | 5 +++- db/memorystore.go | 63 ++++++++++++++++++++++----------------------- wfe/wfe.go | 65 +++++++++++++++++++++++++---------------------- 4 files changed, 76 insertions(+), 67 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index 7176a201..13afe10e 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -148,7 +148,9 @@ func (ca *CAImpl) makeRootCert( return nil, err } + hexSerial := hex.EncodeToString(cert.SerialNumber.Bytes()) newCert := &core.Certificate{ + ID: hexSerial, Cert: cert, DER: der, } @@ -178,7 +180,7 @@ func (ca *CAImpl) newRootIssuer(name string) (*issuer, error) { return nil, err } - ca.log.Printf("Generated new root issuer %s with serial %s and SKI %x\n", rc.Cert.Subject, rc.Cert.SerialNumber.String(), subjectKeyID) + ca.log.Printf("Generated new root issuer %s with serial %s and SKI %x\n", rc.Cert.Subject, rc.ID, subjectKeyID) return &issuer{ key: rk, cert: rc, @@ -194,7 +196,7 @@ func (ca *CAImpl) newIntermediateIssuer(root *issuer, intermediateKey crypto.Sig if err != nil { return nil, err } - ca.log.Printf("Generated new intermediate issuer %s with serial %s and SKI %x\n", ic.Cert.Subject, ic.Cert.SerialNumber.String(), subjectKeyID) + ca.log.Printf("Generated new intermediate issuer %s with serial %s and SKI %x\n", ic.Cert.Subject, ic.ID, subjectKeyID) return &issuer{ key: intermediateKey, cert: ic, @@ -325,7 +327,9 @@ func (ca *CAImpl) newCertificate(domains []string, ips []net.IP, key crypto.Publ issuers[i] = issuerChain } + hexSerial := hex.EncodeToString(cert.SerialNumber.Bytes()) newCert := &core.Certificate{ + ID: hexSerial, AccountID: accountID, Cert: cert, DER: der, @@ -426,7 +430,7 @@ func (ca *CAImpl) CompleteOrder(order *core.Order) { ca.log.Printf("Error: unable to issue order: %s", err.Error()) return } - ca.log.Printf("Issued certificate serial %s for order %s\n", cert.Cert.SerialNumber.String(), order.ID) + ca.log.Printf("Issued certificate serial %s for order %s\n", cert.ID, order.ID) // Lock and update the order to store the issued certificate order.Lock() diff --git a/core/types.go b/core/types.go index a1323f80..33f3cbc8 100644 --- a/core/types.go +++ b/core/types.go @@ -150,6 +150,7 @@ func (ch *Challenge) ExpectedKeyAuthorization(key *jose.JSONWebKey) string { } type Certificate struct { + ID string Cert *x509.Certificate DER []byte IssuerChains [][]*Certificate @@ -165,7 +166,7 @@ func (c Certificate) PEM() []byte { }) if err != nil { panic(fmt.Sprintf("Unable to encode certificate %q to PEM: %s", - c.Cert.SerialNumber.String(), err.Error())) + c.ID, err.Error())) } return buf.Bytes() @@ -217,6 +218,8 @@ type SubjectKeyIDs [][]byte type CertID struct { KeyIdentifier []byte SerialNumber *big.Int + // ID is the pre-computed hex encoding of SerialNumber. + ID string } // SuggestedWindow is a type exposed inside the RenewalInfo resource. diff --git a/db/memorystore.go b/db/memorystore.go index 7abda0f3..464b2ae9 100644 --- a/db/memorystore.go +++ b/db/memorystore.go @@ -56,8 +56,8 @@ type MemoryStore struct { challengesByID map[string]*core.Challenge - certificatesByID map[string]*core.Certificate - revokedCertificatesBySerial map[string]*core.RevokedCertificate + certificatesByID map[string]*core.Certificate + revokedCertificatesByID map[string]*core.RevokedCertificate externalAccountKeysByID map[string][]byte @@ -66,18 +66,18 @@ type MemoryStore struct { func NewMemoryStore() *MemoryStore { return &MemoryStore{ - accountRand: rand.New(rand.NewSource(time.Now().UnixNano())), - accountsByID: make(map[string]*core.Account), - accountsByKeyID: make(map[string]*core.Account), - ordersByIssuedSerial: make(map[string]*core.Order), - ordersByID: make(map[string]*core.Order), - ordersByAccountID: make(map[string][]*core.Order), - authorizationsByID: make(map[string]*core.Authorization), - challengesByID: make(map[string]*core.Challenge), - certificatesByID: make(map[string]*core.Certificate), - revokedCertificatesBySerial: make(map[string]*core.RevokedCertificate), - externalAccountKeysByID: make(map[string][]byte), - blockListByDomain: make([][]string, 0), + accountRand: rand.New(rand.NewSource(time.Now().UnixNano())), + accountsByID: make(map[string]*core.Account), + accountsByKeyID: make(map[string]*core.Account), + ordersByIssuedSerial: make(map[string]*core.Order), + ordersByID: make(map[string]*core.Order), + ordersByAccountID: make(map[string][]*core.Order), + authorizationsByID: make(map[string]*core.Authorization), + challengesByID: make(map[string]*core.Challenge), + certificatesByID: make(map[string]*core.Certificate), + revokedCertificatesByID: make(map[string]*core.RevokedCertificate), + externalAccountKeysByID: make(map[string][]byte), + blockListByDomain: make([][]string, 0), } } @@ -218,18 +218,17 @@ func (m *MemoryStore) AddOrder(order *core.Order) (int, error) { return len(m.ordersByID), nil } -func (m *MemoryStore) AddOrderByIssuedSerial(order *core.Order) (int, error) { +func (m *MemoryStore) AddOrderByIssuedSerial(order *core.Order) error { m.Lock() defer m.Unlock() - if order.CertificateObject == nil || order.CertificateObject.Cert == nil { - return len(m.ordersByIssuedSerial), errors.New("order must have non-empty CertificateObject and Cert") + if order.CertificateObject == nil { + return errors.New("order must have non-empty CertificateObject") } - serial := order.CertificateObject.Cert.SerialNumber.String() - m.ordersByIssuedSerial[serial] = order + m.ordersByIssuedSerial[order.CertificateObject.ID] = order - return len(m.ordersByIssuedSerial), nil + return nil } func (m *MemoryStore) GetOrderByID(id string) *core.Order { @@ -357,19 +356,19 @@ func (m *MemoryStore) AddCertificate(cert *core.Certificate) (int, error) { m.Lock() defer m.Unlock() - serial := cert.Cert.SerialNumber.String() - if serial == "" { - return 0, errors.New("invalid serial") + certID := cert.ID + if len(certID) == 0 { + return 0, errors.New("cert must have a non-empty ID to add to MemoryStore") } - if _, present := m.certificatesByID[serial]; present { - return 0, fmt.Errorf("cert %q already exists", serial) + if _, present := m.certificatesByID[certID]; present { + return 0, fmt.Errorf("cert %q already exists", certID) } - if _, present := m.revokedCertificatesBySerial[serial]; present { - return 0, fmt.Errorf("cert %q already exists (and is revoked)", serial) + if _, present := m.revokedCertificatesByID[certID]; present { + return 0, fmt.Errorf("cert %q already exists (and is revoked)", certID) } - m.certificatesByID[serial] = cert + m.certificatesByID[certID] = cert return len(m.certificatesByID), nil } @@ -398,7 +397,7 @@ func (m *MemoryStore) GetCertificateByDER(der []byte) *core.Certificate { func (m *MemoryStore) GetRevokedCertificateByDER(der []byte) *core.RevokedCertificate { m.RLock() defer m.RUnlock() - for _, c := range m.revokedCertificatesBySerial { + for _, c := range m.revokedCertificatesByID { if reflect.DeepEqual(c.Certificate.DER, der) { return c } @@ -410,8 +409,8 @@ func (m *MemoryStore) GetRevokedCertificateByDER(der []byte) *core.RevokedCertif func (m *MemoryStore) RevokeCertificate(cert *core.RevokedCertificate) { m.Lock() defer m.Unlock() - m.revokedCertificatesBySerial[cert.Certificate.Cert.SerialNumber.String()] = cert - delete(m.certificatesByID, cert.Certificate.Cert.SerialNumber.String()) + m.revokedCertificatesByID[cert.Certificate.ID] = cert + delete(m.certificatesByID, cert.Certificate.ID) } /* @@ -458,7 +457,7 @@ func (m *MemoryStore) GetCertificateBySerial(serialNumber *big.Int) *core.Certif func (m *MemoryStore) GetRevokedCertificateBySerial(serialNumber *big.Int) *core.RevokedCertificate { m.RLock() defer m.RUnlock() - for _, c := range m.revokedCertificatesBySerial { + for _, c := range m.revokedCertificatesByID { if serialNumber.Cmp(c.Certificate.Cert.SerialNumber) == 0 { return c } diff --git a/wfe/wfe.go b/wfe/wfe.go index cd527800..65b13a91 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -7,6 +7,7 @@ import ( "crypto/rsa" "crypto/x509" "encoding/base64" + "encoding/hex" "encoding/json" "encoding/pem" "errors" @@ -674,7 +675,7 @@ func (wfe *WebFrontEndImpl) parseJWS(body string) (*jose.JSONWebSignature, error Signatures []interface{} } if err := json.Unmarshal([]byte(body), &unprotected); err != nil { - return nil, fmt.Errorf("parse error reading JWS: %w", err) + return nil, fmt.Errorf("Parse error reading JWS: %w", err) } // ACME v2 never uses values from the unprotected JWS header. Reject JWS that @@ -693,11 +694,11 @@ func (wfe *WebFrontEndImpl) parseJWS(body string) (*jose.JSONWebSignature, error parsedJWS, err := jose.ParseSigned(body, goodJWSSignatureAlgorithms) if err != nil { - return nil, fmt.Errorf("parse error reading JWS: %w", err) + return nil, fmt.Errorf("Parse error reading JWS: %w", err) } if len(parsedJWS.Signatures) > 1 { - return nil, errors.New("too many signatures in POST body") + return nil, errors.New("Too many signatures in POST body") } if len(parsedJWS.Signatures) == 0 { @@ -1657,13 +1658,13 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme return acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)) } - originalOrder, err := wfe.db.GetOrderByIssuedSerial(certID.SerialNumber.String()) + originalOrder, err := wfe.db.GetOrderByIssuedSerial(certID.ID) if err != nil { return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) } if originalOrder.IsReplaced { - return acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.SerialNumber)) + return acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.ID)) } if originalOrder.AccountID != newOrder.AccountID { @@ -1841,7 +1842,7 @@ func (wfe *WebFrontEndImpl) orderForDisplay( if order.CertificateObject != nil { result.Certificate = wfe.relativeEndpoint( request, - certPath+order.CertificateObject.Cert.SerialNumber.String()) + certPath+order.CertificateObject.ID) } return result @@ -1927,14 +1928,17 @@ func (wfe *WebFrontEndImpl) parseCertID(path string) (*core.CertID, error) { return nil, acme.MalformedProblem("path contained an Authority Key Identifier that did not match a known issuer") } - serialNumber, err := base64.RawURLEncoding.DecodeString(parts[1]) + serialBytes, err := base64.RawURLEncoding.DecodeString(parts[1]) if err != nil { return nil, acme.MalformedProblem(fmt.Sprintf("serial number was not base64url-encoded or contained padding: %s", err)) } + serialNumber := new(big.Int).SetBytes(serialBytes) + serialID := hex.EncodeToString(serialBytes) return &core.CertID{ KeyIdentifier: akid, - SerialNumber: new(big.Int).SetBytes(serialNumber), + SerialNumber: serialNumber, + ID: serialID, }, nil } @@ -2023,9 +2027,6 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( orderExpires := existingOrder.ExpiresDate orderIdentifiers := existingOrder.Identifiers orderReplaces := existingOrder.Replaces - // And then immediately unlock it again - we don't defer() here because - // `maybeIssue` will also acquire a read lock and we call that before - // returning existingOrder.RUnlock() if orderAccountID != existingAcct.ID { @@ -2149,40 +2150,42 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( // Lock and update the order with the parsed CSR and the began processing // state. existingOrder.Lock() + // do a check on the stored order to see if another process + // changed the order status existingOrder.ParsedCSR = parsedCSR existingOrder.BeganProcessing = true existingOrder.Unlock() wfe.log.Printf("Order %s is fully authorized. Processing finalization", orderID) - wfe.ca.CompleteOrder(existingOrder) - // Store the order in this table so we can check if it had previously been replaced. - count, err := wfe.db.AddOrderByIssuedSerial(existingOrder) - if err != nil { - wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error saving order: %s", err)), response) - return - } - wfe.log.Printf("Added order %q to the orderByIssuedSerial DB\n", existingOrder.ID) - wfe.log.Printf("There are now %d orders in the orderByIssuedSerial DB\n", count) + // Perform asynchronous finalization + go func() { + wfe.ca.CompleteOrder(existingOrder) - if orderReplaces != "" { - certID, err := wfe.parseCertID(orderReplaces) + // Store the order in this table so we can check if it had previously been replaced. + err := wfe.db.AddOrderByIssuedSerial(existingOrder) if err != nil { - wfe.sendError(acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)), response) + wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error saving order: %s", err)), response) return } - err = wfe.db.UpdateReplacedOrder(certID.SerialNumber.String()) - if err != nil { - wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error updating replacement order: %s", err)), response) - return + + if orderReplaces != "" { + certID, err := wfe.parseCertID(orderReplaces) + if err != nil { + wfe.sendError(acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)), response) + return + } + err = wfe.db.UpdateReplacedOrder(certID.ID) + if err != nil { + wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error updating replacement order: %s", err)), response) + return + } + wfe.log.Printf("Order %s has been marked as replaced in the DB", orderID) } - wfe.log.Printf("Order %s has been marked as replaced in the DB", orderID) - } + }() // Set the existingOrder to processing before displaying to the user - existingOrder.Lock() existingOrder.Status = acme.StatusProcessing - existingOrder.Unlock() addRetryAfterHeader(response, wfe.retryAfterOrder) From 864e1167433e0a738e2a5e48e10c13a40d4ee309 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 17 May 2024 14:30:09 -0400 Subject: [PATCH 15/21] Did not need to bump minimum go version --- go.mod | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go.mod b/go.mod index d9b7d22b..388cee75 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/letsencrypt/pebble/v2 -go 1.22 +go 1.21 require ( github.com/go-jose/go-jose/v4 v4.0.1 From 6d1c0e74a01f7399ef27c26464022481bc0fe146 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Fri, 17 May 2024 14:36:43 -0400 Subject: [PATCH 16/21] Ignore cyclomatic complexity and cognitive load lints on FinalizeOrder which we know is way to big --- wfe/wfe.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index 65b13a91..7157795b 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1992,7 +1992,7 @@ func (wfe *WebFrontEndImpl) Order( } } -func (wfe *WebFrontEndImpl) FinalizeOrder( +func (wfe *WebFrontEndImpl) FinalizeOrder( //nolint:gocyclo,gocognit _ context.Context, response http.ResponseWriter, request *http.Request, From 9646f8effd1e47a43ad530263f95df9e35ec33d5 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Mon, 20 May 2024 17:14:46 -0400 Subject: [PATCH 17/21] Address comments --- ca/ca.go | 17 ++++++++---- cmd/pebble/main.go | 2 +- core/types.go | 28 +++++++++++++++----- db/memorystore.go | 17 +++++++----- va/va.go | 15 +++++++++-- wfe/wfe.go | 65 +++++++++++++++++++--------------------------- 6 files changed, 85 insertions(+), 59 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index 13afe10e..d7534d95 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -438,16 +438,23 @@ func (ca *CAImpl) CompleteOrder(order *core.Order) { order.Unlock() } -func (ca *CAImpl) GetSubjectKeyIDs() core.SubjectKeyIDs { - skis := core.SubjectKeyIDs{} +// GetIntermediateBySKID attempts to match the incoming Authority Key Idenfitier +// (AKID) bytes to the Subject Key Identifier of an intermediate certificate. It +// returns an error if no match is found. +func (ca *CAImpl) GetIntermediateBySKID(issuer []byte) error { + if issuer == nil { + return errors.New("issuer bytes must not be nil") + } + for _, chain := range ca.chains { - skis = append(skis, chain.root.cert.Cert.SubjectKeyId) for _, intermediate := range chain.intermediates { - skis = append(skis, intermediate.cert.Cert.SubjectKeyId) + if bytes.Equal(intermediate.cert.Cert.SubjectKeyId, issuer) { + return nil + } } } - return skis + return errors.New("no known issuer matches the provided Authority Key Identifier ") } func (ca *CAImpl) GetNumberOfRootCerts() int { diff --git a/cmd/pebble/main.go b/cmd/pebble/main.go index 9fb0886c..fde0ed5d 100644 --- a/cmd/pebble/main.go +++ b/cmd/pebble/main.go @@ -95,7 +95,7 @@ func main() { db := db.NewMemoryStore() ca := ca.New(logger, db, c.Pebble.OCSPResponderURL, alternateRoots, chainLength, c.Pebble.CertificateValidityPeriod) - va := va.New(logger, c.Pebble.HTTPPort, c.Pebble.TLSPort, *strictMode, *resolverAddress) + va := va.New(logger, c.Pebble.HTTPPort, c.Pebble.TLSPort, *strictMode, *resolverAddress, db) for keyID, key := range c.Pebble.ExternalAccountMACKeys { err := db.AddExternalAccountKeyByID(keyID, key) diff --git a/core/types.go b/core/types.go index 33f3cbc8..1b52c43a 100644 --- a/core/types.go +++ b/core/types.go @@ -5,6 +5,7 @@ import ( "crypto" "crypto/x509" "encoding/base64" + "encoding/hex" "encoding/pem" "errors" "fmt" @@ -204,10 +205,6 @@ type ValidationRecord struct { ValidatedAt time.Time } -// SubjectKeyIDs is a convenience type that holds the Subject Key Identifier -// value for each Pebble generated root and intermediate certificate. -type SubjectKeyIDs [][]byte - // CertID represents a unique identifier (CertID) for a certificate as per the // ACME protocol's "renewalInfo" resource, as specified in draft-ietf-acme-ari- // 03. The CertID is a composite string derived from the base64url-encoded @@ -218,8 +215,27 @@ type SubjectKeyIDs [][]byte type CertID struct { KeyIdentifier []byte SerialNumber *big.Int - // ID is the pre-computed hex encoding of SerialNumber. - ID string + // id is the pre-computed hex encoding of SerialNumber. + id string +} + +// SerialHex returns a CertID's id field. +func (c CertID) SerialHex() string { + return c.id +} + +// NewCertID takes bytes representing a serial number and authority key +// identifier and returns a CertID or an error. +func NewCertID(serial []byte, akid []byte) (*CertID, error) { + if serial == nil || akid == nil { + return nil, fmt.Errorf("must send non-nil bytes") + } + + return &CertID{ + KeyIdentifier: akid, + SerialNumber: new(big.Int).SetBytes(serial), + id: hex.EncodeToString(serial), + }, nil } // SuggestedWindow is a type exposed inside the RenewalInfo resource. diff --git a/db/memorystore.go b/db/memorystore.go index 464b2ae9..66c3cab0 100644 --- a/db/memorystore.go +++ b/db/memorystore.go @@ -98,12 +98,13 @@ func (m *MemoryStore) GetAccountByKey(key crypto.PublicKey) (*core.Account, erro return m.accountsByKeyID[keyID], nil } -// UpdateReplacedOrder -// We intentionally don't Lock the database inside this method because the -// inner GetOrderByIssuedSerial which is used elsewhere does an RLock which -// would hang. -// , account *core.Account -func (m *MemoryStore) UpdateReplacedOrder(serial string) error { +// UpdateReplacedOrder takes a serial and marks a parent order as +// replaced/not-replaced or returns an error. +// +// We intentionally don't Lock the database inside this method because the inner +// GetOrderByIssuedSerial which is used elsewhere does an RLock which would +// hang. +func (m *MemoryStore) UpdateReplacedOrder(serial string, shouldBeReplaced bool) error { if serial == "" { return acme.InternalErrorProblem("no serial provided") } @@ -112,7 +113,9 @@ func (m *MemoryStore) UpdateReplacedOrder(serial string) error { if err != nil { return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) } - originalOrder.IsReplaced = true + originalOrder.Lock() + defer originalOrder.Unlock() + originalOrder.IsReplaced = shouldBeReplaced return nil } diff --git a/va/va.go b/va/va.go index f4cffaa3..9933ef66 100644 --- a/va/va.go +++ b/va/va.go @@ -27,6 +27,7 @@ import ( "github.com/letsencrypt/challtestsrv" "github.com/letsencrypt/pebble/v2/acme" "github.com/letsencrypt/pebble/v2/core" + "github.com/letsencrypt/pebble/v2/db" ) const ( @@ -108,12 +109,14 @@ type VAImpl struct { strict bool customResolverAddr string dnsClient *dns.Client + db *db.MemoryStore } func New( log *log.Logger, httpPort, tlsPort int, strict bool, customResolverAddr string, + db *db.MemoryStore, ) *VAImpl { va := &VAImpl{ log: log, @@ -124,6 +127,7 @@ func New( sleepTime: defaultSleepTime, strict: strict, customResolverAddr: customResolverAddr, + db: db, } if customResolverAddr != "" { @@ -209,10 +213,17 @@ func (va VAImpl) setAuthzValid(authz *core.Authorization, chal *core.Challenge) // setOrderError updates an order with an error from an authorization // validation. -func (va VAImpl) setOrderError(order *core.Order, err *acme.ProblemDetails) { +func (va VAImpl) setOrderError(order *core.Order, prob *acme.ProblemDetails) { order.Lock() defer order.Unlock() - order.Error = err + order.Error = prob + + // Mark the parent order as "not replaced yet" so a new replacement order + // can be attempted. + err := va.db.UpdateReplacedOrder(order.CertificateObject.ID, false) + if err != nil { + va.log.Printf("Error updating replacement order: %s", err) + } } // setAuthzInvalid updates an authorization and an associated challenge to be diff --git a/wfe/wfe.go b/wfe/wfe.go index 7157795b..535c7da2 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -7,7 +7,6 @@ import ( "crypto/rsa" "crypto/x509" "encoding/base64" - "encoding/hex" "encoding/json" "encoding/pem" "errors" @@ -1457,6 +1456,11 @@ func (wfe *WebFrontEndImpl) verifyOrder(order *core.Order) *acme.ProblemDetails return problem } } + + if problem := wfe.validateReplacementOrder(order); problem != nil { + return problem + } + return nil } @@ -1649,7 +1653,6 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme } if newOrder.Replaces == "" { - wfe.log.Printf("ARI: order %q is not a replacement\n", newOrder.ID) return nil } @@ -1658,26 +1661,26 @@ func (wfe *WebFrontEndImpl) validateReplacementOrder(newOrder *core.Order) *acme return acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)) } - originalOrder, err := wfe.db.GetOrderByIssuedSerial(certID.ID) + originalOrder, err := wfe.db.GetOrderByIssuedSerial(certID.SerialHex()) if err != nil { return acme.InternalErrorProblem(fmt.Sprintf("could not find an order for the given certificate: %s", err)) } if originalOrder.IsReplaced { - return acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.ID)) + return acme.Conflict(fmt.Sprintf("cannot indicate an order replaces certificate with serial %s, which already has a replacement order", certID.SerialHex())) } if originalOrder.AccountID != newOrder.AccountID { return acme.UnauthorizedProblem("requester account did not request the certificate being replaced by this order") } - // Servers SHOULD check that the identified certificate and the current New - // Order request correspond to the same ACME Account and share a - // preponderance of identifiers, and that the identified certificate has not - // already been marked as replaced by a different finalized Order. Servers - // MAY ignore the replaces field in New Order requests which do not pass - // such checks (but Pebble will not ignore it). - // https://datatracker.ietf.org/doc/html/draft-ietf-acme-ari-03#section-5 + // Servers SHOULD check that the identified certificate and the New Order + // request correspond to the same ACME Account, that they share at least one + // identifier, and that the identified certificate has not already been + // marked as replaced by a different Order that is not "invalid". + // Correspondence checks beyond this (such as requiring exact identifier + // matching) are left up to Server policy. If any of these checks fail, the + // Server SHOULD reject the new-order request. var foundMatchingIdentifier bool for _, id := range originalOrder.Identifiers { if slices.Contains(newOrder.Identifiers, id) { @@ -1767,11 +1770,6 @@ func (wfe *WebFrontEndImpl) NewOrder( return } - if err := wfe.validateReplacementOrder(order); err != nil { - wfe.sendError(err, response) - return - } - // Create the authorizations for the order err = wfe.makeAuthorizations(order, request) if err != nil { @@ -1915,17 +1913,9 @@ func (wfe *WebFrontEndImpl) parseCertID(path string) (*core.CertID, error) { return nil, acme.MalformedProblem(fmt.Sprintf("Authority Key Identifier was not base64url-encoded or contained padding: %s", err)) } - var found bool - skis := wfe.ca.GetSubjectKeyIDs() - for _, skiBytes := range skis { - if bytes.Equal(skiBytes, akid) { - found = true - wfe.log.Printf("Found an issuer matching SubjectKeyID %x", skiBytes) - break - } - } - if !found { - return nil, acme.MalformedProblem("path contained an Authority Key Identifier that did not match a known issuer") + err = wfe.ca.GetIntermediateBySKID(akid) + if err != nil { + return nil, acme.MalformedProblem(err.Error()) } serialBytes, err := base64.RawURLEncoding.DecodeString(parts[1]) @@ -1933,13 +1923,12 @@ func (wfe *WebFrontEndImpl) parseCertID(path string) (*core.CertID, error) { return nil, acme.MalformedProblem(fmt.Sprintf("serial number was not base64url-encoded or contained padding: %s", err)) } - serialNumber := new(big.Int).SetBytes(serialBytes) - serialID := hex.EncodeToString(serialBytes) - return &core.CertID{ - KeyIdentifier: akid, - SerialNumber: serialNumber, - ID: serialID, - }, nil + certID, err := core.NewCertID(serialBytes, akid) + if err != nil { + return nil, acme.MalformedProblem(fmt.Sprintf("error creating certID: %s", err)) + } + + return certID, nil } // Order retrieves the details of an existing order @@ -2165,19 +2154,19 @@ func (wfe *WebFrontEndImpl) FinalizeOrder( //nolint:gocyclo,gocognit // Store the order in this table so we can check if it had previously been replaced. err := wfe.db.AddOrderByIssuedSerial(existingOrder) if err != nil { - wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error saving order: %s", err)), response) + wfe.log.Printf("Error saving order: %s", err) return } if orderReplaces != "" { certID, err := wfe.parseCertID(orderReplaces) if err != nil { - wfe.sendError(acme.MalformedProblem(fmt.Sprintf("parsing ARI CertID failed: %s", err)), response) + wfe.log.Printf("parsing ARI CertID failed: %s", err) return } - err = wfe.db.UpdateReplacedOrder(certID.ID) + err = wfe.db.UpdateReplacedOrder(certID.SerialHex(), true) if err != nil { - wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error updating replacement order: %s", err)), response) + wfe.log.Printf("Error updating replacement order: %s", err) return } wfe.log.Printf("Order %s has been marked as replaced in the DB", orderID) From 8954c406ca5a18c88a3d3aa9f26cda629f26157b Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Tue, 21 May 2024 10:11:18 -0400 Subject: [PATCH 18/21] Address comments --- ca/ca.go | 6 +++--- va/va.go | 6 +++++- va/va_test.go | 2 +- wfe/wfe.go | 2 +- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index d7534d95..ae0175f0 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -438,10 +438,10 @@ func (ca *CAImpl) CompleteOrder(order *core.Order) { order.Unlock() } -// GetIntermediateBySKID attempts to match the incoming Authority Key Idenfitier -// (AKID) bytes to the Subject Key Identifier of an intermediate certificate. It +// RecognizedSKID attempts to match the incoming Authority Key Idenfitier (AKID) +// bytes to the Subject Key Identifier (SKID) of an intermediate certificate. It // returns an error if no match is found. -func (ca *CAImpl) GetIntermediateBySKID(issuer []byte) error { +func (ca *CAImpl) RecognizedSKID(issuer []byte) error { if issuer == nil { return errors.New("issuer bytes must not be nil") } diff --git a/va/va.go b/va/va.go index 9933ef66..58685a8a 100644 --- a/va/va.go +++ b/va/va.go @@ -109,7 +109,11 @@ type VAImpl struct { strict bool customResolverAddr string dnsClient *dns.Client - db *db.MemoryStore + + // The VA having a DB client is indeed strange. This is only used to + // facilitate va.setOrderError changing the ARI related order replacement + // field on failed orders. + db *db.MemoryStore } func New( diff --git a/va/va_test.go b/va/va_test.go index d8d40fe9..8dcc8f75 100644 --- a/va/va_test.go +++ b/va/va_test.go @@ -31,7 +31,7 @@ func TestAuthzRace(_ *testing.T) { // This whole test can be removed if/when the MemoryStore becomes 100% by value ms := db.NewMemoryStore() - va := New(log.New(os.Stdout, "Pebble/TestRace", log.LstdFlags), 14000, 15000, false, "") + va := New(log.New(os.Stdout, "Pebble/TestRace", log.LstdFlags), 14000, 15000, false, "", ms) authz := &core.Authorization{ ID: "auth-id", diff --git a/wfe/wfe.go b/wfe/wfe.go index 535c7da2..db0caa73 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1913,7 +1913,7 @@ func (wfe *WebFrontEndImpl) parseCertID(path string) (*core.CertID, error) { return nil, acme.MalformedProblem(fmt.Sprintf("Authority Key Identifier was not base64url-encoded or contained padding: %s", err)) } - err = wfe.ca.GetIntermediateBySKID(akid) + err = wfe.ca.RecognizedSKID(akid) if err != nil { return nil, acme.MalformedProblem(err.Error()) } From e1b296613c2801c3cb7eaac1e457079130e63229 Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Tue, 21 May 2024 12:10:19 -0400 Subject: [PATCH 19/21] Fix lint --- core/types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/types.go b/core/types.go index 1b52c43a..23c1f141 100644 --- a/core/types.go +++ b/core/types.go @@ -228,7 +228,7 @@ func (c CertID) SerialHex() string { // identifier and returns a CertID or an error. func NewCertID(serial []byte, akid []byte) (*CertID, error) { if serial == nil || akid == nil { - return nil, fmt.Errorf("must send non-nil bytes") + return nil, errors.New("must send non-nil bytes") } return &CertID{ From 96aa6c083d7f466951d851cfe4cad511395ee49f Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Tue, 21 May 2024 15:36:56 -0400 Subject: [PATCH 20/21] Pass the parent serial instead --- va/va.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/va/va.go b/va/va.go index 58685a8a..cc1f098c 100644 --- a/va/va.go +++ b/va/va.go @@ -224,7 +224,7 @@ func (va VAImpl) setOrderError(order *core.Order, prob *acme.ProblemDetails) { // Mark the parent order as "not replaced yet" so a new replacement order // can be attempted. - err := va.db.UpdateReplacedOrder(order.CertificateObject.ID, false) + err := va.db.UpdateReplacedOrder(order.Replaces, false) if err != nil { va.log.Printf("Error updating replacement order: %s", err) } From 5940490d269a233ad1183ea0fc705d4038b6746c Mon Sep 17 00:00:00 2001 From: Phil Porada Date: Thu, 23 May 2024 19:09:09 -0400 Subject: [PATCH 21/21] Remove useless context --- wfe/wfe.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/wfe/wfe.go b/wfe/wfe.go index db0caa73..264fd014 100644 --- a/wfe/wfe.go +++ b/wfe/wfe.go @@ -1864,7 +1864,7 @@ func (wfe *WebFrontEndImpl) RenewalInfo(_ context.Context, response http.Respons return } - renewalInfo, err := wfe.determineARIWindow(context.TODO(), certID) + renewalInfo, err := wfe.determineARIWindow(certID) if err != nil { wfe.sendError(acme.InternalErrorProblem(fmt.Sprintf("Error determining renewal window: %s", err)), response) return @@ -1878,7 +1878,7 @@ func (wfe *WebFrontEndImpl) RenewalInfo(_ context.Context, response http.Respons } } -func (wfe *WebFrontEndImpl) determineARIWindow(_ context.Context, id *core.CertID) (*core.RenewalInfo, error) { +func (wfe *WebFrontEndImpl) determineARIWindow(id *core.CertID) (*core.RenewalInfo, error) { if id == nil { return nil, errors.New("CertID was nil") }