fix: error explicitly when file signer flags are set without a key path - #806
Merged
Merged
Conversation
✅ Deploy Preview for witness-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR prevents a silent “partial success” case when the file signer provider is implicitly enabled via --signer-file-* flags but lacks --signer-file-key-path. Instead of allowing other signer providers (e.g., KMS) to succeed while the file-based certificate is dropped, loadSigners now fails fast with a clear, actionable error.
Changes:
- Add an explicit validation in
loadSignersto hard-error when file-signer cert/intermediate/passphrase flags are set without a file key path. - Add a regression test covering the clearer failure mode/error message when
--signer-file-cert-pathis provided without--signer-file-key-path.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cmd/keyloader.go | Adds a pre-signer-creation validation to reject incompatible file-signer flag combinations with a clear error. |
| cmd/root_test.go | Adds a test ensuring the missing --signer-file-key-path case errors and mentions the required flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Motivation: Setting --signer-file-cert-path (or --signer-file-intermediate-paths / --signer-file-key-passphrase*) without --signer-file-key-path still activates the file signer provider, since any --signer-file-* flag turns it on. The file signer then fails to build with a confusing "failed to open key file: open : no such file or directory" error. Because loadSigners only errors when zero signers load, that failure was silently swallowed whenever another configured signer (e.g. --signer-kms-ref) succeeded: the command exited 0 with the certificate quietly dropped. A maintainer confirmed this diagnosis on the issue and asked that Witness explicitly inform users when incompatible flags like --signer-file-cert-path and --signer-kms-ref are combined, rather than embedding BYOK certificates for KMS signers (explicitly not planned). Approach: In loadSigners (cmd/keyloader.go), before building the file signer, check whether it was selected with a cert/intermediate/passphrase flag but no key path. If so, return a hard, immediate error instead of letting the provider fail internally and continuing past it. The check fires as soon as the file provider is processed in the loop and returns unconditionally, so it does not depend on the iteration order of the provider set or on whether any other signer would have succeeded. Validation: - go build ./... - make test (this repo's documented test command; all packages pass) - Added cmd/root_test.go:Test_loadSignersCertificateWithoutKey, which exercises loadSigners with only the file provider configured (cert path set, key path empty) and asserts a clear "signer-file-key-path" error is returned instead of the signers being silently dropped. - Manually reproduced via the CLI: `witness run --signer-kms-ref=... --signer-file-cert-path <path> ...` now fails fast with a single actionable error instead of the previous two confusing log lines. This manual run used a syntactically invalid KMS ARN, since no live AWS credentials are available in this environment, so it did not exercise the original report's exit-code-0 path directly (that requires a KMS ref that successfully authenticates against a real endpoint). The fix removes the continue-on-failure behavior for this case unconditionally (a hard error is returned before any signer is appended), which by construction prevents the silent-exit-0 outcome regardless of whether a co-configured signer would have succeeded; this is confirmed by code inspection and by the new unit test above, not by a live-AWS repro. Report: in-toto#573 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cmd/root_test.go:112
- The doc comment says this test reproduces the original issue where the invalid file-signer flags were "silently swallowed when another signer provider succeeds", but the test only configures the
fileprovider (so it doesn’t exercise the multi-provider swallow/exit-0 scenario). Either adjust the comment to match what’s actually being tested, or extend the test to include a second provider that succeeds.
// Test_loadSignersCertificateWithoutKey reproduces
// https://github.com/in-toto/witness/issues/573: passing --signer-file-cert-path
// without --signer-file-key-path (e.g. because the certificate was meant for a
// KMS-based signer) must fail loudly with an actionable error instead of being
// silently swallowed when another signer provider succeeds.
Contributor
Author
|
This has been green and rebased for a while now — happy to make any changes if something would help move review along. |
jkjell
approved these changes
Aug 15, 2026
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.
What this PR does / why we need it
When
--signer-file-cert-path(or--signer-file-intermediate-paths/--signer-file-key-passphrase*) is set without--signer-file-key-path, the file signer provider still gets activated (any--signer-file-*flag turns it on) and its build silently fails while logging an unclear error. If another signer provider (e.g.--signer-kms-ref) also succeeds,loadSignerswas returning success anyway, since it only fails when zero signers loaded. The command exits0with the certificate quietly dropped instead of surfacing a clear, actionable error, exactly as reported and confirmed by a maintainer on the issue.This PR adds an explicit check in
loadSigners(cmd/keyloader.go): if the file signer provider is selected with a cert/intermediate/passphrase flag but no key path, it now returns a hard, immediate error instead of silently continuing.Which issue(s) this PR fixes (optional)
Fixes #
Acceptance Criteria Met
Special notes for your reviewer:
This intentionally does not add BYOK certificate-embedding support for KMS signers (explicitly out of scope per the issue thread) — it only makes the existing incompatible-flag combination fail loudly instead of silently, per the maintainer's stated direction on the issue.
Fixes #573