Replies: 4 comments 8 replies
-
|
Thanks for writing this up! It outlines the current state and recommended improvements very clearly. I only have a couple opinions/thoughts to share and curious for feedback: For Sub-problem 2 My opinion is we should default to reading For Sub-problem 3 It feels most natural to me to just always use the keychain to store secret config values (via endpoint/rust's keyring implementation), and not fallback to or encourage secrets to be in plaintext in |
Beta Was this translation helpful? Give feedback.
-
|
what are some examples of the transient UI states that'll live in LocalStorage? |
Beta Was this translation helpful? Give feedback.
-
|
Right now we store things like:
|
Beta Was this translation helpful? Give feedback.
-
|
I think we can also clear state when window closes if we want to keep any of this to avoid many read operations, but then read it from config on startup |
Beta Was this translation helpful? Give feedback.
-
|
I have some potentially rudimentary questions here:
|
Beta Was this translation helpful? Give feedback.
-
|
Good question! In our implementation we don't talk to keychain directly. We use https://docs.rs/keyring/latest/keyring + the |
Beta Was this translation helpful? Give feedback.
-
|
How does it violate the single responsibility principle? I think we are open to keychain alternatives for secrets |
Beta Was this translation helpful? Give feedback.
-
|
I just don't think Secrets should even be managed by goose; at most, it should just redact the secrets (e.g how Terraform handles secrets). Goose is not a secret management tool; adding that to me is mission creep. If people mess up with their secrets, it is their fault. I really liked himalaya approach as you can put in secrets directly in config or you can have something like the following where it executes a command and stores the result. This is also vulnerable but it just looked like a decent compromise to me [accounts.gmail]
default = true
backend.login = "[email protected]"
backend.auth.type = "password"
backend.auth.cmd = "pass show gmail/personal"Or alternatively, if really needed, you can have a dedicated secret management binary/server that talks to Goose over RPC but that might be too much. But then again, I don't use GUIs ... the fact that people prefer GUIs is just bewildering to me so you can somewhat guess that I am not your typical end-user. |
Beta Was this translation helpful? Give feedback.
-
|
But I definitely understand where you are coming from when it comes to leveraging the keychain.
The most straightforward, minimalistic approach when either coding or using software is what I opt in, and the use of Keychain complicated my native build of Goose on Windows. |
Beta Was this translation helpful? Give feedback.
-
|
I think I also had issues with running Goose in devcontainers; This basically made me to use WSL or My Linux Laptop environment without the protection of Containers The fact that I can not use DevContainers is one of my main challenges here I like to let Goose cook ... I like to have him just run code and iterate but I am not comfortable with giving him host level access. I have seen Claude modify |
Beta Was this translation helpful? Give feedback.
-
|
@lily-de here are some of my thoughts on What if we define a formalized spec using JSON Schema or something along that line of thought? That formalized spec is generated from Structs automatically as part of the build script to have it act as a "contract" Regarding managing secrets, I made a minimal
[dependencies]
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
getset = "0.1.2"
zeroize = { version = "1.7.0", features = [
"zeroize_derive",
"derive",
"serde",
] }
//! A single-file example showing `secret` and `store` modules.
mod secret {
use {
getset::Getters,
serde::{Deserialize, Serialize},
std::fmt::{Debug, Display},
zeroize::{Zeroize, ZeroizeOnDrop},
};
/// A wrapper type around secret data, providing Zeroize-on-drop functionality.
#[derive(
Clone,
PartialEq,
Eq,
Serialize,
Deserialize,
Zeroize,
ZeroizeOnDrop,
Getters,
Default,
)]
pub struct Secret<T: Zeroize + Clone + Debug + Default> {
#[getset(get = "pub with_prefix")]
data: T,
_internal: (),
}
impl<T: Zeroize + Clone + Debug + Default> Secret<T> {
/// Creates a new secret value wrapper.
pub fn new(secret: T) -> Self {
Self {
data: secret,
_internal: (),
}
}
}
// Provide custom `Debug` and `Display` implementations
impl<T: Zeroize + Clone + Debug + Default> Debug for Secret<T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "[REDACTED {:?}]", std::any::type_name::<T>())
}
}
impl<T: Zeroize + Clone + Debug + Default> Display for Secret<T> {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.write_str(&"*".repeat(8))
}
}
// Allow converting `T` directly into `Secret<T>`
impl<T: Zeroize + Clone + Debug + Default> From<T> for Secret<T> {
fn from(value: T) -> Self {
Self::new(value)
}
}
/// This is for manually implementing the Drop trait (not used directly, since
/// we derive `ZeroizeOnDrop`). Kept here for debugging purposes.
impl<T: Zeroize + Clone + Debug + Default> Secret<T> {
#[allow(dead_code)]
fn _drop(&mut self) {
self.data.zeroize();
println!("Zeroed. Remaining value: {:?}", self.data);
}
}
// ────────────────────────────────────────────────────────────
// Tests for the Secret module
// ────────────────────────────────────────────────────────────
#[cfg(test)]
mod tests {
use super::*;
use std::{collections::BTreeMap, fmt::Write};
#[test]
fn redacted() {
let secret = Secret::new("VERYSECRET".to_string());
let mut actual = String::new();
let _ = write!(&mut actual, "{}", secret);
assert_eq!(actual, "********");
}
#[test]
fn debug() {
let secret = Secret::new("VERYSECRET".to_string());
let printed = format!("{:?}", secret);
assert_eq!(printed, "[REDACTED \"alloc::string::String\"]");
}
#[test]
fn expose() {
let secret = Secret::new("VERYSECRET".to_string());
let printed = secret.get_data();
assert_eq!(printed, "VERYSECRET");
}
#[test]
fn secret_struct() {
#[derive(Debug)]
struct Wrapper {
#[allow(dead_code)]
password: Secret<String>,
}
let data = "VERYSECRET".to_string();
let data_len = data.len();
let secret = Secret::new(data);
let wrapper = Wrapper { password: secret };
let printed = format!("{:?}", wrapper);
assert_eq!(
printed,
"Wrapper { password: [REDACTED \"alloc::string::String\"] }"
);
let ptr = wrapper.password.get_data().as_ptr();
assert_eq!(wrapper.password.get_data(), unsafe {
let slice = core::slice::from_raw_parts(ptr, data_len);
std::str::from_utf8(slice).unwrap()
});
// The following test is commented out because it can panic:
//
// drop(wrapper);
// assert!(matches!(
// unsafe {
// let slice = std::slice::from_raw_parts(ptr, data_len);
// std::str::from_utf8(slice)
// },
// Err(_)
// ));
}
#[test]
fn memory() {
#[derive(Debug)]
struct EncryptionKey(Box<Secret<[u8; 4]>>);
let encryption_key = EncryptionKey(Box::new(Secret::new(*b"AKey")));
let ptr = encryption_key.0.get_data().as_ptr();
assert_eq!(encryption_key.0.get_data(), unsafe {
core::slice::from_raw_parts(ptr, 4)
});
drop(encryption_key);
// The first byte should now be zeroed.
assert_eq!(0, unsafe { core::slice::from_raw_parts(ptr, 4)[0] });
}
}
}
mod store {
use {
crate::secret::Secret,
serde::{Deserialize, Serialize},
std::{collections::BTreeMap, fmt::Debug},
zeroize::Zeroize,
};
/// A wrapper around a `BTreeMap` that stores `Secret<T>` values by key.
#[derive(Clone, Serialize, Deserialize, Debug)]
pub struct Store<T: Zeroize + Clone + Debug + Default>(
BTreeMap<String, Secret<T>>,
);
impl<T: Zeroize + Clone + Debug + Default> Store<T> {
pub fn new(data: BTreeMap<String, T>) -> Self {
let data = data
.into_iter()
.map(|(k, v)| (k, Secret::new(v)))
.collect::<BTreeMap<String, Secret<T>>>();
Self(data)
}
pub fn set(&mut self, key: String, value: T) {
self.0.insert(key, Secret::new(value));
}
pub fn get(&mut self, key: &str) -> Option<T> {
self.0.get(key).map(|secret| secret.get_data().clone())
}
}
impl<T: Zeroize + Clone + Debug + Default> Default for Store<T> {
fn default() -> Self {
Store(BTreeMap::new())
}
}
// Allow iterating over the store to retrieve (key, T) pairs
impl<T: Zeroize + Clone + Debug + Default> IntoIterator for Store<T> {
type Item = (String, T);
type IntoIter = <BTreeMap<String, T> as IntoIterator>::IntoIter;
fn into_iter(self) -> Self::IntoIter {
self.0
.into_iter()
.map(|(k, s)| (k, s.get_data().clone()))
.collect::<BTreeMap<_, _>>()
.into_iter()
}
}
// ────────────────────────────────────────────────────────────
// Tests for the Store module
// ────────────────────────────────────────────────────────────
#[cfg(test)]
mod tests {
use super::*;
use std::collections::BTreeMap;
#[test]
fn secretstore_intoiter() {
let bt: BTreeMap<String, String> = BTreeMap::from([
("1".to_owned(), "2".to_owned()),
("3".to_owned(), "4".to_owned()),
]);
let ss: Store<String> = Store::new(bt);
let mut iter = ss.into_iter();
assert_eq!(iter.next(), Some(("1".to_owned(), "2".to_owned())));
assert_eq!(iter.next(), Some(("3".to_owned(), "4".to_owned())));
assert_eq!(iter.next(), None);
}
}
}
// Optionally, re-export the modules/types at the top level if desired:
pub use secret::Secret;
pub use store::Store;
#[derive(Debug)]
struct Config {
username: String,
/// `password` is wrapped with `Secret` to redact it in logs
password: Secret<String>,
}
fn main() {
// Create a config with a secret password
let config = Config {
username: "admin_user".to_string(),
password: Secret::new("MyP@ssw0rd!".to_string()),
};
// Notice how Debug prints the password as redacted:
println!("Config (debug) => {config:?}");
// Display prints 8 asterisks by default:
println!("Password (display) => {}", config.password);
// Create a store to hold secrets
let mut secrets_store = Store::default();
secrets_store.set("db_key".into(), "super_secret_db_key".into());
secrets_store.set("api_token".into(), "12345abcd".into());
// Retrieve a secret
if let Some(db_key) = secrets_store.get("db_key") {
println!("Retrieved db_key from store: {}", db_key);
}
// IntoIterator usage: converting store into an iterator of `(key, value)`
for (k, v) in secrets_store {
println!("Store item => key: {k}, value: {v}");
}
} |
Beta Was this translation helpful? Give feedback.
-
|
I think we mostly care about defining secret or not in the UI to avoid exposing it in the frontend, esp if we move to also allowing secrets defined in config.yaml |
Beta Was this translation helpful? Give feedback.
-
The Block Goose dev team is working on unifying Goose configuration into a single
config.yamlfile for syncing state between Goose-UI and Goose-CLI.Below is a current discussion with open questions and proposed solutions --
Unified Config
Author: Lily Delalande
Last updated: Feb 18, 2025
Problem: Single Source of Truth vs. Multiple Stores
Problem: The current design has data spread across multiple locations (keychain, config.yaml, localStorage, environment variables). This can cause out-of-sync issues (e.g., user edits the file via Vim, but the UI never sees the change).
Plan:
Open Questions:
Sub-problem 1: Configuration Hierarchy
Problem: Currently, values can be set in environment variables, config.yaml, the OS keychain, and localStorage (UI). Some of these might override each other without a clear or consistent policy (for example, setting an API token in the UI but environment variable taking precedence in the CLI).
Plan:
Open Questions:
Preferred approach:
Sub-problem 2: Mechanism for Syncing or Reloading Configuration
Problem: If the user edits config.yaml externally (e.g., in Vim), the UI has no immediate knowledge of those changes.
Plan:
Open Questions:
Preferred approach:
Sub-problem 3: Where to Define If a Parameter Is Secret or Not
If we want to ensure that secrets are never stored in config.yaml, we need a single source of truth that declares which parameters are secret—and thus must go to the keychain (or environment variables)—versus which are safe to store in the file.
Options:
Open Questions:
Preferred Approach:
Summary of Key Decisions
Beta Was this translation helpful? Give feedback.
All reactions