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

Add cached OCSP client support to Cert Auth #17093

Merged
merged 44 commits into from
Nov 21, 2022
Merged
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
8f34df9
wip
sgmiller Sep 8, 2022
5b63032
Add cached OCSP client support to Cert Auth
sgmiller Sep 9, 2022
b0798a7
->pointer
sgmiller Sep 9, 2022
87bd5a3
Code cleanup
sgmiller Sep 9, 2022
0b9f785
Fix unit tests
sgmiller Sep 9, 2022
fb65fd5
Use an LRU cache, and only persist up to 1000 of the most recently us…
sgmiller Sep 12, 2022
5d768c0
Fix caching, add fail open mode parameter to cert auth roles
sgmiller Sep 12, 2022
6e00633
reduce logging
sgmiller Sep 12, 2022
44d808d
Add the retry client and GET then POST logic
sgmiller Sep 12, 2022
2b73de7
Drop persisted cache, make cache size configurable, allow for paralle…
sgmiller Sep 12, 2022
0f9cda6
dead code
sgmiller Sep 12, 2022
fca24ee
Update builtin/credential/cert/path_certs.go
sgmiller Sep 13, 2022
59a3769
Hook invalidate to reinit the ocsp cache size
sgmiller Sep 13, 2022
ae8423c
Merge branch 'cert-auth-ocsp' of github.com:/hashicorp/vault into cer…
sgmiller Sep 13, 2022
ea1feb4
locking
sgmiller Sep 13, 2022
e7d790d
Conditionally init the ocsp client
sgmiller Sep 13, 2022
75244c9
Remove cache size config from cert configs, it's a backend global
sgmiller Sep 13, 2022
68c3308
Add field
sgmiller Sep 13, 2022
acc2c29
Remove strangely complex validity logic
sgmiller Sep 13, 2022
81e8d6a
Address more feedback
sgmiller Sep 13, 2022
ca7e8c0
Rework error returning logic
sgmiller Sep 14, 2022
b5f2b03
More edge cases
sgmiller Sep 14, 2022
6ba87f9
MORE edge cases
sgmiller Sep 14, 2022
4abdc5a
Merge branch 'main' into cert-auth-ocsp
sgmiller Sep 14, 2022
1be9cf5
Merge remote-tracking branch 'origin/main' into cert-auth-ocsp
sgmiller Oct 27, 2022
51db45f
Add a test matrix with a builtin responder
sgmiller Nov 3, 2022
8b39c75
Merge remote-tracking branch 'origin/main' into cert-auth-ocsp
sgmiller Nov 3, 2022
0642d7e
changelog
sgmiller Nov 3, 2022
0ff724c
Use an atomic for configUpdated
sgmiller Nov 4, 2022
7e50dd0
Actually use ocsp_enabled, and bind to a random port for testing
sgmiller Nov 4, 2022
222bc58
Update builtin/credential/cert/path_login.go
sgmiller Nov 7, 2022
f192134
Refactor unit tests
sgmiller Nov 8, 2022
b8fb6ed
Add status to cache
sgmiller Nov 8, 2022
9933c3b
Make some functions private
sgmiller Nov 8, 2022
6912f06
Rename for testing, and attribute
sgmiller Nov 8, 2022
bf4e630
Merge remote-tracking branch 'origin/main' into cert-auth-ocsp
sgmiller Nov 8, 2022
4021908
Up to date gofumpt
sgmiller Nov 8, 2022
8143803
remove hash from key, and disable the vault dependent unit test
sgmiller Nov 8, 2022
e91c2cf
Comment out TestMultiOCSP
sgmiller Nov 10, 2022
ff60c7d
imports
sgmiller Nov 15, 2022
65bb99a
more imports
sgmiller Nov 18, 2022
9a2acb3
Address semgrep results
sgmiller Nov 18, 2022
899f36d
Attempt to pass some sort of logging to test_responder
sgmiller Nov 18, 2022
ec3d5ff
fix overzealous search&replace
sgmiller Nov 21, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 44 additions & 3 deletions builtin/credential/cert/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@ import (
"net/http"
"strings"
"sync"
"sync/atomic"
"time"

"github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-multierror"
"github.com/hashicorp/vault/sdk/framework"
"github.com/hashicorp/vault/sdk/helper/ocsp"
"github.com/hashicorp/vault/sdk/logical"
)

Expand All @@ -20,6 +23,13 @@ func Factory(ctx context.Context, conf *logical.BackendConfig) (logical.Backend,
if err := b.Setup(ctx, conf); err != nil {
return nil, err
}
bConf, err := b.Config(ctx, conf.StorageView)
if err != nil {
return nil, err
}
if bConf != nil {
b.updatedConfig(bConf)
}
if err := b.lockThenpopulateCRLs(ctx, conf.StorageView); err != nil {
return nil, err
}
Expand Down Expand Up @@ -49,16 +59,18 @@ func Backend() *backend {
}

b.crlUpdateMutex = &sync.RWMutex{}

return &b
}

