Repository navigation
Conversation
Gemfury registries (pypi.fury.io, npm.fury.io, ...) 302-redirect package downloads to a pre-signed URL on gemfury.s3-accelerate.dualstack.amazonaws.com. That host appears in no credential field, so the egress allowlist blocks the redirect and the update fails even though the authenticated registry request succeeded. Derive the bucket per job from a *.fury.io credential host, alongside the existing ECR starport bucket, instead of adding it to the static defaults: the bucket is shared by all Gemfury accounts, so a static entry would open every tenant's content to every job. The derived host is a constant, so a crafted credential cannot widen it.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The narrowly scoped implementation is consistent with existing dynamic-host handling and has comprehensive regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Adds per-job access to Gemfury’s shared download bucket without widening the global egress allowlist.
Changes:
- Derives the exact Gemfury S3 host from
*.fury.iocredentials. - Adds positive, isolation, lookalike, and deduplication tests.
| File | Description |
|---|---|
internal/handlers/egress_dynamic_hosts.go |
Adds conditional Gemfury redirect-host derivation. |
internal/handlers/egress_dynamic_hosts_test.go |
Verifies exact matching and credential scoping. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Author
|
superseded by #288 |
Contributor
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 are you trying to accomplish?
Since the egress allowlist started enforcing, every Dependabot update that downloads a package from a Gemfury registry fails. The authenticated registry request succeeds, but the download is blocked:
Every Gemfury registry endpoint 302-redirects package downloads to a short-lived pre-signed URL on one Gemfury-owned S3 bucket,
gemfury.s3-accelerate.dualstack.amazonaws.com. That host appears in no credential field, socredentialHostsnever adds it. I checked this directly against bothpypi.fury.io(wheel) andnpm.fury.io(tarball): both redirect to that exact host. Older files are sometimes served inline with a 200, which is why the failure shows up per package rather than per registry.This PR adds the bucket to
registryRedirectHosts, next to the ECR starport bucket. It's added only for jobs that have a credential for a*.fury.iohost.Anything you want to highlight for special attention from reviewers?
Why not the static defaults: the bucket is shared by all Gemfury accounts, with the account in the URL path.
add-egress-allowlist-domainrules out shared multi-tenant hosts like that for the static defaults, because allowing the host would expose every tenant's content to every job. Deriving it per job keeps it closed for everyone who doesn't already use Gemfury. This is the same reasoning as the existing ECR case.Why the derivation is safe: the derived host is a constant. Nothing from the credential is interpolated into it, so a crafted credential can't widen it. It's also matched exactly, like all dynamic hosts. The trigger is
strings.HasSuffix(host, ".fury.io"). That covers all Gemfury endpoints (pypi.,npm.,npm-proxy.,gem., ...) without keeping a list that would go stale. The worst a lookalike credential could do is open this one fixed Gemfury bucket for its own job.Ownership evidence:
pypi.fury.ioandnpm.fury.iothemselves issue the redirect to this host.CN=*.s3-accelerate.amazonaws.com, issuerAmazon RSA 2048 M01).gemfury. S3 bucket names are globally unique.Compared with #286: #286 is the smaller change and follows the JFrog shared S3 buckets precedent. This PR follows the rule in
add-egress-allowlist-domainfor multi-tenant hosts more strictly, at the cost of a little code inregistryRedirectHosts.How will you know you've accomplished your goal?
New tests in
internal/handlers/egress_dynamic_hosts_test.go:TestRegistryRedirectHosts_GemfuryStorageDerived:pypi.fury.iocredential, both the registry and the bucket are allowed.evil.gemfury.s3-accelerate...)gemfuryx.s3-accelerate...)gemfury.s3.amazonaws.com)attacker.s3-accelerate.dualstack.amazonaws.com)TestRegistryRedirectHosts_OnlyGemfuryHosts:pypi.,npm.,npm-proxy.andgem.fury.io.fury.io,pypi.fury.io.evil.com,pypifury.ioorevil.com.TestRegistryRedirectHosts_NotAddedWithoutGemfuryCredential: a job without a Gemfury credential can't reach the bucket.I checked that the tests catch a regression: loosening the matcher to
strings.Contains(h, "fury.io")makes the lookalike cases fail.script/test(-race -shuffle=on -count=2) passes, and so dogo vetandgofmt -l.Checklist