Optional automatic default issuer selection (#17824)

* Correctly preserve other issuer config params

When setting a new default issuer, our helper function would overwrite
other parameters in the issuer configuration entry. However, up until
now, there were none.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add new parameter to allow default to follow new

This parameter will allow operators to have the default issuer
automatically update when a new root is generated or a single issuer
with a key (potentially with others lacking key) is imported.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Storage migration tests fail on new members

These internal members shouldn't be tested by the storage migration
code, and so should be elided from the test results.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Follow new issuer on root generation, import

This updates the two places where issuers can be created (outside of
legacy CA bundle migration which already sets the default) to follow
newly created issuers when the config is set.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add changelog entry

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add documentation

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

* Add test for new default-following behavior

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>

builtin/logical/pki/config_util.go

@@ -46,56 +46,58 @@ func (sc *storageContext) updateDefaultIssuerId(id issuerID) error {
 	}
 
 	if config.DefaultIssuerId != id {
-		oldDefault := config.DefaultIssuerId
+		config.DefaultIssuerId = id
-		newDefault := id
+		return sc.setIssuersConfig(config)
-		now := time.Now().UTC()
+	}
-
+
-		err := sc.setIssuersConfig(&issuerConfigEntry{
+	return nil
-			DefaultIssuerId: newDefault,
+}
-		})
+
-		if err != nil {
+func (sc *storageContext) changeDefaultIssuerTimestamps(oldDefault issuerID, newDefault issuerID) error {
-			return err
+	if newDefault == oldDefault {
-		}
+		return nil
-
+	}
-		// When the default issuer changes, we need to modify four
+
-		// pieces of information:
+	now := time.Now().UTC()
-		//
+
-		// 1. The old default issuer's modification time, as it no
+	// When the default issuer changes, we need to modify four
-		//    longer works for the /cert/ca path.
+	// pieces of information:
-		// 2. The new default issuer's modification time, as it now
+	//
-		//    works for the /cert/ca path.
+	// 1. The old default issuer's modification time, as it no
-		// 3. & 4. Both issuer's CRLs, as they behave the same, under
+	//    longer works for the /cert/ca path.
-		//    the /cert/crl path!
+	// 2. The new default issuer's modification time, as it now
-		for _, thisId := range []issuerID{oldDefault, newDefault} {
+	//    works for the /cert/ca path.
-			if len(thisId) == 0 {
+	// 3. & 4. Both issuer's CRLs, as they behave the same, under
-				continue
+	//    the /cert/crl path!
-			}
+	for _, thisId := range []issuerID{oldDefault, newDefault} {
-
+		if len(thisId) == 0 {
-			// 1 & 2 above.
+			continue
-			issuer, err := sc.fetchIssuerById(thisId)
+		}
-			if err != nil {
+
-				return fmt.Errorf("unable to update issuer (%v)'s modification time: error fetching issuer: %v", thisId, err)
+		// 1 & 2 above.
-			}
+		issuer, err := sc.fetchIssuerById(thisId)
-
+		if err != nil {
-			issuer.LastModified = now
+			return fmt.Errorf("unable to update issuer (%v)'s modification time: error fetching issuer: %v", thisId, err)
-			err = sc.writeIssuer(issuer)
+		}
-			if err != nil {
+
-				return fmt.Errorf("unable to update issuer (%v)'s modification time: error persisting issuer: %v", thisId, err)
+		issuer.LastModified = now
-			}
+		err = sc.writeIssuer(issuer)
-		}
+		if err != nil {
-
+			return fmt.Errorf("unable to update issuer (%v)'s modification time: error persisting issuer: %v", thisId, err)
-		// Fetch and update the localCRLConfigEntry (3&4).
+		}
-		cfg, err := sc.getLocalCRLConfig()
+	}
-		if err != nil {
+
-			return fmt.Errorf("unable to update local CRL config's modification time: error fetching local CRL config: %v", err)
+	// Fetch and update the localCRLConfigEntry (3&4).
-		}
+	cfg, err := sc.getLocalCRLConfig()
-
+	if err != nil {
-		cfg.LastModified = now
+		return fmt.Errorf("unable to update local CRL config's modification time: error fetching local CRL config: %v", err)
-		cfg.DeltaLastModified = now
+	}
-		err = sc.setLocalCRLConfig(cfg)
+
-		if err != nil {
+	cfg.LastModified = now
-			return fmt.Errorf("unable to update local CRL config's modification time: error persisting local CRL config: %v", err)
+	cfg.DeltaLastModified = now
-		}
+	err = sc.setLocalCRLConfig(cfg)
+	if err != nil {
+		return fmt.Errorf("unable to update local CRL config's modification time: error persisting local CRL config: %v", err)
 	}
 
 	return nil

builtin/logical/pki/integation_test.go

@@ -357,6 +357,109 @@ func TestIntegration_CSRGeneration(t *testing.T) {
 	}
 }
 
