diff --git a/agent/connect/ca/provider_consul.go b/agent/connect/ca/provider_consul.go index c98ed733c..898792386 100644 --- a/agent/connect/ca/provider_consul.go +++ b/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 -// used for serial numbers when generating certificates. -func (c *ConsulProvider) incrementProviderIndex(providerState *structs.CAConsulProviderState) error { - newState := *providerState +func (c *ConsulProvider) incrementAndGetNextSerialNumber() (uint64, error) { args := &structs.CARequest{ - Op: structs.CAOpSetProviderState, - ProviderState: &newState, - } - if err := c.Delegate.ApplyCARequest(args); err != nil { - return err + Op: structs.CAOpIncrementProviderSerialNumber, } - 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 diff --git a/agent/connect/ca/provider_consul_test.go b/agent/connect/ca/provider_consul_test.go index a8af767fe..76e0dd5c2 100644 --- a/agent/connect/ca/provider_consul_test.go +++ b/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: ",", diff --git a/agent/consul/consul_ca_delegate.go b/agent/consul/consul_ca_delegate.go index 5f32a9396..9efe687ff 100644 --- a/agent/consul/consul_ca_delegate.go +++ b/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 } diff --git a/agent/consul/fsm/commands_oss.go b/agent/consul/fsm/commands_oss.go index b58ea204c..4ccfc5083 100644 --- a/agent/consul/fsm/commands_oss.go +++ b/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) diff --git a/agent/consul/state/connect_ca.go b/agent/consul/state/connect_ca.go index 2034d3f50..cd7b38727 100644 --- a/agent/consul/state/connect_ca.go +++ b/agent/consul/state/connect_ca.go @@ -8,10 +8,11 @@ import ( ) const ( - caBuiltinProviderTableName = "connect-ca-builtin" - caConfigTableName = "connect-ca-config" - caRootTableName = "connect-ca-roots" - caLeafIndexName = "connect-ca-leaf-certs" + caBuiltinProviderTableName = "connect-ca-builtin" + caBuiltinProviderSerialNumber = "connect-ca-builtin-serial" + caConfigTableName = "connect-ca-config" + caRootTableName = "connect-ca-roots" + caLeafIndexName = "connect-ca-leaf-certs" ) // caBuiltinProviderTableSchema returns a new table schema used for storing @@ -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 +} diff --git a/agent/consul/state/connect_ca_test.go b/agent/consul/state/connect_ca_test.go index 5a8c25591..e23f3ed25 100644 --- a/agent/consul/state/connect_ca_test.go +++ b/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) { diff --git a/agent/structs/connect_ca.go b/agent/structs/connect_ca.go index 4b52db9db..ca79b59e6 100644 --- a/agent/structs/connect_ca.go +++ b/agent/structs/connect_ca.go @@ -172,11 +172,12 @@ type IssuedCert struct { type CAOp string const ( - CAOpSetRoots CAOp = "set-roots" - CAOpSetConfig CAOp = "set-config" - CAOpSetProviderState CAOp = "set-provider-state" - CAOpDeleteProviderState CAOp = "delete-provider-state" - CAOpSetRootsAndConfig CAOp = "set-roots-config" + CAOpSetRoots CAOp = "set-roots" + CAOpSetConfig CAOp = "set-config" + 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