Support replacement orders and error out during duplicate replacement

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

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

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
 		}

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)