Skip to content

Commit

Permalink
Respect secrets in debug output for downloads. (#101)
Browse files Browse the repository at this point in the history 
* Insert seam between parsing download config and creating DownloadManager, as discussed with Robert before and and avoid need to expose internal state of DownloadManager all over the vcpkg codebase.

* Respect secrets in debug output for downloads.

We leaked a SAS token, now expired, in CI, because we run downloads with debug turned on, and that printed the token. We shouldn't print the token even during debug, because that would let anyone who controls our command line, which is anyone, to print the token easily.

* Remove unused download_file function.

* Test that secrets are recorded.

* Nicole CR

Co-authored-by: nicole mazzuca <[email protected]>
  • Loading branch information
@BillyONeal @strega-nil
BillyONeal and strega-nil authored Jun 29, 2021
1 parent 57bd9d2 commit e8977e6
Show file tree
Hide file tree
Showing 7 changed files with 124 additions and 160 deletions.
35 changes: 12 additions & 23 deletions include/vcpkg/base/downloads.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
#include <vcpkg/base/optional.h>
#include <vcpkg/base/view.h>

#include <string>
#include <vector>

namespace vcpkg::Downloads
{
namespace details
Expand All @@ -20,47 +23,33 @@ namespace vcpkg::Downloads
}

void verify_downloaded_file_hash(const Files::Filesystem& fs,
const std::string& url,
const std::string& sanitized_url,
const fs::path& path,
const std::string& sha512);

// Returns url that was successfully downloaded from
std::string download_file(Files::Filesystem& fs,
View<std::string> urls,
View<std::string> headers,
const fs::path& download_path,
const std::string& sha512);

void download_file(Files::Filesystem& fs,
const std::string& url,
View<std::string> headers,
const fs::path& download_path,
const std::string& sha512);

View<std::string> azure_blob_headers();

std::vector<int> download_files(Files::Filesystem& fs, View<std::pair<std::string, fs::path>> url_pairs);
ExpectedS<int> put_file(const Files::Filesystem&, StringView url, View<std::string> headers, const fs::path& file);
std::vector<int> url_heads(View<std::string> urls, View<std::string> headers);
std::string replace_secrets(std::string input, View<std::string> secrets);

struct DownloadManagerConfig
{
Optional<std::string> m_read_url_template;
std::vector<std::string> m_read_headers;
Optional<std::string> m_write_url_template;
std::vector<std::string> m_write_headers;
std::vector<std::string> m_secrets;
bool m_block_origin = false;
};

// Handles downloading and uploading to a content addressable mirror
struct DownloadManager : private DownloadManagerConfig
struct DownloadManager
{
DownloadManager() = default;
explicit DownloadManager(Optional<std::string> read_url_template,
std::vector<std::string> read_headers,
Optional<std::string> write_url_template,
std::vector<std::string> write_headers,
bool block_origin);
explicit DownloadManager(const DownloadManagerConfig& config) : m_config(config) { }
explicit DownloadManager(DownloadManagerConfig&& config) : m_config(std::move(config)) { }

void download_file(Files::Filesystem& fs,
const std::string& url,
Expand All @@ -76,6 +65,7 @@ namespace vcpkg::Downloads
const fs::path& download_path,
const std::string& sha512) const;

// Returns url that was successfully downloaded from
std::string download_file(Files::Filesystem& fs,
View<std::string> urls,
View<std::string> headers,
Expand All @@ -86,8 +76,7 @@ namespace vcpkg::Downloads
const fs::path& path,
const std::string& sha512) const;

const DownloadManagerConfig& internal_get_config() const { return *this; }

bool block_origin() const { return m_block_origin; }
private:
DownloadManagerConfig m_config;
};
}
2 changes: 1 addition & 1 deletion include/vcpkg/binarycaching.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ namespace vcpkg
ExpectedS<std::unique_ptr<IBinaryProvider>> create_binary_provider_from_configs_pure(const std::string& env_string,
View<std::string> args);

ExpectedS<Downloads::DownloadManager> create_download_manager(const Optional<std::string>& arg);
ExpectedS<Downloads::DownloadManagerConfig> parse_download_configuration(const Optional<std::string>& arg);

std::string generate_nuget_packages_config(const Dependencies::ActionPlan& action);

Expand Down
123 changes: 64 additions & 59 deletions src/vcpkg-test/configparser.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,102 +371,107 @@ TEST_CASE ("BinaryConfigParser GCS provider", "[binaryconfigparser]")

