auto-migrate secrets from user.json to their location in the keyring

This will also remove them from their plain-text location.

Further, when the secrets are required they will be obtained
specifically, instead of always having them at hand.

The frontend doesn't ever show these, but needs them, and
it now asks for them when it needs them.
This commit is contained in:
Sebastian Thiel 2024-06-24 17:18:33 +02:00
parent f3e6c7ba94
commit 929eb52224
No known key found for this signature in database
GPG Key ID: 9CB5EE7895E8268B
20 changed files with 426 additions and 133 deletions

6
Cargo.lock generated
View File

@ -2246,8 +2246,10 @@ dependencies = [
"anyhow",
"git2",
"gitbutler-core",
"keyring",
"once_cell",
"pretty_assertions",
"serde_json",
"tempfile",
]
@ -5759,9 +5761,9 @@ dependencies = [
[[package]]
name = "serde_json"
version = "1.0.117"
version = "1.0.118"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "455182ea6142b14f93f4bc5320a2b31c1f266b66a4a5c858b013302a5d8cbfc3"
checksum = "d947f6b3163d8857ea16c4fa0dd4840d52f3041039a85decd46867eb1abef2e4"
dependencies = [
"indexmap 2.2.6",
"itoa 1.0.11",

View File

@ -17,6 +17,7 @@ uuid = { version = "1.8.0", features = ["serde"] }
serde = { version = "1.0", features = ["derive"] }
thiserror = "1.0.61"
tokio = { version = "1.38.0", default-features = false }
keyring = "2.3.3"
gitbutler-git = { path = "crates/gitbutler-git" }
gitbutler-core = { path = "crates/gitbutler-core" }

View File

@ -5,6 +5,9 @@ edition = "2021"
authors = ["GitButler <gitbutler@gitbutler.com>"]
publish = false
[[test]]
name = "secret"
path = "tests/secret/mod.rs"
[dev-dependencies]
once_cell = "1.19"
@ -29,7 +32,7 @@ git2.workspace = true
git2-hooks = "0.3"
gix = { workspace = true, features = ["dirwalk"] }
itertools = "0.13"
keyring = "2.3.3"
keyring.workspace = true
lazy_static = "1.4.0"
md5 = "0.7.0"
hex = "0.4.3"

View File

@ -27,20 +27,17 @@ impl Proxy {
}
}
pub async fn proxy_user(&self, user: users::User) -> users::User {
match Url::parse(&user.picture) {
Ok(picture) => users::User {
picture: self.proxy(&picture).await.map_or_else(
|error| {
tracing::error!(?error, "failed to proxy user picture");
user.picture.clone()
},
|url| url.to_string(),
),
..user
},
Err(_) => user,
pub async fn proxy_user(&self, mut user: users::User) -> users::User {
if let Ok(picture) = Url::parse(&user.picture) {
user.picture = self.proxy(&picture).await.map_or_else(
|error| {
tracing::error!(?error, "failed to proxy user picture");
user.picture.clone()
},
|url| url.to_string(),
);
}
user
}
async fn proxy_virtual_branch_commit(

View File

@ -378,10 +378,10 @@ impl Repository {
"pushing code to gb repo",
);
let access_token = user
.map(|user| user.access_token.clone())
.context("access token is missing")
let user = user
.context("need user to push to gitbutler")
.context(Code::ProjectGitAuth)?;
let access_token = user.access_token()?;
let mut callbacks = git2::RemoteCallbacks::new();
if self.project.omit_certificate_check.unwrap_or(false) {

View File

@ -19,6 +19,11 @@ pub fn retrieve(handle: &str) -> Result<Option<Sensitive<String>>> {
}
}
/// Delete the secret at `handle` permanently.
pub fn delete(handle: &str) -> Result<()> {
Ok(entry_for(handle)?.delete_password()?)
}
fn entry_for(handle: &str) -> Result<keyring::Entry> {
Ok(keyring::Entry::new("gitbutler", handle)?)
}

View File

@ -1,9 +1,16 @@
use anyhow::Context;
use anyhow::Result;
use std::path::PathBuf;
use super::{storage::Storage, User};
/// TODO(ST): useless intermediary - remove
/// TODO(ST): rename to `Login` - seems more akin to what it does
/// This type deals with user-related data which is only known if the user is logged in to GitButler.
///
/// ### Migrations: V1 -> V2
///
/// V2 is implied by not storing the `access_token` in plain text anymore, nor the GitHub token even if present.
/// It happens automatically on [Self::get_user()] and [Self::set_user()]
#[derive(Clone)]
pub struct Controller {
storage: Storage,
@ -18,21 +25,43 @@ impl Controller {
Controller::new(Storage::from_path(path))
}
pub fn get_user(&self) -> anyhow::Result<Option<User>> {
match self.storage.get().context("failed to get user") {
Ok(user) => Ok(user),
Err(err) => {
self.storage.delete().ok();
Err(err)
}
/// Return the current login, or `None` if there is none yet.
pub fn get_user(&self) -> Result<Option<User>> {
let user = self.storage.get().context("failed to get user")?;
if let Some(user) = &user {
write_without_secrets_if_secrets_present(&self.storage, user.clone())?;
}
Ok(user)
}
/// Note that secrets are never written in plain text, but we assure they are stored.
pub fn set_user(&self, user: &User) -> Result<()> {
if !write_without_secrets_if_secrets_present(&self.storage, user.clone())? {
self.storage.set(user).context("failed to set user")
} else {
Ok(())
}
}
pub fn set_user(&self, user: &User) -> anyhow::Result<()> {
self.storage.set(user).context("failed to set user")
}
pub fn delete_user(&self) -> anyhow::Result<()> {
self.storage.delete().context("failed to delete user")
pub fn delete_user(&self) -> Result<()> {
self.storage.delete().context("failed to delete user")?;
crate::secret::delete(User::ACCESS_TOKEN_HANDLE).ok();
crate::secret::delete(User::GITHUB_ACCESS_TOKEN_HANDLE).ok();
Ok(())
}
}
/// As `user` sports interior mutability right now, let's play it safe and work with fully owned items only.
fn write_without_secrets_if_secrets_present(storage: &Storage, user: User) -> Result<bool> {
let mut needs_write = false;
if let Some(gb_token) = user.access_token.borrow_mut().take() {
needs_write |= crate::secret::persist(User::ACCESS_TOKEN_HANDLE, &gb_token).is_ok();
}
if let Some(gh_token) = user.github_access_token.borrow_mut().take() {
needs_write |= crate::secret::persist(User::GITHUB_ACCESS_TOKEN_HANDLE, &gh_token).is_ok();
}
if needs_write {
storage.set(&user)?;
}
Ok(needs_write)
}

View File

@ -1,5 +1,7 @@
use crate::types::Sensitive;
use anyhow::{Context, Result};
use serde::{Deserialize, Serialize};
use std::cell::RefCell;
#[derive(Debug, Deserialize, Serialize, Clone, Default)]
pub struct User {
@ -12,9 +14,50 @@ pub struct User {
pub locale: Option<String>,
pub created_at: String,
pub updated_at: String,
pub access_token: Sensitive<String>,
/// The presence of a GitButler access token is required for a valid user, but it's optional
/// as it's not actually stored anymore, but fetch on demand in a separate step as its
/// storage location is the [secrets store](crate::secret).
#[serde(skip_serializing)]
pub(super) access_token: RefCell<Option<Sensitive<String>>>,
pub role: Option<String>,
pub github_access_token: Option<Sensitive<String>>,
/// The semantics here are the same as for `access_token`, but this token is truly optional.
#[serde(skip_serializing)]
pub(super) github_access_token: RefCell<Option<Sensitive<String>>>,
#[serde(default)]
pub github_username: Option<String>,
}
impl User {
pub(super) const ACCESS_TOKEN_HANDLE: &'static str = "gitbutler_access_token";
pub(super) const GITHUB_ACCESS_TOKEN_HANDLE: &'static str = "github_access_token";
/// Return the access token of the user after fetching it from the secrets store.
///
/// It's cached after the first retrieval.
pub fn access_token(&self) -> Result<Sensitive<String>> {
match self.access_token.borrow().as_ref() {
Some(token) => Ok(token.clone()),
None => {
let err_msg = "BUG: access token for user must have been stored - delete user.json and login again to fix";
let secret =
crate::secret::retrieve(Self::ACCESS_TOKEN_HANDLE)?.context(err_msg)?;
*self.access_token.borrow_mut() = Some(secret.clone());
Ok(secret)
}
}
}
/// Obtain the GitHub access token, if it is stored either on this instance or in the secrets store.
///
/// Note that if retrieved from the secrets store, it will be cached on instance.
pub fn github_access_token(&self) -> Result<Option<Sensitive<String>>> {
match self.github_access_token.borrow().as_ref() {
Some(token) => Ok(Some(token.clone())),
None => {
let secret = crate::secret::retrieve(Self::GITHUB_ACCESS_TOKEN_HANDLE)?;
self.github_access_token.borrow_mut().clone_from(&secret);
Ok(secret)
}
}
}
}

View File

@ -7,7 +7,6 @@ mod git;
mod keys;
mod lock;
mod ops;
mod secret;
mod types;
pub mod virtual_branches;
mod zip;

View File

@ -0,0 +1,15 @@
{
"id": 13612,
"name": "Sebastian Thiel",
"given_name": null,
"family_name": null,
"email": "sebastian.thiel@icloud.com",
"picture": "https://avatars.githubusercontent.com/u/63622?v=4",
"locale": null,
"created_at": "2024-03-26T13:17:05Z",
"updated_at": "2024-06-24T15:21:45Z",
"access_token": "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
"role": null,
"github_access_token": null,
"github_username": null
}

View File

@ -0,0 +1,15 @@
{
"id": 13612,
"name": "Sebastian Thiel",
"given_name": null,
"family_name": null,
"email": "sebastian.thiel@icloud.com",
"picture": "https://avatars.githubusercontent.com/u/63622?v=4",
"locale": null,
"created_at": "2024-03-26T13:17:05Z",
"updated_at": "2024-06-24T15:21:45Z",
"access_token": "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
"role": null,
"github_access_token": "gho_AAAAAAAAAAAAABBBBBBBBBBBBBBBCCCCCCCC",
"github_username": null
}

View File

@ -1,7 +1,6 @@
use std::path::PathBuf;
use std::str;
use gitbutler_core::types::Sensitive;
use gitbutler_core::{
git::credentials::{Credential, Helper, SshCredential},
keys, project_repository, projects, users,
@ -12,7 +11,7 @@ use gitbutler_testsupport::{temp_dir, test_repository};
#[derive(Default)]
struct TestCase<'a> {
remote_url: &'a str,
github_access_token: Option<Sensitive<&'a str>>,
with_github_login: bool,
preferred_key: projects::AuthKey,
}
@ -20,11 +19,14 @@ impl TestCase<'_> {
fn run(&self) -> Vec<(String, Vec<Credential>)> {
let local_app_data = temp_dir();
gitbutler_testsupport::secrets::setup_blackhole_store();
let users = users::Controller::from_path(local_app_data.path());
let user = users::User {
github_access_token: self.github_access_token.map(|s| Sensitive(s.0.to_string())),
..Default::default()
};
let user: users::User = serde_json::from_str(if self.with_github_login {
include_str!("../../tests/fixtures/users/with-github.v1")
} else {
include_str!("../../tests/fixtures/users/login-only.v1")
})
.expect("valid v1 sample user");
users.set_user(&user).unwrap();
let keys = keys::Controller::from_path(local_app_data.path());
@ -56,7 +58,7 @@ mod not_github {
fn https() {
let test_case = TestCase {
remote_url: "https://gitlab.com/test-gitbutler/test.git",
github_access_token: Some(Sensitive("token")),
with_github_login: true,
preferred_key: projects::AuthKey::Local {
private_key_path: PathBuf::from("/tmp/id_rsa"),
},
@ -80,7 +82,7 @@ mod not_github {
fn ssh() {
let test_case = TestCase {
remote_url: "git@gitlab.com:test-gitbutler/test.git",
github_access_token: Some(Sensitive("token")),
with_github_login: true,
preferred_key: projects::AuthKey::Local {
private_key_path: PathBuf::from("/tmp/id_rsa"),
},
@ -115,7 +117,7 @@ mod github {
fn https() {
let test_case = TestCase {
remote_url: "https://github.com/gitbutlerapp/gitbutler.git",
github_access_token: Some(Sensitive("token")),
with_github_login: true,
preferred_key: projects::AuthKey::Local {
private_key_path: PathBuf::from("/tmp/id_rsa"),
},
@ -139,7 +141,7 @@ mod github {
fn ssh() {
let test_case = TestCase {
remote_url: "git@github.com:gitbutlerapp/gitbutler.git",
github_access_token: Some(Sensitive("token")),
with_github_login: true,
preferred_key: projects::AuthKey::Local {
private_key_path: PathBuf::from("/tmp/id_rsa"),
},

View File

@ -0,0 +1,96 @@
use keyring::credential::{CredentialApi, CredentialBuilderApi, CredentialPersistence};
use keyring::Credential;
use std::any::Any;
use std::collections::BTreeMap;
use std::sync::{Arc, Mutex};
#[derive(Default)]
pub(super) struct Store(BTreeMap<String, String>);
pub(super) type SharedStore = Arc<Mutex<Store>>;
struct Entry {
handle: String,
store: SharedStore,
}
impl CredentialApi for Entry {
fn set_password(&self, password: &str) -> keyring::Result<()> {
self.store
.lock()
.unwrap()
.0
.insert(self.handle.clone(), password.into());
Ok(())
}
fn get_password(&self) -> keyring::Result<String> {
match self.store.lock().unwrap().0.get(&self.handle) {
Some(secret) => Ok(secret.clone()),
None => Err(keyring::Error::NoEntry),
}
}
fn delete_password(&self) -> keyring::Result<()> {
self.store.lock().unwrap().0.remove(&self.handle);
Ok(())
}
fn as_any(&self) -> &dyn Any {
self
}
}
pub(super) struct Builder {
pub(super) store: SharedStore,
}
impl CredentialBuilderApi for Builder {
fn build(
&self,
_target: Option<&str>,
_service: &str,
user: &str,
) -> keyring::Result<Box<Credential>> {
let credential = Entry {
handle: user.to_string(),
store: self.store.clone(),
};
Ok(Box::new(credential))
}
fn as_any(&self) -> &dyn std::any::Any {
self
}
/// We keep information in memory
fn persistence(&self) -> CredentialPersistence {
CredentialPersistence::ProcessOnly
}
}
static CURRENT_STORE: Mutex<Option<SharedStore>> = Mutex::new(None);
/// Initialize the credentials store to be isolated and usable for testing.
///
/// Note that this is a resource shared in the process, and deterministic tests must
/// use the `[serial]` annotation.
pub fn setup() {
let store = SharedStore::default();
*CURRENT_STORE.lock().unwrap() = Some(store.clone());
keyring::set_default_credential_builder(Box::new(Builder { store }));
}
/// Return the amount of stored secrets
pub fn count() -> usize {
CURRENT_STORE
.lock()
.unwrap()
.as_ref()
.expect("BUG: call setup")
.lock()
.unwrap()
.0
.len()
}

View File

@ -1,3 +1,6 @@
//! Note that these tests *must* be run in their own process, as they rely on having a deterministic
//! credential store. Due to its global nature, tests cannot run in parallel
//! (or mixed with parallel tests that set their own credential store)
use gitbutler_core::secret;
use gitbutler_core::types::Sensitive;
use serial_test::serial;
@ -5,7 +8,7 @@ use serial_test::serial;
#[test]
#[serial]
fn retrieve_unknown_is_none() {
setup();
credentials::setup();
assert!(secret::retrieve("does not exist for sure")
.expect("no error to ask for non-existing")
.is_none());
@ -14,7 +17,7 @@ fn retrieve_unknown_is_none() {
#[test]
#[serial]
fn store_and_retrieve() -> anyhow::Result<()> {
setup();
credentials::setup();
secret::persist("new", &Sensitive("secret".into()))?;
let secret = secret::retrieve("new")?.expect("it was just stored");
assert_eq!(
@ -25,81 +28,5 @@ fn store_and_retrieve() -> anyhow::Result<()> {
Ok(())
}
fn setup() {
keyring::set_default_credential_builder(Box::<credentials::Builder>::default());
}
mod credentials {
use keyring::credential::{CredentialApi, CredentialBuilderApi, CredentialPersistence};
use keyring::Credential;
use std::any::Any;
use std::collections::BTreeMap;
use std::sync::{Arc, Mutex};
#[derive(Default)]
struct Store {
store: BTreeMap<String, String>,
}
type SharedStore = Arc<Mutex<Store>>;
struct Entry {
handle: String,
inner: SharedStore,
}
impl CredentialApi for Entry {
fn set_password(&self, password: &str) -> keyring::Result<()> {
self.inner
.lock()
.unwrap()
.store
.insert(self.handle.clone(), password.into());
Ok(())
}
fn get_password(&self) -> keyring::Result<String> {
match self.inner.lock().unwrap().store.get(&self.handle) {
Some(secret) => Ok(secret.clone()),
None => Err(keyring::Error::NoEntry),
}
}
fn delete_password(&self) -> keyring::Result<()> {
todo!()
}
fn as_any(&self) -> &dyn Any {
self
}
}
#[derive(Default)]
pub(super) struct Builder {
store: SharedStore,
}
impl CredentialBuilderApi for Builder {
fn build(
&self,
_target: Option<&str>,
_service: &str,
user: &str,
) -> keyring::Result<Box<Credential>> {
let credential = Entry {
handle: user.to_string(),
inner: self.store.clone(),
};
Ok(Box::new(credential))
}
fn as_any(&self) -> &dyn std::any::Any {
self
}
/// We keep information in memory
fn persistence(&self) -> CredentialPersistence {
CredentialPersistence::ProcessOnly
}
}
}
pub(crate) mod credentials;
mod users;

View File

@ -0,0 +1,94 @@
use crate::{credentials, credentials::count as count_secrets};
use serial_test::serial;
use std::path::{Path, PathBuf};
use tempfile::tempdir;
/// Validate that secrets previously stored in plain-text are auto-migrated into the secrets store.
/// From there, data-structures for use by the frontend need to be 'enriched' with secrets before sending them,
/// or before using them.
#[test]
#[serial]
fn auto_migration_of_secrets_on_when_getting_and_setting_user() -> anyhow::Result<()> {
for (name, has_github_token) in [("login-only.v1", false), ("with-github.v1", true)] {
credentials::setup();
let app_data = tempdir()?;
let users = gitbutler_core::users::Controller::from_path(app_data.path());
assert!(
users.get_user()?.is_none(),
"Users are bound to logins, so there is none by default"
);
assert_eq!(count_secrets(), 0, "no secret is associated with anything");
let buf = std::fs::read(user_fixture(name))?;
let user_json_path = app_data.path().join("user.json");
std::fs::write(&user_json_path, &buf)?;
let user = users.get_user()?.expect("previous v1 user was read");
let expected_secrets = if has_github_token { 2 } else { 1 };
assert_eq!(
count_secrets(),
expected_secrets,
"it automatically entered the secrets to the secrets store after getting the existing user"
);
assert_eq!(
user.access_token()?.0,
"aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
"it can make the access token available"
);
if has_github_token {
assert_eq!(
user.github_access_token()?.map(|s| s.0),
Some("gho_AAAAAAAAAAAAABBBBBBBBBBBBBBBCCCCCCCC".into()),
"it can make the access token available"
);
}
let assert_no_secret_in_plain_text = || -> anyhow::Result<()> {
let buf = std::fs::read(&user_json_path)?;
let value: serde_json::Value = serde_json::from_slice(&buf)?;
assert_eq!(
value.get("access_token"),
None,
"access token wasn't written back (right after getting it)"
);
assert_eq!(
value.get("github_access_token"),
None,
"access token wasn't written back"
);
Ok(())
};
assert_no_secret_in_plain_text()?;
users.delete_user()?;
assert_eq!(
count_secrets(),
0,
"deletion of a user automatically deletes its secretes"
);
assert!(
!user_json_path.exists(),
"it deletes the whole file, i.e. all associated user data"
);
users.set_user(&user)?;
assert_eq!(
count_secrets(),
expected_secrets,
"the in-memory users had its secrets cached, so they are picked up and stored officially"
);
assert_no_secret_in_plain_text()?;
}
Ok(())
}
fn user_fixture(name: &str) -> PathBuf {
let fixture = Path::new("tests/fixtures/users").join(name);
assert!(
fixture.exists(),
"BUG: fixture at {fixture:?} ought to exist"
);
fixture
}

View File

@ -15,4 +15,6 @@ once_cell = "1.19"
git2.workspace = true
pretty_assertions = "1.4"
tempfile = "3.10.1"
keyring.workspace = true
serde_json = "1.0"
gitbutler-core = { path = "../gitbutler-core" }

View File

@ -0,0 +1,15 @@
{
"id": 0,
"name": "test",
"given_name": null,
"family_name": null,
"email": "test@email.com",
"picture": "",
"locale": null,
"created_at": "",
"updated_at": "",
"access_token": "token",
"role": null,
"github_access_token": null,
"github_username": null
}

View File

@ -60,3 +60,9 @@ pub fn init_opts_bare() -> git2::RepositoryInitOptions {
opts.bare(true);
opts
}
/// A secrets store to prevent secrets to be written into the systems own store.
///
/// Note that this can't be used if secrets themselves are under test as it' doesn't act
/// like a real store, i.e. stored secrets can't be retrieved anymore.
pub mod secrets;

View File

@ -0,0 +1,45 @@
use std::any::Any;
/// Assure we have a mock secrets store so tests don't start writing secrets into the user's actual store,
/// as this will affect their GitButler instance.
pub fn setup_blackhole_store() {
keyring::set_default_credential_builder(Box::new(BlackholeBuilder))
}
/// A builder that is completely mocked, able to read no secret, but allowing to write any without error.
struct BlackholeBuilder;
struct BlackholeCredential;
impl keyring::credential::CredentialApi for BlackholeCredential {
fn set_password(&self, _password: &str) -> keyring::Result<()> {
Ok(())
}
fn get_password(&self) -> keyring::Result<String> {
Err(keyring::Error::NoEntry)
}
fn delete_password(&self) -> keyring::Result<()> {
Ok(())
}
fn as_any(&self) -> &dyn Any {
self
}
}
impl keyring::credential::CredentialBuilderApi for BlackholeBuilder {
fn build(
&self,
_target: Option<&str>,
_service: &str,
_user: &str,
) -> keyring::Result<Box<keyring::Credential>> {
Ok(Box::new(BlackholeCredential))
}
fn as_any(&self) -> &dyn Any {
self
}
}

View File

@ -4,7 +4,6 @@ use std::{
path::{Path, PathBuf},
};
use gitbutler_core::types::Sensitive;
use gitbutler_core::{git::RepositoryExt, project_repository};
use tempfile::{tempdir, TempDir};
@ -48,12 +47,10 @@ impl Suite {
self.local_app_data.as_ref().unwrap().path()
}
pub fn sign_in(&self) -> gitbutler_core::users::User {
let user = gitbutler_core::users::User {
name: Some("test".to_string()),
email: "test@email.com".to_string(),
access_token: Sensitive("token".to_string()),
..Default::default()
};
crate::secrets::setup_blackhole_store();
let user: gitbutler_core::users::User =
serde_json::from_str(include_str!("fixtures/user/minimal.v1"))
.expect("valid v1 user file");
self.users.set_user(&user).expect("failed to add user");
user
}