Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
59 changes: 57 additions & 2 deletions src/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,32 @@ async fn load_credentials_inner(

// 2. Encrypted credentials (always AuthorizedUser for now)
if enc_path.exists() {
let json_str = credential_store::load_encrypted_from_path(enc_path)
.context("Failed to decrypt credentials")?;
let json_str = match credential_store::load_encrypted_from_path(enc_path) {
Ok(s) => s,
Err(e) => {
// Decryption failed — the encryption key likely changed (e.g. after
// an upgrade that migrated keys between keyring and file storage).
// Remove the stale file so the next `gws auth login` starts fresh,
// and fall through to other credential sources.
Comment on lines +213 to +216
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The comment on line 216 is misleading. It states that the code will "fall through to other credential sources", but the anyhow::bail! macro on line 229 causes the function to return an error immediately, preventing any fall-through. This makes the code's control flow difficult to understand. The code's behavior (bailing out) is correct, but the comment should be updated to reflect this.

Suggested change
// Decryption failed — the encryption key likely changed (e.g. after
// an upgrade that migrated keys between keyring and file storage).
// Remove the stale file so the next `gws auth login` starts fresh,
// and fall through to other credential sources.
// Decryption failed — the encryption key likely changed (e.g. after
// an upgrade that migrated keys between keyring and file storage).
// Remove the stale file so the next `gws auth login` starts fresh.

eprintln!(
"warning: removing undecryptable credentials file ({}): {e:#}",
enc_path.display()
);
let _ = std::fs::remove_file(enc_path);
// Also remove any stale token cache that used the old key.
let token_cache = enc_path.with_file_name("token_cache.json");
let _ = std::fs::remove_file(&token_cache);
let sa_token_cache = enc_path.with_file_name("sa_token_cache.json");
let _ = std::fs::remove_file(&sa_token_cache);

// Fall through to remaining credential sources below.
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This comment is also misleading. The anyhow::bail! macro on the next line will cause the function to return an error immediately, preventing any fall-through to other credential sources. This comment should be removed to avoid confusion about the control flow.

anyhow::bail!(
"Encrypted credentials could not be decrypted (key may have changed \
after an upgrade). The stale file has been removed automatically. \
Please run `gws auth login` to re-authenticate."
);
}
};

let creds: serde_json::Value =
serde_json::from_str(&json_str).context("Failed to parse decrypted credentials")?;
Expand Down Expand Up @@ -614,6 +638,37 @@ mod tests {
}
}

#[tokio::test]
#[serial_test::serial]
async fn test_load_credentials_corrupt_encrypted_file_is_removed() {
// When credentials.enc cannot be decrypted, the file should be removed
// automatically so that a subsequent `gws auth login` starts fresh.
let tmp = tempfile::tempdir().unwrap();
let _home_guard = EnvVarGuard::set("HOME", tmp.path());
let _adc_guard = EnvVarGuard::remove("GOOGLE_APPLICATION_CREDENTIALS");

let dir = tempfile::tempdir().unwrap();
let enc_path = dir.path().join("credentials.enc");

// Write garbage data that cannot be decrypted
std::fs::write(&enc_path, b"not-valid-encrypted-data-at-all-1234567890").unwrap();
assert!(enc_path.exists());

let result = load_credentials_inner(None, &enc_path, &PathBuf::from("/does/not/exist"))
.await;

assert!(result.is_err());
let msg = result.unwrap_err().to_string();
assert!(
msg.contains("could not be decrypted"),
"Error should mention decryption failure, got: {msg}"
);
assert!(
!enc_path.exists(),
"Stale credentials.enc must be removed after decryption failure"
);
}

#[tokio::test]
#[serial_test::serial]
async fn test_get_token_env_var_empty_falls_through() {
Expand Down
24 changes: 24 additions & 0 deletions src/credential_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,12 @@ fn resolve_key(
if decoded.len() == 32 {
let mut arr = [0u8; 32];
arr.copy_from_slice(&decoded);
// Ensure file backup stays in sync with keyring so
// credentials survive keyring loss (e.g. after OS
// upgrades, container restarts, daemon changes).
if !key_file.exists() {
let _ = save_key_file(key_file, &STANDARD.encode(arr));
}
return Ok(arr);
}
}
Expand Down Expand Up @@ -511,6 +517,24 @@ mod tests {
assert_eq!(result, expected);
}

#[test]
fn keyring_backend_creates_file_backup_when_missing() {
use base64::{engine::general_purpose::STANDARD, Engine as _};
let dir = tempfile::tempdir().unwrap();
let key_file = dir.path().join(".encryption_key");
let expected = [7u8; 32];
let mock = MockKeyring::with_password(&STANDARD.encode(expected));
assert!(!key_file.exists(), "file must not exist before test");
let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap();
assert_eq!(result, expected);
assert!(
key_file.exists(),
"file backup must be created when keyring succeeds but file is missing"
);
let file_key = read_key_file(&key_file).unwrap();
assert_eq!(file_key, expected, "file backup must contain the keyring key");
}

#[test]
fn keyring_backend_keeps_file_when_keyring_succeeds() {
use base64::{engine::general_purpose::STANDARD, Engine as _};
Expand Down
Loading