put options to validate certs in an object #90
Merged
Conversation
This file contains hidden or 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
AlCutter
approved these changes
Jan 17, 2025
config.go
Outdated
| type ChainValidationConfig struct { | ||
| // Path to the file containing root certificates that are acceptable to the | ||
| // log. The certs are served through get-roots endpoint. | ||
| RootsPemFile string |
There was a problem hiding this comment.
Nit: RootsPEMFile (https://google.github.io/styleguide/go/decisions.html#initialisms)
cmd/gcp/main.go
Outdated
| if err != nil { | ||
| klog.Exitf("Failed to create checkpoint signer: %v", err) | ||
| klog.Exitf("Failed to initialize log config: %v", err) |
There was a problem hiding this comment.
Suggested change
| klog.Exitf("Failed to initialize log config: %v", err) | |
| klog.Exitf("Invalid log config: %v", err) |
cmd/gcp/main.go
Outdated
| if err != nil { | ||
| klog.Exitf("Invalid config: %v", err) | ||
| klog.Exitf("Failed to initiate storage backend: %v", err) |
There was a problem hiding this comment.
Suggested change
| klog.Exitf("Failed to initiate storage backend: %v", err) | |
| klog.Exitf("Failed to initialize storage backend: %v", err) |
config.go
Outdated
| @@ -29,6 +29,35 @@ import ( | |||
| "k8s.io/klog/v2" | |||
| ) | |||
|
|
|||
| type ChainValidationConfig struct { | |||
| // Path to the file containing root certificates that are acceptable to the | |||
There was a problem hiding this comment.
Nit on doc comments in this struct - they should really start with the name of the thing being commented on, e.g. "RootsPEMFile is the path to a file con..."
Probably worth a comment on the struct too since it's exported?
config.go
Outdated
Comment on lines
36
to
42
| // If RejectExpired is true then the certificate validity period will be | ||
| // checked against the current time during the validation of submissions. | ||
| // This will cause expired certificates to be rejected. | ||
| RejectExpired bool | ||
| // If RejectUnexpired is true then CTFE rejects certificates that are either | ||
| // currently valid or not yet valid. | ||
| RejectUnexpired bool |
There was a problem hiding this comment.
Not for this PR, but do we still need RejectUnexpired?
And if not, would the user setting NotAfterStart or NotAfterLimit to non-nil values be enough?
There was a problem hiding this comment.
Left a TODO. It's currently used by daedalus.
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.
Towards #5
This PR puts settings related to chain validation into their own object.