-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add cached OCSP client support to Cert Auth #17093
Changes from 31 commits
8f34df9
5b63032
b0798a7
87bd5a3
0b9f785
fb65fd5
5d768c0
6e00633
44d808d
2b73de7
0f9cda6
fca24ee
59a3769
ae8423c
ea1feb4
e7d790d
75244c9
68c3308
acc2c29
81e8d6a
ca7e8c0
b5f2b03
6ba87f9
4abdc5a
1be9cf5
51db45f
8b39c75
0642d7e
0ff724c
7e50dd0
222bc58
f192134
b8fb6ed
9933c3b
6912f06
bf4e630
4021908
8143803
e91c2cf
ff60c7d
65bb99a
9a2acb3
899f36d
ec3d5ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,8 @@ import ( | |
"strings" | ||
"time" | ||
|
||
sockaddr "github.com/hashicorp/go-sockaddr" | ||
"github.com/hashicorp/go-sockaddr" | ||
|
||
"github.com/hashicorp/vault/sdk/framework" | ||
"github.com/hashicorp/vault/sdk/helper/tokenutil" | ||
"github.com/hashicorp/vault/sdk/logical" | ||
|
@@ -47,7 +48,32 @@ Must be x509 PEM encoded.`, | |
EditType: "file", | ||
}, | ||
}, | ||
|
||
"ocsp_enabled": { | ||
cipherboy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Type: framework.TypeBool, | ||
Description: `Whether to attempt OCSP verification of certificates at login`, | ||
}, | ||
"ocsp_ca_certificates": { | ||
Type: framework.TypeString, | ||
Description: `Any additional CA certificates needed to communicate with OCSP servers`, | ||
DisplayAttrs: &framework.DisplayAttributes{ | ||
EditType: "file", | ||
}, | ||
}, | ||
"ocsp_servers_override": { | ||
Type: framework.TypeCommaStringSlice, | ||
Description: `A comma-separated list of OCSP server addresses. If unset, the OCSP server is determined | ||
from the AuthorityInformationAccess extension on the certificate being inspected.`, | ||
}, | ||
"ocsp_fail_open": { | ||
Type: framework.TypeBool, | ||
Default: false, | ||
Description: "If set to true, if an OCSP revocation cannot be made successfully, login will proceed rather than failing. If false, failing to get an OCSP status fails the request.", | ||
}, | ||
"ocsp_query_all_servers": { | ||
Type: framework.TypeBool, | ||
Default: false, | ||
Description: "If set to true, rather than accepting the first successful OCSP response, query all servers and consider the certificate valid only if all servers agree.", | ||
}, | ||
"allowed_names": { | ||
Type: framework.TypeCommaStringSlice, | ||
Description: `A comma-separated list of names. | ||
|
@@ -294,6 +320,21 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr | |
if certificateRaw, ok := d.GetOk("certificate"); ok { | ||
cert.Certificate = certificateRaw.(string) | ||
} | ||
if ocspCertificatesRaw, ok := d.GetOk("ocsp_ca_certificates"); ok { | ||
cert.OcspCaCertificates = ocspCertificatesRaw.(string) | ||
} | ||
if ocspEnabledRaw, ok := d.GetOk("ocsp_enabled"); ok { | ||
cert.OcspEnabled = ocspEnabledRaw.(bool) | ||
} | ||
if ocspServerOverrides, ok := d.GetOk("ocsp_servers_override"); ok { | ||
cert.OcspServersOverride = ocspServerOverrides.([]string) | ||
} | ||
if ocspFailOpen, ok := d.GetOk("ocsp_fail_open"); ok { | ||
cert.OcspFailOpen = ocspFailOpen.(bool) | ||
} | ||
if ocspQueryAll, ok := d.GetOk("ocsp_query_all_servers"); ok { | ||
cert.OcspQueryAllServers = ocspQueryAll.(bool) | ||
} | ||
if displayNameRaw, ok := d.GetOk("display_name"); ok { | ||
cert.DisplayName = displayNameRaw.(string) | ||
} | ||
|
@@ -399,7 +440,7 @@ func (b *backend) pathCertWrite(ctx context.Context, req *logical.Request, d *fr | |
} | ||
} | ||
if !clientAuth { | ||
return logical.ErrorResponse("non-CA certificates should have TLS client authentication set as an extended key usage"), nil | ||
return logical.ErrorResponse("nonCA certificates should have TLS client authentication set as an extended key usage"), nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We do seem to consistently use |
||
} | ||
} | ||
|
||
|
@@ -438,6 +479,12 @@ type CertEntry struct { | |
RequiredExtensions []string | ||
AllowedMetadataExtensions []string | ||
BoundCIDRs []*sockaddr.SockAddrMarshaler | ||
|
||
OcspCaCertificates string | ||
OcspEnabled bool | ||
OcspServersOverride []string | ||
OcspFailOpen bool | ||
OcspQueryAllServers bool | ||
} | ||
|
||
const pathCertHelpSyn = ` | ||
|
@@ -451,4 +498,5 @@ that are allowed to authenticate. | |
Deleting a certificate will not revoke auth for prior authenticated connections. | ||
To do this, do a revoke on "login". If you don't need to revoke login immediately, | ||
then the next renew will cause the lease to expire. | ||
|
||
` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ import ( | |
"github.com/hashicorp/vault/sdk/logical" | ||
) | ||
|
||
const maxCacheSize = 100000 | ||
|
||
func pathConfig(b *backend) *framework.Path { | ||
return &framework.Path{ | ||
Pattern: "config", | ||
|
@@ -22,6 +24,11 @@ func pathConfig(b *backend) *framework.Path { | |
Default: false, | ||
Description: `If set, metadata of the certificate including the metadata corresponding to allowed_metadata_extensions will be stored in the alias. Defaults to false.`, | ||
}, | ||
"ocsp_cache_size": { | ||
Type: framework.TypeInt, | ||
Default: 100, | ||
Description: `The size of the in memory OCSP response cache, shared by all configured certs`, | ||
}, | ||
}, | ||
|
||
Callbacks: map[logical.Operation]framework.OperationFunc{ | ||
|
@@ -32,18 +39,25 @@ func pathConfig(b *backend) *framework.Path { | |
} | ||
|
||
func (b *backend) pathConfigWrite(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { | ||
disableBinding := data.Get("disable_binding").(bool) | ||
enableIdentityAliasMetadata := data.Get("enable_identity_alias_metadata").(bool) | ||
|
||
entry, err := logical.StorageEntryJSON("config", config{ | ||
DisableBinding: disableBinding, | ||
EnableIdentityAliasMetadata: enableIdentityAliasMetadata, | ||
}) | ||
config, err := b.Config(ctx, req.Storage) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if err := req.Storage.Put(ctx, entry); err != nil { | ||
if disableBindingRaw, ok := data.GetOk("disable_binding"); ok { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My understanding, and correct me if I'm wrong, is this might change it from a POST/PUT style behavior into a PATCH style behavior; I'd rather see this preserved and an explicit PATCH handler added? I think it depends on whether or not a default value was chosen, but given that we do set a default for My 2c. There were a couple of recent threads about it, hashicorp/vault-plugin-auth-kubernetes#163 and #17716. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the status quo for this auth engine. Note that the same thing is true in path_certs.go. Changing it would break backwards compatibility. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean by "breaks backwards compatibility"? By my read here, this PR is what breaks it, as we previously had: disableBinding := data.Get("disable_binding").(bool)
enableIdentityAliasMetadata := data.Get("enable_identity_alias_metadata").(bool) |
||
config.DisableBinding = disableBindingRaw.(bool) | ||
} | ||
if enableIdentityAliasMetadataRaw, ok := data.GetOk("enable_identity_alias_metadata"); ok { | ||
config.EnableIdentityAliasMetadata = enableIdentityAliasMetadataRaw.(bool) | ||
} | ||
if cacheSizeRaw, ok := data.GetOk("ocsp_cache_size"); ok { | ||
cacheSize := cacheSizeRaw.(int) | ||
if cacheSize < 2 || cacheSize > maxCacheSize { | ||
return logical.ErrorResponse("invalid cache size, must be >= 2 and <= %d", maxCacheSize), nil | ||
} | ||
config.OcspCacheSize = cacheSize | ||
} | ||
if err := b.storeConfig(ctx, req.Storage, config); err != nil { | ||
return nil, err | ||
} | ||
return nil, nil | ||
|
@@ -58,6 +72,7 @@ func (b *backend) pathConfigRead(ctx context.Context, req *logical.Request, d *f | |
data := map[string]interface{}{ | ||
"disable_binding": cfg.DisableBinding, | ||
"enable_identity_alias_metadata": cfg.EnableIdentityAliasMetadata, | ||
"ocsp_cache_size": cfg.OcspCacheSize, | ||
} | ||
|
||
return &logical.Response{ | ||
|
@@ -85,4 +100,5 @@ func (b *backend) Config(ctx context.Context, s logical.Storage) (*config, error | |
type config struct { | ||
DisableBinding bool `json:"disable_binding"` | ||
EnableIdentityAliasMetadata bool `json:"enable_identity_alias_metadata"` | ||
OcspCacheSize int `json:"ocsp_cache_size"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No clue about the helpers here, but should this just be
testAccStepLoginWithNameInvalid
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite, but I did add some new helpers rather than the expected err bool