New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stop exposing real account tokens in plaintext #7533
Merged
Merged
Conversation
This file contains 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
github-actions
bot
added
L: php:composer
Issues and code for Composer
L: elixir:hex
Elixir packages via hex
labels
Jul 7, 2023
mctofu
approved these changes
Jul 7, 2023
bdragon
approved these changes
Jul 7, 2023
jeffwidman
force-pushed
the
stop-putting-account-tokens-in-the-clear
branch
from
July 7, 2023 20:29
6781cf6
to
30e6744
Compare
After I rotated the Gemfury/Hex.PM tokens, these unit tests started failing, which made us realize we've been exposing the account tokens in the clear. Historically this was fine because: 1. These private registries were used solely for test accounts, it didn't matter if they were compromised. 1. Dependabot was a startup manned by a small team, so everyone knew these registries were publicly exposed and shouldn't be re-used for anything private. However, 2️⃣ is no longer true... now that is part of GitHub, the team has grown so there's a risk that someone could upload something that should stay private to these orgs. Unfortunately, neither Gemfury nor Hex.PM support limiting the scope of a token to particular package(s). So while they're read-only, they still expose everything on the account. After discussing further, we decided that we no longer want to expose these tokens, so flipping them to use Env Vars. Furthermore, these tests exercise private registry auth code in `dependabot-core`, but we don't use that in production at GitHub. Instead, we use a [private credential proxy](https://github.com/dependabot/dependabot-core#private-registry-credential-management). Long term, we've batted around the idea of open sourcing that, in which case we'd remove all the private registry auth code from `dependabot-core`. So this PR removes the plain text tokens (and I will also delete them from the registry side to ensure they aren't usable). And wires up env var support so that if we _do_ need to run these tests, we can still do that. But it does _not_ go to the trouble of wiring up setting the env var using GitHub actions because there is a reasonable chance this code will all be deleted in the not-too-distant future. And until then, it's not even used by GitHub production, only by folks running `dependabot-core` standalone. So the impact if this does break is fairly small and in that case we'd happily accept a contribution to fix things, but we're unlikely to invest the engineering efforts ourselves to fix it.
jeffwidman
force-pushed
the
stop-putting-account-tokens-in-the-clear
branch
from
July 7, 2023 20:34
30e6744
to
b522d30
Compare
I have revoked the tokens that were previously exposed here in plain text. |
brettfo
pushed a commit
to brettfo/dependabot-core
that referenced
this pull request
Oct 11, 2023
After I rotated the Gemfury/Hex.PM tokens, these unit tests started failing, which made us realize we've been exposing the account tokens in the clear. Historically this was fine because: 1. These private registries were used solely for test accounts, it didn't matter if they were compromised. 1. Dependabot was a startup manned by a small team, so everyone knew these registries were publicly exposed and shouldn't be re-used for anything private. However, 2️⃣ is no longer true... now that is part of GitHub, the team has grown so there's a risk that someone could upload something that should stay private to these orgs. Unfortunately, neither Gemfury nor Hex.PM support limiting the scope of a token to particular package(s). So while they're read-only, they still expose everything on the account. After discussing further, we decided that we no longer want to expose these tokens, so flipping them to use Env Vars. Furthermore, these tests exercise private registry auth code in `dependabot-core`, but we don't use that in production at GitHub. Instead, we use a [private credential proxy](https://github.com/dependabot/dependabot-core#private-registry-credential-management). Long term, we've batted around the idea of open sourcing that, in which case we'd remove all the private registry auth code from `dependabot-core`. So this PR removes the plain text tokens (and I will also delete them from the registry side to ensure they aren't usable). And wires up env var support so that if we _do_ need to run these tests, we can still do that. But it does _not_ go to the trouble of wiring up setting the env var using GitHub actions because there is a reasonable chance this code will all be deleted in the not-too-distant future. And until then, it's not even used by GitHub production, only by folks running `dependabot-core` standalone. So the impact if this does break is fairly small and in that case we'd happily accept a contribution to fix things, but we're unlikely to invest the engineering efforts ourselves to fix it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
EE
Engineering Efficiency
L: elixir:hex
Elixir packages via hex
L: php:composer
Issues and code for Composer
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.
After I rotated the Gemfury/Hex.PM tokens, these unit tests started failing, which made us realize we've been exposing the account tokens in the clear.
Historically this was fine because:
However, 2️⃣ is no longer true... now that is part of GitHub, the team has grown so there's a risk that someone could upload something that should stay private to these orgs.
Unfortunately, neither Gemfury nor Hex.PM support limiting the scope of a token to particular package(s). So while they're read-only, they still expose everything on the account.
After discussing further, we decided that we no longer want to expose these tokens, so flipping them to use Env Vars.
Furthermore, these tests exercise private registry auth code in
dependabot-core
, but we don't use that in production at GitHub. Instead, we use a private credentialproxy.
Long term, we've batted around the idea of open sourcing that, in which case we'd remove all the private registry auth code from
dependabot-core
.So this PR removes the plain text tokens (and I will also delete them from the registry side to ensure they aren't usable). And wires up env var support so that if we do need to run these tests, we can still do that. But it does not go to the trouble of wiring up setting the env var using GitHub actions because there is a reasonable chance this code will all be deleted in the not-too-distant future. And until then, it's not even used by GitHub production, only by folks running
dependabot-core
standalone. So the impact if this does break is fairly small and in that case we'd happily accept a contribution to fix things, but we're unlikely to invest the engineering efforts ourselves to fix it.This supersedes the approach I took in:
hex.pm/orgs/dependabot
token #7532