+func TestIntegration_AutoIssuer(t *testing.T) {
+	t.Parallel()
+	b, s := createBackendWithStorage(t)
+
+	// Generate two roots. The first should become default under the existing
+	// behavior; when we update the config and generate a second, it should
+	// take over as default. Deleting the first and re-importing it will make
+	// it default again, and then disabling the option and removing and
+	// reimporting the second and creating a new root won't affect it again.
+	resp, err := CBWrite(b, s, "root/generate/internal", map[string]interface{}{
+		"common_name": "Root X1",
+		"issuer_name": "root-1",
+		"key_type":    "ec",
+	})
+	requireSuccessNonNilResponse(t, resp, err)
+	issuerIdOne := resp.Data["issuer_id"]
+	require.NotEmpty(t, issuerIdOne)
+	certOne := resp.Data["certificate"]
+	require.NotEmpty(t, certOne)
+
+	resp, err = CBRead(b, s, "config/issuers")
+	requireSuccessNonNilResponse(t, resp, err)
+	require.Equal(t, issuerIdOne, resp.Data["default"])
+
+	// Enable the new config option.
+	_, err = CBWrite(b, s, "config/issuers", map[string]interface{}{
+		"default":                       issuerIdOne,
+		"default_follows_latest_issuer": true,
+	})
+	require.NoError(t, err)
+
+	// Now generate the second root; it should become default.
+	resp, err = CBWrite(b, s, "root/generate/internal", map[string]interface{}{
+		"common_name": "Root X2",
+		"issuer_name": "root-2",
+		"key_type":    "ec",
+	})
+	requireSuccessNonNilResponse(t, resp, err)
+	issuerIdTwo := resp.Data["issuer_id"]
+	require.NotEmpty(t, issuerIdTwo)
+	certTwo := resp.Data["certificate"]
+	require.NotEmpty(t, certTwo)
+
+	resp, err = CBRead(b, s, "config/issuers")
+	requireSuccessNonNilResponse(t, resp, err)
+	require.Equal(t, issuerIdTwo, resp.Data["default"])
+
+	// Deleting the first shouldn't affect the default issuer.
+	_, err = CBDelete(b, s, "issuer/root-1")
+	require.NoError(t, err)
+	resp, err = CBRead(b, s, "config/issuers")
+	requireSuccessNonNilResponse(t, resp, err)
+	require.Equal(t, issuerIdTwo, resp.Data["default"])
+
+	// But reimporting it should update it to the new issuer's value.
+	resp, err = CBWrite(b, s, "issuers/import/bundle", map[string]interface{}{
+		"pem_bundle": certOne,
+	})
+	requireSuccessNonNilResponse(t, resp, err)
+	issuerIdOneReimported := issuerID(resp.Data["imported_issuers"].([]string)[0])
+
+	resp, err = CBRead(b, s, "config/issuers")
+	requireSuccessNonNilResponse(t, resp, err)
+	require.Equal(t, issuerIdOneReimported, resp.Data["default"])
+
+	// Now update the config to disable this option again.
+	_, err = CBWrite(b, s, "config/issuers", map[string]interface{}{
+		"default":                       issuerIdOneReimported,
+		"default_follows_latest_issuer": false,
+	})
+	require.NoError(t, err)
+
+	// Generating a new root shouldn't update the default.
+	resp, err = CBWrite(b, s, "root/generate/internal", map[string]interface{}{
+		"common_name": "Root X3",
+		"issuer_name": "root-3",
+		"key_type":    "ec",
+	})
+	requireSuccessNonNilResponse(t, resp, err)
+	issuerIdThree := resp.Data["issuer_id"]
+	require.NotEmpty(t, issuerIdThree)
+
+	resp, err = CBRead(b, s, "config/issuers")
+	requireSuccessNonNilResponse(t, resp, err)
+	require.Equal(t, issuerIdOneReimported, resp.Data["default"])
+
+	// Deleting and re-importing root 2 should also not affect it.
+	_, err = CBDelete(b, s, "issuer/root-2")
+	require.NoError(t, err)
+	resp, err = CBRead(b, s, "config/issuers")
+	requireSuccessNonNilResponse(t, resp, err)
+	require.Equal(t, issuerIdOneReimported, resp.Data["default"])
+
+	resp, err = CBWrite(b, s, "issuers/import/bundle", map[string]interface{}{
+		"pem_bundle": certTwo,
+	})
+	requireSuccessNonNilResponse(t, resp, err)
+	require.Equal(t, 1, len(resp.Data["imported_issuers"].([]string)))
+	resp, err = CBRead(b, s, "config/issuers")
+	requireSuccessNonNilResponse(t, resp, err)
+	require.Equal(t, issuerIdOneReimported, resp.Data["default"])
+}
+
 func genTestRootCa(t *testing.T, b *backend, s logical.Storage) (issuerID, keyID) {
 	return genTestRootCaWithIssuerName(t, b, s, "")
 }

