fix(initdata): harden cert validation and improve validate UX#45
Merged
Conversation
Cert validation changes: - Reject all non-CA certificates (CA:TRUE required for trust anchors); leaf certs with SAN+serverAuth were previously accepted in error - Reject SHA-1 and MD5 signed certificates - Reject RSA keys shorter than 1024 bits - Reject certificates with unknown critical extensions - Consolidate error message: validateCACert now emits the user-friendly "not a CA certificate" message directly; validateCerts removed (dead code) validate command UX: - Command now prints all diagnostics to stderr itself; Cobra error machinery is silenced surgically inside RunE (after flag parsing) so unknown-flag errors still print normally - Exit code 0 = passed, 1 = validation failed or input error - SilenceUsage on root command prevents usage block on operational errors Test and fixture updates: - Regenerate fixtures: valid-with-both.toml now uses two CA certs; valid-with-leaf-cert.toml replaced by invalid-leaf-cert.toml - Add tests for SHA-1, MD5, weak RSA key, and unknown critical extensions - Update validate tests to assert on stderr content Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens initdata embedded certificate validation to treat all embedded cert fields as trust anchors (CA-only), while also adjusting initdata validate CLI behavior to emit user-facing diagnostics directly and standardize exit codes. It also refreshes fixtures and tests to cover the new validation rules.
Changes:
- Enforce CA-only embedded certificate validation and reject weak/unsafe certificate properties (e.g., SHA-1/MD5, weak RSA keys, unknown critical extensions).
- Update
initdata validateUX to print diagnostics itself and return consistent exit codes without Cobra duplicating error output. - Regenerate/replace test fixtures and expand tests to assert on stderr and new rejection cases.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates documentation of initdata validation rules for embedded certificates. |
| cmd/root.go | Sets root Cobra command SilenceUsage to avoid usage dumps on operational errors. |
| cmd/initdata/validate.go | Adjusts validate command messaging/exit behavior and updates validation description text. |
| cmd/initdata/validate_test.go | Updates tests to capture/assert stderr output and new CA-only behavior. |
| cmd/initdata/common.go | Hardens CA certificate validation (weak sig algs, RSA size, critical extensions) and removes leaf-cert validation logic. |
| cmd/initdata/common_test.go | Reworks tests to reflect CA-only rules and adds coverage for newly rejected cert properties. |
| cmd/initdata/testdata/valid-with-leaf-cert.toml | Removes fixture that previously (incorrectly) treated a leaf cert as valid. |
| cmd/initdata/testdata/valid-with-ca-cert.toml | Regenerates CA fixture certificate material. |
| cmd/initdata/testdata/valid-with-both.toml | Updates fixture to use CA certs in both embedded cert positions. |
| cmd/initdata/testdata/invalid-leaf-no-san.toml | Regenerates invalid leaf fixture certificate material. |
| cmd/initdata/testdata/invalid-leaf-cert.toml | Adds new fixture representing a “well-formed” leaf cert that must now be rejected. |
| cmd/initdata/testdata/invalid-expired-cert.toml | Regenerates expired cert fixture material. |
| cmd/initdata/testdata/gen/main.go | Updates fixture generator to produce two CA certs and the new leaf-invalid fixture. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // errValidationFailed is a sentinel returned when runValidate has already | ||
| // printed its own diagnostics and wants a non-zero exit without Cobra | ||
| // printing an additional "Error: ..." line. | ||
| var errValidationFailed = errors.New("") |
Comment on lines
98
to
106
| entries, err := extractCertsFromInitdata(id.Data) | ||
| if err != nil { | ||
| failures = append(failures, fmt.Sprintf("cert extraction failed: %v", err)) | ||
| } else if len(entries) > 0 { | ||
| reportCerts(entries) | ||
| if err := validateCertsBySource(entries); err != nil { | ||
| failures = append(failures, err.Error()) | ||
| } | ||
| } |
| - `version` is `0.1.0` and `algorithm` is one of `sha256`, `sha384`, `sha512` | ||
| - Required keys `aa.toml` and `cdh.toml` are present (`policy.rego` is optional) | ||
| - Embedded certificates pass rustls rules: CA certs must have `keyCertSign`; leaf certs must have a SubjectAltName and `extendedKeyUsage=serverAuth` and must not be self-signed | ||
| - Embedded certificates must be CA certificates (`CA:TRUE`, `keyCertSign`); rejected: leaf/non-CA certs, expired certs, SHA-1 or MD5 signatures, unknown critical extensions, RSA keys shorter than 1024 bits |
Comment on lines
+21
to
+27
| oldStderr := os.Stderr | ||
| os.Stderr = w | ||
| err = runValidate(nil, nil) | ||
| _ = w.Close() | ||
| os.Stderr = oldStderr | ||
| out, _ := io.ReadAll(r) | ||
| _ = r.Close() |
Comment on lines
+127
to
+135
| if isWeakSignatureAlg(cert.SignatureAlgorithm) { | ||
| return fmt.Errorf("certificate %q: weak signature algorithm %s", cert.Subject.CommonName, cert.SignatureAlgorithm) | ||
| } | ||
| if rsaKey, ok := cert.PublicKey.(*rsa.PublicKey); ok && rsaKey.N.BitLen() < 1024 { | ||
| return fmt.Errorf("certificate %q: RSA key is %d bits, minimum is 1024", cert.Subject.CommonName, rsaKey.N.BitLen()) | ||
| } | ||
| if len(cert.UnhandledCriticalExtensions) > 0 { | ||
| return fmt.Errorf("certificate %q: has unknown critical extensions", cert.Subject.CommonName) | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
cmd/initdata/common.go:178
- validateCertsBySource returns an error prefixed with "cert validation failed:\n ...". Since runValidate already prints its own "Validation failed:" header and then prints each failure line, this produces redundant headers and awkward indentation for multi-line cert errors. Consider having validateCertsBySource return just the per-cert diagnostics (without its own "cert validation failed" wrapper), or return a slice/typed error so runValidate can format consistently.
// validateCertsBySource applies CA certificate rules to each embedded cert and
// includes the source field in error messages so the user can locate the
// problematic cert. All initdata cert fields are trust anchor positions —
// only CA certificates (CA:TRUE, keyCertSign) are accepted.
func validateCertsBySource(entries []certEntry) error {
var errs []string
for _, e := range entries {
if err := validateCACert(e.cert); err != nil {
errs = append(errs, fmt.Sprintf("%s: %s", e.source, err.Error()))
}
}
if len(errs) > 0 {
return fmt.Errorf("cert validation failed:\n %s", strings.Join(errs, "\n "))
}
Comment on lines
+190
to
+194
| // diagWriter wraps an io.Writer and captures the first write error, short- | ||
| // circuiting subsequent writes. Callers check err once at the end rather than | ||
| // after every fmt.Fprintf call. | ||
| type diagWriter struct { | ||
| w io.Writer |
| expPEM, _ := mustCreateCert(expTmpl, expTmpl, &expKey.PublicKey, expKey) | ||
|
|
||
| // Leaf cert with no SAN — valid structure but fails rustls leaf rules. | ||
| // Non-CA leaf cert with no SAN — fails on two counts: not a CA cert, no SAN. |
- errValidationFailed: use non-empty sentinel message "validation failed" so the error is informative if ever logged or printed unexpectedly - reportCerts: accept io.Writer instead of writing to os.Stdout directly; runValidate passes os.Stderr so cert details and failure output are on the same stream, keeping stdout clean for scripting - Unknown critical extensions error now includes OIDs in dot notation (e.g. 2.5.29.17) to help users identify the offending extension - README: "expired certs" -> "expired or not-yet-valid certs" to match the actual checkExpiry behaviour (NotBefore in the future is also rejected) - runValidateStderr test helper: use t.Cleanup to guarantee os.Stderr is restored even if runValidate panics; apply same fix to TestRunValidate_URLMismatchGoesToStderr Signed-off-by: Pradipta Banerjee <pradipta.banerjee@gmail.com>
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
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.
Cert validation changes:
validate command UX:
Test and fixture updates: