Using account IDs different from key IDs (letsencrypt#154)

This PR changes the way account IDs are created. They used to be the SHA256 of the account's public key before. Now, they are numeric IDs (incremented by one for every new account).

This is a prerequisite for correctly implementing account key roll-over, for which the account's ID must no longer depend on the account's public key (see letsencrypt#151).

db/memorystore.go

@@ -1,10 +1,17 @@
 package db
 
 import (
+	"crypto"
+	"crypto/sha256"
+	"crypto/x509"
+	"encoding/hex"
 	"fmt"
 	"reflect"
+	"strconv"
 	"sync"
 
+	"gopkg.in/square/go-jose.v2"
+
 	"github.com/jmhodges/clock"
 	"github.com/letsencrypt/pebble/core"
 )
@@ -17,10 +24,14 @@ type MemoryStore struct {
 
 	clk clock.Clock
 
-	// Each Accounts's ID is the hex encoding of a SHA256 sum over its public
-	// key bytes.
+	accountIDCounter int
+
 	accountsByID map[string]*core.Account
 
+	// Each Accounts's key ID is the hex encoding of a SHA256 sum over its public
+	// key bytes.
+	accountsByKeyID map[string]*core.Account
+
 	ordersByID map[string]*core.Order
 
 	authorizationsByID map[string]*core.Authorization
@@ -33,7 +44,9 @@ type MemoryStore struct {
 func NewMemoryStore(clk clock.Clock) *MemoryStore {
 	return &MemoryStore{
 		clk:                clk,
+		accountIDCounter:   1,
 		accountsByID:       make(map[string]*core.Account),
+		accountsByKeyID:    make(map[string]*core.Account),
 		ordersByID:         make(map[string]*core.Order),
 		authorizationsByID: make(map[string]*core.Authorization),
 		challengesByID:     make(map[string]*core.Challenge),
@@ -47,6 +60,17 @@ func (m *MemoryStore) GetAccountByID(id string) *core.Account {
 	return m.accountsByID[id]
 }
 
+func (m *MemoryStore) GetAccountByKey(key crypto.PublicKey) (*core.Account, error) {
+	keyID, err := keyToID(key)
+	if err != nil {
+		return nil, err
+	}
+
+	m.RLock()
+	defer m.RUnlock()
+	return m.accountsByKeyID[keyID], nil
+}
+
 func (m *MemoryStore) UpdateAccountByID(id string, acct *core.Account) error {
 	m.Lock()
 	defer m.Unlock()
@@ -61,20 +85,29 @@ func (m *MemoryStore) AddAccount(acct *core.Account) (int, error) {
 	m.Lock()
 	defer m.Unlock()
 
-	acctID := acct.ID
-	if len(acctID) == 0 {
-		return 0, fmt.Errorf("account must have a non-empty ID to add to MemoryStore")
-	}
+	acctID := strconv.Itoa(m.accountIDCounter)
+	m.accountIDCounter += 1
 
 	if acct.Key == nil {
 		return 0, fmt.Errorf("account must not have a nil Key")
 	}
 
+	keyID, err := keyToID(acct.Key)
+	if err != nil {
+		return 0, err
+	}
+
 	if _, present := m.accountsByID[acctID]; present {
 		return 0, fmt.Errorf("account %q already exists", acctID)
 	}
 
+	if _, present := m.accountsByKeyID[keyID]; present {
+		return 0, fmt.Errorf("account with key already exists")
+	}
+
+	acct.ID = acctID
 	m.accountsByID[acctID] = acct
+	m.accountsByKeyID[keyID] = acct
 	return len(m.accountsByID), nil
 }
 
@@ -206,3 +239,27 @@ func (m *MemoryStore) RevokeCertificate(cert *core.Certificate) {
 	defer m.Unlock()
 	delete(m.certificatesByID, cert.ID)
 }
+
+/*
+ * keyToID produces a string with the hex representation of the SHA256 digest
+ * over a provided public key. We use this to associate public keys to
+ * acme.Account objects, and to ensure every account has a unique public key.
+ */
+func keyToID(key crypto.PublicKey) (string, error) {
+	switch t := key.(type) {
+	case *jose.JSONWebKey:
+		if t == nil {
+			return "", fmt.Errorf("Cannot compute ID of nil key")
+		}
+		return keyToID(t.Key)
+	case jose.JSONWebKey:
+		return keyToID(t.Key)
+	default:
+		keyDER, err := x509.MarshalPKIXPublicKey(key)
+		if err != nil {
+			return "", err
+		}
+		spkiDigest := sha256.Sum256(keyDER)
+		return hex.EncodeToString(spkiDigest[:]), nil
+	}
+}

wfe/wfe.go

@@ -3,10 +3,8 @@ package wfe
 import (
 	"context"
 	"crypto"
-	"crypto/sha256"
 	"crypto/x509"
 	"encoding/base64"
-	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -346,32 +344,6 @@ func (wfe *WebFrontEndImpl) Nonce(
 	response.WriteHeader(http.StatusNoContent)
 }
 
-/*
- * keyToID produces a string with the hex representation of the SHA256 digest
- * over a provided public key. We use this for acme.Account ID values
- * because it makes looking up a account by key easy (required by the spec
- * for retreiving existing account), and becauase it makes the reg URLs
- * somewhat human digestable/comparable.
- */
-func keyToID(key crypto.PublicKey) (string, error) {
-	switch t := key.(type) {
-	case *jose.JSONWebKey:
-		if t == nil {
-			return "", fmt.Errorf("Cannot compute ID of nil key")
-		}
-		return keyToID(t.Key)
-	case jose.JSONWebKey:
-		return keyToID(t.Key)
-	default:
-		keyDER, err := x509.MarshalPKIXPublicKey(key)
-		if err != nil {
-			return "", err
-		}
-		spkiDigest := sha256.Sum256(keyDER)
-		return hex.EncodeToString(spkiDigest[:]), nil
-	}
-}
-
 func (wfe *WebFrontEndImpl) parseJWS(body string) (*jose.JSONWebSignature, error) {
 	// Parse the raw JWS JSON to check that:
 	// * the unprotected Header field is not being used.
@@ -768,16 +740,8 @@ func (wfe *WebFrontEndImpl) NewAccount(
 		return
 	}
 
-	keyID, err := keyToID(key)
-	if err != nil {
-		wfe.sendError(acme.MalformedProblem(err.Error()), response)
-		return
-	}
-
 	// Lookup existing account to exit early if it exists
-	// NOTE: We don't use wfe.getAccountByKey here because we want to treat a
-	//       "missing" account as a non-error
-	existingAcct := wfe.db.GetAccountByID(keyID)
+	existingAcct, _ := wfe.db.GetAccountByKey(key)
 	if existingAcct != nil {
 		// If there is an existing account then return a Location header pointing to
 		// the account and a 200 OK response
@@ -811,7 +775,6 @@ func (wfe *WebFrontEndImpl) NewAccount(
 			Status: acme.StatusValid,
 		},
 		Key: key,
-		ID:  keyID,
 	}
 
 	// Verify that the contact information provided is supported & valid
@@ -1433,16 +1396,12 @@ func (wfe *WebFrontEndImpl) getChallenge(
 // getAcctByKey finds a account by key or returns a problem pointer if an
 // existing account can't be found or the key is invalid.
 func (wfe *WebFrontEndImpl) getAcctByKey(key crypto.PublicKey) (*core.Account, *acme.ProblemDetails) {
-	// Compute the account ID for the signer's key
-	regID, err := keyToID(key)
+	// Find the existing account object for that key
+	existingAcct, err := wfe.db.GetAccountByKey(key)
 	if err != nil {
-		wfe.log.Printf("keyToID err: %s\n", err.Error())
-		return nil, acme.MalformedProblem("Error computing key digest")
+		return nil, acme.AccountDoesNotExistProblem("Error while retrieving key ID from public key")
 	}
-
-	// Find the existing account object for that key ID
-	var existingAcct *core.Account
-	if existingAcct = wfe.db.GetAccountByID(regID); existingAcct == nil {
+	if existingAcct == nil {
 		return nil, acme.AccountDoesNotExistProblem(
 			"URL in JWS 'kid' field does not correspond to an account")
 	}
@@ -1747,14 +1706,12 @@ func (wfe *WebFrontEndImpl) revokeCertByKeyID(
 		return prob
 	}
 
-	keyID, err := keyToID(key)
+	existingAcct, err := wfe.db.GetAccountByKey(key)
 	if err != nil {
-		return acme.MalformedProblem(err.Error())
+		return acme.MalformedProblem(fmt.Sprintf("Cannot obtain key ID from public key (%s)", err.Error()))
 	}
-
-	existingAcct := wfe.db.GetAccountByID(keyID)
 	if existingAcct == nil {
-		return acme.UnauthorizedProblem(fmt.Sprintf("Account with keyID %q does not exist", keyID))
+		return acme.UnauthorizedProblem("No account found corresponding to public key authenticating this request")
 	}
 
 	// An account is only authorized to revoke its own certificates presently.