Skip to content
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

Auth token #248

Merged
merged 11 commits into from
Feb 22, 2024
Merged
24 changes: 2 additions & 22 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ wat = "1.0.85"
wasmprinter = "0.2.78"
keyring = "2.3.0"
dialoguer = "0.11.0"
rpassword = "7.3.1"
itertools = "0.12.1"
secrecy= { workspace = true }

[dev-dependencies]
reqwest = { workspace = true }
Expand Down
1 change: 1 addition & 0 deletions crates/client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ wasm-encoder.workspace = true
wasmprinter = "0.2.75"
sha256 = "1.4.0"
ptree = { workspace = true }
secrecy= { workspace = true }

[target.'cfg(windows)'.dependencies.windows-sys]
version = "0.52"
Expand Down
44 changes: 35 additions & 9 deletions crates/client/src/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use reqwest::{
header::{HeaderMap, HeaderName, HeaderValue},
Body, IntoUrl, Method, RequestBuilder, Response, StatusCode,
};
use secrecy::{ExposeSecret, Secret};
use serde::de::DeserializeOwned;
use std::{borrow::Cow, collections::HashMap};
use thiserror::Error;
Expand Down Expand Up @@ -157,12 +158,10 @@ async fn into_result<T: DeserializeOwned, E: DeserializeOwned + Into<ClientError
}

trait WithWargHeader {
type Client;
fn warg_header(self, registry_header: &Option<RegistryDomain>) -> Result<RequestBuilder>;
}

