Prevent silent attestation downgrade on gem push - #9825
Open
hsbt wants to merge 16 commits into
Open
Conversation
On GitHub Actions the push command tests took the auto-attestation path and spawned real `gem exec sigstore-cli` subprocesses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GitHub Actions documents the variable as "true", so any other value (including "false" or an empty string) should not trigger the auto-attestation path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
attest! returned only the tempfile path, so GC could finalize the Tempfile and unlink the bundle before it was read back, silently degrading the push to unattested. Tempfile.create scopes the file to the block and attest! now returns the bundle content instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The whole attested push was wrapped in rescue StandardError, so a failure to read an explicit --attestation file still published the gem unattested with exit 0, and a network error after the server may have accepted the multipart push retried it unattested, letting an on-path attacker strip attestations by cutting the first connection. Only the opportunistic auto-signing step falls back now, explicit attestation errors abort the push, and each bundle is validated as JSON before being joined into the attestations array. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Scrubbing GITHUB_ACTIONS in setup left both skip tests passing no matter what the host and engine guards do, since the env term alone decided the branch. Verified by removing the guards: both now fail. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Narrowing the rescue to the signing step made the old wording wrong in every case it can now print: no push has been attempted at that point, so nothing is being retried. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JSON.parse accepts top-level scalars, so a file holding just null or a number passed validation and was sent on as "[null]". Hoisting the require above the begin also keeps a LoadError from being reported as an undefined JSON constant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both comments described the same asymmetry, once per branch, and the explicit branch is already explained by the one above the condition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
setup scrubs it and teardown restores the whole environment, so the per-test ensure blocks were redundant once the scrub landed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The file is now validated as JSON before the push, so the help should state the format it has to be in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Errno messages carry the path as a rb_sysopen suffix, and the prefix already names the file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A path with a null byte raised a bare ArgumentError, escaping the uniform message the rescue was there to provide. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The map and join that build the attestations array had never been run with more than a single element. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Nothing exercised the non-zero exit status branch, so dropping the guard left the suite green while a truncated bundle went out as an unattested push. Verified by removing it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JRuby and TruffleRuby do not use CRuby's " @ rb_sysopen - <path>" suffix, so the trim was a no-op there and the assertion failed on all three. Every portable form of it costs more than the cosmetic gain. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The JSON validation on sigstore-cli's output had no test: removing the call left the whole suite green. Verified by removing it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.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
1 participant
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.
gem pushwrapped the whole attested push inrescue StandardError. A failure to read a file given to--attestationprinted a warning and published the gem unattested with exit 0, contradicting the comment right above that branch. The same rescue covered the HTTP POST, so a read timeout after the server had already accepted the multipart push retried it unattested, which also lets an on-path attacker strip attestations by cutting the first connection.Only the opportunistic auto-signing step falls back now. Errors from an explicit
--attestationabort the push, and the request stays outside the rescue.Three smaller problems from the same review are fixed here too.
attest!returned only the tempfile path, so GC could unlink the bundle before it was read.ENV["GITHUB_ACTIONS"]was tested for truthiness instead of against"true". Attestation bundles were concatenated into a JSON array unvalidated. The test helper also scrubsGITHUB_ACTIONS, which was making CI spawn realgem exec sigstore-clisubprocesses.#9325