Skip to content
Merged
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
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions tvc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ p256 = { workspace = true }
reqwest = { workspace = true }
serde = { workspace = true }
serde_json = { workspace = true }
serde_with = { workspace = true }
thiserror = { workspace = true }
toml = { workspace = true }
tokio = { workspace = true, features = ["fs"] }
Expand Down
4 changes: 4 additions & 0 deletions tvc/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ impl Commands {
AppCommands::Delete(args) => commands::app::delete::run(ctx, args).await,
},
Commands::Keys { command } => match command {
KeysCommands::BackupOperatorKey(args) => args.run(ctx).await.map(Into::into),
KeysCommands::CreateQuorumKey(args) => {
commands::keys::create_quorum_key::run(ctx, args).await
}
Expand Down Expand Up @@ -393,6 +394,8 @@ enum AppCommands {

#[derive(Debug, Subcommand)]
enum KeysCommands {
/// Back up a local operator key by copying its key file to a chosen destination.
BackupOperatorKey(commands::keys::backup_operator_key::Args),
/// Create a hosted quorum key encrypted to hosted operator keys.
CreateQuorumKey(commands::keys::create_quorum_key::Args),
/// Generate and shamir-split a local quorum key, encrypting each share to an operator key.
Expand All @@ -419,6 +422,7 @@ impl AppCommands {
impl KeysCommands {
fn name(&self) -> &'static str {
match self {
KeysCommands::BackupOperatorKey(_) => "keys backup-operator-key",
KeysCommands::CreateQuorumKey(_) => "keys create-quorum-key",
KeysCommands::GenerateLocalQuorumKey(_) => "keys generate-local-quorum-key",
KeysCommands::InitLocalQuorumKey(_) => "keys init-local-quorum-key",
Expand Down
2 changes: 1 addition & 1 deletion tvc/src/commands/app/create.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ async fn load_saved_operator_public_key() -> Option<String> {
let (alias, org_config) = config.active_org_config()?;
let local = org_config.select_local_record(alias).ok()?;
let operator_key = StoredQosOperatorKey::load(&local.key_path).await.ok()??;
Some(operator_key.public_key)
Some(operator_key.public_key.to_string())
}

async fn run_with_config(ctx: &mut StdCtx, args: Args, app_config: AppConfig) -> Result<Outcome> {
Expand Down
2 changes: 1 addition & 1 deletion tvc/src/commands/app/init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,5 +112,5 @@ async fn load_operator_public_key() -> Option<String> {
let (alias, org_config) = config.active_org_config()?;
let local = org_config.select_local_record(alias).ok()?;
let operator_key = StoredQosOperatorKey::load(&local.key_path).await.ok()??;
Some(operator_key.public_key)
Some(operator_key.public_key.to_string())
}
319 changes: 319 additions & 0 deletions tvc/src/commands/keys/backup_operator_key.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,319 @@
//! Operator key backup command - copies a local operator key file to a
//! user-chosen destination.

use crate::{
commands::{Run, login::find_org},
config::turnkey::{Config, QosOperatorPublicKey, StoredQosOperatorKey},
outcome::Outcome,
output::StdCtx,
prompts::{self, error_required_in_non_interactive},
};
use anyhow::{Context, Result, anyhow, bail};
use clap::Args as ClapArgs;
use serde::Serialize;
use std::{
fmt::{self, Display, Formatter},
path::PathBuf,
};

/// Back up a local operator key by copying its key file to a chosen
/// destination.
#[derive(Debug, ClapArgs)]
#[command(about, long_about = None)]
pub struct Args {
/// Organization alias or ID whose operator key to back up.
/// Defaults to the active organization.
#[arg(long, env = "TVC_ORG", value_name = "ORG")]
org: Option<String>,
/// Destination file for the backup copy.
#[arg(short, long, value_name = "PATH", env = "TVC_OPERATOR_KEY_BACKUP_OUT")]
output: Option<PathBuf>,
/// Overwrite the destination if it already exists.
#[arg(long)]
overwrite: bool,
}

impl Run for Args {
type Outcome = OperatorKeyBackedUp;

async fn run(self, ctx: &mut StdCtx) -> Result<OperatorKeyBackedUp> {
// Reject before loading config or resolving the organization when
// there is no way to prompt for the destination: --non-interactive,
// JSON mode, or a non-TTY stdin.
let can_prompt = !ctx.is_non_interactive() && prompts::stdin_can_prompt();

if !can_prompt && self.output.is_none() {
return Err(error_required_in_non_interactive("--output"));
}

let config = Config::load().await?;

let (alias, org_config) = match &self.org {
Some(query) => find_org(&config, query).ok_or_else(|| {
anyhow!(
"Login profile '{query}' not found. \
Run `tvc login` to see configured profiles."
)
})?,
None => config
.active_org_config()
.ok_or_else(|| anyhow!("No active organization. Run `tvc login` first."))?,
};

let source = &org_config.select_local_record(alias)?.key_path;

let destination = match self.output {
// --output is a CLI argument: validate it, honoring --overwrite
// and the non-interactive fence.
Some(output) => {
if output.is_dir() {
bail!(
"destination {} is a directory; include a file name",
output.display()
);
}

if output.exists() && !self.overwrite {
if !can_prompt {
bail!(
"destination {} already exists; pass --overwrite to replace it",
output.display()
);
}

prompts::confirm_or_bail(
&format!("Overwrite {}?", output.display()),
"backup",
)?;
}

output
}
// No --output: the shared interactive flow. Declining the
// overwrite cancels the command - backing up is all it does.
None => prompt_for_backup_destination(alias)?
.ok_or_else(|| anyhow!("operation cancelled by user: backup"))?,
};

back_up(alias.to_string(), source.clone(), destination).await
}
}

/// Prompt for a backup destination: the default file name, the
/// directory-destination rejection, and the overwrite question live here, so
/// every interactive caller asks them the same way. Returns `None` when the
/// user declines the overwrite; callers own what declining means.
pub(crate) fn prompt_for_backup_destination(alias: &str) -> Result<Option<PathBuf>> {
let destination: PathBuf = prompts::text(
"Backup file path",
Some(&format!("operator-{alias}-backup.json")),
)?
.into();

if destination.is_dir() {
bail!(
"destination {} is a directory; include a file name",
destination.display()
);
}

if destination.exists()
&& !prompts::confirm(&format!("Overwrite {}?", destination.display()), false)?
{
return Ok(None);
}

Ok(Some(destination))
}

/// Copy the operator key at `source` to `destination` byte-for-byte and
/// return the backup report.
///
/// The source is parsed to validate it and capture the public key, but the
/// original bytes are written verbatim so any unknown fields survive the
/// copy. This is the only place an [`OperatorKeyBackedUp`] is constructed.
pub(crate) async fn back_up(
alias: String,
source: PathBuf,
destination: PathBuf,
) -> Result<OperatorKeyBackedUp> {
let bytes = tokio::fs::read(&source).await.map_err(|e| match e.kind() {
std::io::ErrorKind::NotFound => anyhow!(
"No operator key found at {}. Run `tvc login` first.",
source.display()
),
_ => anyhow::Error::new(e)
.context(format!("failed to read operator key: {}", source.display())),
})?;

let key: StoredQosOperatorKey = serde_json::from_slice(&bytes).with_context(|| {
format!(
"operator key at {} is not a valid operator key file",
source.display()
)
})?;
Comment on lines +149 to +154

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(AGENT) medium — "parsed to validate it" is a much weaker claim than it reads as

The doc at line 95 says the source "is parsed to validate it." What's actually checked is 130 bytes of hex on the public key. private_key is still an unparsed String — which means the one field that determines whether the backup is restorable is never looked at:

$ cat operator.json     # public_key: 130 random bytes, private_key: "not-even-hex"
$ tvc keys backup-operator-key --output bk.json
Operator key backed up!            (exit 0)

A backup that cannot be restored reports success in green. Per AGENTS.md's parse-don't-validate rule this is the remaining unparsed field on the DTO, and the parser already exists (HexSeed / LocalPair::from_hex_seed, pair.rs:30/:67) — plus LocalPair::public_key() returns the same composite encoding, so the round-trip check is nearly free:

Suggested change
let key: StoredQosOperatorKey = serde_json::from_slice(&bytes).with_context(|| {
format!(
"operator key at {} is not a valid operator key file",
source.display()
)
})?;
let key: StoredQosOperatorKey = serde_json::from_slice(&bytes).with_context(|| {
format!(
"operator key at {} is not a valid operator key file",
source.display()
)
})?;
// A backup is only worth reporting as one if it can be restored, so prove
// the stored seed derives the stored public key before copying.
let derived = LocalPair::from_hex_seed(&key.private_key).with_context(|| {
format!(
"operator key at {} has an unusable private key",
source.display()
)
})?;
if QosOperatorPublicKey::try_from(derived.public_key().as_slice())? != key.public_key {
bail!(
"operator key at {} is inconsistent: its private key does not derive its public key",
source.display()
);
}

Needs use crate::pair::{LocalPair, Pair};. If you'd rather keep back_up cheap and I/O-only, the alternative is to soften the doc comment — but then the success message probably shouldn't be as confident as it is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(human) I think this is good type tightening for the private key. I also think that maybe we could store it as something besides a String for additional redaction-safety for logging

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we also don't use zeorize in here. I think this is a larger refactor though

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is my biggest concern in the PR, because it's an easy fix to delay, forget about, then accidentally log or leak private key info. Any chance we can store it as HexSeed in this PR? If not, can TVC-278 be like an immediate follow ticket?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I actually already did a bunch of TVC-278 in here, just not this one - so can follow up right after. The issue that's described here is that we don't properly validate the key before backing it up, but it's a corner case. We shouldn't be backing up any keys that weren't created by the cli to begin with. You'd have to go in and manually manipulate the key.

I agree that it should happen and we don't want to keep the key as a string, etc etc... but all these things existed before this ticket. I'm not making the security posture worse with this backup mechanism


// A bare-filename destination has the empty path as its parent, which
// `create_dir_all` accepts as a no-op.
if let Some(parent) = destination.parent() {
tokio::fs::create_dir_all(parent)
.await
.with_context(|| format!("failed to create backup directory: {}", parent.display()))?;
}

// Written with default (umask) permissions, matching
// `StoredQosOperatorKey::save`; tightening both is tracked by TVC-241.
tokio::fs::copy(&source, &destination)
.await
.with_context(|| format!("failed to write backup: {}", destination.display()))?;
Comment on lines +164 to +168

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(AGENT) blocking — the backup of a private key is created world-readable

Deferring StoredQosOperatorKey::save's permissions to TVC-241 is reasonable; deferring them here is less so. This command's entire purpose is writing private key material to a new path the user chose, and the output text three lines later tells them to treat the file as a secret ("The backup contains the PRIVATE key. Store it somewhere safe"). At umask defaults that file is typically 0644. The code should agree with the advice it prints, and it's a local fix rather than a cross-cutting one:

Suggested change
// Written with default (umask) permissions, matching
// `StoredQosOperatorKey::save`; tightening both is tracked by TVC-241.
tokio::fs::write(&destination, &bytes)
.await
.with_context(|| format!("failed to write backup: {}", destination.display()))?;
// The backup holds private key material, so create it 0600 rather than at
// umask defaults. TVC-241 tracks the same tightening for the primary key
// file, which this command reads but does not write.
let mut options = tokio::fs::OpenOptions::new();
options.write(true).create(true).truncate(true);
#[cfg(unix)]
options.mode(0o600);
let write_context = || format!("failed to write backup: {}", destination.display());
options
.open(&destination)
.await
.with_context(write_context)?
.write_all(&bytes)
.await
.with_context(write_context)?;

Needs use tokio::io::AsyncWriteExt; and, under #[cfg(unix)], use std::os::unix::fs::OpenOptionsExt;. Happy to be told this belongs in TVC-241 with the other one instead — but then it's worth saying so in the doc comment, since a reader currently sees "matching save" and may not realize that means 0644.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(human) I'm not familiar with Unix conventions but this seems fine, albeit not blocking imo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I disagree with all the robots here. This should be a read and copy. We shouldn't have to think about perms for backup, just copy the original file's perms. Also, copy as atomic and allows the OS to handle the copy in the most efficient way

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Re-signed: 91ddd64


Ok(OperatorKeyBackedUp {
alias,
public_key: key.public_key,
source_path: source,
backup_path: destination,
})
}

#[derive(Default, Serialize)]
#[cfg_attr(test, derive(Debug))]
#[serde(rename_all = "camelCase")]
pub struct OperatorKeyBackedUp {
alias: String,
public_key: QosOperatorPublicKey,
source_path: PathBuf,
backup_path: PathBuf,
}

impl From<OperatorKeyBackedUp> for Outcome {
fn from(backed_up: OperatorKeyBackedUp) -> Self {
Outcome::OperatorKeyBackedUp(backed_up)
}
}

impl Display for OperatorKeyBackedUp {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
write!(
f,
r#"Operator key backed up!

Org: {}
Public key: {}
Source: {}
Backup: {}

The backup contains the PRIVATE key. Store it somewhere safe - a password
manager or an encrypted offline drive - never in source control or chat.

To restore: copy the backup file back to the source path above, then run
`tvc login`."#,
self.alias,
self.public_key,
self.source_path.display(),
self.backup_path.display()
)
}
}

#[cfg(test)]
mod tests {
use super::*;
use tempfile::TempDir;

#[tokio::test]
async fn backs_up_key_bytes_verbatim() {
let temp = TempDir::new().unwrap();
let source = temp.path().join("operator.json");
let destination = temp.path().join("backups/operator-backup.json");
// Unknown fields must survive the copy: the file is written verbatim,
// not re-serialized.
let public_hex = hex::encode(
qos_p256::P256Pair::generate()
.unwrap()
.public_key()
.to_bytes(),
);
let content = format!(
r#"{{
"public_key": "{public_hex}",
"private_key": "priv-hex",
"future_field": 42
}}"#
);
std::fs::write(&source, &content).unwrap();

let report = back_up("default".to_string(), source.clone(), destination.clone())
.await
.unwrap();

assert_eq!(std::fs::read_to_string(&destination).unwrap(), content);
assert_eq!(report.public_key.to_string(), public_hex);
}

#[tokio::test]
async fn missing_source_names_path() {
let temp = TempDir::new().unwrap();
let source = temp.path().join("operator.json");

let error = back_up(
"default".to_string(),
source.clone(),
temp.path().join("out.json"),
)
.await
.expect_err("missing source must fail");

assert_eq!(
error.to_string(),
format!(
"No operator key found at {}. Run `tvc login` first.",
source.display()
)
);
}

#[tokio::test]
async fn malformed_source_names_path() {
let temp = TempDir::new().unwrap();
let source = temp.path().join("operator.json");
std::fs::write(&source, "not json").unwrap();

let error = back_up(
"default".to_string(),
source.clone(),
temp.path().join("out.json"),
)
.await
.expect_err("malformed source must fail");

assert_eq!(
error.to_string(),
format!(
"operator key at {} is not a valid operator key file",
source.display()
)
);
}

#[test]
fn outcome_serializes_expected_json() {
let public_key = QosOperatorPublicKey::default();
let outcome = Outcome::from(OperatorKeyBackedUp {
alias: "default".to_string(),
public_key,
source_path: PathBuf::from("/keys/operator.json"),
backup_path: PathBuf::from("/backups/operator-backup.json"),
});

assert_eq!(
serde_json::to_value(&outcome).unwrap(),
serde_json::json!({
"reason": "operator_key_backed_up",
"alias": "default",
"publicKey": public_key.to_string(),
"sourcePath": "/keys/operator.json",
"backupPath": "/backups/operator-backup.json",
})
);
}
}
2 changes: 1 addition & 1 deletion tvc/src/commands/keys/init_local_quorum_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,5 +73,5 @@ async fn load_operator_public_key() -> Option<String> {
let (alias, org_config) = config.active_org_config()?;
let local = org_config.select_local_record(alias).ok()?;
let operator_key = StoredQosOperatorKey::load(&local.key_path).await.ok()??;
Some(operator_key.public_key)
Some(operator_key.public_key.to_string())
}
1 change: 1 addition & 0 deletions tvc/src/commands/keys/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
//! Key management commands.

pub mod backup_operator_key;
pub mod create_quorum_key;
pub mod generate_local_quorum_key;
pub mod init_local_quorum_key;
Expand Down
Loading
Loading