From b522d30c77ad16987ad736ab5109bd0ab6382336 Mon Sep 17 00:00:00 2001 From: Jeff Widman Date: Fri, 7 Jul 2023 12:58:59 -0700 Subject: [PATCH] Stop exposing real account tokens in plaintext 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, :two: is no longer true... now that :dependabot: 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. --- .../composer/file_updater/lockfile_updater_spec.rb | 4 +++- composer/spec/dependabot/composer/update_checker_spec.rb | 8 ++++++-- hex/spec/dependabot/hex/update_checker_spec.rb | 8 ++++++-- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/composer/spec/dependabot/composer/file_updater/lockfile_updater_spec.rb b/composer/spec/dependabot/composer/file_updater/lockfile_updater_spec.rb index 15968eecd4c..1136a706381 100644 --- a/composer/spec/dependabot/composer/file_updater/lockfile_updater_spec.rb +++ b/composer/spec/dependabot/composer/file_updater/lockfile_updater_spec.rb @@ -455,6 +455,7 @@ end context "with good credentials" do + let(:gemfury_deploy_token) { ENV.fetch("GEMFURY_DEPLOY_TOKEN", nil) } let(:credentials) do [{ "type" => "git_source", @@ -464,12 +465,13 @@ }, { "type" => "composer_repository", "registry" => "php.fury.io", - "username" => "yFu9PBmw1HxNjFB818TW", # Throwaway account + "username" => gemfury_deploy_token, "password" => "" }] end it "has details of the updated item" do + skip("skipped because env var GEMFURY_DEPLOY_TOKEN is not set") if gemfury_deploy_token.nil? expect(updated_lockfile_content).to include("\"version\":\"2.2.0\"") end end diff --git a/composer/spec/dependabot/composer/update_checker_spec.rb b/composer/spec/dependabot/composer/update_checker_spec.rb index d0a8143033a..de143ccf3d9 100644 --- a/composer/spec/dependabot/composer/update_checker_spec.rb +++ b/composer/spec/dependabot/composer/update_checker_spec.rb @@ -330,6 +330,7 @@ end context "with good credentials" do + let(:gemfury_deploy_token) { ENV.fetch("GEMFURY_DEPLOY_TOKEN", nil) } let(:credentials) do [{ "type" => "git_source", @@ -339,12 +340,15 @@ }, { "type" => "composer_repository", "registry" => "php.fury.io", - "username" => "yFu9PBmw1HxNjFB818TW", # Throwaway account + "username" => gemfury_deploy_token, "password" => "" }] end - it { is_expected.to be >= Gem::Version.new("2.2.0") } + it "returns the expected version" do + skip("skipped because env var GEMFURY_DEPLOY_TOKEN is not set") if gemfury_deploy_token.nil? + is_expected.to be >= Gem::Version.new("2.2.0") + end end context "with bad credentials" do diff --git a/hex/spec/dependabot/hex/update_checker_spec.rb b/hex/spec/dependabot/hex/update_checker_spec.rb index e849fb6558a..d5e75957947 100644 --- a/hex/spec/dependabot/hex/update_checker_spec.rb +++ b/hex/spec/dependabot/hex/update_checker_spec.rb @@ -275,6 +275,7 @@ end context "with good credentials" do + let(:hex_pm_org_token) { ENV.fetch("HEX_PM_ORGANIZATION_TOKEN", nil) } let(:credentials) do [{ "type" => "git_source", @@ -284,11 +285,14 @@ }, { "type" => "hex_organization", "organization" => "dependabot", - "token" => "b6294cd1e1cf158e9f65ea6b02a9a1ec" + "token" => hex_pm_org_token }] end - it { is_expected.to eq(Gem::Version.new("1.1.0")) } + it "returns the expected version" do + skip("skipped because env var HEX_PM_ORGANIZATION_TOKEN is not set") if hex_pm_org_token.nil? + is_expected.to eq(Gem::Version.new("1.1.0")) + end end context "with bad credentials" do