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.

Sign up for GitHub

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

Environment Variable for Authentication Token Refactor #75

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Environment Variable for Authentication Token Refactor #75

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9620519
Add EnvVar as a parameter
amanjeev Jan 14, 2025
286eb6c
Refactor to add error if env var is not utf-8 encoded.
amanjeev Jan 22, 2025
2fdadbf
Better naming for test
amanjeev Feb 5, 2025
f485b1f
Use Default for EnvVars instead of normal impl.
amanjeev Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions crates/criticalup-cli/src/commands/auth_remove.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@

use crate::errors::Error;
use crate::Context;
use criticalup_core::state::State;
use criticalup_core::state::{EnvVars, State};

pub(crate) async fn run(ctx: &Context) -> Result<(), Error> {
let state = State::load(&ctx.config).await?;

if state.authentication_token(None).await.is_some() {
let env_vars = EnvVars::read().await?;

if state.authentication_token(None, &env_vars).await.is_some() {
state.set_authentication_token(None);
state.persist().await?;
}
Expand Down
5 changes: 4 additions & 1 deletion crates/criticalup-core/src/download_server_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

use crate::config::Config;
use crate::errors::{DownloadServerError, Error};
use crate::state;
use crate::state::State;
use criticaltrust::keys::PublicKey;
use criticaltrust::manifests::ReleaseManifest;
Expand Down Expand Up @@ -124,9 +125,11 @@ impl DownloadServerClient {
None
};

let env_vars = state::EnvVars::read().await?;

let header = self
.state
.authentication_token(path_to_token_file)
.authentication_token(path_to_token_file, &env_vars)
Copy link
Member

Choose a reason for hiding this comment

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

Noted elsewhere but this is the only usage of the token file path in our codebase, it creates some very confusing logic and perhaps we should refactor it out and have EnvVars intelligently detect if it's inside docker and look at /run/secrets when it's being built.

.await
.as_ref()
.and_then(|token| HeaderValue::from_str(&format!("Bearer {}", token.unseal())).ok());
Expand Down
8 changes: 8 additions & 0 deletions crates/criticalup-core/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
use criticaltrust::Error as TrustError;
use reqwest::Error as ReqError;
use reqwest::StatusCode;
use std::env::VarError;
use std::path::PathBuf;

/// We're using a custom error enum instead of `Box<dyn Error>` or one of the crates providing a
Expand Down Expand Up @@ -95,6 +96,13 @@ pub enum Error {

#[error("Failed to load keys into keychain.")]
KeychainLoadingFailed(#[source] criticaltrust::Error),

#[error("Environment variable '{}' is not UTF-8 encoded.", name)]
EnvVarNotUtf8 {
name: String,
#[source]
kind: VarError,
},
}

#[derive(Debug, thiserror::Error)]
Expand Down
104 changes: 91 additions & 13 deletions crates/criticalup-core/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

use std::cell::{Ref, RefCell};
use std::collections::{BTreeMap, BTreeSet};
use std::env::VarError;
use std::path::{Path, PathBuf};
use std::rc::Rc;

Expand Down Expand Up @@ -60,20 +61,21 @@ impl State {
pub async fn authentication_token(
&self,
token_path: Option<&str>,
env_vars: &EnvVars,
Copy link
Member

@Hoverbear Hoverbear Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This kind of muddies the waters on what State is. (In fact, having token_path here also muddies it, maybe we should consider removing the sole use of token_path here and find more clear logic).

At least from my reading, State represents what is on disk, yet now state.authentication_token(...) might return me the value from an env var instead of my state file (... or some random file path I passed in!?), and it requires me to pass in EnvVars.

Seems to me the logic around getting what the canonical token is should be something like:

let env = EnvVars::read();
let state = State::load();

// Determine the canonical auth token
// Having the env var set is higher priority than the on-disk
let auth_token = if let Some(env_token) = env.authentication_token() {
  env_token
} else if let Some(state_token) = state.authentication_token() {
  state_token
}

println!("The token is {}", auth_token);

Copy link
Member Author

@amanjeev amanjeev Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, this is muddying for sure. This has an issue that the user will have to do this (call EnvVars and State) at the call site. Are you suggesting that a separate module for Authentication token that can do this for the user?

Copy link
Member

Choose a reason for hiding this comment

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

I guess what I'm saying is having a State module which represents the on-disk state, which has some authentication_token function that accepts a token_path and an env_vars feels silly.

If I'm using criticalup-core as a library (which only we are right now), if I know I have token_path, I should simply read that file path instead of asking the State::authentication_token token path to do that for me.

Similarly, if I am using criticalup-core as a library and I have an environment variable holding that token, why should I have to ask State::authentication_token? I already know the token, I can just read it from the environment variable.

tldr: The State::authentication_token() function should produce the token present in the state, regardless of what might be present in the environment or otherwise passed to the program somehow -- I'm asking the State struct, after all.

) -> Option<AuthenticationToken> {
match token_path {
Some(token_path) => {
let token_path = std::path::Path::new(token_path);
if token_path.exists() {
match tokio::fs::read_to_string(token_path).await {
Ok(token) => Some(AuthenticationToken(token.to_string().trim().into())),
Err(_) => self.authentication_token_inner(),
Err(_) => self.authentication_token_inner(env_vars),
}
} else {
self.authentication_token_inner()
self.authentication_token_inner(env_vars)
}
}
None => self.authentication_token_inner(),
None => self.authentication_token_inner(env_vars),
}
}

Expand All @@ -82,10 +84,10 @@ impl State {
/// Attempts to read from:
/// 1. The `CRITICALUP_TOKEN_ENV_VAR_NAME` environment
/// 2. The state
fn authentication_token_inner(&self) -> Option<AuthenticationToken> {
match std::env::var(CRITICALUP_TOKEN_ENV_VAR_NAME) {
Ok(token_from_env) => Some(AuthenticationToken(token_from_env)),
Err(_) => {
fn authentication_token_inner(&self, env_vars: &EnvVars) -> Option<AuthenticationToken> {
match &env_vars.criticalup_token {
Some(token) => Some(AuthenticationToken(token.to_string())),
None => {
let borrowed = self.inner.borrow();
borrowed.repr.authentication_token.clone()
}
Expand Down Expand Up @@ -374,6 +376,34 @@ impl StateInstallation {
}
}

#[derive(Default)]
amanjeev marked this conversation as resolved.
Show resolved Hide resolved
pub struct EnvVars {
Copy link
Member

Choose a reason for hiding this comment

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

Paths does a similar thing (configurs some values based on env vars) here:

criticalup/crates/criticalup-core/src/config/paths.rs

Lines 12 to 21 in 2363ed2

pub struct Paths {
pub(crate) state_file: PathBuf,
pub proxies_dir: PathBuf,
pub installation_dir: PathBuf,
pub cache_dir: PathBuf,
#[cfg(test)]
pub(crate) root: PathBuf,
}

Specifically here:

criticalup/crates/criticalup-core/src/config/paths.rs

Lines 57 to 63 in 2363ed2

fn find_root(whitelabel: &WhitelabelConfig) -> Option<PathBuf> {
match env::var_os("CRITICALUP_ROOT") {
Some(val) if val.is_empty() => platform_specific_root(whitelabel),
Some(val) => Some(PathBuf::from(val)),
None => platform_specific_root(whitelabel),
}
}

As well as:

criticalup/crates/criticalup-core/src/config/paths.rs

Lines 69 to 75 in 2363ed2

fn find_cache_dir(whitelabel: &WhitelabelConfig) -> Option<PathBuf> {
match env::var_os("CRITICALUP_CACHE_DIR") {
Some(val) if val.is_empty() => platform_specific_cache_dir(whitelabel),
Some(val) => Some(PathBuf::from(val)),
None => platform_specific_cache_dir(whitelabel),
}
}

This makes me wonder if Paths should be configured with the instance of these EnvVars instead, or if the root/cache_dir passed to it should not be options, but instead PathBufs acquired from EnvVars somehow. After all, CRITICALUP_CACHE_DIR is indeed an env var, and if we ever want to test that feature having it pull from env::var will be very problematic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it makes sense to me to include these in EnvVars.

criticalup_token: Option<String>,
}

impl EnvVars {
pub async fn read() -> Result<Self, Error> {
let mut env_vars = EnvVars::default();
match std::env::var(CRITICALUP_TOKEN_ENV_VAR_NAME) {
Ok(value) => {
if !value.is_empty() {
env_vars.criticalup_token = Some(value);
}
}
Err(var_err) => {
if let VarError::NotUnicode(err) = var_err {
return Err(Error::EnvVarNotUtf8 {
name: CRITICALUP_TOKEN_ENV_VAR_NAME.to_string(),
kind: VarError::NotUnicode(err),
});
}
}
}

Ok(env_vars)
}
}

#[derive(Clone, Serialize, Deserialize)]
#[cfg_attr(test, derive(PartialEq, Eq))]
pub struct AuthenticationToken(String);
Expand Down Expand Up @@ -419,6 +449,7 @@ impl std::fmt::Debug for AuthenticationToken {

#[cfg(test)]
mod tests {
use crate::state::EnvVars;
use crate::test_utils::TestEnvironment;

use super::*;
Expand All @@ -431,6 +462,10 @@ mod tests {
}}
}

const ENV_VARS: EnvVars = EnvVars {
criticalup_token: None,
};

#[tokio::test]
async fn test_load_state_without_existing_file() {
let test = TestEnvironment::prepare().await;
Expand Down Expand Up @@ -460,7 +495,7 @@ mod tests {
let state = State::load(test_env.config()).await.unwrap();
assert_eq!(
Some(AuthenticationToken("hello".into())),
state.authentication_token(None).await
state.authentication_token(None, &ENV_VARS).await
);
}

Expand Down Expand Up @@ -751,7 +786,7 @@ mod tests {
state.set_authentication_token(None);

// Make sure the state file has authentication token as None.
assert_eq!(state.authentication_token(None).await, None);
assert_eq!(state.authentication_token(None, &ENV_VARS).await, None);

let file_token_content = "my-awesome-token-from-file";
let token_name = "CRITICALUP_TOKEN";
Expand All @@ -763,7 +798,10 @@ mod tests {
std::fs::write(secrets_dir.join(token_name), file_token_content).unwrap();
let token = test_env
.state()
.authentication_token(Some(secrets_dir.join(token_name).to_str().unwrap()))
.authentication_token(
Some(secrets_dir.join(token_name).to_str().unwrap()),
&ENV_VARS,
)
.await;
assert_eq!(Some(AuthenticationToken(file_token_content.into())), token)
}
Expand All @@ -774,12 +812,12 @@ mod tests {
let state = test_env.state();

state.set_authentication_token(None);
assert_eq!(None, state.authentication_token(None).await);
assert_eq!(None, state.authentication_token(None, &ENV_VARS).await);

state.set_authentication_token(Some(AuthenticationToken("hello world".into())));
assert_eq!(
Some(AuthenticationToken("hello world".into())),
state.authentication_token(None).await
state.authentication_token(None, &ENV_VARS).await
);
}

Expand All @@ -794,7 +832,10 @@ mod tests {
test_env.state().persist().await.unwrap();

let new_state = State::load(test_env.config()).await.unwrap();
assert_eq!(Some(token), new_state.authentication_token(None).await);
assert_eq!(
Some(token),
new_state.authentication_token(None, &ENV_VARS).await
);
}

#[tokio::test]
Expand Down Expand Up @@ -1009,4 +1050,41 @@ mod tests {
unused_installations
)
}

#[tokio::test]
async fn test_set_authn_token_env_var() {
amanjeev marked this conversation as resolved.
Show resolved Hide resolved
let test_env = TestEnvironment::with().state().prepare().await;
let state = test_env.state();

let token = "my_awesome_token".to_string();

let env_vars = EnvVars {
criticalup_token: Some(token.clone()),
};

state.set_authentication_token(None);
assert_eq!(
Some(AuthenticationToken(token)),
state.authentication_token(None, &env_vars).await
);

let env_vars = EnvVars {
criticalup_token: None,
};
assert_eq!(None, state.authentication_token(None, &env_vars).await);
}

#[tokio::test]
async fn test_read_env_var() {
std::env::set_var(CRITICALUP_TOKEN_ENV_VAR_NAME, "HoustonWeHaveAToken!");
let ev = EnvVars::read().await.unwrap();
assert_eq!(
ev.criticalup_token,
Some("HoustonWeHaveAToken!".to_string())
);

std::env::remove_var(CRITICALUP_TOKEN_ENV_VAR_NAME);
let ev = EnvVars::read().await.unwrap();
assert_eq!(ev.criticalup_token, None);
}
}
Loading