A69: Certificate Revocation List Enhancements #382
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
erm-g
requested changes
Jul 26, 2023
AXX-crl-enhancements.md
Outdated
| Per RFC5280, during revocation checking one of three status should be returned - `RevocationUnrevoked`, `RevocationRevoked`, and `RevocationUndetermined`. The user must specify whether `RevocationUndetermined` should be treated as `RevocationRevoked` or `RevocationUnrevoked` (failing open vs. failing closed). | ||
|
|
||
| Assumptions: | ||
| 1. There should be no direct linkage between this interface and the creation/distribution of CRLs. gRPC is a user of CRLs and credentials, not a PKI itself. |
There was a problem hiding this comment.
I'd add some 'high level overview' section (maybe as a comment to a diagram?) saying that existing impls use map as an in-memory storage and a provider to update it using some IOs. PKIs are completely out of scope of this gRFC
There was a problem hiding this comment.
Also let's add an assumption that we can't guess CRL provider by a name of a file / metadata of strings. However if someone wants to rely on it they can do a specific impl
There was a problem hiding this comment.
Added the second
For the first, this document has a structure defined by a template. I could have a High Level Overview sub-section in the Proposal section.
existing impls use map as an in-memory storage and a provider to update it using some IOs
I'm a little confused at this comment - the existing CRL impls don't do this?
ejona86
approved these changes
Sep 29, 2023
There was a problem hiding this comment.
A few more comments on top of @erm-g's. But looks good.
@markdroth, @dfawley, can y'all take a look?
gtcooke94
added a commit
to grpc/grpc
that referenced
this pull request
Oct 17, 2023
The basic APIs for the CRL Reloading features. This adds external types to represent CRL Providers, CRLs, and CertificateInfo. Internally we will use `CrlImpl` - this layer is needed to hide OpenSSL details from the user. GRFC - grpc/proposal#382 Things Done * Add external API for `CrlProvider`, `Crl`, `CertInfo` (`CertInfo` is used during CRL lookup rather than passing the entire certificate). * Add code paths in `ssl_transport_security` to utilize CRL providers * Add `StaticCrlProvider` * Refactor `crl_ssl_transport_security_test.cc` so it is more extensible and can be used with providers
erm-g
reviewed
Nov 2, 2023
erm-g
reviewed
Nov 2, 2023
erm-g
reviewed
Nov 2, 2023
gtcooke94
added a commit
to grpc/grpc
that referenced
this pull request
Nov 3, 2023
This adds the directory reloader implementation of the CrlProvider. This will periodically reload CRL files in a directory per [gRFC A69](grpc/proposal#382) Included in this is the following: * A public API to create the `DirectoryReloaderCrlProvider` * A basic directory interface in gprpp and platform specific impls for getting the list of files in a directory (unfortunately prior C++17, there is no std::filesystem, so we have to have platform specific impls) * The implementation of `DirectoryReloaderCrlProvider` takes an event_engine and a directory interface. This allows us to test using the fuzzing event engine for time mocking, and to implement a test directory interface so we avoid having to make temporary directories and files in the tests. This is notably not in `include`, and the `CreateDirectoryReloaderCrlProvider` is the only way to construct one from the public API, so we don't expose the event engine and directory details to the user. --------- Co-authored-by: gtcooke94 <gtcooke94@users.noreply.github.com>
gtcooke94
added a commit
to gtcooke94/grpc
that referenced
this pull request
Nov 13, 2023
) This adds the directory reloader implementation of the CrlProvider. This will periodically reload CRL files in a directory per [gRFC A69](grpc/proposal#382) Included in this is the following: * A public API to create the `DirectoryReloaderCrlProvider` * A basic directory interface in gprpp and platform specific impls for getting the list of files in a directory (unfortunately prior C++17, there is no std::filesystem, so we have to have platform specific impls) * The implementation of `DirectoryReloaderCrlProvider` takes an event_engine and a directory interface. This allows us to test using the fuzzing event engine for time mocking, and to implement a test directory interface so we avoid having to make temporary directories and files in the tests. This is notably not in `include`, and the `CreateDirectoryReloaderCrlProvider` is the only way to construct one from the public API, so we don't expose the event engine and directory details to the user. --------- Co-authored-by: gtcooke94 <gtcooke94@users.noreply.github.com>
gtcooke94
added a commit
to gtcooke94/grpc
that referenced
this pull request
Nov 13, 2023
) This adds the directory reloader implementation of the CrlProvider. This will periodically reload CRL files in a directory per [gRFC A69](grpc/proposal#382) Included in this is the following: * A public API to create the `DirectoryReloaderCrlProvider` * A basic directory interface in gprpp and platform specific impls for getting the list of files in a directory (unfortunately prior C++17, there is no std::filesystem, so we have to have platform specific impls) * The implementation of `DirectoryReloaderCrlProvider` takes an event_engine and a directory interface. This allows us to test using the fuzzing event engine for time mocking, and to implement a test directory interface so we avoid having to make temporary directories and files in the tests. This is notably not in `include`, and the `CreateDirectoryReloaderCrlProvider` is the only way to construct one from the public API, so we don't expose the event engine and directory details to the user. --------- Co-authored-by: gtcooke94 <gtcooke94@users.noreply.github.com>
ejona86
approved these changes
Nov 20, 2023
markdroth
reviewed
Nov 27, 2023
markdroth
approved these changes
Jan 31, 2025
dfawley
approved these changes
Feb 28, 2025
|
|
||
| Users can also implement the interface with whatever behaviors they specifically desire. | ||
|
|
||
| Following [RFC5280](https://datatracker.ietf.org/doc/html/rfc5280), during revocation checking one of three status should be returned - `RevocationUnrevoked`, `RevocationRevoked`, and `RevocationUndetermined` ([RFC5280 defines `Undetermined` in the CRL Processing section](https://datatracker.ietf.org/doc/html/rfc5280#section-6.3.3). There are many reasons a certificate can be revoked, but in the end gRPC cares about a certificate being revoked or not, thus we differentiate between `RevocationUnrevoked` and `RevocationRevoked`. The user must specify whether `RevocationUndetermined` should be treated as `RevocationRevoked` or `RevocationUnrevoked` (failing open vs. failing closed). |
There was a problem hiding this comment.
Suggested change
| Following [RFC5280](https://datatracker.ietf.org/doc/html/rfc5280), during revocation checking one of three status should be returned - `RevocationUnrevoked`, `RevocationRevoked`, and `RevocationUndetermined` ([RFC5280 defines `Undetermined` in the CRL Processing section](https://datatracker.ietf.org/doc/html/rfc5280#section-6.3.3). There are many reasons a certificate can be revoked, but in the end gRPC cares about a certificate being revoked or not, thus we differentiate between `RevocationUnrevoked` and `RevocationRevoked`. The user must specify whether `RevocationUndetermined` should be treated as `RevocationRevoked` or `RevocationUnrevoked` (failing open vs. failing closed). | |
| Following [RFC5280](https://datatracker.ietf.org/doc/html/rfc5280), during revocation checking one of three status should be returned - `RevocationUnrevoked`, `RevocationRevoked`, and `RevocationUndetermined` ([RFC5280 defines `Undetermined` in the CRL Processing section](https://datatracker.ietf.org/doc/html/rfc5280#section-6.3.3)). There are many reasons a certificate can be revoked, but in the end gRPC cares about a certificate being revoked or not, thus we differentiate between `RevocationUnrevoked` and `RevocationRevoked`. The user must specify whether `RevocationUndetermined` should be treated as `RevocationRevoked` or `RevocationUnrevoked` (failing open vs. failing closed). |
Comment on lines
+146
to
+147
| // crlProvider and RootDir/Cache cannot both be set | ||
| + CrlProvider crlProvider |
There was a problem hiding this comment.
Suggested change
| // crlProvider and RootDir/Cache cannot both be set | |
| + CrlProvider crlProvider | |
| + // crlProvider and RootDir/Cache cannot both be set | |
| + CRLProvider crlProvider |
| Regarding sharing between connections - since the providers are in a high-level configuration, the same provider should be used across many individual connections and handshakes. [Here is an example of how the credential reloading provider is configured](https://github.com/grpc/grpc-go/blob/7935c4f759419cb0ab4cc50070f5487b33a19bb7/security/advancedtls/examples/credential_reloading_from_files/server/main.go#L58-L63). In this example, the server would use this provider for all connections. | ||
|
|
||
| #### Directory Reloader Specifics | ||
|  |
There was a problem hiding this comment.
There's similar capitalization issues in the image, but that's fine.
F9DA
Comment on lines
+163
to
+175
| ```go | ||
| type CrlDirectoryReloadingProvider struct { | ||
| ReloadInterval time.Duration | ||
| CrlDirectory string | ||
| // maps issuer hash to crl | ||
| crls map[string]*CertificateListExt | ||
| } | ||
| func (provider *CrlDirectoryReloadingProvider) Crl(x509.Certificate) (*CertificateListExt, error) {...} | ||
|
|
||
| func (provider *CrlDirectoryReloadingProvider) run(ctx context.Context) { | ||
| // golang ticker that refreshes the Crls | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Suggested change
| ```go | |
| type CrlDirectoryReloadingProvider struct { | |
| ReloadInterval time.Duration | |
| CrlDirectory string | |
| // maps issuer hash to crl | |
| crls map[string]*CertificateListExt | |
| } | |
| func (provider *CrlDirectoryReloadingProvider) Crl(x509.Certificate) (*CertificateListExt, error) {...} | |
| func (provider *CrlDirectoryReloadingProvider) run(ctx context.Context) { | |
| // golang ticker that refreshes the Crls | |
| } | |
| ``` | |
| ```go | |
| type CRLDirectoryReloadingProvider struct { | |
| ReloadInterval time.Duration | |
| CRLDirectory string | |
| // maps issuer hash to crl | |
| crls map[string]*CertificateListExt | |
| } | |
| func (provider *CRLDirectoryReloadingProvider) CRL(x509.Certificate) (*CertificateListExt, error) {...} | |
| func (provider *CRLDirectoryReloadingProvider) run(ctx context.Context) { | |
| // golang ticker that refreshes the CRLs | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.