type backend struct {
*framework.Backend
MapCertId *framework.PathMap

crls map[string]CRLInfo
crlUpdateMutex *sync.RWMutex
crls map[string]CRLInfo
crlUpdateMutex *sync.RWMutex
ocspClientMutex sync.RWMutex
ocspClient *ocsp.Client
configUpdated atomic.Bool
}

func (b *backend) invalidate(_ context.Context, key string) {
Expand All @@ -67,9 +79,25 @@ func (b *backend) invalidate(_ context.Context, key string) {
b.crlUpdateMutex.Lock()
defer b.crlUpdateMutex.Unlock()
b.crls = nil
case key == "config":
b.configUpdated.Store(true)
}
}

func (b *backend) initOCSPClient(cacheSize int) {
b.ocspClient = ocsp.New(func() hclog.Logger {
return b.Logger()
}, cacheSize)
}

func (b *backend) updatedConfig(config *config) error {
b.ocspClientMutex.Lock()
defer b.ocspClientMutex.Unlock()
b.initOCSPClient(config.OcspCacheSize)
b.configUpdated.Store(false)
return nil
}

func (b *backend) fetchCRL(ctx context.Context, storage logical.Storage, name string, crl *CRLInfo) error {
response, err := http.Get(crl.CDP.Url)
if err != nil {
Expand Down Expand Up @@ -104,6 +132,19 @@ func (b *backend) updateCRLs(ctx context.Context, req *logical.Request) error {
return errs.ErrorOrNil()
}

func (b *backend) storeConfig(ctx context.Context, storage logical.Storage, config *config) error {
entry, err := logical.StorageEntryJSON("config", config)
if err != nil {
return err
}

if err := storage.Put(ctx, entry); err != nil {
return err
}
b.updatedConfig(config)
return nil
}

const backendHelp = `
The "cert" credential provider allows authentication using
TLS client certificates. A client connects to Vault and uses
Expand Down
43 changes: 25 additions & 18 deletions builtin/credential/cert/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1062,12 +1062,13 @@ func TestBackend_CRLs(t *testing.T) {
}

func testFactory(t *testing.T) logical.Backend {
storage := &logical.InmemStorage{}
b, err := Factory(context.Background(), &logical.BackendConfig{
System: &logical.StaticSystemView{
DefaultLeaseTTLVal: 1000 * time.Second,
MaxLeaseTTLVal: 1800 * time.Second,
},
StorageView: &logical.InmemStorage{},
StorageView: storage,
})
if err != nil {
t.Fatalf("error: %s", err)
Expand Down Expand Up @@ -1893,27 +1894,33 @@ type allowed struct {
metadata_ext string // allowed metadata extensions to add to identity alias
}

func testAccStepCert(
t *testing.T, name string, cert []byte, policies string, testData allowed, expectError bool,
) logicaltest.TestStep {
func testAccStepCert(t *testing.T, name string, cert []byte, policies string, testData allowed, expectError bool) logicaltest.TestStep {
return testAccStepCertWithExtraParams(t, name, cert, policies, testData, expectError, nil)
}

func testAccStepCertWithExtraParams(t *testing.T, name string, cert []byte, policies string, testData allowed, expectError bool, extraParams map[string]interface{}) logicaltest.TestStep {
data := map[string]interface{}{
"certificate": string(cert),
"policies": policies,
"display_name": name,
"allowed_names": testData.names,
"allowed_common_names": testData.common_names,
"allowed_dns_sans": testData.dns,
"allowed_email_sans": testData.emails,
"allowed_uri_sans": testData.uris,
"allowed_organizational_units": testData.organizational_units,
"required_extensions": testData.ext,
"allowed_metadata_extensions": testData.metadata_ext,
"lease": 1000,
}
for k, v := range extraParams {
data[k] = v
}
return logicaltest.TestStep{
Operation: logical.UpdateOperation,
Path: "certs/" + name,
ErrorOk: expectError,
Data: map[string]interface{}{
"certificate": string(cert),
"policies": policies,
"display_name": name,
"allowed_names": testData.names,
"allowed_common_names": testData.common_names,
"allowed_dns_sans": testData.dns,
"allowed_email_sans": testData.emails,
"allowed_uri_sans": testData.uris,
"allowed_organizational_units": testData.organizational_units,
"required_extensions": testData.ext,
"allowed_metadata_extensions": testData.metadata_ext,
"lease": 1000,
},
Data: data,
Check: func(resp *logical.Response) error {
if resp == nil && expectError {
return fmt.Errorf("expected error but received nil")
Expand Down
54 changes: 51 additions & 3 deletions builtin/credential/cert/path_certs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We do seem to consistently use non-CA in display messages.

}
}

Expand Down Expand Up @@ -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 = `
Expand All @@ -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.

`
32 changes: 24 additions & 8 deletions builtin/credential/cert/path_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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{
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 disable_binding, I don't think this quite goes all the way towards PATCH, so I'm curious why GetOk was chosen over Get?

My 2c.

There were a couple of recent threads about it, hashicorp/vault-plugin-auth-kubernetes#163 and #17716.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand All @@ -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{
Expand Down Expand Up @@ -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"`
}
Loading