builtin/logical/pki/path_config_ca.go

@@ -4,6 +4,7 @@ import (
 	"context"
 
 	"github.com/hashicorp/vault/sdk/framework"
+	"github.com/hashicorp/vault/sdk/helper/errutil"
 	"github.com/hashicorp/vault/sdk/logical"
 )
 
@@ -52,6 +53,11 @@ func pathConfigIssuers(b *backend) *framework.Path {
 				Type:        framework.TypeString,
 				Description: `Reference (name or identifier) to the default issuer.`,
 			},
+			"default_follows_latest_issuer": {
+				Type:        framework.TypeBool,
+				Description: `Whether the default issuer should automatically follow the latest generated or imported issuer. Defaults to false.`,
+				Default:     false,
+			},
 		},
 
 		Operations: map[logical.Operation]framework.OperationHandler{
@@ -107,11 +113,16 @@ func (b *backend) pathCAIssuersRead(ctx context.Context, req *logical.Request, _
 		return logical.ErrorResponse("Error loading issuers configuration: " + err.Error()), nil
 	}
 
+	return b.formatCAIssuerConfigRead(config), nil
+}
+
+func (b *backend) formatCAIssuerConfigRead(config *issuerConfigEntry) *logical.Response {
 	return &logical.Response{
 		Data: map[string]interface{}{
-			defaultRef: config.DefaultIssuerId,
+			defaultRef:                      config.DefaultIssuerId,
+			"default_follows_latest_issuer": config.DefaultFollowsLatestIssuer,
 		},
-	}, nil
+	}
 }
 
 func (b *backend) pathCAIssuersWrite(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
@@ -124,36 +135,50 @@ func (b *backend) pathCAIssuersWrite(ctx context.Context, req *logical.Request,
 		return logical.ErrorResponse("Cannot update defaults until migration has completed"), nil
 	}
 
+	sc := b.makeStorageContext(ctx, req.Storage)
+
+	// Validate the new default reference.
 	newDefault := data.Get(defaultRef).(string)
 	if len(newDefault) == 0 || newDefault == defaultRef {
 		return logical.ErrorResponse("Invalid issuer specification; must be non-empty and can't be 'default'."), nil
 	}
-
-	sc := b.makeStorageContext(ctx, req.Storage)
 	parsedIssuer, err := sc.resolveIssuerReference(newDefault)
 	if err != nil {
 		return logical.ErrorResponse("Error resolving issuer reference: " + err.Error()), nil
+		return nil, errutil.UserError{Err: "Error resolving issuer reference: " + err.Error()}
 	}
-
-	response := &logical.Response{
-		Data: map[string]interface{}{
-			"default": parsedIssuer,
-		},
-	}
-
 	entry, err := sc.fetchIssuerById(parsedIssuer)
 	if err != nil {
 		return logical.ErrorResponse("Unable to fetch issuer: " + err.Error()), nil
 	}
 
+	// Get the other new parameters. This doesn't exist on the /root/replace
+	// variant of this call.
+	var followIssuer bool
+	followIssuersRaw, followOk := data.GetOk("default_follows_latest_issuer")
+	if followOk {
+		followIssuer = followIssuersRaw.(bool)
+	}
+
+	// Update the config
+	config, err := sc.getIssuersConfig()
+	if err != nil {
+		return logical.ErrorResponse("Unable to fetch existing issuers configuration: " + err.Error()), nil
+	}
+	config.DefaultIssuerId = parsedIssuer
+	if followOk {
+		config.DefaultFollowsLatestIssuer = followIssuer
+	}
+
+	// Add our warning if necessary.
+	response := b.formatCAIssuerConfigRead(config)
 	if len(entry.KeyID) == 0 {
 		msg := "This selected default issuer has no key associated with it. Some operations like issuing certificates and signing CRLs will be unavailable with the requested default issuer until a key is imported or the default issuer is changed."
 		response.AddWarning(msg)
 		b.Logger().Error(msg)
 	}
 
-	err = sc.updateDefaultIssuerId(parsedIssuer)
+	if err := sc.setIssuersConfig(config); err != nil {
-	if err != nil {
 		return logical.ErrorResponse("Error updating issuer configuration: " + err.Error()), nil
 	}
 

builtin/logical/pki/path_manage_issuers.go

@@ -258,6 +258,27 @@ func (b *backend) pathImportIssuers(ctx context.Context, req *logical.Request, d
 
 			return nil, err
 		}
+
+		var issuersWithKeys []string
+		for _, issuer := range createdIssuers {
+			if issuerKeyMap[issuer] != "" {
+				issuersWithKeys = append(issuersWithKeys, issuer)
+			}
+		}
+
+		// Check whether we need to update our default issuer configuration.
+		config, err := sc.getIssuersConfig()
+		if err != nil {
+			response.AddWarning("Unable to fetch default issuers configuration to update default issuer if necessary: " + err.Error())
+		} else if config.DefaultFollowsLatestIssuer {
+			if len(issuersWithKeys) == 1 {
+				if err := sc.updateDefaultIssuerId(issuerID(issuersWithKeys[0])); err != nil {
+					response.AddWarning("Unable to update this new root as the default issuer: " + err.Error())
+				}
+			} else if len(issuersWithKeys) > 1 {
+				response.AddWarning("Default issuer left unchanged: could not select new issuer automatically as multiple imported issuers had key material in Vault.")
+			}
+		}
 	}
 
 	// While we're here, check if we should warn about a bad default key. We

builtin/logical/pki/path_root.go

@@ -279,6 +279,16 @@ func (b *backend) pathCAGenerateRoot(ctx context.Context, req *logical.Request,
 		resp.AddWarning("Max path length of the generated certificate is zero. This certificate cannot be used to issue intermediate CA certificates.")
 	}
 
+	// Check whether we need to update our default issuer configuration.
+	config, err := sc.getIssuersConfig()
+	if err != nil {
+		resp.AddWarning("Unable to fetch default issuers configuration to update default issuer if necessary: " + err.Error())
+	} else if config.DefaultFollowsLatestIssuer {
+		if err := sc.updateDefaultIssuerId(myIssuer.ID); err != nil {
+			resp.AddWarning("Unable to update this new root as the default issuer: " + err.Error())
+		}
+	}
+
 	resp = addWarnings(resp, warnings)
 
 	return resp, nil

builtin/logical/pki/storage.go

@@ -187,7 +187,12 @@ type keyConfigEntry struct {
 }
 
 type issuerConfigEntry struct {
-	DefaultIssuerId issuerID `json:"default"`
+	// This new fetchedDefault field allows us to detect if the default
+	// issuer was modified, in turn dispatching the timestamp updater
+	// if necessary.
+	fetchedDefault             issuerID `json:"-"`
+	DefaultIssuerId            issuerID `json:"default"`
+	DefaultFollowsLatestIssuer bool     `json:"default_follows_latest_issuer"`
 }
 
 type storageContext struct {
@@ -658,6 +663,9 @@ func (sc *storageContext) deleteIssuer(id issuerID) (bool, error) {
 	wasDefault := false
 	if config.DefaultIssuerId == id {
 		wasDefault = true
+		// Overwrite the fetched default issuer as we're going to remove this
+		// entry.
+		config.fetchedDefault = issuerID("")
 		config.DefaultIssuerId = issuerID("")
 		if err := sc.setIssuersConfig(config); err != nil {
 			return wasDefault, err
@@ -912,7 +920,15 @@ func (sc *storageContext) setIssuersConfig(config *issuerConfigEntry) error {
 		return err
 	}
 
-	return sc.Storage.Put(sc.Context, json)
+	if err := sc.Storage.Put(sc.Context, json); err != nil {
+		return err
+	}
+
+	if err := sc.changeDefaultIssuerTimestamps(config.fetchedDefault, config.DefaultIssuerId); err != nil {
+		return err
+	}
+
+	return nil
 }
 
 func (sc *storageContext) getIssuersConfig() (*issuerConfigEntry, error) {
@@ -927,6 +943,7 @@ func (sc *storageContext) getIssuersConfig() (*issuerConfigEntry, error) {
 			return nil, errutil.InternalError{Err: fmt.Sprintf("unable to decode issuer configuration: %v", err)}
 		}
 	}
+	issuerConfig.fetchedDefault = issuerConfig.DefaultIssuerId
 
 	return issuerConfig, nil
 }

builtin/logical/pki/storage_migrations_test.go

@@ -127,7 +127,7 @@ func Test_migrateStorageOnlyKey(t *testing.T) {
 
 	issuersConfig, err := sc.getIssuersConfig()
 	require.NoError(t, err)
-	require.Equal(t, &issuerConfigEntry{}, issuersConfig)
+	require.Equal(t, issuerID(""), issuersConfig.DefaultIssuerId)
 
 	// Make sure if we attempt to re-run the migration nothing happens...
 	err = migrateStorage(ctx, b, s)
@@ -220,7 +220,7 @@ func Test_migrateStorageSimpleBundle(t *testing.T) {
 
 	issuersConfig, err := sc.getIssuersConfig()
 	require.NoError(t, err)
-	require.Equal(t, &issuerConfigEntry{DefaultIssuerId: issuerId}, issuersConfig)
+	require.Equal(t, issuerId, issuersConfig.DefaultIssuerId)
 
 	// Make sure if we attempt to re-run the migration nothing happens...
 	err = migrateStorage(ctx, b, s)

builtin/logical/pki/storage_test.go

@@ -18,6 +18,14 @@ func Test_ConfigsRoundTrip(t *testing.T) {
 	b, s := createBackendWithStorage(t)
 	sc := b.makeStorageContext(ctx, s)
 
+	// Create an empty key, issuer for testing.
+	key := keyEntry{ID: genKeyId()}
+	err := sc.writeKey(key)
+	require.NoError(t, err)
+	issuer := &issuerEntry{ID: genIssuerId()}
+	err = sc.writeIssuer(issuer)
+	require.NoError(t, err)
+
 	// Verify we handle nothing stored properly
 	keyConfigEmpty, err := sc.getKeysConfig()
 	require.NoError(t, err)
@@ -29,10 +37,10 @@ func Test_ConfigsRoundTrip(t *testing.T) {
 
 	// Now attempt to store and reload properly
 	origKeyConfig := &keyConfigEntry{
-		DefaultKeyId: genKeyId(),
+		DefaultKeyId: key.ID,
 	}
 	origIssuerConfig := &issuerConfigEntry{
-		DefaultIssuerId: genIssuerId(),
+		DefaultIssuerId: issuer.ID,
 	}
 
 	err = sc.setKeysConfig(origKeyConfig)
@@ -46,7 +54,7 @@ func Test_ConfigsRoundTrip(t *testing.T) {
 
 	issuerConfig, err := sc.getIssuersConfig()
 	require.NoError(t, err)
-	require.Equal(t, origIssuerConfig, issuerConfig)
+	require.Equal(t, origIssuerConfig.DefaultIssuerId, issuerConfig.DefaultIssuerId)
 }
 
 func Test_IssuerRoundTrip(t *testing.T) {

changelog/17824.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+secrets/pki: Allow issuer creation, import to change default issuer via `default_follows_latest_issuer`.
+```

website/content/api-docs/secret/pki.mdx

@@ -2962,7 +2962,8 @@ $ curl \
 ```json
 {
   "data": {
-    "default": "3dc79a5a-7a6c-70e2-1123-94b88557ba12"
+    "default": "3dc79a5a-7a6c-70e2-1123-94b88557ba12",
+    "default_follows_latest_issuer": "false"
   }
 }
 ```
@@ -2982,6 +2983,27 @@ This endpoint allows setting the value of the default issuer.
   either a name or an ID). When no value is specified and the path is
   `/pki/root/replace`, the default value of `"next"` will be used.
 
+- `default_follows_latest_issuer` `(bool: false)` - Specifies whether a
+  root creation or an issuer import operation updates the default issuer
+  to the newly added issuer.
+
+  While the new multi-issuer functionality of 1.11 was backwards compatible
+  on a per-API basis, some applications relied explicitly on unsafe behavior
+  across multiple APIs that we addressed. For instance, calling
+  `/intermediate/generate/:type` would silently remove any (potentially
+  in-use!) key material and generate new private keys. While our response to
+  this endpoint is backwards compatible (returning a new key and safely
+  preserving old keys), some applications implicitly relied on this behavior.
+  This new option is meant to provide compatibility across API calls to these
+  callers: the newly created issuer (once _imported_ -- not on intermediate
+  generation) will become the default and it will look (to anyone strictly
+  using old APIs) that it is the only issuer in the mount. However, it is
+  encouraged for applications to update to the newer, safer semantics
+  associated with [multi-issuer rotation](/docs/secrets/pki/rotation-primitives).
+
+~> Note: When an import creates more than one new issuer with key material
+   known to this mount, no default update will occur.
+
 #### Sample Payload
 
 ```json