TEST_CASE ("AssetConfigParser azurl provider", "[assetconfigparser]")
{
CHECK(create_download_manager({}));
CHECK(!create_download_manager("x-azurl"));
CHECK(!create_download_manager("x-azurl,"));
CHECK(create_download_manager("x-azurl,value"));
CHECK(create_download_manager("x-azurl,value,"));
CHECK(!create_download_manager("x-azurl,value,,"));
CHECK(!create_download_manager("x-azurl,value,,invalid"));
CHECK(create_download_manager("x-azurl,value,,read"));
CHECK(create_download_manager("x-azurl,value,,readwrite"));
CHECK(!create_download_manager("x-azurl,value,,readwrite,"));
CHECK(create_download_manager("x-azurl,https://abc/123,?foo"));
CHECK(create_download_manager("x-azurl,https://abc/123,foo"));
CHECK(create_download_manager("x-azurl,ftp://magic,none"));
CHECK(create_download_manager("x-azurl,ftp://magic,none"));
CHECK(parse_download_configuration({}));
CHECK(!parse_download_configuration("x-azurl"));
CHECK(!parse_download_configuration("x-azurl,"));
CHECK(parse_download_configuration("x-azurl,value"));
CHECK(parse_download_configuration("x-azurl,value,"));
CHECK(!parse_download_configuration("x-azurl,value,,"));
CHECK(!parse_download_configuration("x-azurl,value,,invalid"));
CHECK(parse_download_configuration("x-azurl,value,,read"));
CHECK(parse_download_configuration("x-azurl,value,,readwrite"));
CHECK(!parse_download_configuration("x-azurl,value,,readwrite,"));
CHECK(parse_download_configuration("x-azurl,https://abc/123,?foo"));
CHECK(parse_download_configuration("x-azurl,https://abc/123,foo"));
CHECK(parse_download_configuration("x-azurl,ftp://magic,none"));
CHECK(parse_download_configuration("x-azurl,ftp://magic,none"));

{
Downloads::DownloadManager empty;
CHECK(empty.internal_get_config().m_write_headers.empty());
CHECK(empty.internal_get_config().m_read_headers.empty());
Downloads::DownloadManagerConfig empty;
CHECK(empty.m_write_headers.empty());
CHECK(empty.m_read_headers.empty());
}
{
Downloads::DownloadManager dm = Test::unwrap(create_download_manager("x-azurl,https://abc/123,foo"));
CHECK(dm.internal_get_config().m_read_url_template == "https://abc/123/<SHA>?foo");
CHECK(dm.internal_get_config().m_read_headers.empty());
CHECK(dm.internal_get_config().m_write_url_template == nullopt);
Downloads::DownloadManagerConfig dm = Test::unwrap(parse_download_configuration("x-azurl,https://abc/123,foo"));
CHECK(dm.m_read_url_template == "https://abc/123/<SHA>?foo");
CHECK(dm.m_read_headers.empty());
CHECK(dm.m_write_url_template == nullopt);
}
{
Downloads::DownloadManager dm = Test::unwrap(create_download_manager("x-azurl,https://abc/123/,foo"));
CHECK(dm.internal_get_config().m_read_url_template == "https://abc/123/<SHA>?foo");
CHECK(dm.internal_get_config().m_read_headers.empty());
CHECK(dm.internal_get_config().m_write_url_template == nullopt);
Downloads::DownloadManagerConfig dm =
Test::unwrap(parse_download_configuration("x-azurl,https://abc/123/,foo"));
CHECK(dm.m_read_url_template == "https://abc/123/<SHA>?foo");
CHECK(dm.m_read_headers.empty());
CHECK(dm.m_write_url_template == nullopt);
CHECK(dm.m_secrets == std::vector<std::string>{"foo"});
}
{
Downloads::DownloadManager dm = Test::unwrap(create_download_manager("x-azurl,https://abc/123,?foo"));
CHECK(dm.internal_get_config().m_read_url_template == "https://abc/123/<SHA>?foo");
CHECK(dm.internal_get_config().m_read_headers.empty());
CHECK(dm.internal_get_config().m_write_url_template == nullopt);
Downloads::DownloadManagerConfig dm =
Test::unwrap(parse_download_configuration("x-azurl,https://abc/123,?foo"));
CHECK(dm.m_read_url_template == "https://abc/123/<SHA>?foo");
CHECK(dm.m_read_headers.empty());
CHECK(dm.m_write_url_template == nullopt);
CHECK(dm.m_secrets == std::vector<std::string>{"?foo"});
}
{
Downloads::DownloadManager dm = Test::unwrap(create_download_manager("x-azurl,https://abc/123"));
CHECK(dm.internal_get_config().m_read_url_template == "https://abc/123/<SHA>");
CHECK(dm.internal_get_config().m_read_headers.empty());
CHECK(dm.internal_get_config().m_write_url_template == nullopt);
Downloads::DownloadManagerConfig dm = Test::unwrap(parse_download_configuration("x-azurl,https://abc/123"));
CHECK(dm.m_read_url_template == "https://abc/123/<SHA>");
CHECK(dm.m_read_headers.empty());
CHECK(dm.m_write_url_template == nullopt);
}
{
Downloads::DownloadManager dm = Test::unwrap(create_download_manager("x-azurl,https://abc/123,,readwrite"));
CHECK(dm.internal_get_config().m_read_url_template == "https://abc/123/<SHA>");
CHECK(dm.internal_get_config().m_read_headers.empty());
CHECK(dm.internal_get_config().m_write_url_template == "https://abc/123/<SHA>");
Test::check_ranges(dm.internal_get_config().m_write_headers, Downloads::azure_blob_headers());
Downloads::DownloadManagerConfig dm =
Test::unwrap(parse_download_configuration("x-azurl,https://abc/123,,readwrite"));
CHECK(dm.m_read_url_template == "https://abc/123/<SHA>");
CHECK(dm.m_read_headers.empty());
CHECK(dm.m_write_url_template == "https://abc/123/<SHA>");
Test::check_ranges(dm.m_write_headers, Downloads::azure_blob_headers());
}
{
Downloads::DownloadManager dm = Test::unwrap(create_download_manager("x-azurl,https://abc/123,foo,readwrite"));
CHECK(dm.internal_get_config().m_read_url_template == "https://abc/123/<SHA>?foo");
CHECK(dm.internal_get_config().m_read_headers.empty());
CHECK(dm.internal_get_config().m_write_url_template == "https://abc/123/<SHA>?foo");
Test::check_ranges(dm.internal_get_config().m_write_headers, Downloads::azure_blob_headers());
Downloads::DownloadManagerConfig dm =
Test::unwrap(parse_download_configuration("x-azurl,https://abc/123,foo,readwrite"));
CHECK(dm.m_read_url_template == "https://abc/123/<SHA>?foo");
CHECK(dm.m_read_headers.empty());
CHECK(dm.m_write_url_template == "https://abc/123/<SHA>?foo");
Test::check_ranges(dm.m_write_headers, Downloads::azure_blob_headers());
CHECK(dm.m_secrets == std::vector<std::string>{"foo"});
}
}

