Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional automatic default issuer selection #17824

Merged
merged 7 commits into from
Nov 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
84 changes: 43 additions & 41 deletions builtin/logical/pki/config_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,57 +46,59 @@ func (sc *storageContext) updateDefaultIssuerId(id issuerID) error {
}

if config.DefaultIssuerId != id {
oldDefault := config.DefaultIssuerId
newDefault := id
now := time.Now().UTC()
config.DefaultIssuerId = id
return sc.setIssuersConfig(config)
}

err := sc.setIssuersConfig(&issuerConfigEntry{
DefaultIssuerId: newDefault,
})
if err != nil {
return err
}
return nil
}

func (sc *storageContext) changeDefaultIssuerTimestamps(oldDefault issuerID, newDefault issuerID) error {
if newDefault == oldDefault {
return nil
}

// When the default issuer changes, we need to modify four
// pieces of information:
//
// 1. The old default issuer's modification time, as it no
// longer works for the /cert/ca path.
// 2. The new default issuer's modification time, as it now
// works for the /cert/ca path.
// 3. & 4. Both issuer's CRLs, as they behave the same, under
// the /cert/crl path!
for _, thisId := range []issuerID{oldDefault, newDefault} {
if len(thisId) == 0 {
continue
}

// 1 & 2 above.
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)
}

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)
}
now := time.Now().UTC()

// When the default issuer changes, we need to modify four
// pieces of information:
//
// 1. The old default issuer's modification time, as it no
// longer works for the /cert/ca path.
// 2. The new default issuer's modification time, as it now
// works for the /cert/ca path.
// 3. & 4. Both issuer's CRLs, as they behave the same, under
// the /cert/crl path!
for _, thisId := range []issuerID{oldDefault, newDefault} {
if len(thisId) == 0 {
continue
}

// Fetch and update the localCRLConfigEntry (3&4).
cfg, err := sc.getLocalCRLConfig()
// 1 & 2 above.
issuer, err := sc.fetchIssuerById(thisId)
if err != nil {
return fmt.Errorf("unable to update local CRL config's modification time: error fetching local CRL config: %v", err)
return fmt.Errorf("unable to update issuer (%v)'s modification time: error fetching issuer: %v", thisId, err)
kitography marked this conversation as resolved.
Show resolved Hide resolved
}

cfg.LastModified = now
cfg.DeltaLastModified = now
err = sc.setLocalCRLConfig(cfg)
issuer.LastModified = now
err = sc.writeIssuer(issuer)
if err != nil {
return fmt.Errorf("unable to update local CRL config's modification time: error persisting local CRL config: %v", err)
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)
}

cfg.LastModified = now
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
}
103 changes: 103 additions & 0 deletions builtin/logical/pki/integation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, "")
}
Expand Down
49 changes: 37 additions & 12 deletions builtin/logical/pki/path_config_ca.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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.`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is meant to allow opt-in to unbreak a change, should the original change be documented as breaking, and point to this new field as a way to opting into the previous behavior if necessary, while suggesting that callers move to using the new multi-issuer functionality more explicitly as they can? (just a thought)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, its in the web docs that I've not pushed, one second.

Default: false,
},
},

Operations: map[logical.Operation]framework.OperationHandler{
Expand Down Expand Up @@ -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) {
Expand All @@ -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()}
}
entry, err := sc.fetchIssuerById(parsedIssuer)
if err != nil {
return logical.ErrorResponse("Unable to fetch issuer: " + err.Error()), nil
}

response := &logical.Response{
Data: map[string]interface{}{
"default": parsedIssuer,
},
// 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)
}

entry, err := sc.fetchIssuerById(parsedIssuer)
// Update the config
config, err := sc.getIssuersConfig()
if err != nil {
return logical.ErrorResponse("Unable to fetch issuer: " + err.Error()), 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 != nil {
if err := sc.setIssuersConfig(config); err != nil {
return logical.ErrorResponse("Error updating issuer configuration: " + err.Error()), nil
}

Expand Down
21 changes: 21 additions & 0 deletions builtin/logical/pki/path_manage_issuers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions builtin/logical/pki/path_root.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down