ci: skip the binary cache setup when credentials are unavailable - #236
Conversation
GitHub does not expose repository secrets to workflows triggered by a pull
request opened from a fork. The three inputs given to the cache action are
then empty strings, and the S3 store rejects every request:
post-build-hook: error: AWS error fetching 'nix-cache-info':
Unable to parse ExceptionName: InvalidRequest Message: Invalid Request.
error: program '.../post-build-hook' failed with exit code 123
The failure comes from the upload hook rather than from the build itself, so
the test suite reports a failure even though the derivations built fine. Every
pull request coming from a fork has been affected.
Guard the cache setup on the presence of the credentials. Internal branches and
pushes to main keep reading from and writing to the cache, while forks build
against cache.nixos.org alone and report the actual result of the test suite.
|
Hi there, are you using AI for your changes? |
Hello, I don't use AI for coding, especially when it comes to “sensitive data.” However, I do use it to review my changes in case there are any errors, and especially for the content of my pull requests. This is to follow the guidelines in contributing.md and avoid making mistakes. |
Can you point me where do you see credentials errors in the CI? |
|
The OVH endpoint answers an unsigned request with 1. The action never receives the inputs. Push on PR #235 from a fork, run 31264063350: The three secret-backed inputs are gone. The runner omits 2. No credentials file is written. Post-job cleanup on That line does not appear anywhere in the fork run, whose cleanup only removes the 3. The substituter is unusable as a result.
As a cross-check, an unauthenticated To be explicit about what this does not claim: I am not saying the suite goes green on forks, only that the job would report its actual result instead of failing in the upload hook. Since the hook kills the build before any test executes, I have no data on the real outcome. |
|
And I'm more than willing to discuss my point of view. |
|
Thank you for elaborating. You are right on (1) & (2). On (3), it's definitely weird, we had intended this cache to be public, so I will double check on the infrastructure side what's going on. Your curl example doesn't return access denied, it returns invalid request ("Not S3 Request"), if you write in vhost-style, it will return access denied. It would be helpful for our future engagements if you can disclose your use of AI and how do you use it and where. Some stuff registers as AI prose sometimes and AI prose makes it hard to understand whether it's you who made this analysis or a tool that assisted you. |
Yes, no problem at all. I'd be happy to explain this to you in any way you prefer—on GitHub, via email, or during a class or meeting. |
Problem
The
[Sécurix] Test suiteworkflow fails on every pull request opened from a fork, regardless of the change it carries:GitHub does not expose repository secrets to workflows triggered by
pull_requestfrom a fork, soNIX_SIGNING_PRIVATE_KEY,AWS_ACCESS_KEY_IDandAWS_SECRET_KEYare all empty strings. The S3 store then rejects every request, and the post-build hook takesnix-builddown with it.Two things make this hard to read from the outside:
Affected pull requests I could find: #228, #230, #231, #234, #235. Internal branches such as #233 are unaffected.
Change
Guard the cache setup on the presence of the credentials. The
secretscontext is not available in anif:condition, so the check goes through a job-levelenventry, which does have access to it.main: unchanged, the cache is still read from and written to;cache.nixos.orgalone, so the job reports the actual result of the test suite.Note
docs/manual/src/user/cache.mdalready recommends separating an untrusted CI cache from a trusted CD cache for projects accepting external contributions. That would be the more complete answer here, but it needs bucket and secret changes that only maintainers can make. This patch only stops fork pull requests from reporting a failure they did not cause, and does not stand in the way of that approach.Verification
Since
pull_requestruns the workflow definition carried by the pull request, this one exercises the change directly: the fork path of the condition is what its ownTest suiterun goes through. The runs are held inaction_requiredbecause the pull request touches.github/workflows, so approving them is the verification. A greenTest suitehere is the result the fix is meant to produce; the credentials-present path stays covered bymainafter merge.Checked before opening:
nix-cache-info, confirming the bucket is unusable as a substituter without credentials, and that no read-only fallback is available;secretscontext is unavailable injobs.<job_id>.steps.ifbut available injobs.<job_id>.env, and theenvcontext is available insteps.if, per the contexts reference;secret_keysis what drives the upload inzombiezen/setup-nix-cache-action, and the credentials are needed for downloads too, so skipping the whole step is the correct granularity;