connect: derive connect certificate serial numbers from a memdb index instead of the provider table max index (#7011)

agent/connect/ca/provider_consul.go

@@ -45,7 +45,7 @@ type ConsulProvider struct {
 
 type ConsulProviderStateDelegate interface {
 	State() *state.Store
-	ApplyCARequest(*structs.CARequest) error
+	ApplyCARequest(*structs.CARequest) (interface{}, error)
 }
 
 // Configure sets up the provider using the given configuration.
@@ -91,7 +91,7 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error {
 			Op:            structs.CAOpSetProviderState,
 			ProviderState: &newState,
 		}
-		if err := c.Delegate.ApplyCARequest(createReq); err != nil {
+		if _, err := c.Delegate.ApplyCARequest(createReq); err != nil {
 			return err
 		}
 
@@ -99,7 +99,7 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error {
 			Op:            structs.CAOpDeleteProviderState,
 			ProviderState: providerState,
 		}
-		if err := c.Delegate.ApplyCARequest(deleteReq); err != nil {
+		if _, err := c.Delegate.ApplyCARequest(deleteReq); err != nil {
 			return err
 		}
 
@@ -115,7 +115,7 @@ func (c *ConsulProvider) Configure(cfg ProviderConfig) error {
 		Op:            structs.CAOpSetProviderState,
 		ProviderState: &newState,
 	}
-	if err := c.Delegate.ApplyCARequest(args); err != nil {
+	if _, err := c.Delegate.ApplyCARequest(args); err != nil {
 		return err
 	}
 
@@ -147,7 +147,7 @@ func (c *ConsulProvider) ActiveRoot() (string, error) {
 // GenerateRoot initializes a new root certificate and private key
 // if needed.
 func (c *ConsulProvider) GenerateRoot() error {
-	idx, providerState, err := c.getState()
+	_, providerState, err := c.getState()
 	if err != nil {
 		return err
 	}
@@ -173,7 +173,12 @@ func (c *ConsulProvider) GenerateRoot() error {
 
 	// Generate the root CA if necessary
 	if c.config.RootCert == "" {
-		ca, err := c.generateCA(newState.PrivateKey, idx+1)
+		nextSerial, err := c.incrementAndGetNextSerialNumber()
+		if err != nil {
+			return fmt.Errorf("error computing next serial number: %v", err)
+		}
+
+		ca, err := c.generateCA(newState.PrivateKey, nextSerial)
 		if err != nil {
 			return fmt.Errorf("error generating CA: %v", err)
 		}
@@ -187,7 +192,7 @@ func (c *ConsulProvider) GenerateRoot() error {
 		Op:            structs.CAOpSetProviderState,
 		ProviderState: &newState,
 	}
-	if err := c.Delegate.ApplyCARequest(args); err != nil {
+	if _, err := c.Delegate.ApplyCARequest(args); err != nil {
 		return err
 	}
 
@@ -231,7 +236,7 @@ func (c *ConsulProvider) GenerateIntermediateCSR() (string, error) {
 		Op:            structs.CAOpSetProviderState,
 		ProviderState: &newState,
 	}
-	if err := c.Delegate.ApplyCARequest(args); err != nil {
+	if _, err := c.Delegate.ApplyCARequest(args); err != nil {
 		return "", err
 	}
 
@@ -267,7 +272,7 @@ func (c *ConsulProvider) SetIntermediate(intermediatePEM, rootPEM string) error
 		Op:            structs.CAOpSetProviderState,
 		ProviderState: &newState,
 	}
-	if err := c.Delegate.ApplyCARequest(args); err != nil {
+	if _, err := c.Delegate.ApplyCARequest(args); err != nil {
 		return err
 	}
 
@@ -301,7 +306,7 @@ func (c *ConsulProvider) Cleanup() error {
 		Op:            structs.CAOpDeleteProviderState,
 		ProviderState: &structs.CAConsulProviderState{ID: c.id},
 	}
-	if err := c.Delegate.ApplyCARequest(args); err != nil {
+	if _, err := c.Delegate.ApplyCARequest(args); err != nil {
 		return err
 	}
 
@@ -317,7 +322,7 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
 	defer c.Unlock()
 
 	// Get the provider state
-	idx, providerState, err := c.getState()
+	_, providerState, err := c.getState()
 	if err != nil {
 		return "", err
 	}
@@ -362,9 +367,14 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
 		return "", fmt.Errorf("error parsing CA cert: %s", err)
 	}
 
+	nextSerial, err := c.incrementAndGetNextSerialNumber()
+	if err != nil {
+		return "", fmt.Errorf("error computing next serial number: %v", err)
+	}
+
 	// Cert template for generation
 	sn := &big.Int{}
-	sn.SetUint64(idx + 1)
+	sn.SetUint64(nextSerial)
 	// Sign the certificate valid from 1 minute in the past, this helps it be
 	// accepted right away even when nodes are not in close time sync across the
 	// cluster. A minute is more than enough for typical DC clock drift.
@@ -407,11 +417,6 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
 		return "", fmt.Errorf("error encoding certificate: %s", err)
 	}
 
-	err = c.incrementProviderIndex(providerState)
-	if err != nil {
-		return "", err
-	}
-
 	// Set the response
 	return buf.String(), nil
 }
@@ -421,7 +426,7 @@ func (c *ConsulProvider) Sign(csr *x509.CertificateRequest) (string, error) {
 // are met. It should return a signed CA certificate with a path length constraint
 // of 0 to ensure that the certificate cannot be used to generate further CA certs.
 func (c *ConsulProvider) SignIntermediate(csr *x509.CertificateRequest) (string, error) {
-	idx, providerState, err := c.getState()
+	_, providerState, err := c.getState()
 	if err != nil {
 		return "", err
 	}
@@ -447,9 +452,14 @@ func (c *ConsulProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
 		return "", fmt.Errorf("error parsing CA cert: %s", err)
 	}
 
+	nextSerial, err := c.incrementAndGetNextSerialNumber()
+	if err != nil {
+		return "", fmt.Errorf("error computing next serial number: %v", err)
+	}
+
 	// Cert template for generation
 	sn := &big.Int{}
-	sn.SetUint64(idx + 1)
+	sn.SetUint64(nextSerial)
 	// Sign the certificate valid from 1 minute in the past, this helps it be
 	// accepted right away even when nodes are not in close time sync across the
 	// cluster. A minute is more than enough for typical DC clock drift.
@@ -485,11 +495,6 @@ func (c *ConsulProvider) SignIntermediate(csr *x509.CertificateRequest) (string,
 		return "", fmt.Errorf("error encoding certificate: %s", err)
 	}
 
-	err = c.incrementProviderIndex(providerState)
-	if err != nil {
-		return "", err
-	}
-
 	// Set the response
 	return buf.String(), nil
 }
@@ -504,7 +509,7 @@ func (c *ConsulProvider) CrossSignCA(cert *x509.Certificate) (string, error) {
 	}
 
 	// Get the provider state
-	idx, providerState, err := c.getState()
+	_, providerState, err := c.getState()
 	if err != nil {
 		return "", err
 	}
@@ -524,9 +529,14 @@ func (c *ConsulProvider) CrossSignCA(cert *x509.Certificate) (string, error) {
 		return "", err
 	}
 
+	nextSerial, err := c.incrementAndGetNextSerialNumber()
+	if err != nil {
+		return "", fmt.Errorf("error computing next serial number: %v", err)
+	}
+
 	// Create the cross-signing template from the existing root CA
 	serialNum := &big.Int{}
-	serialNum.SetUint64(idx + 1)
+	serialNum.SetUint64(nextSerial)
 	template := *cert
 	template.SerialNumber = serialNum
 	template.SignatureAlgorithm = rootCA.SignatureAlgorithm
@@ -555,11 +565,6 @@ func (c *ConsulProvider) CrossSignCA(cert *x509.Certificate) (string, error) {
 		return "", fmt.Errorf("error encoding private key: %s", err)
 	}
 
-	err = c.incrementProviderIndex(providerState)
-	if err != nil {
-		return "", err
-	}
-
 	return buf.String(), nil
 }
 
@@ -584,19 +589,17 @@ func (c *ConsulProvider) getState() (uint64, *structs.CAConsulProviderState, err
 	return idx, providerState, nil
 }
 
-// incrementProviderIndex does a write to increment the provider state store table index
+func (c *ConsulProvider) incrementAndGetNextSerialNumber() (uint64, error) {
-// used for serial numbers when generating certificates.
-func (c *ConsulProvider) incrementProviderIndex(providerState *structs.CAConsulProviderState) error {
-	newState := *providerState
 	args := &structs.CARequest{
-		Op:            structs.CAOpSetProviderState,
+		Op: structs.CAOpIncrementProviderSerialNumber,
-		ProviderState: &newState,
-	}
-	if err := c.Delegate.ApplyCARequest(args); err != nil {
-		return err
 	}
 
-	return nil
+	raw, err := c.Delegate.ApplyCARequest(args)
+	if err != nil {
+		return 0, err
+	}
+
+	return raw.(uint64), nil
 }
 
 // generateCA makes a new root CA using the current private key

agent/connect/ca/provider_consul_test.go

@@ -20,28 +20,30 @@ func (c *consulCAMockDelegate) State() *state.Store {
 	return c.state
 }
 
-func (c *consulCAMockDelegate) ApplyCARequest(req *structs.CARequest) error {
+func (c *consulCAMockDelegate) ApplyCARequest(req *structs.CARequest) (interface{}, error) {
 	idx, _, err := c.state.CAConfig(nil)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	switch req.Op {
 	case structs.CAOpSetProviderState:
 		_, err := c.state.CASetProviderState(idx+1, req.ProviderState)
 		if err != nil {
-			return err
+			return nil, err
 		}
 
-		return nil
+		return true, nil
 	case structs.CAOpDeleteProviderState:
 		if err := c.state.CADeleteProviderState(req.ProviderState.ID); err != nil {
-			return err
+			return nil, err
 		}
 
-		return nil
+		return true, nil
+	case structs.CAOpIncrementProviderSerialNumber:
+		return uint64(2), nil
 	default:
-		return fmt.Errorf("Invalid CA operation '%s'", req.Op)
+		return nil, fmt.Errorf("Invalid CA operation '%s'", req.Op)
 	}
 }
 
@@ -452,7 +454,7 @@ func TestConsulCAProvider_MigrateOldID(t *testing.T) {
 	delegate := newMockDelegate(t, conf)
 
 	// Create an entry with an old-style ID.
-	err := delegate.ApplyCARequest(&structs.CARequest{
+	_, err := delegate.ApplyCARequest(&structs.CARequest{
 		Op: structs.CAOpSetProviderState,
 		ProviderState: &structs.CAConsulProviderState{
 			ID: ",",

agent/consul/consul_ca_delegate.go

@@ -15,14 +15,14 @@ func (c *consulCADelegate) State() *state.Store {
 	return c.srv.fsm.State()
 }
 
-func (c *consulCADelegate) ApplyCARequest(req *structs.CARequest) error {
+func (c *consulCADelegate) ApplyCARequest(req *structs.CARequest) (interface{}, error) {
 	resp, err := c.srv.raftApply(structs.ConnectCARequestType, req)
 	if err != nil {
-		return err
+		return nil, err
 	}
 	if respErr, ok := resp.(error); ok {
-		return respErr
+		return nil, respErr
 	}
 
-	return nil
+	return resp, nil
 }

agent/consul/fsm/commands_oss.go

@@ -357,6 +357,13 @@ func (c *FSM) applyConnectCAOperation(buf []byte, index uint64) interface{} {
 			return err
 		}
 		return act
+	case structs.CAOpIncrementProviderSerialNumber:
+		sn, err := c.state.CAIncrementProviderSerialNumber()
+		if err != nil {
+			return err
+		}
+
+		return sn
 	default:
 		c.logger.Printf("[WARN] consul.fsm: Invalid CA operation '%s'", req.Op)
 		return fmt.Errorf("Invalid CA operation '%s'", req.Op)

agent/consul/state/connect_ca.go

@@ -9,6 +9,7 @@ import (
 
 const (
 	caBuiltinProviderTableName    = "connect-ca-builtin"
+	caBuiltinProviderSerialNumber = "connect-ca-builtin-serial"
 	caConfigTableName             = "connect-ca-config"
 	caRootTableName               = "connect-ca-roots"
 	caLeafIndexName               = "connect-ca-leaf-certs"
@@ -482,3 +483,31 @@ func (s *Store) CARootsAndConfig(ws memdb.WatchSet) (uint64, structs.CARoots, *s
 
 	return idx, roots, config, nil
 }
+
+func (s *Store) CAIncrementProviderSerialNumber() (uint64, error) {
+	tx := s.db.Txn(true)
+	defer tx.Abort()
+
+	existing, err := tx.First("index", "id", caBuiltinProviderSerialNumber)
+	if err != nil {
+		return 0, fmt.Errorf("failed built-in CA serial number lookup: %s", err)
+	}
+
+	var last uint64
+	if existing != nil {
+		last = existing.(*IndexEntry).Value
+	} else {
+		// Serials used to be based on the raft indexes in the provider table,
+		// so bootstrap off of that.
+		last = maxIndexTxn(tx, caBuiltinProviderTableName)
+	}
+	next := last + 1
+
+	if err := tx.Insert("index", &IndexEntry{caBuiltinProviderSerialNumber, next}); err != nil {
+		return 0, fmt.Errorf("failed updating index: %s", err)
+	}
+
+	tx.Commit()
+
+	return next, nil
+}

agent/consul/state/connect_ca_test.go

@@ -424,6 +424,23 @@ func TestStore_CABuiltinProvider(t *testing.T) {
 		assert.Equal(idx, uint64(1))
 		assert.Equal(expected, state)
 	}
+
+	{
+		// Since we've already written to the builtin provider table the serial
+		// numbers will initialize from the max index of the provider table.
+		// That's why this first serial is 2 and not 1.
+		sn, err := s.CAIncrementProviderSerialNumber()
+		assert.NoError(err)
+		assert.Equal(uint64(2), sn)
+
+		sn, err = s.CAIncrementProviderSerialNumber()
+		assert.NoError(err)
+		assert.Equal(uint64(3), sn)
+
+		sn, err = s.CAIncrementProviderSerialNumber()
+		assert.NoError(err)
+		assert.Equal(uint64(4), sn)
+	}
 }
 
 func TestStore_CABuiltinProvider_Snapshot_Restore(t *testing.T) {

agent/structs/connect_ca.go

@@ -177,6 +177,7 @@ const (
 	CAOpSetProviderState              CAOp = "set-provider-state"
 	CAOpDeleteProviderState           CAOp = "delete-provider-state"
 	CAOpSetRootsAndConfig             CAOp = "set-roots-config"
+	CAOpIncrementProviderSerialNumber CAOp = "increment-provider-serial"
 )
 
 // CARequest is used to modify connect CA data. This is used by the