Skip to content

Commit

Permalink
Clean up various warnings within the PKI package (#15230)
Browse files Browse the repository at this point in the history
  • Loading branch information
stevendpclark authored Apr 29, 2022
1 parent b6353a9 commit 5ad5384
Show file tree
Hide file tree
Showing 14 changed files with 48 additions and 94 deletions.
12 changes: 1 addition & 11 deletions builtin/logical/pki/cert_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,19 +64,9 @@ var (
leftWildLabelRegex = regexp.MustCompile(`^(` + allWildRegex + `|` + startWildRegex + `|` + endWildRegex + `|` + middleWildRegex + `)$`)

// OIDs for X.509 certificate extensions used below.
oidExtensionBasicConstraints = []int{2, 5, 29, 19}
oidExtensionSubjectAltName = []int{2, 5, 29, 17}
oidExtensionSubjectAltName = []int{2, 5, 29, 17}
)

func oidInExtensions(oid asn1.ObjectIdentifier, extensions []pkix.Extension) bool {
for _, e := range extensions {
if e.Id.Equal(oid) {
return true
}
}
return false
}

func getFormat(data *framework.FieldData) string {
format := data.Get("format").(string)
switch format {
Expand Down
38 changes: 2 additions & 36 deletions builtin/logical/pki/path_config_ca.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func pathReplaceRoot(b *backend) *framework.Path {
}
}

func (b *backend) pathCAIssuersRead(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathCAIssuersRead(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
config, err := getIssuersConfig(ctx, req.Storage)
if err != nil {
return logical.ErrorResponse("Error loading issuers configuration: " + err.Error()), nil
Expand Down Expand Up @@ -186,7 +186,7 @@ func pathConfigKeys(b *backend) *framework.Path {
}
}

func (b *backend) pathKeyDefaultRead(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathKeyDefaultRead(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
config, err := getKeysConfig(ctx, req.Storage)
if err != nil {
return logical.ErrorResponse("Error loading keys configuration: " + err.Error()), nil
Expand Down Expand Up @@ -229,37 +229,3 @@ This path allows configuration of key parameters.
The "default" parameter controls which key is the default used by signing paths.
`

const pathConfigCAGenerateHelpSyn = `
Generate a new CA certificate and private key used for signing.
`

const pathConfigCAGenerateHelpDesc = `
This path generates a CA certificate and private key to be used for
credentials generated by this mount. The path can either
end in "internal" or "exported"; this controls whether the
unencrypted private key is exported after generation. This will
be your only chance to export the private key; for security reasons
it cannot be read or exported later.
If the "type" option is set to "self-signed", the generated
certificate will be a self-signed root CA. Otherwise, this mount
will act as an intermediate CA; a CSR will be returned, to be signed
by your chosen CA (which could be another mount of this backend).
Note that the CRL path will be set to this mount's CRL path; if you
need further customization it is recommended that you create a CSR
separately and get it signed. Either way, use the "config/ca/set"
endpoint to load the signed certificate into Vault.
`

const pathConfigCASignHelpSyn = `
Generate a signed CA certificate from a CSR.
`

const pathConfigCASignHelpDesc = `
This path generates a CA certificate to be used for credentials
generated by the certificate's destination mount.
Use the "config/ca/set" endpoint to load the signed certificate
into Vault another Vault mount.
`
2 changes: 1 addition & 1 deletion builtin/logical/pki/path_config_crl.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (b *backend) CRL(ctx context.Context, s logical.Storage) (*crlConfig, error
return &result, nil
}

func (b *backend) pathCRLRead(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathCRLRead(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
config, err := b.CRL(ctx, req.Storage)
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/pki/path_config_urls.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func writeURLs(ctx context.Context, req *logical.Request, entries *certutil.URLE
return nil
}

func (b *backend) pathReadURL(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathReadURL(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
entries, err := getURLs(ctx, req)
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/pki/path_fetch.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func pathFetchListCerts(b *backend) *framework.Path {
}
}

func (b *backend) pathFetchCertList(ctx context.Context, req *logical.Request, data *framework.FieldData) (response *logical.Response, retErr error) {
func (b *backend) pathFetchCertList(ctx context.Context, req *logical.Request, _ *framework.FieldData) (response *logical.Response, retErr error) {
entries, err := req.Storage.List(ctx, "certs/")
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/pki/path_fetch_keys.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const (
their identifier and their name (if set).`
)

func (b *backend) pathListKeysHandler(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathListKeysHandler(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
var responseKeys []string
responseInfo := make(map[string]interface{})

Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/pki/path_revoke.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func (b *backend) pathRevokeWrite(ctx context.Context, req *logical.Request, dat
return revokeCert(ctx, b, req, serial, false)
}

func (b *backend) pathRotateCRLRead(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathRotateCRLRead(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
b.revokeStorageLock.RLock()
defer b.revokeStorageLock.RUnlock()

Expand Down
10 changes: 5 additions & 5 deletions builtin/logical/pki/path_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ func (b *backend) pathRoleRead(ctx context.Context, req *logical.Request, data *
return resp, nil
}

func (b *backend) pathRoleList(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathRoleList(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
entries, err := req.Storage.List(ctx, "role/")
if err != nil {
return nil, err
Expand Down Expand Up @@ -710,16 +710,16 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data
}
}

allow_wildcard_certificates, present := data.GetOk("allow_wildcard_certificates")
allowWildcardCertificates, present := data.GetOk("allow_wildcard_certificates")
if !present {
// While not the most secure default, when AllowWildcardCertificates isn't
// explicitly specified in the request, we automatically set it to true to
// preserve compatibility with previous versions of Vault.
allow_wildcard_certificates = true
allowWildcardCertificates = true
}
*entry.AllowWildcardCertificates = allow_wildcard_certificates.(bool)
*entry.AllowWildcardCertificates = allowWildcardCertificates.(bool)

// Ensure issuers ref is set to to a non-empty value. Note that we never
// Ensure issuers ref is set to a non-empty value. Note that we never
// resolve the reference (to an issuerId) at role creation time; instead,
// resolve it at use time. This allows values such as `default` or other
// user-assigned names to "float" and change over time.
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/pki/path_root.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func pathDeleteRoot(b *backend) *framework.Path {
return ret
}

func (b *backend) pathCADeleteRoot(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathCADeleteRoot(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
issuers, err := listIssuers(ctx, req.Storage)
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/pki/path_tidy.go
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ func (b *backend) pathTidyWrite(ctx context.Context, req *logical.Request, d *fr
return logical.RespondWithStatusCode(resp, req, http.StatusAccepted)
}

func (b *backend) pathTidyStatusRead(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
func (b *backend) pathTidyStatusRead(_ context.Context, _ *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
// If this node is a performance secondary return an ErrReadOnly so that the request gets forwarded,
// but only if the PKI backend is not a local mount.
if b.System().ReplicationState().HasState(consts.ReplicationPerformanceSecondary) && !b.System().LocalMount() {
Expand Down
2 changes: 1 addition & 1 deletion builtin/logical/pki/secret_certs.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ reference`,
}
}

func (b *backend) secretCredsRevoke(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
func (b *backend) secretCredsRevoke(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {
if req.Secret == nil {
return nil, fmt.Errorf("secret is nil in request")
}
Expand Down
10 changes: 4 additions & 6 deletions builtin/logical/pki/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,12 @@ func writeKey(ctx context.Context, s logical.Storage, key keyEntry) error {
}

func deleteKey(ctx context.Context, s logical.Storage, id keyID) (bool, error) {
wasDefault := false

config, err := getKeysConfig(ctx, s)
if err != nil {
return wasDefault, err
return false, err
}

wasDefault := false
if config.DefaultKeyId == id {
wasDefault = true
config.DefaultKeyId = keyID("")
Expand Down Expand Up @@ -368,13 +367,12 @@ func writeIssuer(ctx context.Context, s logical.Storage, issuer *issuerEntry) er
}

func deleteIssuer(ctx context.Context, s logical.Storage, id issuerID) (bool, error) {
wasDefault := false

config, err := getIssuersConfig(ctx, s)
if err != nil {
return wasDefault, err
return false, err
}

wasDefault := false
if config.DefaultIssuerId == id {
wasDefault = true
config.DefaultIssuerId = issuerID("")
Expand Down
52 changes: 26 additions & 26 deletions builtin/logical/pki/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,36 +103,36 @@ func Test_KeysIssuerImport(t *testing.T) {
issuer1.ID = ""
issuer1.KeyID = ""

key1_ref1, existing, err := importKey(ctx, s, key1.PrivateKey, "key1")
key1Ref1, existing, err := importKey(ctx, s, key1.PrivateKey, "key1")
require.NoError(t, err)
require.False(t, existing)
require.Equal(t, strings.TrimSpace(key1.PrivateKey), strings.TrimSpace(key1_ref1.PrivateKey))
require.Equal(t, strings.TrimSpace(key1.PrivateKey), strings.TrimSpace(key1Ref1.PrivateKey))

// Make sure if we attempt to re-import the same private key, no import/updates occur.
// So the existing flag should be set to true and we do not update the existing Name field.
key1_ref2, existing, err := importKey(ctx, s, key1.PrivateKey, "ignore-me")
// So the existing flag should be set to true, and we do not update the existing Name field.
key1Ref2, existing, err := importKey(ctx, s, key1.PrivateKey, "ignore-me")
require.NoError(t, err)
require.True(t, existing)
require.Equal(t, key1.PrivateKey, key1_ref1.PrivateKey)
require.Equal(t, key1_ref1.ID, key1_ref2.ID)
require.Equal(t, key1_ref1.Name, key1_ref2.Name)
require.Equal(t, key1.PrivateKey, key1Ref1.PrivateKey)
require.Equal(t, key1Ref1.ID, key1Ref2.ID)
require.Equal(t, key1Ref1.Name, key1Ref2.Name)

issuer1_ref1, existing, err := importIssuer(ctx, s, issuer1.Certificate, "issuer1")
issuer1Ref1, existing, err := importIssuer(ctx, s, issuer1.Certificate, "issuer1")
require.NoError(t, err)
require.False(t, existing)
require.Equal(t, strings.TrimSpace(issuer1.Certificate), strings.TrimSpace(issuer1_ref1.Certificate))
require.Equal(t, key1_ref1.ID, issuer1_ref1.KeyID)
require.Equal(t, "issuer1", issuer1_ref1.Name)
require.Equal(t, strings.TrimSpace(issuer1.Certificate), strings.TrimSpace(issuer1Ref1.Certificate))
require.Equal(t, key1Ref1.ID, issuer1Ref1.KeyID)
require.Equal(t, "issuer1", issuer1Ref1.Name)

// Make sure if we attempt to re-import the same issuer, no import/updates occur.
// So the existing flag should be set to true and we do not update the existing Name field.
issuer1_ref2, existing, err := importIssuer(ctx, s, issuer1.Certificate, "ignore-me")
// So the existing flag should be set to true, and we do not update the existing Name field.
issuer1Ref2, existing, err := importIssuer(ctx, s, issuer1.Certificate, "ignore-me")
require.NoError(t, err)
require.True(t, existing)
require.Equal(t, strings.TrimSpace(issuer1.Certificate), strings.TrimSpace(issuer1_ref1.Certificate))
require.Equal(t, issuer1_ref1.ID, issuer1_ref2.ID)
require.Equal(t, key1_ref1.ID, issuer1_ref2.KeyID)
require.Equal(t, issuer1_ref1.Name, issuer1_ref2.Name)
require.Equal(t, strings.TrimSpace(issuer1.Certificate), strings.TrimSpace(issuer1Ref1.Certificate))
require.Equal(t, issuer1Ref1.ID, issuer1Ref2.ID)
require.Equal(t, key1Ref1.ID, issuer1Ref2.KeyID)
require.Equal(t, issuer1Ref1.Name, issuer1Ref2.Name)

err = writeIssuer(ctx, s, &issuer2)
require.NoError(t, err)
Expand All @@ -141,21 +141,21 @@ func Test_KeysIssuerImport(t *testing.T) {
require.NoError(t, err)

// Same double import tests as above, but make sure if the previous was created through writeIssuer not importIssuer.
issuer2_ref, existing, err := importIssuer(ctx, s, issuer2.Certificate, "ignore-me")
issuer2Ref, existing, err := importIssuer(ctx, s, issuer2.Certificate, "ignore-me")
require.NoError(t, err)
require.True(t, existing)
require.Equal(t, strings.TrimSpace(issuer2.Certificate), strings.TrimSpace(issuer2_ref.Certificate))
require.Equal(t, issuer2.ID, issuer2_ref.ID)
require.Equal(t, "", issuer2_ref.Name)
require.Equal(t, issuer2.KeyID, issuer2_ref.KeyID)
require.Equal(t, strings.TrimSpace(issuer2.Certificate), strings.TrimSpace(issuer2Ref.Certificate))
require.Equal(t, issuer2.ID, issuer2Ref.ID)
require.Equal(t, "", issuer2Ref.Name)
require.Equal(t, issuer2.KeyID, issuer2Ref.KeyID)

// Same double import tests as above, but make sure if the previous was created through writeKey not importKey.
key2_ref, existing, err := importKey(ctx, s, key2.PrivateKey, "ignore-me")
key2Ref, existing, err := importKey(ctx, s, key2.PrivateKey, "ignore-me")
require.NoError(t, err)
require.True(t, existing)
require.Equal(t, key2.PrivateKey, key2_ref.PrivateKey)
require.Equal(t, key2.ID, key2_ref.ID)
require.Equal(t, "", key2_ref.Name)
require.Equal(t, key2.PrivateKey, key2Ref.PrivateKey)
require.Equal(t, key2.ID, key2Ref.ID)
require.Equal(t, "", key2Ref.Name)
}

func genIssuerAndKey(t *testing.T, b *backend) (issuerEntry, keyEntry) {
Expand Down
4 changes: 2 additions & 2 deletions builtin/logical/pki/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,12 @@ func getKeyName(ctx context.Context, s logical.Storage, data *framework.FieldDat
if !nameMatcher.MatchString(keyName) {
return "", errutil.UserError{Err: "key name contained invalid characters"}
}
key_id, err := resolveKeyReference(ctx, s, keyName)
keyId, err := resolveKeyReference(ctx, s, keyName)
if err == nil {
return "", errKeyNameInUse
}

if err != nil && key_id != KeyRefNotFound {
if err != nil && keyId != KeyRefNotFound {
return "", errutil.InternalError{Err: err.Error()}
}
}
Expand Down

0 comments on commit 5ad5384

Please sign in to comment.