TEST_CASE ("AssetConfigParser clear provider", "[assetconfigparser]")
{
CHECK(create_download_manager("clear"));
CHECK(!create_download_manager("clear,"));
CHECK(create_download_manager("x-azurl,value;clear"));
CHECK(parse_download_configuration("clear"));
CHECK(!parse_download_configuration("clear,"));
CHECK(parse_download_configuration("x-azurl,value;clear"));
auto value_or = [](auto o, auto v) {
if (o)
return std::move(*o.get());
else
return std::move(v);
};

Downloads::DownloadManager empty;
Downloads::DownloadManagerConfig empty;

CHECK(value_or(create_download_manager("x-azurl,https://abc/123,foo;clear"), empty)
.internal_get_config()
.m_read_url_template == nullopt);
CHECK(value_or(create_download_manager("clear;x-azurl,https://abc/123/,foo"), empty)
.internal_get_config()
.m_read_url_template == "https://abc/123/<SHA>?foo");
CHECK(value_or(parse_download_configuration("x-azurl,https://abc/123,foo;clear"), empty).m_read_url_template ==
nullopt);
CHECK(value_or(parse_download_configuration("clear;x-azurl,https://abc/123/,foo"), empty).m_read_url_template ==
"https://abc/123/<SHA>?foo");
}

TEST_CASE ("AssetConfigParser x-block-origin provider", "[assetconfigparser]")
{
CHECK(create_download_manager("x-block-origin"));
CHECK(!create_download_manager("x-block-origin,"));
CHECK(parse_download_configuration("x-block-origin"));
CHECK(!parse_download_configuration("x-block-origin,"));
auto value_or = [](auto o, auto v) {
if (o)
return std::move(*o.get());
else
return std::move(v);
};

Downloads::DownloadManager empty;
Downloads::DownloadManagerConfig empty;

CHECK(!value_or(create_download_manager({}), empty).block_origin());
CHECK(value_or(create_download_manager("x-block-origin"), empty).block_origin());
CHECK(!value_or(create_download_manager("x-block-origin;clear"), empty).block_origin());
CHECK(!value_or(parse_download_configuration({}), empty).m_block_origin);
CHECK(value_or(parse_download_configuration("x-block-origin"), empty).m_block_origin);
CHECK(!value_or(parse_download_configuration("x-block-origin;clear"), empty).m_block_origin);
}
Loading

0 comments on commit e8977e6

Please sign in to comment.