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.

Sign up for GitHub

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

Jump to bottom

security/advancedtls: FileWatcher CRL provider initialization enhancement #6760

Merged
merged 4 commits into from
Nov 8, 2023
Merged

security/advancedtls: FileWatcher CRL provider initialization enhancement #6760

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
312a6cd
Add initial scan as a part of FWCP creation
erm-g Nov 2, 2023
d97b6a9
Add comment about default value for RefreshDuration
erm-g Nov 2, 2023
465ebac
Promote Close() to the interface level
erm-g Nov 6, 2023
6101672
Revert "Promote Close() to the interface level"
erm-g Nov 8, 2023
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
15 changes: 11 additions & 4 deletions security/advancedtls/crl_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ type CRLProvider interface {
//
// [RFC5280 - Undetermined]: https://datatracker.ietf.org/doc/html/rfc5280#section-6.3.3
CRL(cert *x509.Certificate) (*CRL, error)

// Close cleans up resources allocated by the provider.
Close()
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? This forces the static CRL provider to implement it, when it doesn't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was taking another look at gRFC and realized that Close() is specified at the interface level there - grpc/proposal#382
Then I checked the Go repo and realized that having `Close()' at interface level plus no-op impls (if needed) is an existing pattern -

grpc-go/balancer/balancer.go

Line 388 in 70f1a40

Close()

During internal discussion folks suggested to make that change - WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

We need to close the LB policy from gRPC; hence it's included in that interface. You don't close the CRL provider from advancedTLS. So the same reasoning doesn't apply in this case. I would not add it here, since it isn't necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was designed to be very close to the Credential Reloading feature which has Close in the interface

grpc-go/credentials/tls/certprovider/provider.go

Lines 75 to 89 in 6bed353

// Provider makes it possible to keep channel credential implementations up to
// date with secrets that they rely on to secure communications on the
// underlying channel.
//
// Provider implementations are free to rely on local or remote sources to fetch
// the latest secrets, and free to share any state between different
// instantiations as they deem fit.
type Provider interface {
// KeyMaterial returns the key material sourced by the Provider.
// Callers are expected to use the returned value as read-only.
KeyMaterial(ctx context.Context) (*KeyMaterial, error)
// Close cleans up resources allocated by the Provider.
Close()
}

Copy link
Member

Choose a reason for hiding this comment

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

If the cert provider doesn't need it (i.e. gRPC never calls it), then we should consider removing it from there. I'm not sure whether that's the case.
But that's not justification to add it here. It's not needed here, so we probably shouldn't add it.

Feel free to disregard my advice if you really want this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with Doug here, for example Close() from provider.go is used in xDS case by gRPC for recreation.
Since we don't have such cases for CRL, I'd prefer to drop it from the interface. @gtcooke94 WDYT?

}

// StaticCRLProvider implements CRLProvider interface by accepting raw content
Expand Down Expand Up @@ -87,11 +90,14 @@ func (p *StaticCRLProvider) CRL(cert *x509.Certificate) (*CRL, error) {
return p.crls[cert.Issuer.ToRDNSequence().String()], nil
}

// Close is a no-op.
func (p *StaticCRLProvider) Close() {}

// FileWatcherOptions represents a data structure holding a configuration for
// FileWatcherCRLProvider.
type FileWatcherOptions struct {
CRLDirectory string // Path of the directory containing CRL files
RefreshDuration time.Duration // Time interval between CRLDirectory scans, can't be smaller than 1 minute
RefreshDuration time.Duration // Time interval (default value is 1 hour) between CRLDirectory scans, can't be smaller than 1 minute
CRLReloadingFailedCallback func(err error) // Custom callback executed when a CRL file can’t be processed
}

Expand All @@ -109,8 +115,9 @@ type FileWatcherCRLProvider struct {

// NewFileWatcherCRLProvider returns a new instance of the
// FileWatcherCRLProvider. It uses FileWatcherOptions to validate and apply
// configuration required for creating a new instance. Users should call Close
// to stop the background refresh of CRLDirectory.
// configuration required for creating a new instance. The initial scan of
// CRLDirectory is performed inside this function. Users should call Close to
// stop the background refresh of CRLDirectory.
func NewFileWatcherCRLProvider(o FileWatcherOptions) (*FileWatcherCRLProvider, error) {
if err := o.validate(); err != nil {
return nil, err
Expand All @@ -121,6 +128,7 @@ func NewFileWatcherCRLProvider(o FileWatcherOptions) (*FileWatcherCRLProvider, e
stop: make(chan struct{}),
done: make(chan struct{}),
}
provider.scanCRLDirectory()
go provider.run()
return provider, nil
}
Expand Down Expand Up @@ -149,7 +157,6 @@ func (p *FileWatcherCRLProvider) run() {
defer close(p.done)
ticker := time.NewTicker(p.opts.RefreshDuration)
defer ticker.Stop()
p.scanCRLDirectory()

for {
select {
Expand Down
6 changes: 2 additions & 4 deletions security/advancedtls/crl_provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func (s) TestStaticCRLProvider(t *testing.T) {
rawCRLs = append(rawCRLs, rawCRL)
}
p := NewStaticCRLProvider(rawCRLs)

// Each test data entry contains a description of a certificate chain,
// certificate chain itself, and if CRL is not expected to be found.
tests := []struct {
Expand Down Expand Up @@ -154,10 +155,6 @@ func (s) TestFileWatcherCRLProvider(t *testing.T) {
t.Fatal("Unexpected error while creating FileWatcherCRLProvider:", err)
}

// We need to make sure that initial CRLDirectory scan is completed before
// querying the internal map.
p.Close()

// Each test data entry contains a description of a certificate chain,
// certificate chain itself, and if CRL is not expected to be found.
tests := []struct {
Expand Down Expand Up @@ -197,6 +194,7 @@ func (s) TestFileWatcherCRLProvider(t *testing.T) {
}
})
}
p.Close()
if diff := cmp.Diff(len(nonCRLFilesSet), nonCRLFilesUnderCRLDirectory); diff != "" {
t.Errorf("Unexpected number Number of callback executions\ndiff (-got +want):\n%s", diff)
}
Expand Down