impl WithWargHeader for RequestBuilder {
type Client = Client;
fn warg_header(self, registry_header: &Option<RegistryDomain>) -> Result<RequestBuilder> {
if let Some(reg) = registry_header {
Ok(self.header(REGISTRY_HEADER_NAME, HeaderValue::try_from(reg.clone())?))
Expand All @@ -172,25 +171,46 @@ impl WithWargHeader for RequestBuilder {
}
}

trait WithAuth {
fn auth(self, auth_token: &Option<Secret<String>>) -> RequestBuilder;
}

impl WithAuth for RequestBuilder {
fn auth(self, auth_token: &Option<Secret<String>>) -> reqwest::RequestBuilder {
if let Some(tok) = auth_token {
self.bearer_auth(tok.expose_secret())
} else {
self
}
}
}

/// Represents a Warg API client for communicating with
/// a Warg registry server.
pub struct Client {
url: RegistryUrl,
client: reqwest::Client,
warg_registry_header: Option<RegistryDomain>,
auth_token: Option<Secret<String>>,
}

impl Client {
/// Creates a new API client with the given URL.
pub fn new(url: impl IntoUrl) -> Result<Self> {
pub fn new(url: impl IntoUrl, auth_token: Option<Secret<String>>) -> Result<Self> {
let url = RegistryUrl::new(url)?;
Ok(Self {
url,
client: reqwest::Client::new(),
warg_registry_header: None,
auth_token,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend using https://docs.rs/reqwest/0.11.24/reqwest/struct.ClientBuilder.html#method.default_headers to replace all the changes in this file. The first example in the method docs there is basically what you would need (plus prepending Bearer to the value).

Copy link
Collaborator

@calvinrp calvinrp Feb 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a tradeoff on using the default_headers for the Client, where you might accidentally provide the auth header on a request that shouldn't have it. For instance, on the upload and download requests which may be URLs on a different host server that shouldn't have access to the auth secret.

It might be safer to forget the auth header on a request than to accidentally leak a secret. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think I agree with Calvin here... though if there's something in the middle, the default headers is definitely a smaller change. Went ahead and pushed the other suggestions.

})
}

/// Gets auth token
pub fn auth_token(&self) -> &Option<Secret<String>> {
&self.auth_token
}

/// Gets the URL of the API client.
pub fn url(&self) -> &RegistryUrl {
&self.url
Expand All @@ -206,6 +226,7 @@ impl Client {
self.client
.get(url)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.send()
.await?,
)
Expand All @@ -226,6 +247,7 @@ impl Client {
self.client
.get(url)
.header(registry_header, header_val)
.auth(self.auth_token())
.send()
.await?,
)
Expand All @@ -248,6 +270,7 @@ impl Client {
.post(url)
.json(&request)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.send()
.await?;
into_result::<_, MonitorError>(response).await
Expand All @@ -265,6 +288,7 @@ impl Client {
.post(&url)
.json(&request)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.send()
.await?;

Expand All @@ -291,6 +315,7 @@ impl Client {
.client
.post(url)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.json(&request)
.send()
.await?;
Expand All @@ -306,6 +331,7 @@ impl Client {
self.client
.get(url)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.send()
.await?,
)
Expand All @@ -329,6 +355,7 @@ impl Client {
.post(url)
.json(&request)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.send()
.await?;
into_result::<_, PackageError>(response).await
Expand All @@ -347,6 +374,7 @@ impl Client {
self.client
.get(url)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.send()
.await?,
)
Expand All @@ -365,6 +393,7 @@ impl Client {
self.client
.get(url)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.send()
.await?,
)
Expand All @@ -389,12 +418,7 @@ impl Client {

tracing::debug!("downloading content `{digest}` from `{url}`");

let response = self
.client
.get(url)
.warg_header(self.get_warg_registry())?
.send()
.await?;
let response = self.client.get(url).send().await?;
if !response.status().is_success() {
tracing::debug!(
"failed to download content `{digest}` from `{url}`: {status}",
Expand Down Expand Up @@ -434,6 +458,7 @@ impl Client {
.post(url)
.json(&request)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.send()
.await?,
)
Expand All @@ -455,6 +480,7 @@ impl Client {
.post(url)
.json(&request)
.warg_header(self.get_warg_registry())?
.auth(self.auth_token())
.send()
.await?,
)
Expand Down
23 changes: 19 additions & 4 deletions crates/client/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use crate::storage::PackageInfo;
use anyhow::{anyhow, Context, Result};
use reqwest::header::HeaderValue;
use reqwest::{Body, IntoUrl};
use secrecy::Secret;
use semver::{Version, VersionReq};
use std::cmp::Ordering;
use std::fs;
Expand Down Expand Up @@ -63,12 +64,19 @@ where
impl<R: RegistryStorage, C: ContentStorage, N: NamespaceMapStorage> Client<R, C, N> {
/// Creates a new client for the given URL, registry storage, and
/// content storage.
pub fn new(url: impl IntoUrl, registry: R, content: C, namespace_map: N) -> ClientResult<Self> {
pub fn new(
url: impl IntoUrl,
registry: R,
content: C,
namespace_map: N,
auth_token: Option<Secret<String>>,
) -> ClientResult<Self> {
let api = api::Client::new(url, auth_token)?;
Ok(Self {
registry,
content,
namespace_map,
api: api::Client::new(url)?,
api,
})
}

Expand Down Expand Up @@ -138,7 +146,7 @@ impl<R: RegistryStorage, C: ContentStorage, N: NamespaceMapStorage> Client<R, C,
if let Ok(Some(nm)) = namespace_state {
if let warg_protocol::operator::NamespaceState::Imported { registry } = nm {
self.api
.set_warg_registry(Some(RegistryDomain::from_str(registry)?));
.set_warg_registry(Some(RegistryDomain::from_str(&registry)?));
}
true
} else {
Expand Down Expand Up @@ -917,6 +925,7 @@ impl FileSystemClient {
pub fn try_new_with_config(
url: Option<&str>,
config: &Config,
auth_token: Option<Secret<String>>,
) -> Result<StorageLockResult<Self>, ClientError> {
let StoragePaths {
registry_url: url,
Expand All @@ -940,6 +949,7 @@ impl FileSystemClient {
packages,
content,
namespace_map,
auth_token,
)?))
}

Expand All @@ -949,7 +959,11 @@ impl FileSystemClient {
/// URL, an error is returned.
///
/// This method blocks if storage locks cannot be acquired.
pub fn new_with_config(url: Option<&str>, config: &Config) -> Result<Self, ClientError> {
pub fn new_with_config(
url: Option<&str>,
config: &Config,
auth_token: Option<Secret<String>>,
) -> Result<Self, ClientError> {
let StoragePaths {
registry_url,
registries_dir,
Expand All @@ -961,6 +975,7 @@ impl FileSystemClient {
FileSystemRegistryStorage::lock(registries_dir)?,
FileSystemContentStorage::lock(content_dir)?,
FileSystemNamespaceMapStorage::new(namespace_map_path),
auth_token,
)
}
}
Expand Down
9 changes: 8 additions & 1 deletion src/bin/warg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ use std::process::exit;
use tracing_subscriber::EnvFilter;
use warg_cli::commands::{
BundleCommand, ClearCommand, ConfigCommand, DependenciesCommand, DownloadCommand, InfoCommand,
KeyCommand, LockCommand, PublishCommand, ResetCommand, Retry, UpdateCommand,
KeyCommand, LockCommand, LoginCommand, LogoutCommand, PublishCommand, ResetCommand, Retry,
UpdateCommand,
};
use warg_client::ClientError;

Expand Down Expand Up @@ -35,6 +36,8 @@ enum WargCli {
Publish(PublishCommand),
Reset(ResetCommand),
Clear(ClearCommand),
Login(LoginCommand),
Logout(LogoutCommand),
}

#[tokio::main]
Expand All @@ -55,6 +58,8 @@ async fn main() -> Result<()> {
WargCli::Publish(cmd) => cmd.exec(None).await,
WargCli::Reset(cmd) => cmd.exec().await,
WargCli::Clear(cmd) => cmd.exec().await,
WargCli::Login(cmd) => cmd.exec().await,
WargCli::Logout(cmd) => cmd.exec().await,
} {
if let Some(e) = e.downcast_ref::<ClientError>() {
describe_client_error_or_retry(e).await?;
Expand Down Expand Up @@ -142,6 +147,8 @@ async fn describe_client_error_or_retry(e: &ClientError) -> Result<()> {
}
WargCli::Reset(cmd) => cmd.exec().await,
WargCli::Clear(cmd) => cmd.exec().await,
WargCli::Login(cmd) => cmd.exec().await,
WargCli::Logout(cmd) => cmd.exec().await,
} {
if let Some(e) = e.downcast_ref::<ClientError>() {
describe_client_error(e).await?;
Expand Down
Loading
Loading