From 19d074530a3ac951af537152e3480a44b267fb6f Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Thu, 23 Apr 2026 07:44:30 -0700 Subject: [PATCH 1/6] Implement automatic temporary delegations --- CHANGELOG.md | 1 + crates/icp-cli/src/main.rs | 7 +- crates/icp-cli/tests/identity_tests.rs | 43 ++++++ crates/icp/src/context/init.rs | 4 + crates/icp/src/identity/key.rs | 198 ++++++++++++++++++++++++- crates/icp/src/identity/mod.rs | 22 ++- 6 files changed, 264 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b07448626..a4100e121 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased +* feat: Password-protected identities now only need your password once every five minutes (configurable) * feat: `icp identity delegation request/sign/use` now permit creating and importing identity delegations * feat: `icp identity import` now takes `--seed-curve`, for seed phrases for non-k256 keys. * fix: `icp canister settings show` now outputs only the canister settings, consistent with the command name diff --git a/crates/icp-cli/src/main.rs b/crates/icp-cli/src/main.rs index 9d580708a..e335e4986 100644 --- a/crates/icp-cli/src/main.rs +++ b/crates/icp-cli/src/main.rs @@ -152,7 +152,12 @@ async fn main() -> Result<(), Error> { .map_err(|e| e.to_string()) }), }; - let ctx = icp::context::initialize(cli.project_root_override, cli.debug, password_func)?; + let ctx = icp::context::initialize( + cli.project_root_override, + cli.debug, + password_func, + Some(std::time::Duration::from_secs(600)), + )?; let telemetry_session = telemetry::setup(&ctx, &raw_args, &Cli::command()).await; diff --git a/crates/icp-cli/tests/identity_tests.rs b/crates/icp-cli/tests/identity_tests.rs index b668e6ff2..1c06de97e 100644 --- a/crates/icp-cli/tests/identity_tests.rs +++ b/crates/icp-cli/tests/identity_tests.rs @@ -1465,3 +1465,46 @@ fn identity_link_hsm_delete() { .success() .stdout(contains("hsm-identity").not()); } + +/// After unlocking a password-protected identity once, subsequent commands must succeed +/// even when the password file is empty (i.e. the session delegation is reused). +#[test] +fn pem_session_delegation_avoids_second_password_prompt() { + let ctx = TestContext::new(); + + let mut password_file = NamedTempFile::new().unwrap(); + password_file.write_all(b"test-password-xyz").unwrap(); + let password_path = password_file.into_temp_path(); + + // Create a password-protected identity. + ctx.icp() + .args([ + "identity", + "new", + "pw-session-test", + "--storage", + "password", + ]) + .arg("--storage-password-file") + .arg(&password_path) + .assert() + .success(); + + // First use: unlocks the PEM and creates a session delegation. + ctx.icp() + .arg("--identity-password-file") + .arg(&password_path) + .args(["identity", "principal", "--identity", "pw-session-test"]) + .assert() + .success(); + + // Second use: password file is empty — decryption would fail if attempted. + // The session delegation must be used instead. + let empty_file = NamedTempFile::new().unwrap(); + ctx.icp() + .arg("--identity-password-file") + .arg(empty_file.path()) + .args(["identity", "principal", "--identity", "pw-session-test"]) + .assert() + .success(); +} diff --git a/crates/icp/src/context/init.rs b/crates/icp/src/context/init.rs index 636fb2336..12f6e3cf8 100644 --- a/crates/icp/src/context/init.rs +++ b/crates/icp/src/context/init.rs @@ -9,6 +9,8 @@ use crate::context::Context; use crate::directories::{Access as _, Directories}; use crate::prelude::*; use crate::store_artifact::ArtifactStore; +use std::time::Duration; + use crate::{ Lazy, ProjectLoadImpl, agent, identity, identity::PasswordFunc, manifest, network, store_id, }; @@ -37,6 +39,7 @@ pub fn initialize( project_root_override: Option, debug: bool, password_func: PasswordFunc, + pem_session_duration: Option, ) -> Result { // Setup global directory structure let dirs = Arc::new(Directories::new().context(DirectoriesSnafu)?); @@ -90,6 +93,7 @@ pub fn initialize( let idload = Arc::new(identity::Loader::new( dirs.identity().context(IdentityDirectorySnafu)?, password_func, + pem_session_duration, telemetry_data.clone(), )); if let Ok(mockdir) = std::env::var("ICP_CLI_KEYRING_MOCK_DIR") { diff --git a/crates/icp/src/identity/key.rs b/crates/icp/src/identity/key.rs index 117e52944..eb4dadfaa 100644 --- a/crates/icp/src/identity/key.rs +++ b/crates/icp/src/identity/key.rs @@ -1,13 +1,14 @@ use std::{ fmt::{self, Display, Formatter}, sync::Arc, + time::{Duration, SystemTime, UNIX_EPOCH}, }; use ic_agent::{ Identity, identity::{ - AnonymousIdentity, BasicIdentity, DelegatedIdentity, DelegationError, Prime256v1Identity, - Secp256k1Identity, + AnonymousIdentity, BasicIdentity, DelegatedIdentity, Delegation as AgentDelegation, + DelegationError, Prime256v1Identity, Secp256k1Identity, }, }; use ic_ed25519::PrivateKeyFormat; @@ -31,7 +32,8 @@ use crate::{ lock::{LRead, LWrite}, }, identity::{ - IdentityPaths, delegation, + IdentityPaths, + delegation::{self, SignedDelegation}, manifest::{ DelegationKeyStorage, IdentityDefaults, IdentityKeyAlgorithm, IdentityList, IdentitySpec, LoadIdentityManifestError, PemFormat, WriteIdentityManifestError, @@ -142,6 +144,7 @@ pub fn load_identity( list: &IdentityList, name: &str, password_func: impl FnOnce() -> Result, + pem_session_duration: Option, ) -> Result, LoadIdentityError> { let identity = list .identities @@ -151,7 +154,14 @@ pub fn load_identity( match identity { IdentitySpec::Pem { format, algorithm, .. - } => load_pem_identity(dirs, name, format, algorithm, password_func), + } => load_pem_identity( + dirs, + name, + format, + algorithm, + password_func, + pem_session_duration, + ), IdentitySpec::Keyring { algorithm, .. } => load_keyring_identity(name, algorithm), IdentitySpec::Hsm { module, @@ -176,7 +186,16 @@ fn load_pem_identity( format: &PemFormat, algorithm: &IdentityKeyAlgorithm, password_func: impl FnOnce() -> Result, + pem_session_duration: Option, ) -> Result, LoadIdentityError> { + // For password-protected PEMs, check for a valid cached session delegation first. + if *format == PemFormat::Pbes2 + && pem_session_duration.is_some() + && let Some(id) = try_load_pem_session(dirs, name) + { + return Ok(id); + } + let pem_path = dirs.key_pem_path(name); let origin = PemOrigin::File { path: pem_path.clone(), @@ -186,11 +205,21 @@ fn load_pem_identity( .parse::() .context(ParsePemSnafu { origin: &origin })?; - match format { - PemFormat::Pbes2 => load_pbes2_identity(&doc, algorithm, password_func, &origin), + let identity = match format { + PemFormat::Pbes2 => load_pbes2_identity(&doc, algorithm, password_func, &origin)?, + PemFormat::Plaintext => load_plaintext_identity(&doc, algorithm, &origin)?, + }; - PemFormat::Plaintext => load_plaintext_identity(&doc, algorithm, &origin), + // After unlocking a Pbes2 PEM, create and cache a short-lived session delegation. + if *format == PemFormat::Pbes2 + && let Some(duration) = pem_session_duration + && let Some(delegated) = + create_pem_session_and_build_identity(dirs, name, &*identity, duration) + { + return Ok(delegated); } + + Ok(identity) } fn load_pbes2_identity( @@ -273,6 +302,136 @@ fn dlg_keyring_key(name: &str) -> String { format!("delegation:{name}") } +/// Tries to load a previously cached PEM session delegation from keyring + disk. +/// +/// Returns `None` on any failure (missing, expired, or keyring unavailable) so the +/// caller falls back to normal PEM loading. +fn try_load_pem_session(dirs: LRead<&IdentityPaths>, name: &str) -> Option> { + // Load chain from disk; missing file is the common case on first use. + let chain_path = dirs.delegation_chain_path(name); + let chain = delegation::load(&chain_path).ok()?; + + if delegation::is_expiring_soon(&chain, FIVE_MINUTES_NANOS).ok()? { + return None; + } + + // Load session key from keyring. + let username = dlg_keyring_key(name); + let entry = Entry::new(SERVICE_NAME, &username).ok()?; + let pem_str = entry.get_password().ok()?; + let origin = PemOrigin::Keyring { + service: SERVICE_NAME.to_string(), + username, + }; + let pem = pem_str.parse::().ok()?; + let session_identity = + load_plaintext_identity(&pem, &IdentityKeyAlgorithm::Prime256v1, &origin).ok()?; + + let (from_key, signed_delegations) = delegation::to_agent_types(&chain).ok()?; + DelegatedIdentity::new(from_key, Box::new(session_identity), signed_delegations) + .ok() + .map(|id| Arc::new(id) as Arc) +} + +/// Creates a short-lived P256 session delegation signed by `identity`, stores the session +/// key in the keyring and the chain on disk, and returns a `DelegatedIdentity`. +/// +/// Returns `None` if any step fails; callers fall back to using `identity` directly. +fn create_pem_session_and_build_identity( + dirs: LRead<&IdentityPaths>, + name: &str, + identity: &dyn Identity, + duration: std::time::Duration, +) -> Option> { + let signer_pubkey = identity.public_key()?; + + let mut key_bytes = Zeroizing::new([0u8; 32]); + rand::rng().fill_bytes(key_bytes.as_mut()); + let session_key = p256::SecretKey::from_slice(&key_bytes[..]) + .expect("random 32 bytes are a valid p256 scalar"); + let session_identity = Prime256v1Identity::from_private_key(session_key.clone()); + let session_pubkey = session_identity + .public_key() + .expect("p256 always has a public key"); + + let now_nanos = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system clock before unix epoch") + .as_nanos() as u64; + let expiration = now_nanos.saturating_add(duration.as_nanos() as u64); + + let agent_delegation = AgentDelegation { + pubkey: session_pubkey.clone(), + expiration, + targets: None, + }; + + let sig = identity.sign_delegation(&agent_delegation).ok()?; + let signature_bytes = sig.signature?; + + // Walk any existing delegation chain (e.g. if `identity` is already delegated), + // then append the new delegation to the session key. + let mut wire_delegations: Vec = sig + .delegations + .unwrap_or_default() + .into_iter() + .map(|sd| delegation::SignedDelegation { + signature: hex::encode(&sd.signature), + delegation: delegation::Delegation { + pubkey: hex::encode(&sd.delegation.pubkey), + expiration: format!("{:x}", sd.delegation.expiration), + targets: sd + .delegation + .targets + .as_ref() + .map(|ts| ts.iter().map(|p| hex::encode(p.as_slice())).collect()), + }, + }) + .collect(); + + wire_delegations.push(SignedDelegation { + signature: hex::encode(&signature_bytes), + delegation: delegation::Delegation { + pubkey: hex::encode(&session_pubkey), + expiration: format!("{expiration:x}"), + targets: None, + }, + }); + + let chain = delegation::DelegationChain { + public_key: hex::encode(&signer_pubkey), + delegations: wire_delegations, + }; + + // Store session key in keyring; bail out if unavailable. + let doc = session_key.to_pkcs8_der().expect("infallible PKI encoding"); + let pem_str: Zeroizing = doc + .to_pem(PrivateKeyInfo::PEM_LABEL, Default::default()) + .expect("infallible PKI encoding"); + let entry = Entry::new(SERVICE_NAME, &dlg_keyring_key(name)).ok()?; + entry.set_password(&pem_str).ok()?; + + // Store chain on disk; on failure undo the keyring entry and bail. + let chain_path = match (*dirs).ensure_delegation_chain_path(name) { + Ok(p) => p, + Err(_) => { + let _ = entry.delete_credential(); + return None; + } + }; + if delegation::save(&chain_path, &chain).is_err() { + let _ = entry.delete_credential(); + return None; + } + + // Build identity directly from in-memory data — no read-back needed. + let (from_key, signed_delegations) = delegation::to_agent_types(&chain).ok()?; + let session_arc: Arc = Arc::new(session_identity); + DelegatedIdentity::new(from_key, Box::new(session_arc), signed_delegations) + .ok() + .map(|id| Arc::new(id) as Arc) +} + fn load_keyring_identity( name: &str, algorithm: &IdentityKeyAlgorithm, @@ -513,12 +672,14 @@ pub enum LoadIdentityInContextError { pub async fn load_identity_in_context( dirs: LRead<&IdentityPaths>, password_func: impl FnOnce() -> Result, + pem_session_duration: Option, ) -> Result, LoadIdentityInContextError> { let identity = load_identity( dirs, &IdentityList::load_from(dirs)?, &(IdentityDefaults::load_from(dirs)?).default, password_func, + pem_session_duration, )?; Ok(identity) @@ -781,6 +942,24 @@ pub fn rename_identity( let new_path = dirs.key_pem_path(new_name); let contents = fs::read(&old_path).context(CopyKeyFileSnafu)?; fs::write(&new_path, &contents).context(CopyKeyFileSnafu)?; + + // Best-effort: migrate any cached session delegation. + if let Ok(old_entry) = Entry::new(SERVICE_NAME, &dlg_keyring_key(old_name)) + && let Ok(pem_str) = old_entry.get_password() + { + if let Ok(new_entry) = Entry::new(SERVICE_NAME, &dlg_keyring_key(new_name)) { + let _ = new_entry.set_password(&pem_str); + } + let _ = old_entry.delete_credential(); + } + let old_chain_path = dirs.delegation_chain_path(old_name); + if let Ok(chain_bytes) = fs::read(&old_chain_path) + && let Ok(new_chain_path) = (*dirs).ensure_delegation_chain_path(new_name) + { + let _ = fs::write(&new_chain_path, &chain_bytes); + let _ = fs::remove_file(&old_chain_path); + } + OldKeyMaterial::Pem(old_path) } IdentitySpec::Keyring { .. } => { @@ -1006,6 +1185,11 @@ pub fn delete_identity( // Delete the PEM file let pem_path = dirs.key_pem_path(name); fs::remove_file(&pem_path)?; + // Best-effort: clean up any cached session delegation. + if let Ok(entry) = Entry::new(SERVICE_NAME, &dlg_keyring_key(name)) { + let _ = entry.delete_credential(); + } + let _ = fs::remove_file(&dirs.delegation_chain_path(name)); } IdentitySpec::Keyring { .. } => { // Delete the keyring entry diff --git a/crates/icp/src/identity/mod.rs b/crates/icp/src/identity/mod.rs index c59384647..ea684dfab 100644 --- a/crates/icp/src/identity/mod.rs +++ b/crates/icp/src/identity/mod.rs @@ -1,4 +1,7 @@ -use std::sync::{Arc, Mutex}; +use std::{ + sync::{Arc, Mutex}, + time::Duration, +}; use async_trait::async_trait; use ic_agent::Identity; @@ -120,6 +123,7 @@ pub type PasswordFunc = Box Result + Send + Sync>; pub struct Loader { dir: IdentityDirectories, password_func: PasswordFunc, + pem_session_duration: Option, telemetry_data: Arc, #[allow(clippy::type_complexity)] cache: Mutex, Option)>>, @@ -129,11 +133,13 @@ impl Loader { pub fn new( dir: IdentityDirectories, password_func: PasswordFunc, + pem_session_duration: Option, telemetry_data: Arc, ) -> Self { Self { dir, password_func, + pem_session_duration, telemetry_data, cache: Mutex::new(HashMap::new()), } @@ -151,13 +157,20 @@ impl Load for Loader { } let password_func = &self.password_func; + let pem_session_duration = self.pem_session_duration; let (identity, storage_type) = match &id { IdentitySelection::Default => { self.dir .with_read(async |dirs| -> Result<_, LoadIdentityInContextError> { let list = IdentityList::load_from(dirs)?; let default_name = manifest::IdentityDefaults::load_from(dirs)?.default; - let identity = load_identity(dirs, &list, &default_name, password_func)?; + let identity = load_identity( + dirs, + &list, + &default_name, + password_func, + pem_session_duration, + )?; let storage_type = list.identities.get(&default_name).map(|spec| spec.into()); Ok((identity, storage_type)) @@ -174,6 +187,7 @@ impl Load for Loader { &IdentityList::load_from(dirs)?, "anonymous", || unreachable!(), + None, )?, Some(IdentityStorageType::Anonymous), )) @@ -185,7 +199,8 @@ impl Load for Loader { self.dir .with_read(async |dirs| -> Result<_, LoadIdentityInContextError> { let list = IdentityList::load_from(dirs)?; - let identity = load_identity(dirs, &list, name, password_func)?; + let identity = + load_identity(dirs, &list, name, password_func, pem_session_duration)?; let storage_type = list.identities.get(name).map(|spec| spec.into()); Ok((identity, storage_type)) }) @@ -290,6 +305,7 @@ mod tests { let loader = Loader::new( dirs, Box::new(|| unimplemented!()), + None, Arc::new(TelemetryData::default()), ); let i1 = loader From d84041bfc5d4ae64a1bf844754e2c7e11dab4237 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Thu, 23 Apr 2026 07:44:30 -0700 Subject: [PATCH 2/6] Add a setting for it --- crates/icp-cli/src/commands/settings.rs | 71 +++++++++++++++++++++++++ crates/icp-cli/src/main.rs | 14 ++++- crates/icp/src/settings.rs | 12 +++++ docs/reference/cli.md | 16 ++++++ 4 files changed, 111 insertions(+), 2 deletions(-) diff --git a/crates/icp-cli/src/commands/settings.rs b/crates/icp-cli/src/commands/settings.rs index fd6d8bed5..4ded60593 100644 --- a/crates/icp-cli/src/commands/settings.rs +++ b/crates/icp-cli/src/commands/settings.rs @@ -1,3 +1,5 @@ +use std::{fmt, str::FromStr}; + use clap::{Args, Subcommand}; use icp::{ context::Context, @@ -28,6 +30,8 @@ enum Setting { Telemetry(TelemetryArgs), /// Enable or disable the CLI update check UpdateCheck(UpdateCheckArgs), + /// Set the session length for password-protected PEM identities + SessionLength(SessionLengthArgs), } #[derive(Debug, Args)] @@ -49,11 +53,52 @@ struct UpdateCheckArgs { value: Option, } +#[derive(Debug, Args)] +struct SessionLengthArgs { + /// Set to `m` (e.g. `5m`) or `disabled`. If omitted, prints the current value. + /// + /// Note that due to clock drift, 5 minutes are added to the given value, + /// so `5m` produces a 10-minute-expiry delegation. `disabled` turns off + /// session caching entirely. + value: Option, +} + +/// A session-length value: either `m` (whole minutes) or `disabled`. +#[derive(Debug, Clone)] +pub struct SessionLengthValue(pub Option); + +impl FromStr for SessionLengthValue { + type Err = String; + + fn from_str(s: &str) -> Result { + if s == "disabled" { + return Ok(Self(None)); + } + let digits = s + .strip_suffix('m') + .ok_or_else(|| format!("expected `m` or `disabled`, got `{s}`"))?; + let n: u32 = digits + .parse() + .map_err(|_| format!("expected a whole number before `m`, got `{digits}`"))?; + Ok(Self(Some(n))) + } +} + +impl fmt::Display for SessionLengthValue { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self.0 { + Some(n) => write!(f, "{n}m"), + None => write!(f, "disabled"), + } + } +} + pub(crate) async fn exec(ctx: &Context, args: &SettingsArgs) -> Result<(), anyhow::Error> { match &args.setting { Setting::Autocontainerize(sub_args) => exec_autocontainerize(ctx, sub_args).await, Setting::Telemetry(sub_args) => exec_telemetry(ctx, sub_args).await, Setting::UpdateCheck(sub_args) => exec_update_check(ctx, sub_args).await, + Setting::SessionLength(sub_args) => exec_session_length(ctx, sub_args).await, } } @@ -143,3 +188,29 @@ async fn exec_update_check(ctx: &Context, args: &UpdateCheckArgs) -> Result<(), } } } + +async fn exec_session_length(ctx: &Context, args: &SessionLengthArgs) -> Result<(), anyhow::Error> { + let dirs = ctx.dirs.settings()?; + + match &args.value { + Some(SessionLengthValue(value)) => { + let value = *value; + dirs.with_write(async |dirs| { + let mut settings = Settings::load_from(dirs.read())?; + settings.session_length = value; + settings.write_to(dirs)?; + info!("Set session-length to {}", SessionLengthValue(value)); + Ok(()) + }) + .await? + } + + None => { + let settings = dirs + .with_read(async |dirs| Settings::load_from(dirs)) + .await??; + println!("{}", SessionLengthValue(settings.session_length)); + Ok(()) + } + } +} diff --git a/crates/icp-cli/src/main.rs b/crates/icp-cli/src/main.rs index e335e4986..bcb4261bf 100644 --- a/crates/icp-cli/src/main.rs +++ b/crates/icp-cli/src/main.rs @@ -1,7 +1,7 @@ use anyhow::Error; use clap::{CommandFactory, Parser}; use commands::Command; -use icp::prelude::*; +use icp::{directories::Access, prelude::*}; use tracing::{Instrument, debug, info, subscriber::set_global_default, trace_span}; use tracing_subscriber::{Registry, layer::SubscriberExt}; @@ -152,11 +152,21 @@ async fn main() -> Result<(), Error> { .map_err(|e| e.to_string()) }), }; + let pem_session_duration = { + let dirs = icp::directories::Directories::new()?; + let settings_dirs = dirs.settings()?; + let settings = settings_dirs + .with_read(async |dirs| icp::settings::Settings::load_from(dirs)) + .await??; + settings + .session_length + .map(|m| std::time::Duration::from_secs(u64::from(m + 5) * 60)) + }; let ctx = icp::context::initialize( cli.project_root_override, cli.debug, password_func, - Some(std::time::Duration::from_secs(600)), + pem_session_duration, )?; let telemetry_session = telemetry::setup(&ctx, &raw_args, &Cli::command()).await; diff --git a/crates/icp/src/settings.rs b/crates/icp/src/settings.rs index 6ce09d25c..8f6e96f3a 100644 --- a/crates/icp/src/settings.rs +++ b/crates/icp/src/settings.rs @@ -65,6 +65,13 @@ pub struct Settings { /// Whether the CLI update check is enabled. #[serde(default)] pub update_check: UpdateCheck, + + /// Session length in minutes for password-protected PEM identities. + /// + /// Five minutes are added to this value at use time, so the default of + /// `Some(5)` produces a 10-minute session. `None` disables session caching. + #[serde(default = "default_session_length")] + pub session_length: Option, } #[derive(Debug, Clone, Copy, Default, Serialize, Deserialize, strum::Display, PartialEq, Eq)] @@ -84,6 +91,10 @@ fn default_telemetry_enabled() -> bool { true } +fn default_session_length() -> Option { + Some(5) +} + impl Default for Settings { fn default() -> Self { Self { @@ -91,6 +102,7 @@ impl Default for Settings { autocontainerize: false, telemetry_enabled: true, update_check: UpdateCheck::default(), + session_length: default_session_length(), } } } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 9ab50d494..9254a30c8 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -70,6 +70,7 @@ This document contains the help content for the `icp` command-line program. * [`icp settings autocontainerize`↴](#icp-settings-autocontainerize) * [`icp settings telemetry`↴](#icp-settings-telemetry) * [`icp settings update-check`↴](#icp-settings-update-check) +* [`icp settings session-length`↴](#icp-settings-session-length) * [`icp sync`↴](#icp-sync) * [`icp token`↴](#icp-token) * [`icp token balance`↴](#icp-token-balance) @@ -1485,6 +1486,7 @@ Configure user settings * `autocontainerize` — Use Docker for the network launcher even when native mode is requested * `telemetry` — Enable or disable anonymous usage telemetry * `update-check` — Enable or disable the CLI update check +* `session-length` — Set the session length for password-protected PEM identities @@ -1533,6 +1535,20 @@ Enable or disable the CLI update check +## `icp settings session-length` + +Set the session length for password-protected PEM identities + +**Usage:** `icp settings session-length [VALUE]` + +###### **Arguments:** + +* `` — Set to `m` (e.g. `5m`) or `disabled`. If omitted, prints the current value. + + Note that due to clock drift, 5 minutes are added to the given value, so `5m` produces a 10-minute-expiry delegation. `disabled` turns off session caching entirely. + + + ## `icp sync` Synchronize canisters From ed2e73c52f169ae12ada8d1086b02c1767d2f41b Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Thu, 23 Apr 2026 07:44:30 -0700 Subject: [PATCH 3/6] Add support to icp identity login --- crates/icp-cli/src/commands/identity/login.rs | 151 +++++++++++++----- crates/icp-cli/src/commands/identity/mod.rs | 1 - crates/icp-cli/src/main.rs | 6 +- crates/icp-cli/tests/identity_tests.rs | 93 +++++++++++ crates/icp/src/context/init.rs | 5 +- crates/icp/src/context/mod.rs | 4 + crates/icp/src/identity/key.rs | 123 +++++++++++--- crates/icp/src/identity/mod.rs | 17 +- docs/reference/cli.md | 18 +++ 9 files changed, 341 insertions(+), 77 deletions(-) diff --git a/crates/icp-cli/src/commands/identity/login.rs b/crates/icp-cli/src/commands/identity/login.rs index b2652e54a..c2e8b02e9 100644 --- a/crates/icp-cli/src/commands/identity/login.rs +++ b/crates/icp-cli/src/commands/identity/login.rs @@ -1,71 +1,122 @@ +use std::time::Duration; + use clap::Args; -use dialoguer::Password; use icp::{ context::Context, identity::{ key, - manifest::{IdentityList, IdentitySpec}, + manifest::{IdentityList, IdentitySpec, PemFormat}, }, + settings::Settings, }; use snafu::{OptionExt, ResultExt, Snafu}; use tracing::info; -use crate::commands::identity::link::ii; +use crate::commands::identity::{delegation::sign::DurationArg, link::ii}; -/// Re-authenticate an Internet Identity delegation +/// Re-authenticate an Internet Identity delegation or create a PEM session delegation #[derive(Debug, Args)] pub(crate) struct LoginArgs { /// Name of the identity to re-authenticate name: String, + + /// Session delegation duration (e.g. "30m", "8h", "1d"). + /// Required for PEM identities when session caching is disabled in settings. + /// Not applicable for Internet Identity. + #[arg(long)] + duration: Option, } pub(crate) async fn exec(ctx: &Context, args: &LoginArgs) -> Result<(), LoginError> { - let (algorithm, storage, host) = ctx + let spec = ctx .dirs .identity()? .with_read(async |dirs| { let list = IdentityList::load_from(dirs)?; - let spec = list - .identities + list.identities .get(&args.name) - .context(IdentityNotFoundSnafu { name: &args.name })?; - match spec { - IdentitySpec::InternetIdentity { - algorithm, - storage, - host, - .. - } => Ok((algorithm.clone(), *storage, host.clone())), - _ => NotIiSnafu { name: &args.name }.fail(), - } + .cloned() + .context(IdentityNotFoundSnafu { name: &args.name }) }) .await??; - let der_public_key = ctx - .dirs - .identity()? - .with_read(async |dirs| { - key::load_ii_session_public_key(dirs, &args.name, &algorithm, &storage, || { - Password::new() - .with_prompt("Enter identity password") - .interact() - .map_err(|e| e.to_string()) - }) - }) - .await? - .context(LoadSessionKeySnafu)?; + match spec { + IdentitySpec::InternetIdentity { + algorithm, + storage, + host, + .. + } => { + if args.duration.is_some() { + return DurationSnafu { name: &args.name }.fail(); + } - let chain = ii::recv_delegation(&host, &der_public_key) - .await - .context(PollSnafu)?; + let password_func = ctx.password_func.clone(); + let der_public_key = ctx + .dirs + .identity()? + .with_read(async |dirs| { + key::load_ii_session_public_key(dirs, &args.name, &algorithm, &storage, || { + password_func() + }) + }) + .await? + .context(LoadSessionKeySnafu)?; - ctx.dirs - .identity()? - .with_write(async |dirs| key::update_ii_delegation(dirs, &args.name, &chain)) - .await? - .context(UpdateDelegationSnafu)?; + let chain = ii::recv_delegation(&host, &der_public_key) + .await + .context(PollSnafu)?; + + ctx.dirs + .identity()? + .with_write(async |dirs| key::update_ii_delegation(dirs, &args.name, &chain)) + .await? + .context(UpdateDelegationSnafu)?; - info!("Identity `{}` re-authenticated", args.name); + info!("Identity `{}` re-authenticated", args.name); + } + + IdentitySpec::Pem { + format: PemFormat::Pbes2, + algorithm, + .. + } => { + let duration = match &args.duration { + Some(d) => Duration::from_nanos(d.as_nanos()) + Duration::from_secs(5 * 60), + None => { + let settings = ctx + .dirs + .settings()? + .with_read(async |dirs| Settings::load_from(dirs)) + .await??; + settings + .session_length + .map(|m| Duration::from_secs(u64::from(m + 5) * 60)) + .context(DurationRequiredSnafu { name: &args.name })? + } + }; + + let password_func = ctx.password_func.clone(); + ctx.dirs + .identity()? + .with_read(async |dirs| { + key::create_explicit_pem_session( + dirs, + &args.name, + &algorithm, + || password_func(), + duration, + ) + }) + .await? + .context(CreatePemSessionSnafu)?; + + info!("Session delegation created for identity `{}`", args.name); + } + _ => { + return UnsupportedIdentityTypeSnafu { name: &args.name }.fail(); + } + } Ok(()) } @@ -73,20 +124,31 @@ pub(crate) async fn exec(ctx: &Context, args: &LoginArgs) -> Result<(), LoginErr #[derive(Debug, Snafu)] pub(crate) enum LoginError { #[snafu(transparent)] - LockIdentityDir { source: icp::fs::lock::LockError }, + LockDir { source: icp::fs::lock::LockError }, #[snafu(transparent)] LoadManifest { source: icp::identity::manifest::LoadIdentityManifestError, }, + #[snafu(transparent)] + LoadSettings { + source: icp::settings::LoadSettingsError, + }, + #[snafu(display("no identity found with name `{name}`"))] IdentityNotFound { name: String }, + #[snafu(display("`--duration` cannot be used with Internet Identity `{name}`"))] + Duration { name: String }, + #[snafu(display( - "identity `{name}` is not an Internet Identity; use `icp identity link ii` instead" + "session caching is disabled; specify `--duration` to create a session delegation for `{name}`" ))] - NotIi { name: String }, + DurationRequired { name: String }, + + #[snafu(display("identity `{name}` does not support logins"))] + UnsupportedIdentityType { name: String }, #[snafu(display("failed to load II session key"))] LoadSessionKey { source: key::LoadIdentityError }, @@ -98,4 +160,9 @@ pub(crate) enum LoginError { UpdateDelegation { source: key::UpdateIiDelegationError, }, + + #[snafu(display("failed to create PEM session delegation"))] + CreatePemSession { + source: key::CreateExplicitPemSessionError, + }, } diff --git a/crates/icp-cli/src/commands/identity/mod.rs b/crates/icp-cli/src/commands/identity/mod.rs index 4b461bbaa..79534c539 100644 --- a/crates/icp-cli/src/commands/identity/mod.rs +++ b/crates/icp-cli/src/commands/identity/mod.rs @@ -26,7 +26,6 @@ pub(crate) enum Command { #[command(subcommand)] Link(link::Command), List(list::ListArgs), - #[command(hide = true)] // todo remove when II login is out of beta Login(login::LoginArgs), New(new::NewArgs), Principal(principal::PrincipalArgs), diff --git a/crates/icp-cli/src/main.rs b/crates/icp-cli/src/main.rs index bcb4261bf..a2462e0f9 100644 --- a/crates/icp-cli/src/main.rs +++ b/crates/icp-cli/src/main.rs @@ -1,3 +1,5 @@ +use std::sync::Arc; + use anyhow::Error; use clap::{CommandFactory, Parser}; use commands::Command; @@ -140,12 +142,12 @@ async fn main() -> Result<(), Error> { ); let password_func: icp::identity::PasswordFunc = match cli.identity_password_file { - Some(path) => Box::new(move || { + Some(path) => Arc::new(move || { icp::fs::read_to_string(&path) .map(|s| s.trim().to_string()) .map_err(|e| e.to_string()) }), - None => Box::new(|| { + None => Arc::new(|| { dialoguer::Password::new() .with_prompt("Enter identity password") .interact() diff --git a/crates/icp-cli/tests/identity_tests.rs b/crates/icp-cli/tests/identity_tests.rs index 1c06de97e..2ee4c4711 100644 --- a/crates/icp-cli/tests/identity_tests.rs +++ b/crates/icp-cli/tests/identity_tests.rs @@ -1508,3 +1508,96 @@ fn pem_session_delegation_avoids_second_password_prompt() { .assert() .success(); } + +/// `icp identity login --duration` explicitly creates a PEM session, allowing subsequent +/// commands to succeed without a password even when automatic session caching is disabled. +#[test] +fn pem_explicit_login_creates_session() { + let ctx = TestContext::new(); + + // Disable automatic session caching. + ctx.icp() + .args(["settings", "session-length", "disabled"]) + .assert() + .success(); + + let mut password_file = NamedTempFile::new().unwrap(); + password_file.write_all(b"test-password-xyz").unwrap(); + let password_path = password_file.into_temp_path(); + + ctx.icp() + .args([ + "identity", + "new", + "explicit-session-test", + "--storage", + "password", + ]) + .arg("--storage-password-file") + .arg(&password_path) + .assert() + .success(); + + // Explicit login creates the session delegation. + ctx.icp() + .arg("--identity-password-file") + .arg(&password_path) + .args([ + "identity", + "login", + "explicit-session-test", + "--duration", + "10m", + ]) + .assert() + .success(); + + // Session is now cached; subsequent commands succeed without a password. + let empty_file = NamedTempFile::new().unwrap(); + ctx.icp() + .arg("--identity-password-file") + .arg(empty_file.path()) + .args([ + "identity", + "principal", + "--identity", + "explicit-session-test", + ]) + .assert() + .success(); +} + +/// When automatic session caching is disabled and `--duration` is omitted, +/// `icp identity login` must fail with a clear error for PEM identities. +#[test] +fn pem_login_requires_duration_when_sessions_disabled() { + let ctx = TestContext::new(); + + ctx.icp() + .args(["settings", "session-length", "disabled"]) + .assert() + .success(); + + let mut password_file = NamedTempFile::new().unwrap(); + password_file.write_all(b"test-password-xyz").unwrap(); + let password_path = password_file.into_temp_path(); + + ctx.icp() + .args([ + "identity", + "new", + "no-duration-test", + "--storage", + "password", + ]) + .arg("--storage-password-file") + .arg(&password_path) + .assert() + .success(); + + ctx.icp() + .args(["identity", "login", "no-duration-test"]) + .assert() + .failure() + .stderr(contains("--duration")); +} diff --git a/crates/icp/src/context/init.rs b/crates/icp/src/context/init.rs index 12f6e3cf8..b371b48c7 100644 --- a/crates/icp/src/context/init.rs +++ b/crates/icp/src/context/init.rs @@ -89,10 +89,12 @@ pub fn initialize( // Telemetry data bag (written by subsystems, read at session finish) let telemetry_data = Arc::new(crate::telemetry_data::TelemetryData::default()); + let password_func: identity::PasswordFunc = Arc::from(password_func); + // Identity loader let idload = Arc::new(identity::Loader::new( dirs.identity().context(IdentityDirectorySnafu)?, - password_func, + password_func.clone(), pem_session_duration, telemetry_data.clone(), )); @@ -126,5 +128,6 @@ pub fn initialize( syncer, debug, telemetry_data, + password_func, }) } diff --git a/crates/icp/src/context/mod.rs b/crates/icp/src/context/mod.rs index 270d08a52..d54916beb 100644 --- a/crates/icp/src/context/mod.rs +++ b/crates/icp/src/context/mod.rs @@ -103,6 +103,9 @@ pub struct Context { /// Telemetry data collected during command execution pub telemetry_data: Arc, + + /// Password reader for identity decryption; shared with the identity loader. + pub password_func: Arc Result + Send + Sync>, } impl Context { @@ -576,6 +579,7 @@ impl Context { syncer: Arc::new(crate::canister::sync::UnimplementedMockSyncer), debug: false, telemetry_data: Arc::new(crate::telemetry_data::TelemetryData::default()), + password_func: Arc::new(|| Err("no password available in mock context".to_string())), } } } diff --git a/crates/icp/src/identity/key.rs b/crates/icp/src/identity/key.rs index eb4dadfaa..4d19dce16 100644 --- a/crates/icp/src/identity/key.rs +++ b/crates/icp/src/identity/key.rs @@ -190,7 +190,6 @@ fn load_pem_identity( ) -> Result, LoadIdentityError> { // For password-protected PEMs, check for a valid cached session delegation first. if *format == PemFormat::Pbes2 - && pem_session_duration.is_some() && let Some(id) = try_load_pem_session(dirs, name) { return Ok(id); @@ -213,7 +212,7 @@ fn load_pem_identity( // After unlocking a Pbes2 PEM, create and cache a short-lived session delegation. if *format == PemFormat::Pbes2 && let Some(duration) = pem_session_duration - && let Some(delegated) = + && let Ok(delegated) = create_pem_session_and_build_identity(dirs, name, &*identity, duration) { return Ok(delegated); @@ -333,17 +332,54 @@ fn try_load_pem_session(dirs: LRead<&IdentityPaths>, name: &str) -> Option) } +#[derive(Debug, Snafu)] +pub enum CreateExplicitPemSessionError { + #[snafu(transparent)] + ReadFile { source: crate::fs::IoError }, + + #[snafu(display("failed to parse PEM from `{path}`"))] + ParsePemForSession { + path: PathBuf, + #[snafu(source(from(pem::PemError, Box::new)))] + source: Box, + }, + + #[snafu(transparent)] + DecryptPem { source: LoadIdentityError }, + + #[snafu(display("failed to sign session delegation: {message}"))] + SignDelegation { message: String }, + + #[snafu(display("failed to create keyring entry for session key"))] + CreateSessionKeyringEntry { source: keyring::Error }, + + #[snafu(display("failed to store session key in keyring"))] + SetSessionKeyringPassword { source: keyring::Error }, + + #[snafu(display("failed to create session delegation directory"))] + EnsureSessionDelegationDir { source: crate::fs::IoError }, + + #[snafu(display("failed to save session delegation chain to `{path}`"))] + SaveSessionDelegation { + path: PathBuf, + source: delegation::SaveError, + }, +} + /// Creates a short-lived P256 session delegation signed by `identity`, stores the session /// key in the keyring and the chain on disk, and returns a `DelegatedIdentity`. /// -/// Returns `None` if any step fails; callers fall back to using `identity` directly. +/// Returns an error if any step fails. Automatic callers silence errors via `let Ok(...) =`; +/// explicit callers propagate them. fn create_pem_session_and_build_identity( dirs: LRead<&IdentityPaths>, name: &str, identity: &dyn Identity, duration: std::time::Duration, -) -> Option> { - let signer_pubkey = identity.public_key()?; +) -> Result, CreateExplicitPemSessionError> { + let signer_pubkey = identity + .public_key() + .expect("called only with non-anonymous identity"); let mut key_bytes = Zeroizing::new([0u8; 32]); rand::rng().fill_bytes(key_bytes.as_mut()); @@ -366,8 +402,12 @@ fn create_pem_session_and_build_identity( targets: None, }; - let sig = identity.sign_delegation(&agent_delegation).ok()?; - let signature_bytes = sig.signature?; + let sig = identity + .sign_delegation(&agent_delegation) + .map_err(|message| CreateExplicitPemSessionError::SignDelegation { message })?; + let signature_bytes = sig + .signature + .expect("non-anonymous identity always produces a signature"); // Walk any existing delegation chain (e.g. if `identity` is already delegated), // then append the new delegation to the session key. @@ -403,33 +443,66 @@ fn create_pem_session_and_build_identity( delegations: wire_delegations, }; - // Store session key in keyring; bail out if unavailable. + // Store session key in keyring. let doc = session_key.to_pkcs8_der().expect("infallible PKI encoding"); let pem_str: Zeroizing = doc .to_pem(PrivateKeyInfo::PEM_LABEL, Default::default()) .expect("infallible PKI encoding"); - let entry = Entry::new(SERVICE_NAME, &dlg_keyring_key(name)).ok()?; - entry.set_password(&pem_str).ok()?; - - // Store chain on disk; on failure undo the keyring entry and bail. - let chain_path = match (*dirs).ensure_delegation_chain_path(name) { - Ok(p) => p, - Err(_) => { + let entry = + Entry::new(SERVICE_NAME, &dlg_keyring_key(name)).context(CreateSessionKeyringEntrySnafu)?; + entry + .set_password(&pem_str) + .context(SetSessionKeyringPasswordSnafu)?; + + // Store chain on disk; on failure undo the keyring entry. + let chain_path = (*dirs) + .ensure_delegation_chain_path(name) + .map_err(|source| { let _ = entry.delete_credential(); - return None; - } - }; - if delegation::save(&chain_path, &chain).is_err() { + CreateExplicitPemSessionError::EnsureSessionDelegationDir { source } + })?; + delegation::save(&chain_path, &chain).map_err(|source| { let _ = entry.delete_credential(); - return None; - } + CreateExplicitPemSessionError::SaveSessionDelegation { + path: chain_path.clone(), + source, + } + })?; // Build identity directly from in-memory data — no read-back needed. - let (from_key, signed_delegations) = delegation::to_agent_types(&chain).ok()?; + let (from_key, signed_delegations) = + delegation::to_agent_types(&chain).expect("freshly created chain is always valid"); let session_arc: Arc = Arc::new(session_identity); - DelegatedIdentity::new(from_key, Box::new(session_arc), signed_delegations) - .ok() - .map(|id| Arc::new(id) as Arc) + let delegated = DelegatedIdentity::new(from_key, Box::new(session_arc), signed_delegations) + .expect("freshly created chain is always valid"); + Ok(Arc::new(delegated) as Arc) +} + +/// Creates a PEM session delegation explicitly, prompting for the password and storing +/// the new session key and chain. +/// +/// Unlike the automatic path (which silences errors), this propagates them. +/// The `duration` should already include the 5-minute clock-drift boost. +pub fn create_explicit_pem_session( + dirs: LRead<&IdentityPaths>, + name: &str, + algorithm: &IdentityKeyAlgorithm, + password_func: impl FnOnce() -> Result, + duration: Duration, +) -> Result<(), CreateExplicitPemSessionError> { + let pem_path = dirs.key_pem_path(name); + let origin = PemOrigin::File { + path: pem_path.clone(), + }; + + let doc = fs::read_to_string(&pem_path)? + .parse::() + .context(ParsePemForSessionSnafu { path: &pem_path })?; + + let identity = load_pbes2_identity(&doc, algorithm, password_func, &origin)?; + + create_pem_session_and_build_identity(dirs, name, &*identity, duration)?; + Ok(()) } fn load_keyring_identity( diff --git a/crates/icp/src/identity/mod.rs b/crates/icp/src/identity/mod.rs index ea684dfab..bc3ea9cbc 100644 --- a/crates/icp/src/identity/mod.rs +++ b/crates/icp/src/identity/mod.rs @@ -118,7 +118,7 @@ pub trait Load: Sync + Send { } /// A function that prompts for a password and returns it, or an error message. -pub type PasswordFunc = Box Result + Send + Sync>; +pub type PasswordFunc = Arc Result + Send + Sync>; pub struct Loader { dir: IdentityDirectories, @@ -156,7 +156,7 @@ impl Load for Loader { return Ok(Arc::clone(cached)); } - let password_func = &self.password_func; + let password_func = self.password_func.clone(); let pem_session_duration = self.pem_session_duration; let (identity, storage_type) = match &id { IdentitySelection::Default => { @@ -168,7 +168,7 @@ impl Load for Loader { dirs, &list, &default_name, - password_func, + || password_func(), pem_session_duration, )?; let storage_type = @@ -199,8 +199,13 @@ impl Load for Loader { self.dir .with_read(async |dirs| -> Result<_, LoadIdentityInContextError> { let list = IdentityList::load_from(dirs)?; - let identity = - load_identity(dirs, &list, name, password_func, pem_session_duration)?; + let identity = load_identity( + dirs, + &list, + name, + || password_func(), + pem_session_duration, + )?; let storage_type = list.identities.get(name).map(|spec| spec.into()); Ok((identity, storage_type)) }) @@ -304,7 +309,7 @@ mod tests { .unwrap(); let loader = Loader::new( dirs, - Box::new(|| unimplemented!()), + Arc::new(|| unimplemented!()), None, Arc::new(TelemetryData::default()), ); diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 9254a30c8..1edb3822b 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -53,6 +53,7 @@ This document contains the help content for the `icp` command-line program. * [`icp identity link`↴](#icp-identity-link) * [`icp identity link hsm`↴](#icp-identity-link-hsm) * [`icp identity list`↴](#icp-identity-list) +* [`icp identity login`↴](#icp-identity-login) * [`icp identity new`↴](#icp-identity-new) * [`icp identity principal`↴](#icp-identity-principal) * [`icp identity rename`↴](#icp-identity-rename) @@ -929,6 +930,7 @@ Manage your identities * `import` — Import a new identity * `link` — Link an external key to a new identity * `list` — List the identities +* `login` — Re-authenticate an Internet Identity delegation or create a PEM session delegation * `new` — Create a new identity * `principal` — Display the principal for the current identity * `rename` — Rename an identity @@ -1154,6 +1156,22 @@ List the identities +## `icp identity login` + +Re-authenticate an Internet Identity delegation or create a PEM session delegation + +**Usage:** `icp identity login [OPTIONS] ` + +###### **Arguments:** + +* `` — Name of the identity to re-authenticate + +###### **Options:** + +* `--duration ` — Session delegation duration (e.g. "30m", "8h", "1d"). Required for PEM identities when session caching is disabled in settings. Not applicable for Internet Identity + + + ## `icp identity new` Create a new identity From c68547a063d143fa862f11bd4cc1e91662d8f784 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Thu, 23 Apr 2026 08:16:42 -0700 Subject: [PATCH 4/6] clippy --- crates/icp/src/context/init.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/icp/src/context/init.rs b/crates/icp/src/context/init.rs index b371b48c7..b8642dce5 100644 --- a/crates/icp/src/context/init.rs +++ b/crates/icp/src/context/init.rs @@ -89,8 +89,6 @@ pub fn initialize( // Telemetry data bag (written by subsystems, read at session finish) let telemetry_data = Arc::new(crate::telemetry_data::TelemetryData::default()); - let password_func: identity::PasswordFunc = Arc::from(password_func); - // Identity loader let idload = Arc::new(identity::Loader::new( dirs.identity().context(IdentityDirectorySnafu)?, From befd24b1290c65ab4c924fd6f1b0033b2c8498c8 Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Thu, 23 Apr 2026 08:23:09 -0700 Subject: [PATCH 5/6] feedback Co-authored-by: Copilot --- crates/icp-cli/src/commands/identity/login.rs | 7 ++++--- crates/icp-cli/src/main.rs | 2 +- crates/icp/src/identity/key.rs | 16 ++++++++++------ docs/reference/cli.md | 2 +- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/crates/icp-cli/src/commands/identity/login.rs b/crates/icp-cli/src/commands/identity/login.rs index c2e8b02e9..e8056555f 100644 --- a/crates/icp-cli/src/commands/identity/login.rs +++ b/crates/icp-cli/src/commands/identity/login.rs @@ -20,9 +20,10 @@ pub(crate) struct LoginArgs { /// Name of the identity to re-authenticate name: String, - /// Session delegation duration (e.g. "30m", "8h", "1d"). + /// Session delegation duration (e.g. "30m", "8h", "1d"). Note that 5m extra is + /// added when creating the delegation to account for clock drift. /// Required for PEM identities when session caching is disabled in settings. - /// Not applicable for Internet Identity. + /// Not applicable for Internet Identity (yet). #[arg(long)] duration: Option, } @@ -91,7 +92,7 @@ pub(crate) async fn exec(ctx: &Context, args: &LoginArgs) -> Result<(), LoginErr .await??; settings .session_length - .map(|m| Duration::from_secs(u64::from(m + 5) * 60)) + .map(|m| Duration::from_secs((u64::from(m) + 5) * 60)) .context(DurationRequiredSnafu { name: &args.name })? } }; diff --git a/crates/icp-cli/src/main.rs b/crates/icp-cli/src/main.rs index a2462e0f9..857ed7e5c 100644 --- a/crates/icp-cli/src/main.rs +++ b/crates/icp-cli/src/main.rs @@ -162,7 +162,7 @@ async fn main() -> Result<(), Error> { .await??; settings .session_length - .map(|m| std::time::Duration::from_secs(u64::from(m + 5) * 60)) + .map(|m| std::time::Duration::from_secs((u64::from(m) + 5) * 60)) }; let ctx = icp::context::initialize( cli.project_root_override, diff --git a/crates/icp/src/identity/key.rs b/crates/icp/src/identity/key.rs index 4d19dce16..674a3cfaa 100644 --- a/crates/icp/src/identity/key.rs +++ b/crates/icp/src/identity/key.rs @@ -294,7 +294,7 @@ const SERVICE_NAME: &str = "icp-cli"; /// Returns the keyring username for a delegation session key. /// -/// The `delegate:` prefix discriminates session keys from regular identities — +/// The `delegation:` prefix discriminates session keys from regular identities — /// no code path that operates on regular identity names can accidentally /// export these keys. fn dlg_keyring_key(name: &str) -> String { @@ -390,11 +390,15 @@ fn create_pem_session_and_build_identity( .public_key() .expect("p256 always has a public key"); - let now_nanos = SystemTime::now() - .duration_since(UNIX_EPOCH) - .expect("system clock before unix epoch") - .as_nanos() as u64; - let expiration = now_nanos.saturating_add(duration.as_nanos() as u64); + let now_nanos = u64::try_from( + SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("system clock before unix epoch") + .as_nanos(), + ) + .expect("time wrapped around"); + let expiration = + now_nanos.saturating_add(duration.as_nanos().try_into().expect("time wrapped around")); let agent_delegation = AgentDelegation { pubkey: session_pubkey.clone(), diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 1edb3822b..9694f9b85 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -1168,7 +1168,7 @@ Re-authenticate an Internet Identity delegation or create a PEM session delegati ###### **Options:** -* `--duration ` — Session delegation duration (e.g. "30m", "8h", "1d"). Required for PEM identities when session caching is disabled in settings. Not applicable for Internet Identity +* `--duration ` — Session delegation duration (e.g. "30m", "8h", "1d"). Note that 5m extra is added when creating the delegation to account for clock drift. Required for PEM identities when session caching is disabled in settings. Not applicable for Internet Identity (yet) From d997166e09541a60883b931bb2197a66afb42d1f Mon Sep 17 00:00:00 2001 From: Adam Spofford Date: Fri, 24 Apr 2026 08:30:43 -0700 Subject: [PATCH 6/6] feedback --- CHANGELOG.md | 2 +- crates/icp-cli/src/commands/settings.rs | 30 ++++++++--- crates/icp-cli/tests/identity_tests.rs | 54 ++++++++++++++++++++ crates/icp/src/identity/key.rs | 66 ++++++++++++++++++++----- docs/reference/cli.md | 2 +- 5 files changed, 133 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4100e121..2d36a2701 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Unreleased -* feat: Password-protected identities now only need your password once every five minutes (configurable) +* feat: Password-protected identities now only need your password once per session. The session length defaults to 5 minutes and can be changed with `icp settings session-length ` (e.g. `30m`, `1h`) or turned off with `icp settings session-length disabled`. You can also explicitly create or refresh a session with `icp identity login [--duration ]`. * feat: `icp identity delegation request/sign/use` now permit creating and importing identity delegations * feat: `icp identity import` now takes `--seed-curve`, for seed phrases for non-k256 keys. * fix: `icp canister settings show` now outputs only the canister settings, consistent with the command name diff --git a/crates/icp-cli/src/commands/settings.rs b/crates/icp-cli/src/commands/settings.rs index 4ded60593..231f2d3af 100644 --- a/crates/icp-cli/src/commands/settings.rs +++ b/crates/icp-cli/src/commands/settings.rs @@ -55,7 +55,7 @@ struct UpdateCheckArgs { #[derive(Debug, Args)] struct SessionLengthArgs { - /// Set to `m` (e.g. `5m`) or `disabled`. If omitted, prints the current value. + /// Duration (e.g. `5m`, `1h`, `2d`) or `disabled`. If omitted, prints the current value. /// /// Note that due to clock drift, 5 minutes are added to the given value, /// so `5m` produces a 10-minute-expiry delegation. `disabled` turns off @@ -63,7 +63,7 @@ struct SessionLengthArgs { value: Option, } -/// A session-length value: either `m` (whole minutes) or `disabled`. +/// A session-length value: a duration with suffix (`m`, `h`, `d`) or `disabled`. #[derive(Debug, Clone)] pub struct SessionLengthValue(pub Option); @@ -74,13 +74,27 @@ impl FromStr for SessionLengthValue { if s == "disabled" { return Ok(Self(None)); } - let digits = s - .strip_suffix('m') - .ok_or_else(|| format!("expected `m` or `disabled`, got `{s}`"))?; - let n: u32 = digits + let (digits, unit_secs) = if let Some(d) = s.strip_suffix('m') { + (d, 60u64) + } else if let Some(d) = s.strip_suffix('h') { + (d, 3600) + } else if let Some(d) = s.strip_suffix('d') { + (d, 86400) + } else { + return Err(format!( + "expected a duration like `5m`, `1h`, `2d`, or `disabled`; got `{s}`" + )); + }; + let n: u64 = digits .parse() - .map_err(|_| format!("expected a whole number before `m`, got `{digits}`"))?; - Ok(Self(Some(n))) + .map_err(|_| format!("expected a whole number before the suffix, got `{digits}`"))?; + let total_secs = n + .checked_mul(unit_secs) + .ok_or_else(|| "duration too large".to_string())?; + // Round up to whole minutes. + let minutes = total_secs.div_ceil(60); + let minutes = u32::try_from(minutes).map_err(|_| "duration too large".to_string())?; + Ok(Self(Some(minutes))) } } diff --git a/crates/icp-cli/tests/identity_tests.rs b/crates/icp-cli/tests/identity_tests.rs index 2ee4c4711..ac1cf9a63 100644 --- a/crates/icp-cli/tests/identity_tests.rs +++ b/crates/icp-cli/tests/identity_tests.rs @@ -1601,3 +1601,57 @@ fn pem_login_requires_duration_when_sessions_disabled() { .failure() .stderr(contains("--duration")); } + +/// Renaming a password-protected identity migrates its session delegation so that +/// subsequent commands using the new name succeed without a password. +#[test] +fn pem_session_migrated_on_rename() { + let ctx = TestContext::new(); + + let mut password_file = NamedTempFile::new().unwrap(); + password_file.write_all(b"rename-session-pw").unwrap(); + let password_path = password_file.into_temp_path(); + + // Create a password-protected identity. + ctx.icp() + .args([ + "identity", + "new", + "rename-session-src", + "--storage", + "password", + ]) + .arg("--storage-password-file") + .arg(&password_path) + .assert() + .success(); + + // First use: creates a session delegation under the original name. + ctx.icp() + .arg("--identity-password-file") + .arg(&password_path) + .args(["identity", "principal", "--identity", "rename-session-src"]) + .assert() + .success(); + + // Rename the identity — session delegation must be migrated to the new name. + ctx.icp() + .args([ + "identity", + "rename", + "rename-session-src", + "rename-session-dst", + ]) + .assert() + .success(); + + // Use new name with an empty password file: session delegation must be reused, + // proving the migration happened and decryption was not attempted. + let empty_file = NamedTempFile::new().unwrap(); + ctx.icp() + .arg("--identity-password-file") + .arg(empty_file.path()) + .args(["identity", "principal", "--identity", "rename-session-dst"]) + .assert() + .success(); +} diff --git a/crates/icp/src/identity/key.rs b/crates/icp/src/identity/key.rs index 674a3cfaa..00c527e8e 100644 --- a/crates/icp/src/identity/key.rs +++ b/crates/icp/src/identity/key.rs @@ -23,6 +23,7 @@ use rand::Rng; use scrypt::Params; use sec1::{der::Decode, pem::PemLabel}; use snafu::{OptionExt, ResultExt, Snafu, ensure}; +use tracing::debug; use url::Url; use zeroize::Zeroizing; @@ -212,10 +213,11 @@ fn load_pem_identity( // After unlocking a Pbes2 PEM, create and cache a short-lived session delegation. if *format == PemFormat::Pbes2 && let Some(duration) = pem_session_duration - && let Ok(delegated) = - create_pem_session_and_build_identity(dirs, name, &*identity, duration) { - return Ok(delegated); + match create_pem_session_and_build_identity(dirs, name, &*identity, duration) { + Ok(delegated) => return Ok(delegated), + Err(e) => debug!(identity = name, "failed to create session delegation: {e}"), + } } Ok(identity) @@ -308,26 +310,68 @@ fn dlg_keyring_key(name: &str) -> String { fn try_load_pem_session(dirs: LRead<&IdentityPaths>, name: &str) -> Option> { // Load chain from disk; missing file is the common case on first use. let chain_path = dirs.delegation_chain_path(name); - let chain = delegation::load(&chain_path).ok()?; + let chain = delegation::load(&chain_path) + .inspect_err(|e| debug!(identity = name, "no cached session chain: {e}")) + .ok()?; - if delegation::is_expiring_soon(&chain, FIVE_MINUTES_NANOS).ok()? { + if delegation::is_expiring_soon(&chain, FIVE_MINUTES_NANOS) + .inspect_err(|e| debug!(identity = name, "failed to check session expiry: {e}")) + .ok()? + { + debug!( + identity = name, + "cached session is expiring soon; will re-authenticate" + ); return None; } // Load session key from keyring. let username = dlg_keyring_key(name); - let entry = Entry::new(SERVICE_NAME, &username).ok()?; - let pem_str = entry.get_password().ok()?; + let entry = Entry::new(SERVICE_NAME, &username) + .inspect_err(|e| { + debug!( + identity = name, + "failed to open keyring entry for session key: {e}" + ) + }) + .ok()?; + let pem_str = entry + .get_password() + .inspect_err(|e| { + debug!( + identity = name, + "failed to read session key from keyring: {e}" + ) + }) + .ok()?; let origin = PemOrigin::Keyring { service: SERVICE_NAME.to_string(), username, }; - let pem = pem_str.parse::().ok()?; + let pem = pem_str + .parse::() + .inspect_err(|e| debug!(identity = name, "failed to parse session key PEM: {e}")) + .ok()?; let session_identity = - load_plaintext_identity(&pem, &IdentityKeyAlgorithm::Prime256v1, &origin).ok()?; - - let (from_key, signed_delegations) = delegation::to_agent_types(&chain).ok()?; + load_plaintext_identity(&pem, &IdentityKeyAlgorithm::Prime256v1, &origin) + .inspect_err(|e| debug!(identity = name, "failed to load session identity: {e}")) + .ok()?; + + let (from_key, signed_delegations) = delegation::to_agent_types(&chain) + .inspect_err(|e| { + debug!( + identity = name, + "failed to convert session delegation chain: {e}" + ) + }) + .ok()?; DelegatedIdentity::new(from_key, Box::new(session_identity), signed_delegations) + .inspect_err(|e| { + debug!( + identity = name, + "failed to construct delegated identity: {e}" + ) + }) .ok() .map(|id| Arc::new(id) as Arc) } diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 9694f9b85..3510387af 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -1561,7 +1561,7 @@ Set the session length for password-protected PEM identities ###### **Arguments:** -* `` — Set to `m` (e.g. `5m`) or `disabled`. If omitted, prints the current value. +* `` — Duration (e.g. `5m`, `1h`, `2d`) or `disabled`. If omitted, prints the current value. Note that due to clock drift, 5 minutes are added to the given value, so `5m` produces a 10-minute-expiry delegation. `disabled` turns off session caching entirely.