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

security/advancedtls: FileWatcher CRL provider initialization enhancement #6760

Merged
merged 4 commits into from
Nov 8, 2023

Conversation

erm-g
Copy link
Contributor

@erm-g erm-g commented Nov 2, 2023

Part of CRL Provider effort. The functionality in this PR includes enhancement for FileWatcher provider initialization logic. It also adds minor godoc improvements.

RELEASE NOTES: N/A

@erm-g erm-g added the Type: Behavior Change Behavior changes not categorized as bugs label Nov 2, 2023
@erm-g erm-g added this to the 1.60 Release milestone Nov 2, 2023
Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Merging #6760 (6101672) into master (70f1a40) will decrease coverage by 0.04%.
Report is 8 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files

@erm-g erm-g requested a review from dfawley November 2, 2023 01:54
@dfawley dfawley changed the title crl provider: FileWatcher provider initialization enhancement security/advancedtls: FileWatcher CRL provider initialization enhancement Nov 2, 2023
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

It seems like a new test (or test case) should be added to cover making sure the initial scan happens synchronously.

@erm-g
Copy link
Contributor Author

erm-g commented Nov 2, 2023

It seems like a new test (or test case) should be added to cover making sure the initial scan happens synchronously.

It's covered by updated TestFileWatcherCRLProvider
If the initial load is async, then CRL check happens before the CRLDirectory content is loaded and the test fails, I double checked it locally.

@dfawley
Copy link
Member

dfawley commented Nov 3, 2023

It's covered by updated TestFileWatcherCRLProvider

Oh, I see. The old Close was done to block until the scan happened. Very odd to still access p after calling p.Close, which should normally be considered illegal, but that's not happening anymore which is good.

@erm-g erm-g requested a review from gtcooke94 November 6, 2023 16:50
Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 55 to 57

// 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 -


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

// 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?

@erm-g erm-g merged commit be1d1c1 into grpc:master Nov 8, 2023
14 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants