From 2522d5ca5149731bdd753e93fc229460c3a54aaa Mon Sep 17 00:00:00 2001 From: Sinan Eldem Date: Tue, 25 Aug 2026 21:13:29 +0300 Subject: [PATCH 1/4] feat(admin): add account and two-factor API contracts Defines the self-service account and MFA operations as `rc-core` traits with a bounded `rc-s3` transport, mirroring the server's contract. None of them takes a target identity, so they cannot be used to act on another account; managing someone else's credentials goes through the user-management API instead. `SecretValue` wraps every password and code so it is zeroed on drop and cannot be printed into a log or a panic message. The JSON and empty-body request paths are separate helpers: treating an empty response as a default value would report "two-factor authentication is off" for a response that never arrived. --- crates/core/src/admin/account.rs | 338 +++++++++++++++++++++++++++++++ crates/core/src/admin/mod.rs | 7 + crates/s3/src/admin.rs | 256 +++++++++++++++++++++-- 3 files changed, 580 insertions(+), 21 deletions(-) create mode 100644 crates/core/src/admin/account.rs diff --git a/crates/core/src/admin/account.rs b/crates/core/src/admin/account.rs new file mode 100644 index 00000000..70db9bd4 --- /dev/null +++ b/crates/core/src/admin/account.rs @@ -0,0 +1,338 @@ +//! Self-service account and two-factor authentication operations. +//! +//! These act on whoever the alias authenticates as: none of them take a target +//! identity, so `rc` cannot use them to touch another account. Managing someone +//! else's credentials goes through the user-management API instead. +//! +//! # Why the CLI never enforces the second factor +//! +//! `rc` signs every request with the alias's long-term access key. That path is +//! not gated by 2FA and must not be: gating it would break every script the +//! moment a human turned 2FA on for their own account, and it would add no +//! protection, because whoever holds the secret key already has full access. The +//! second factor guards session minting — the interactive console login — which +//! `rc` does not use. This is the same division AWS draws. + +use async_trait::async_trait; +use serde::{Deserialize, Serialize}; +use zeroize::Zeroizing; + +use crate::Result; + +/// Runtime capability required by the self-service account commands. +pub const ACCOUNT_CAPABILITY: &str = "admin.account.info"; + +/// Runtime capability required by the two-factor commands. +pub const ACCOUNT_MFA_CAPABILITY: &str = "admin.account.mfa"; + +/// Runtime capability required by the administrative MFA inspection/reset. +pub const USER_MFA_CAPABILITY: &str = "admin.user.mfa"; + +/// Response bound for account and MFA payloads. +/// +/// Generous enough for a QR SVG (a few kilobytes) and a full recovery-code set, +/// tight enough that a misbehaving endpoint cannot stream unbounded data into +/// the CLI. +pub const MAX_ACCOUNT_RESPONSE_BYTES: usize = 256 * 1024; + +/// How the calling credential was established. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum IdentityType { + Root, + Iam, + Sts, + ServiceAccount, +} + +impl std::fmt::Display for IdentityType { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str(match self { + Self::Root => "root", + Self::Iam => "iam", + Self::Sts => "sts", + Self::ServiceAccount => "service-account", + }) + } +} + +/// Where the identity's long-term secret lives. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "kebab-case")] +pub enum CredentialsSource { + /// Provisioned from the server process environment; immutable at runtime. + Env, + /// Stored in the IAM object store; mutable through the admin API. + Iam, +} + +impl std::fmt::Display for CredentialsSource { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str(match self { + Self::Env => "env", + Self::Iam => "iam", + }) + } +} + +/// Which self-service mutations the server will accept for this identity. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, Default)] +pub struct AccountMutability { + #[serde(default)] + pub password: bool, + #[serde(default)] + pub username: bool, +} + +/// MFA state reported alongside the account summary. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub struct AccountMfaSummary { + #[serde(default)] + pub enabled: bool, + #[serde(default)] + pub pending: bool, + #[serde(default)] + pub activated_at: Option, + #[serde(default)] + pub recovery_codes_remaining: u32, + #[serde(default)] + pub last_verified_at: Option, + #[serde(default)] + pub enrollment_available: bool, + #[serde(default)] + pub enrollment_blocked_reason: Option, +} + +/// The identity behind the alias, as the server describes it. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct AccountInfo { + pub access_key: String, + pub identity_type: IdentityType, + #[serde(default)] + pub session_access_key: Option, + pub is_admin: bool, + pub status: String, + #[serde(default)] + pub member_of: Vec, + #[serde(default)] + pub policies: Vec, + pub credentials_source: CredentialsSource, + pub mutable: AccountMutability, + pub mfa: AccountMfaSummary, +} + +/// Two-factor state for the calling identity. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub struct MfaStatus { + #[serde(default)] + pub enabled: bool, + #[serde(default)] + pub pending: bool, + pub algorithm: String, + pub digits: u8, + pub period_seconds: u32, + #[serde(default)] + pub activated_at: Option, + #[serde(default)] + pub pending_expires_at: Option, + #[serde(default)] + pub recovery_codes_remaining: u32, + #[serde(default)] + pub last_verified_at: Option, + #[serde(default)] + pub enrollment_available: bool, + #[serde(default)] + pub enrollment_blocked_reason: Option, +} + +/// A started enrollment. +/// +/// The shared secret appears here exactly once. `rc` renders it and drops it; it +/// is never written to the alias config or any other file. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct MfaEnrollment { + pub secret_base32: String, + pub otpauth_uri: String, + /// Server-rendered SVG. Carried for parity with the console; `rc` prints the + /// terminal form instead. + #[serde(default)] + pub qr_svg: String, + /// Server-rendered Unicode block art, ready to print. + pub qr_utf8: String, + pub algorithm: String, + pub digits: u8, + pub period_seconds: u32, + pub expires_at: String, +} + +/// A freshly generated recovery-code set. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct RecoveryCodes { + pub recovery_codes: Vec, + pub generated_at: String, +} + +/// Sessions invalidated by a credential rotation. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, Default)] +pub struct PasswordChangeResult { + #[serde(default)] + pub sessions_revoked: u32, +} + +/// Another identity's two-factor state, for an administrator. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct UserMfaStatus { + pub access_key: String, + pub enabled: bool, + #[serde(default)] + pub activated_at: Option, + #[serde(default)] + pub recovery_codes_remaining: u32, +} + +/// A secret held only long enough to send, then zeroed. +/// +/// Wrapped so a password or code cannot survive in a freed allocation, and so +/// `Debug` cannot print it into a log or a panic message. +#[derive(Clone)] +pub struct SecretValue(Zeroizing); + +impl std::fmt::Debug for SecretValue { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("SecretValue([REDACTED])") + } +} + +impl SecretValue { + pub fn new(value: String) -> Self { + Self(Zeroizing::new(value)) + } + + pub fn expose(&self) -> &str { + self.0.as_str() + } + + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } +} + +#[async_trait] +pub trait AccountApi: Send + Sync { + /// Describe the identity the alias authenticates as. + async fn account_info(&self) -> Result; + + /// Rotate the alias identity's own secret key. + /// + /// The current secret is required as proof of knowledge; the server rejects + /// the call without it even though the request is signed. + async fn account_change_password( + &self, + current_secret_key: &SecretValue, + new_secret_key: &SecretValue, + ) -> Result; +} + +#[async_trait] +pub trait AccountMfaApi: Send + Sync { + async fn account_mfa_status(&self) -> Result; + + /// Start (or restart) an enrollment. Does not change the active factor. + async fn account_mfa_enroll(&self) -> Result; + + /// Confirm a pending enrollment and receive the first recovery-code set. + async fn account_mfa_activate(&self, code: &SecretValue) -> Result; + + /// Turn the factor off. Requires the code and the account password. + async fn account_mfa_disable( + &self, + code: &SecretValue, + current_secret_key: &SecretValue, + ) -> Result<()>; + + /// Replace the recovery-code set. + async fn account_mfa_recovery_codes(&self, code: &SecretValue) -> Result; +} + +#[async_trait] +pub trait UserCredentialApi: Send + Sync { + /// Reset another identity's secret key, preserving its status and policies. + async fn set_user_secret_key( + &self, + access_key: &str, + secret_key: &SecretValue, + ) -> Result; + + async fn user_mfa_status(&self, access_key: &str) -> Result; + + /// Clear another identity's second factor: the break-glass path for a user + /// who lost both their authenticator and their recovery codes. + async fn user_mfa_reset(&self, access_key: &str) -> Result<()>; +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn identity_type_renders_the_wire_value() { + assert_eq!(IdentityType::ServiceAccount.to_string(), "service-account"); + assert_eq!(IdentityType::Root.to_string(), "root"); + assert_eq!( + serde_json::to_string(&IdentityType::ServiceAccount).expect("serialize"), + "\"service-account\"" + ); + } + + #[test] + fn credentials_source_renders_the_wire_value() { + assert_eq!(CredentialsSource::Env.to_string(), "env"); + assert_eq!( + serde_json::to_string(&CredentialsSource::Iam).expect("serialize"), + "\"iam\"" + ); + } + + #[test] + fn secret_values_never_print_their_contents() { + let secret = SecretValue::new("super-secret".to_string()); + + assert_eq!(format!("{secret:?}"), "SecretValue([REDACTED])"); + assert!(!format!("{secret:?}").contains("super-secret")); + assert_eq!(secret.expose(), "super-secret"); + } + + #[test] + fn account_info_decodes_a_minimal_server_response() { + // Optional fields are absent on a server that has nothing to report; + // decoding must not require them. + let decoded: AccountInfo = serde_json::from_str( + r#"{ + "access_key": "sinan", + "identity_type": "iam", + "is_admin": true, + "status": "enabled", + "credentials_source": "iam", + "mutable": {"password": true, "username": false}, + "mfa": {} + }"#, + ) + .expect("deserialize"); + + assert_eq!(decoded.access_key, "sinan"); + assert!(decoded.mutable.password); + assert!(!decoded.mfa.enabled); + assert!(decoded.member_of.is_empty()); + } + + #[test] + fn mfa_status_decodes_without_optional_timestamps() { + let decoded: MfaStatus = + serde_json::from_str(r#"{"algorithm":"SHA1","digits":6,"period_seconds":30}"#) + .expect("deserialize"); + + assert!(!decoded.enabled); + assert_eq!(decoded.digits, 6); + assert!(decoded.activated_at.is_none()); + } +} diff --git a/crates/core/src/admin/mod.rs b/crates/core/src/admin/mod.rs index 7cca887f..208822f6 100644 --- a/crates/core/src/admin/mod.rs +++ b/crates/core/src/admin/mod.rs @@ -4,6 +4,7 @@ //! IAM users, policies, groups, service accounts, and cluster operations. mod access_keys; +mod account; mod bucket_metadata; mod capabilities; mod cluster; @@ -27,6 +28,12 @@ pub use access_keys::{ MAX_IAM_ACCESS_KEY_SELECTOR_BYTES, MAX_IAM_ACCESS_KEY_SELECTORS, MAX_IAM_ACCESS_KEYS_RESPONSE_BYTES, }; +pub use account::{ + ACCOUNT_CAPABILITY, ACCOUNT_MFA_CAPABILITY, AccountApi, AccountInfo, AccountMfaApi, + AccountMfaSummary, AccountMutability, CredentialsSource, IdentityType, + MAX_ACCOUNT_RESPONSE_BYTES, MfaEnrollment, MfaStatus, PasswordChangeResult, RecoveryCodes, + SecretValue, USER_MFA_CAPABILITY, UserCredentialApi, UserMfaStatus, +}; pub use bucket_metadata::{ BUCKET_METADATA_CAPABILITY, BucketMetadataApi, BucketMetadataArchive, MAX_BUCKET_METADATA_ARCHIVE_BYTES, diff --git a/crates/s3/src/admin.rs b/crates/s3/src/admin.rs index 50dabf2d..075ab911 100644 --- a/crates/s3/src/admin.rs +++ b/crates/s3/src/admin.rs @@ -14,12 +14,12 @@ use aws_sigv4::sign::v4; use bytes::Bytes; use futures::StreamExt; use rc_core::admin::{ - AccessKeyInfo, AccessKeyKind, AccessKeyProvider, AccessKeyRecord, AdminApi, BucketMetadataApi, - BucketMetadataArchive, BucketQuota, BulkAccessKeyApi, BulkAccessKeyQuery, CapabilityApi, - CapabilityAvailability, CapabilityEntry, CapabilityReport, ClusterInfo, - ClusterSnapshotDocument, ClusterSnapshotMetadata, ClusterSnapshotSummary, ConfigApi, - ConfigDocument, ConfigHelp, ConfigHistoryEntry, ConfigMutationResult, - CreateServiceAccountRequest, DecommissionPoolStatus, DecommissionStatus, + AccessKeyInfo, AccessKeyKind, AccessKeyProvider, AccessKeyRecord, AccountApi, AccountInfo, + AccountMfaApi, AdminApi, BucketMetadataApi, BucketMetadataArchive, BucketQuota, + BulkAccessKeyApi, BulkAccessKeyQuery, CapabilityApi, CapabilityAvailability, CapabilityEntry, + CapabilityReport, ClusterInfo, ClusterSnapshotDocument, ClusterSnapshotMetadata, + ClusterSnapshotSummary, ConfigApi, ConfigDocument, ConfigHelp, ConfigHistoryEntry, + ConfigMutationResult, CreateServiceAccountRequest, DecommissionPoolStatus, DecommissionStatus, DetailedHealthSnapshot, DiagnosticCapability, DiagnosticReadApi, EncryptedInspectArchive, ExtensionsCatalog, Group, GroupStatus, HealRuntimeState, HealScanMode, HealStartRequest, HealStatus, HealTaskRequest, IAM_ACCESS_KEYS_BULK_CAPABILITY, @@ -30,7 +30,7 @@ use rc_core::admin::{ InspectArchiveCapabilityContract, InspectArchiveTransportRequest, KmsApi, KmsBackendKind, KmsCacheSummary, KmsCancelKeyDeletionResult, KmsConfigSummary, KmsConfigureRequest, KmsCreateKeyRequest, KmsCreateKeyResult, KmsDeleteKeyRequest, KmsDeleteKeyResult, KmsKey, - KmsKeyPage, KmsKeyState, KmsKeyUsage, KmsServiceState, KmsStatus, + KmsKeyPage, KmsKeyState, KmsKeyUsage, KmsServiceState, KmsStatus, MAX_ACCOUNT_RESPONSE_BYTES, MAX_BUCKET_METADATA_ARCHIVE_BYTES, MAX_DIAGNOSTIC_RESPONSE_BYTES, MAX_IAM_ACCESS_KEY_RESULTS, MAX_IAM_ACCESS_KEYS_RESPONSE_BYTES, MAX_IAM_ARCHIVE_BYTES, MAX_IAM_IMPORT_RESPONSE_BYTES, MAX_IAM_POLICY_DETACH_REQUEST_BYTES, MAX_IAM_POLICY_DETACH_RESPONSE_BYTES, @@ -40,20 +40,20 @@ use rc_core::admin::{ MAX_SITE_REPLICATION_ERROR_RESPONSE_BYTES, MAX_SITE_REPLICATION_REPAIR_RESPONSE_BYTES, MAX_SITE_REPLICATION_REQUEST_BYTES, MAX_SITE_REPLICATION_SUCCESS_RESPONSE_BYTES, ManualTransitionJobResponse, ManualTransitionRunRequest, ManualTransitionRunResponse, - MetricsBatch, MetricsQuery, ModuleSwitches, ObservabilityApi, OidcMutationApi, - OidcMutationRequest, OidcMutationResult, OidcProvider, OidcProviderList, OidcReadApi, - OidcValidationRequest, OidcValidationResult, PeerSiteSpec, Policy, PolicyDetachEntity, - PolicyDetachRequest, PolicyDetachResult, PolicyEntitiesQuery, PolicyEntitiesResult, - PolicyEntity, PolicyInfo, PoolStatus, PoolTarget, RealtimeMetrics, RebalanceStartResult, - RebalanceStatus, ReplicateEditStatus, ReplicationDiff, ReplicationDiffApi, - ReplicationInspectionApi, ReplicationMetricScope, ReplicationMetrics, ReplicationMrf, - RuntimeCapabilitiesSnapshot, RuntimeCapabilityStatus, ScannerStatus, ServiceAccount, - ServiceAccountCreateResponse, ServiceActionResult, SiteRemoveSpec, SiteReplicationInfo, - SiteReplicationPeer, SiteReplicationRepairApi, SiteReplicationRepairCapabilityContract, - SiteReplicationRepairOperationStatus, SiteReplicationRepairPreflight, - SiteReplicationRepairRequest, SiteReplicationResyncOperation, SiteReplicationResyncStatus, - SiteStatusOptions, StorageInfo, UpdateGroupMembersRequest, UpdateServiceAccountRequest, User, - UserStatus, + MetricsBatch, MetricsQuery, MfaEnrollment, MfaStatus, ModuleSwitches, ObservabilityApi, + OidcMutationApi, OidcMutationRequest, OidcMutationResult, OidcProvider, OidcProviderList, + OidcReadApi, OidcValidationRequest, OidcValidationResult, PasswordChangeResult, PeerSiteSpec, + Policy, PolicyDetachEntity, PolicyDetachRequest, PolicyDetachResult, PolicyEntitiesQuery, + PolicyEntitiesResult, PolicyEntity, PolicyInfo, PoolStatus, PoolTarget, RealtimeMetrics, + RebalanceStartResult, RebalanceStatus, RecoveryCodes, ReplicateEditStatus, ReplicationDiff, + ReplicationDiffApi, ReplicationInspectionApi, ReplicationMetricScope, ReplicationMetrics, + ReplicationMrf, RuntimeCapabilitiesSnapshot, RuntimeCapabilityStatus, ScannerStatus, + SecretValue, ServiceAccount, ServiceAccountCreateResponse, ServiceActionResult, SiteRemoveSpec, + SiteReplicationInfo, SiteReplicationPeer, SiteReplicationRepairApi, + SiteReplicationRepairCapabilityContract, SiteReplicationRepairOperationStatus, + SiteReplicationRepairPreflight, SiteReplicationRepairRequest, SiteReplicationResyncOperation, + SiteReplicationResyncStatus, SiteStatusOptions, StorageInfo, UpdateGroupMembersRequest, + UpdateServiceAccountRequest, User, UserCredentialApi, UserMfaStatus, UserStatus, }; use rc_core::{Alias, Error, Result}; use reqwest::header::{CACHE_CONTROL, CONTENT_TYPE, HOST, HeaderMap, HeaderName, HeaderValue}; @@ -720,6 +720,81 @@ impl AdminClient { serde_json::from_slice(&response_body).map_err(Error::Json) } + /// One bounded, signed admin request for the account/MFA family, returning + /// the raw response body. + /// + /// Bounded because an MFA response carries a rendered QR and a recovery-code + /// set: generous, but never unbounded. + async fn request_account_bytes( + &self, + method: Method, + path: &str, + body: Option<&[u8]>, + ) -> Result> { + let url = self.admin_url(path); + let body_bytes = body.unwrap_or_default(); + let headers = self.request_headers(body_bytes)?; + let signed_headers = self + .sign_request(&method, &url, &headers, body_bytes) + .await?; + let mut request = self.http_client.request(method, &url); + for (name, value) in &signed_headers { + request = request.header(name, value); + } + if !body_bytes.is_empty() { + request = request.body(body_bytes.to_vec()); + } + + let response = request + .send() + .await + .map_err(|_| Error::Network("Account administration request failed".to_string()))?; + let status = response.status(); + let response_body = read_bounded_response_body( + response, + MAX_ACCOUNT_RESPONSE_BYTES, + "Account administration response", + ) + .await?; + + if !status.is_success() { + return Err(self.map_error(status, &String::from_utf8_lossy(&response_body))); + } + + Ok(response_body) + } + + /// A request whose response is a JSON document. + /// + /// An empty body is an error rather than a default value: silently returning + /// an empty struct would report "two-factor authentication is off" for a + /// response that never arrived. + async fn request_account_json Deserialize<'de>>( + &self, + method: Method, + path: &str, + body: Option<&[u8]>, + ) -> Result { + let response_body = self.request_account_bytes(method, path, body).await?; + if response_body.is_empty() { + return Err(Error::General( + "RustFS returned an empty account administration response".to_string(), + )); + } + serde_json::from_slice(&response_body).map_err(Error::Json) + } + + /// A request whose success carries no body. + async fn request_account_empty( + &self, + method: Method, + path: &str, + body: Option<&[u8]>, + ) -> Result<()> { + self.request_account_bytes(method, path, body).await?; + Ok(()) + } + async fn request_oidc Deserialize<'de>>( &self, method: Method, @@ -3321,6 +3396,145 @@ impl OidcReadApi for AdminClient { } } +#[async_trait] +impl AccountApi for AdminClient { + async fn account_info(&self) -> Result { + self.request_account_json(Method::GET, "/account/info", None) + .await + } + + async fn account_change_password( + &self, + current_secret_key: &SecretValue, + new_secret_key: &SecretValue, + ) -> Result { + if current_secret_key.is_empty() || new_secret_key.is_empty() { + return Err(Error::InvalidPath( + "Both the current and the new secret key are required".to_string(), + )); + } + + let body = serde_json::to_vec(&serde_json::json!({ + "current_secret_key": current_secret_key.expose(), + "new_secret_key": new_secret_key.expose(), + })) + .map_err(|_| Error::General("Failed to encode the password change request".to_string()))?; + + self.request_account_json(Method::POST, "/account/password", Some(&body)) + .await + } +} + +#[async_trait] +impl AccountMfaApi for AdminClient { + async fn account_mfa_status(&self) -> Result { + self.request_account_json(Method::GET, "/account/mfa", None) + .await + } + + async fn account_mfa_enroll(&self) -> Result { + // An empty JSON object rather than no body: the endpoint is a POST and + // some proxies drop a bodyless one. + self.request_account_json(Method::POST, "/account/mfa/enroll", Some(b"{}")) + .await + } + + async fn account_mfa_activate(&self, code: &SecretValue) -> Result { + let body = encode_code_request(code)?; + self.request_account_json(Method::POST, "/account/mfa/activate", Some(&body)) + .await + } + + async fn account_mfa_disable( + &self, + code: &SecretValue, + current_secret_key: &SecretValue, + ) -> Result<()> { + if code.is_empty() || current_secret_key.is_empty() { + return Err(Error::InvalidPath( + "Turning off two-factor authentication needs both a code and the account password" + .to_string(), + )); + } + + let body = serde_json::to_vec(&serde_json::json!({ + "code": code.expose(), + "current_secret_key": current_secret_key.expose(), + })) + .map_err(|_| Error::General("Failed to encode the disable request".to_string()))?; + + self.request_account_empty(Method::POST, "/account/mfa/disable", Some(&body)) + .await + } + + async fn account_mfa_recovery_codes(&self, code: &SecretValue) -> Result { + let body = encode_code_request(code)?; + self.request_account_json(Method::POST, "/account/mfa/recovery-codes", Some(&body)) + .await + } +} + +#[async_trait] +impl UserCredentialApi for AdminClient { + async fn set_user_secret_key( + &self, + access_key: &str, + secret_key: &SecretValue, + ) -> Result { + if access_key.is_empty() { + return Err(Error::InvalidPath( + "Access key must not be empty".to_string(), + )); + } + if secret_key.is_empty() { + return Err(Error::InvalidPath( + "Secret key must not be empty".to_string(), + )); + } + + let body = serde_json::to_vec(&serde_json::json!({ "secret_key": secret_key.expose() })) + .map_err(|_| Error::General("Failed to encode the secret key request".to_string()))?; + let path = format!( + "/set-user-secret-key?accessKey={}", + urlencoding::encode(access_key) + ); + + self.request_account_json(Method::PUT, &path, Some(&body)) + .await + } + + async fn user_mfa_status(&self, access_key: &str) -> Result { + if access_key.is_empty() { + return Err(Error::InvalidPath( + "Access key must not be empty".to_string(), + )); + } + let path = format!("/user/mfa?accessKey={}", urlencoding::encode(access_key)); + self.request_account_json(Method::GET, &path, None).await + } + + async fn user_mfa_reset(&self, access_key: &str) -> Result<()> { + if access_key.is_empty() { + return Err(Error::InvalidPath( + "Access key must not be empty".to_string(), + )); + } + let path = format!("/user/mfa?accessKey={}", urlencoding::encode(access_key)); + self.request_account_empty(Method::DELETE, &path, None) + .await + } +} + +fn encode_code_request(code: &SecretValue) -> Result> { + if code.is_empty() { + return Err(Error::InvalidPath( + "A verification code is required".to_string(), + )); + } + serde_json::to_vec(&serde_json::json!({ "code": code.expose() })) + .map_err(|_| Error::General("Failed to encode the verification request".to_string())) +} + #[async_trait] impl IamReadApi for AdminClient { async fn policy_entities(&self, query: &PolicyEntitiesQuery) -> Result { From 45524fd557af95994d4b38e1931595e908c89378 Mon Sep 17 00:00:00 2001 From: Sinan Eldem Date: Tue, 25 Aug 2026 21:13:29 +0300 Subject: [PATCH 2/4] feat(admin): add rc admin account and user two-factor commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds `rc admin account info|passwd|mfa {status,enroll,activate,disable, recovery-codes}` for the identity the alias authenticates as, and `rc admin user passwd|mfa {status,reset}` for another identity. The split mirrors the server, where `/account/*` never takes a target and the user endpoints always do, so a command cannot act on the wrong account. Every command works non-interactively. No password is accepted on the command line, where it would be captured by shell history and visible in `ps`: passwords come from `--*-from-env` or `--*-file`, or from a prompt with echo off when stdin is a terminal. In `--json` mode, or without a terminal, a command that would need to prompt exits with a usage error naming the flag to pass — it never blocks on input it cannot receive. The server renders the QR code and `rc` prints the block art it returns, so there is no QR encoder here and the console shows the same symbol from the same source. It is skipped below 33 columns, where a wrapped symbol cannot be scanned. `--output-file` writes recovery codes with mode 0600 and refuses to overwrite an existing file, which may hold the only copy of a previous set. `user mfa reset` names its target and needs `--yes` when scripted. Two-factor authentication does not gate `rc` itself, and the reference documents why: gating signed requests would break every script the moment a human enabled it, while adding no protection. --- crates/cli/src/commands/admin/account.rs | 893 +++++++++++++++++++++++ crates/cli/src/commands/admin/mod.rs | 6 + crates/cli/src/commands/admin/user.rs | 377 +++++++++- crates/cli/src/output/mod.rs | 1 + crates/cli/src/output/qr.rs | 78 ++ crates/cli/src/secret_input.rs | 134 ++++ docs/reference/rc/admin.md | 103 ++- 7 files changed, 1590 insertions(+), 2 deletions(-) create mode 100644 crates/cli/src/commands/admin/account.rs create mode 100644 crates/cli/src/output/qr.rs diff --git a/crates/cli/src/commands/admin/account.rs b/crates/cli/src/commands/admin/account.rs new file mode 100644 index 00000000..a6a0f162 --- /dev/null +++ b/crates/cli/src/commands/admin/account.rs @@ -0,0 +1,893 @@ +//! Self-service account commands: `rc admin account …` +//! +//! `account` manages the identity the alias authenticates as; `user` (in the +//! sibling module) manages somebody else's. That split mirrors the server, where +//! `/account/*` never takes a target and the user endpoints always do, and it +//! means a command cannot accidentally act on the wrong identity. +//! +//! Every command works non-interactively. Secrets and codes come from +//! `--*-from-env` or `--*-file` so nothing sensitive lands on the command line, +//! where it would be visible in shell history and in `ps`. On a terminal, and +//! only when the output is human-readable, the same values may be prompted for +//! instead. + +use std::path::PathBuf; + +use clap::Subcommand; +use serde::Serialize; + +use super::get_admin_client; +use crate::exit_code::ExitCode; +use crate::output::{Formatter, qr}; +use crate::secret_input::{SecretSource, can_prompt, read_code_interactive}; +use rc_core::admin::{ + AccountApi, AccountInfo, AccountMfaApi, MfaEnrollment, MfaStatus, RecoveryCodes, SecretValue, +}; +use rc_core::{Error, Result}; + +const PASSWD_AFTER_HELP: &str = "\ +Examples: + rc admin account passwd local + rc admin account passwd local --current-password-from-env OLD_PW --new-password-from-env NEW_PW + rc admin account passwd local --current-password-file ./old.txt --new-password-file ./new.txt"; + +const MFA_AFTER_HELP: &str = "\ +Examples: + rc admin account mfa status local + rc admin account mfa enroll local + rc admin account mfa activate local --code 123456 + rc admin account mfa recovery-codes local --code-from-env RC_MFA_CODE --output-file ./codes.txt + +Note: `rc` signs requests with the alias access key, which two-factor +authentication does not gate. The second factor guards interactive console +logins, so enabling it never breaks a script."; + +/// Self-service account subcommands +#[derive(Subcommand, Debug)] +pub enum AccountCommands { + /// Show the identity this alias authenticates as + Info(InfoArgs), + + /// Change this identity's own password (S3 secret key) + #[command(after_help = PASSWD_AFTER_HELP)] + Passwd(PasswdArgs), + + /// Manage this identity's two-factor authentication + #[command(subcommand)] + Mfa(MfaCommands), +} + +#[derive(clap::Args, Debug)] +pub struct InfoArgs { + /// Alias name of the server + pub alias: String, +} + +#[derive(clap::Args, Debug)] +pub struct PasswdArgs { + /// Alias name of the server + pub alias: String, + + /// Read the current password from this environment variable + #[arg(long, value_name = "NAME")] + pub current_password_from_env: Option, + + /// Read the current password from the first line of this file + #[arg(long, value_name = "PATH")] + pub current_password_file: Option, + + /// Read the new password from this environment variable + #[arg(long, value_name = "NAME")] + pub new_password_from_env: Option, + + /// Read the new password from the first line of this file + #[arg(long, value_name = "PATH")] + pub new_password_file: Option, +} + +#[derive(Subcommand, Debug)] +#[command(after_help = MFA_AFTER_HELP)] +pub enum MfaCommands { + /// Show two-factor authentication state + Status(MfaStatusArgs), + + /// Start enrollment and print the QR code and setup key + Enroll(MfaEnrollArgs), + + /// Confirm a pending enrollment and print the recovery codes + Activate(MfaCodeArgs), + + /// Turn off two-factor authentication + Disable(MfaDisableArgs), + + /// Replace the recovery codes + #[command(name = "recovery-codes")] + RecoveryCodes(MfaCodeArgs), +} + +#[derive(clap::Args, Debug)] +pub struct MfaStatusArgs { + /// Alias name of the server + pub alias: String, +} + +#[derive(clap::Args, Debug)] +pub struct MfaEnrollArgs { + /// Alias name of the server + pub alias: String, + + /// Do not render the QR code; print only the setup key and URI + #[arg(long)] + pub no_qr: bool, +} + +#[derive(clap::Args, Debug)] +pub struct MfaCodeArgs { + /// Alias name of the server + pub alias: String, + + /// Verification code from the authenticator app, or a recovery code + #[arg(long, value_name = "CODE")] + pub code: Option, + + /// Read the verification code from this environment variable + #[arg(long, value_name = "NAME", conflicts_with = "code")] + pub code_from_env: Option, + + /// Write the recovery codes to this file instead of stdout + #[arg(long, value_name = "PATH")] + pub output_file: Option, +} + +#[derive(clap::Args, Debug)] +pub struct MfaDisableArgs { + /// Alias name of the server + pub alias: String, + + /// Verification code from the authenticator app, or a recovery code + #[arg(long, value_name = "CODE")] + pub code: Option, + + /// Read the verification code from this environment variable + #[arg(long, value_name = "NAME", conflicts_with = "code")] + pub code_from_env: Option, + + /// Read the account password from this environment variable + #[arg(long, value_name = "NAME")] + pub password_from_env: Option, + + /// Read the account password from the first line of this file + #[arg(long, value_name = "PATH")] + pub password_file: Option, +} + +// --------------------------------------------------------------------------- +// JSON output shapes +// --------------------------------------------------------------------------- + +#[derive(Serialize)] +struct AccountInfoOutput { + access_key: String, + identity_type: String, + #[serde(skip_serializing_if = "Option::is_none")] + session_access_key: Option, + is_admin: bool, + status: String, + credentials_source: String, + password_mutable: bool, + username_mutable: bool, + policies: Vec, + member_of: Vec, + mfa_enabled: bool, + mfa_pending: bool, + recovery_codes_remaining: u32, + mfa_enrollment_available: bool, + #[serde(skip_serializing_if = "Option::is_none")] + mfa_enrollment_blocked_reason: Option, +} + +impl From for AccountInfoOutput { + fn from(info: AccountInfo) -> Self { + Self { + access_key: info.access_key, + identity_type: info.identity_type.to_string(), + session_access_key: info.session_access_key, + is_admin: info.is_admin, + status: info.status, + credentials_source: info.credentials_source.to_string(), + password_mutable: info.mutable.password, + username_mutable: info.mutable.username, + policies: info.policies, + member_of: info.member_of, + mfa_enabled: info.mfa.enabled, + mfa_pending: info.mfa.pending, + recovery_codes_remaining: info.mfa.recovery_codes_remaining, + mfa_enrollment_available: info.mfa.enrollment_available, + mfa_enrollment_blocked_reason: info.mfa.enrollment_blocked_reason, + } + } +} + +#[derive(Serialize)] +struct PasswordChangeOutput { + success: bool, + access_key: String, + sessions_revoked: u32, + message: String, +} + +#[derive(Serialize)] +struct MfaStatusOutput { + enabled: bool, + pending: bool, + algorithm: String, + digits: u8, + period_seconds: u32, + #[serde(skip_serializing_if = "Option::is_none")] + activated_at: Option, + #[serde(skip_serializing_if = "Option::is_none")] + last_verified_at: Option, + recovery_codes_remaining: u32, + enrollment_available: bool, + #[serde(skip_serializing_if = "Option::is_none")] + enrollment_blocked_reason: Option, +} + +impl From for MfaStatusOutput { + fn from(status: MfaStatus) -> Self { + Self { + enabled: status.enabled, + pending: status.pending, + algorithm: status.algorithm, + digits: status.digits, + period_seconds: status.period_seconds, + activated_at: status.activated_at, + last_verified_at: status.last_verified_at, + recovery_codes_remaining: status.recovery_codes_remaining, + enrollment_available: status.enrollment_available, + enrollment_blocked_reason: status.enrollment_blocked_reason, + } + } +} + +/// The enrollment secret, echoed for a caller that will complete setup itself. +/// +/// `qr_svg` is deliberately absent: it is several kilobytes of markup with no +/// use in a terminal pipeline, and including it would make the JSON output +/// unreadable for no gain. +#[derive(Serialize)] +struct MfaEnrollOutput { + secret_base32: String, + otpauth_uri: String, + algorithm: String, + digits: u8, + period_seconds: u32, + expires_at: String, +} + +#[derive(Serialize)] +struct RecoveryCodesOutput { + recovery_codes: Vec, + generated_at: String, +} + +#[derive(Serialize)] +struct MfaOperationOutput { + success: bool, + message: String, +} + +/// Execute an account subcommand +pub async fn execute(cmd: AccountCommands, formatter: &Formatter) -> ExitCode { + match cmd { + AccountCommands::Info(args) => execute_info(args, formatter).await, + AccountCommands::Passwd(args) => execute_passwd(args, formatter).await, + AccountCommands::Mfa(mfa_cmd) => execute_mfa(mfa_cmd, formatter).await, + } +} + +async fn execute_info(args: InfoArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + match client.account_info().await { + Ok(info) => { + if formatter.is_json() { + formatter.json(&AccountInfoOutput::from(info)); + } else { + print_account_info(&info, formatter); + } + ExitCode::Success + } + Err(error) => fail(formatter, "Failed to read account information", error), + } +} + +fn print_account_info(info: &AccountInfo, formatter: &Formatter) { + formatter.println(&format!( + "Username: {}", + formatter.style_name(&info.access_key) + )); + formatter.println(&format!("Identity: {}", info.identity_type)); + formatter.println(&format!( + "Role: {}", + if info.is_admin { + "administrator" + } else { + "user" + } + )); + formatter.println(&format!("Status: {}", info.status)); + formatter.println(&format!("Credentials: {}", info.credentials_source)); + + if let Some(session) = &info.session_access_key { + formatter.println(&format!("Session key: {session}")); + } + if !info.policies.is_empty() { + formatter.println(&format!("Policies: {}", info.policies.join(", "))); + } + if !info.member_of.is_empty() { + formatter.println(&format!("Groups: {}", info.member_of.join(", "))); + } + + formatter.println(&format!( + "2FA: {}", + if info.mfa.enabled { "on" } else { "off" } + )); + if info.mfa.enabled { + formatter.println(&format!( + "Recovery: {} codes remaining", + info.mfa.recovery_codes_remaining + )); + } + + // Say why a mutation is unavailable rather than letting the user discover it + // by having the request rejected. + if !info.mutable.password { + formatter.println(""); + formatter.println("This identity's password cannot be changed here."); + if let Some(reason) = &info.mfa.enrollment_blocked_reason { + formatter.println(&format!(" {reason}")); + } + } +} + +async fn execute_passwd(args: PasswdArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + let interactive = can_prompt(formatter.is_json()); + let current_source = match SecretSource::resolve( + args.current_password_from_env, + args.current_password_file, + interactive, + "current-password", + ) { + Ok(source) => source, + Err(error) => return usage_failure(formatter, error), + }; + let new_source = match SecretSource::resolve( + args.new_password_from_env, + args.new_password_file, + interactive, + "new-password", + ) { + Ok(source) => source, + Err(error) => return usage_failure(formatter, error), + }; + + let current = match current_source.load("Current password: ") { + Ok(value) => SecretValue::new(value.to_string()), + Err(error) => return usage_failure(formatter, error), + }; + let new = match new_source.load("New password: ") { + Ok(value) => SecretValue::new(value.to_string()), + Err(error) => return usage_failure(formatter, error), + }; + + let access_key = match client.account_info().await { + Ok(info) => info.access_key, + // Reporting the identity is a convenience; failing to read it must not + // block the rotation. + Err(_) => args.alias.clone(), + }; + + match client.account_change_password(¤t, &new).await { + Ok(result) => { + let message = if result.sessions_revoked > 0 { + format!( + "Password updated. {} session(s) were signed out.", + result.sessions_revoked + ) + } else { + "Password updated.".to_string() + }; + + if formatter.is_json() { + formatter.json(&PasswordChangeOutput { + success: true, + access_key, + sessions_revoked: result.sessions_revoked, + message, + }); + } else { + formatter.println(&message); + formatter.println( + "Update the alias with `rc alias set` so future commands use the new key.", + ); + } + ExitCode::Success + } + Err(error) => fail(formatter, "Failed to change the password", error), + } +} + +async fn execute_mfa(cmd: MfaCommands, formatter: &Formatter) -> ExitCode { + match cmd { + MfaCommands::Status(args) => execute_mfa_status(args, formatter).await, + MfaCommands::Enroll(args) => execute_mfa_enroll(args, formatter).await, + MfaCommands::Activate(args) => execute_mfa_activate(args, formatter).await, + MfaCommands::Disable(args) => execute_mfa_disable(args, formatter).await, + MfaCommands::RecoveryCodes(args) => execute_mfa_recovery_codes(args, formatter).await, + } +} + +async fn execute_mfa_status(args: MfaStatusArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + match client.account_mfa_status().await { + Ok(status) => { + if formatter.is_json() { + formatter.json(&MfaStatusOutput::from(status)); + } else { + formatter.println(&format!( + "Two-factor authentication: {}", + if status.enabled { "on" } else { "off" } + )); + if status.enabled { + formatter.println(&format!( + " Algorithm: {} / {} digits / {}s period", + status.algorithm, status.digits, status.period_seconds + )); + if let Some(activated) = &status.activated_at { + formatter.println(&format!(" Enabled on: {activated}")); + } + if let Some(last) = &status.last_verified_at { + formatter.println(&format!(" Last used: {last}")); + } + formatter.println(&format!( + " Recovery: {} code(s) remaining", + status.recovery_codes_remaining + )); + if status.recovery_codes_remaining == 0 { + formatter.println( + " No recovery codes left. Run `rc admin account mfa recovery-codes` to generate a new set.", + ); + } + } + if status.pending { + formatter.println(" A pending enrollment is waiting for `mfa activate`."); + } + if !status.enrollment_available + && let Some(reason) = &status.enrollment_blocked_reason + { + formatter.println(&format!(" Enrollment unavailable: {reason}")); + } + } + ExitCode::Success + } + Err(error) => fail(formatter, "Failed to read two-factor state", error), + } +} + +async fn execute_mfa_enroll(args: MfaEnrollArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + match client.account_mfa_enroll().await { + Ok(enrollment) => { + if formatter.is_json() { + formatter.json(&MfaEnrollOutput { + secret_base32: enrollment.secret_base32.clone(), + otpauth_uri: enrollment.otpauth_uri.clone(), + algorithm: enrollment.algorithm.clone(), + digits: enrollment.digits, + period_seconds: enrollment.period_seconds, + expires_at: enrollment.expires_at.clone(), + }); + } else { + print_enrollment(&enrollment, args.no_qr, formatter); + } + ExitCode::Success + } + Err(error) => fail(formatter, "Failed to start two-factor enrollment", error), + } +} + +fn print_enrollment(enrollment: &MfaEnrollment, no_qr: bool, formatter: &Formatter) { + formatter.println("Scan this QR code with your authenticator app:"); + qr::print_qr(formatter, &enrollment.qr_utf8, no_qr); + + formatter.println(&format!( + "Manual setup key: {}", + group_secret(&enrollment.secret_base32) + )); + formatter.println(&format!("Setup URI: {}", enrollment.otpauth_uri)); + formatter.println(&format!( + "Parameters: {} / {} digits / {}s period", + enrollment.algorithm, enrollment.digits, enrollment.period_seconds + )); + formatter.println(&format!("Expires: {}", enrollment.expires_at)); + formatter.println(""); + formatter.println("Then confirm with:"); + formatter.println(" rc admin account mfa activate --code <6-digit code>"); +} + +/// Group a base32 secret in fours so a human can transcribe it without losing +/// their place. Authenticator apps ignore the spaces. +fn group_secret(secret: &str) -> String { + secret + .as_bytes() + .chunks(4) + .map(|chunk| String::from_utf8_lossy(chunk).to_string()) + .collect::>() + .join(" ") +} + +async fn execute_mfa_activate(args: MfaCodeArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + let code = match resolve_code(args.code, args.code_from_env, formatter) { + Ok(code) => code, + Err(exit) => return exit, + }; + + match client.account_mfa_activate(&code).await { + Ok(codes) => emit_recovery_codes(codes, args.output_file, formatter, true), + Err(error) => fail(formatter, "Failed to confirm the enrollment", error), + } +} + +async fn execute_mfa_recovery_codes(args: MfaCodeArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + let code = match resolve_code(args.code, args.code_from_env, formatter) { + Ok(code) => code, + Err(exit) => return exit, + }; + + match client.account_mfa_recovery_codes(&code).await { + Ok(codes) => emit_recovery_codes(codes, args.output_file, formatter, false), + Err(error) => fail(formatter, "Failed to generate recovery codes", error), + } +} + +async fn execute_mfa_disable(args: MfaDisableArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + let code = match resolve_code(args.code, args.code_from_env, formatter) { + Ok(code) => code, + Err(exit) => return exit, + }; + + let interactive = can_prompt(formatter.is_json()); + let password_source = match SecretSource::resolve( + args.password_from_env, + args.password_file, + interactive, + "password", + ) { + Ok(source) => source, + Err(error) => return usage_failure(formatter, error), + }; + let password = match password_source.load("Account password: ") { + Ok(value) => SecretValue::new(value.to_string()), + Err(error) => return usage_failure(formatter, error), + }; + + match client.account_mfa_disable(&code, &password).await { + Ok(()) => { + let message = "Two-factor authentication is off.".to_string(); + if formatter.is_json() { + formatter.json(&MfaOperationOutput { + success: true, + message, + }); + } else { + formatter.println(&message); + } + ExitCode::Success + } + Err(error) => fail( + formatter, + "Failed to turn off two-factor authentication", + error, + ), + } +} + +/// Resolve a verification code from a flag, an environment variable, or a prompt. +fn resolve_code( + inline: Option, + from_env: Option, + formatter: &Formatter, +) -> std::result::Result { + if let Some(code) = inline { + let code = code.trim().to_string(); + if code.is_empty() { + return Err(usage_failure( + formatter, + Error::InvalidPath("The verification code is empty".to_string()), + )); + } + return Ok(SecretValue::new(code)); + } + + if let Some(name) = from_env { + return match SecretSource::resolve(Some(name), None, false, "code") + .and_then(|source| source.load("")) + { + Ok(value) => Ok(SecretValue::new(value.to_string())), + Err(error) => Err(usage_failure(formatter, error)), + }; + } + + if !can_prompt(formatter.is_json()) { + return Err(usage_failure( + formatter, + Error::InvalidPath( + "Provide the verification code with --code or --code-from-env when running non-interactively or with --json" + .to_string(), + ), + )); + } + + match read_code_interactive("Verification code: ") { + Ok(value) => Ok(SecretValue::new(value.to_string())), + Err(error) => Err(usage_failure(formatter, error)), + } +} + +/// Print or write a recovery-code set. +/// +/// These exist in plaintext exactly once, so the human-readable path is loud +/// about that and the file path refuses to clobber an existing file rather than +/// destroying a set the user may not have stored yet. +fn emit_recovery_codes( + codes: RecoveryCodes, + output_file: Option, + formatter: &Formatter, + activated: bool, +) -> ExitCode { + if let Some(path) = output_file { + if let Err(error) = write_recovery_codes(&path, &codes.recovery_codes) { + return fail(formatter, "Failed to write the recovery codes", error); + } + if formatter.is_json() { + formatter.json(&MfaOperationOutput { + success: true, + message: format!("Recovery codes written to {}", path.display()), + }); + } else { + formatter.println(&format!( + "{} recovery code(s) written to {} (mode 0600).", + codes.recovery_codes.len(), + path.display() + )); + } + return ExitCode::Success; + } + + if formatter.is_json() { + formatter.json(&RecoveryCodesOutput { + recovery_codes: codes.recovery_codes, + generated_at: codes.generated_at, + }); + return ExitCode::Success; + } + + if activated { + formatter.println("Two-factor authentication is now on."); + } else { + formatter.println("Previous recovery codes no longer work."); + } + formatter.println(""); + formatter.println("Save these recovery codes. They are shown only once:"); + for (index, code) in codes.recovery_codes.iter().enumerate() { + formatter.println(&format!(" {:2}. {code}", index + 1)); + } + formatter.println(""); + formatter.println("Each code works once. Store them somewhere only you can reach."); + ExitCode::Success +} + +/// Write recovery codes to a new file with owner-only permissions. +fn write_recovery_codes(path: &std::path::Path, codes: &[String]) -> Result<()> { + use std::io::Write as _; + + let mut options = std::fs::OpenOptions::new(); + // `create_new` so an existing file is never silently overwritten: it may + // hold the only copy of a previous set. + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(0o600); + } + + let mut file = options.open(path).map_err(|error| { + if error.kind() == std::io::ErrorKind::AlreadyExists { + Error::Conflict(format!( + "{} already exists; choose another path", + path.display() + )) + } else { + Error::Io(error) + } + })?; + + for code in codes { + writeln!(file, "{code}").map_err(Error::Io)?; + } + file.flush().map_err(Error::Io)?; + Ok(()) +} + +fn fail(formatter: &Formatter, context: &str, error: Error) -> ExitCode { + let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); + formatter.error(&format!("{context}: {error}")); + code +} + +fn usage_failure(formatter: &Formatter, error: Error) -> ExitCode { + formatter.error(&error.to_string()); + ExitCode::UsageError +} + +#[cfg(test)] +mod tests { + use super::*; + use clap::Parser; + + #[derive(Debug, Parser)] + struct TestCli { + #[command(subcommand)] + command: AccountCommands, + } + + #[test] + fn info_parses_with_only_an_alias() { + let cli = TestCli::parse_from(["account", "info", "local"]); + match cli.command { + AccountCommands::Info(args) => assert_eq!(args.alias, "local"), + other => panic!("unexpected command: {other:?}"), + } + } + + #[test] + fn passwd_accepts_both_secrets_from_the_environment() { + // The automation path: nothing sensitive on the command line. + let cli = TestCli::parse_from([ + "account", + "passwd", + "local", + "--current-password-from-env", + "OLD", + "--new-password-from-env", + "NEW", + ]); + match cli.command { + AccountCommands::Passwd(args) => { + assert_eq!(args.current_password_from_env.as_deref(), Some("OLD")); + assert_eq!(args.new_password_from_env.as_deref(), Some("NEW")); + } + other => panic!("unexpected command: {other:?}"), + } + } + + #[test] + fn a_code_cannot_be_given_twice() { + // `--code` and `--code-from-env` together is ambiguous, so clap rejects + // it rather than letting one silently win. + let result = TestCli::try_parse_from([ + "account", + "mfa", + "activate", + "local", + "--code", + "123456", + "--code-from-env", + "RC_CODE", + ]); + assert!(result.is_err()); + } + + #[test] + fn enroll_accepts_suppressing_the_qr() { + let cli = TestCli::parse_from(["account", "mfa", "enroll", "local", "--no-qr"]); + match cli.command { + AccountCommands::Mfa(MfaCommands::Enroll(args)) => { + assert!(args.no_qr); + assert_eq!(args.alias, "local"); + } + other => panic!("unexpected command: {other:?}"), + } + } + + #[test] + fn recovery_codes_accepts_an_output_file() { + let cli = TestCli::parse_from([ + "account", + "mfa", + "recovery-codes", + "local", + "--code", + "123456", + "--output-file", + "/tmp/codes.txt", + ]); + match cli.command { + AccountCommands::Mfa(MfaCommands::RecoveryCodes(args)) => { + assert_eq!(args.output_file, Some(PathBuf::from("/tmp/codes.txt"))); + } + other => panic!("unexpected command: {other:?}"), + } + } + + #[test] + fn secrets_are_grouped_for_transcription() { + assert_eq!(group_secret("JBSWY3DPEHPK3PXP"), "JBSW Y3DP EHPK 3PXP"); + assert_eq!(group_secret(""), ""); + } + + #[test] + fn writing_recovery_codes_refuses_to_clobber_an_existing_file() { + // That file may hold the only copy of a previous set. + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("codes.txt"); + + write_recovery_codes(&path, &["AAAA-BBBB".to_string()]).expect("first write"); + let error = write_recovery_codes(&path, &["CCCC-DDDD".to_string()]) + .expect_err("second write must fail"); + + assert!(matches!(error, Error::Conflict(_)), "{error:?}"); + assert_eq!( + std::fs::read_to_string(&path).expect("read back"), + "AAAA-BBBB\n" + ); + } + + #[cfg(unix)] + #[test] + fn recovery_code_files_are_owner_only() { + use std::os::unix::fs::PermissionsExt as _; + + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("codes.txt"); + write_recovery_codes(&path, &["AAAA-BBBB".to_string()]).expect("write"); + + let mode = std::fs::metadata(&path) + .expect("metadata") + .permissions() + .mode(); + assert_eq!(mode & 0o777, 0o600, "unexpected mode {:o}", mode & 0o777); + } +} diff --git a/crates/cli/src/commands/admin/mod.rs b/crates/cli/src/commands/admin/mod.rs index e5ae3ab2..4e7011f6 100644 --- a/crates/cli/src/commands/admin/mod.rs +++ b/crates/cli/src/commands/admin/mod.rs @@ -4,6 +4,7 @@ //! service accounts, and cluster operations through the RustFS Admin API. mod access_key; +mod account; mod bucket_metadata; mod capabilities; mod config; @@ -39,6 +40,10 @@ use rc_s3::AdminClient; /// Admin subcommands for IAM and cluster management #[derive(Subcommand, Debug)] pub enum AdminCommands { + /// Manage the identity this alias authenticates as + #[command(subcommand)] + Account(account::AccountCommands), + /// Discover effective RustFS runtime capabilities Capabilities(capabilities::CapabilitiesArgs), @@ -135,6 +140,7 @@ pub async fn execute(cmd: AdminCommands, output_config: OutputConfig) -> ExitCod let formatter = Formatter::new(output_config); match cmd { + AdminCommands::Account(account_cmd) => account::execute(account_cmd, &formatter).await, AdminCommands::Capabilities(args) => capabilities::execute(args, &formatter).await, AdminCommands::Diagnostics(command) => diagnostics::execute(command, &formatter).await, AdminCommands::Config(config_cmd) => config::execute(config_cmd, &formatter).await, diff --git a/crates/cli/src/commands/admin/user.rs b/crates/cli/src/commands/admin/user.rs index 4543919a..bc6703d6 100644 --- a/crates/cli/src/commands/admin/user.rs +++ b/crates/cli/src/commands/admin/user.rs @@ -2,13 +2,17 @@ //! //! Commands for managing IAM users: list, add, info, remove, enable, disable. +use std::path::PathBuf; + use clap::Subcommand; use serde::Serialize; use super::get_admin_client; use crate::exit_code::ExitCode; use crate::output::Formatter; -use rc_core::admin::{AdminApi, User, UserStatus}; +use crate::secret_input::{SecretSource, can_prompt}; +use rc_core::Error; +use rc_core::admin::{AdminApi, SecretValue, User, UserCredentialApi, UserStatus}; const ADD_USER_AFTER_HELP: &str = "\ Examples: @@ -38,6 +42,81 @@ pub enum UserCommands { /// Disable a user Disable(DisableArgs), + + /// Reset a user's password (S3 secret key) + #[command(after_help = PASSWD_AFTER_HELP)] + Passwd(PasswdArgs), + + /// Inspect or clear a user's two-factor authentication + #[command(subcommand)] + Mfa(UserMfaCommands), +} + +const PASSWD_AFTER_HELP: &str = "\ +Examples: + rc admin user passwd local analyst --password-from-env NEW_PW + rc admin user passwd local analyst --password-file ./new.txt + +Unlike re-creating the user, this changes only the secret key: the account's +status, policies and group memberships are left alone."; + +const USER_MFA_AFTER_HELP: &str = "\ +Examples: + rc admin user mfa status local analyst + rc admin user mfa reset local analyst --yes + +`reset` is the break-glass path for a user who lost both their authenticator and +their recovery codes. It removes their second factor, so the account is left +protected by its password alone until they enrol again."; + +#[derive(clap::Args, Debug)] +#[command(after_help = PASSWD_AFTER_HELP)] +pub struct PasswdArgs { + /// Alias name of the server + pub alias: String, + + /// Access key of the user whose password is being reset + pub access_key: String, + + /// Read the new password from this environment variable + #[arg(long, value_name = "NAME")] + pub password_from_env: Option, + + /// Read the new password from the first line of this file + #[arg(long, value_name = "PATH")] + pub password_file: Option, +} + +#[derive(Subcommand, Debug)] +#[command(after_help = USER_MFA_AFTER_HELP)] +pub enum UserMfaCommands { + /// Show whether a user has two-factor authentication enabled + Status(UserMfaStatusArgs), + + /// Clear a user's second factor (break-glass) + Reset(UserMfaResetArgs), +} + +#[derive(clap::Args, Debug)] +pub struct UserMfaStatusArgs { + /// Alias name of the server + pub alias: String, + + /// Access key of the user to inspect + pub access_key: String, +} + +#[derive(clap::Args, Debug)] +pub struct UserMfaResetArgs { + /// Alias name of the server + pub alias: String, + + /// Access key of the user whose second factor is being cleared + pub access_key: String, + + /// Confirm without an interactive prompt + #[arg(long)] + pub yes: bool, } #[derive(clap::Args, Debug)] @@ -142,6 +221,222 @@ pub async fn execute(cmd: UserCommands, formatter: &Formatter) -> ExitCode { UserCommands::Remove(args) => execute_remove(args, formatter).await, UserCommands::Enable(args) => execute_enable(args, formatter).await, UserCommands::Disable(args) => execute_disable(args, formatter).await, + UserCommands::Passwd(args) => execute_passwd(args, formatter).await, + UserCommands::Mfa(mfa_cmd) => execute_user_mfa(mfa_cmd, formatter).await, + } +} + +/// JSON output for a password reset +#[derive(Serialize)] +struct UserPasswordOutput { + success: bool, + access_key: String, + sessions_revoked: u32, + message: String, +} + +/// JSON output for a user's two-factor state +#[derive(Serialize)] +struct UserMfaStatusOutput { + access_key: String, + enabled: bool, + #[serde(skip_serializing_if = "Option::is_none")] + activated_at: Option, + recovery_codes_remaining: u32, +} + +async fn execute_passwd(args: PasswdArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + if args.access_key.is_empty() { + return formatter.fail_with_suggestion( + ExitCode::UsageError, + "Access key cannot be empty", + "Provide the access key of the user whose password is being reset.", + ); + } + + let source = match SecretSource::resolve( + args.password_from_env, + args.password_file, + can_prompt(formatter.is_json()), + "password", + ) { + Ok(source) => source, + Err(error) => { + formatter.error(&error.to_string()); + return ExitCode::UsageError; + } + }; + let secret = match source.load(&format!("New password for {}: ", args.access_key)) { + Ok(value) => SecretValue::new(value.to_string()), + Err(error) => { + formatter.error(&error.to_string()); + return ExitCode::UsageError; + } + }; + + match client.set_user_secret_key(&args.access_key, &secret).await { + Ok(result) => { + let message = if result.sessions_revoked > 0 { + format!( + "Password reset for '{}'. {} session(s) were signed out.", + args.access_key, result.sessions_revoked + ) + } else { + format!("Password reset for '{}'.", args.access_key) + }; + + if formatter.is_json() { + formatter.json(&UserPasswordOutput { + success: true, + access_key: args.access_key, + sessions_revoked: result.sessions_revoked, + message, + }); + } else { + formatter.println(&message); + } + ExitCode::Success + } + Err(error) => { + let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); + formatter.error(&format!("Failed to reset the password: {error}")); + code + } + } +} + +async fn execute_user_mfa(cmd: UserMfaCommands, formatter: &Formatter) -> ExitCode { + match cmd { + UserMfaCommands::Status(args) => execute_user_mfa_status(args, formatter).await, + UserMfaCommands::Reset(args) => execute_user_mfa_reset(args, formatter).await, + } +} + +async fn execute_user_mfa_status(args: UserMfaStatusArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + match client.user_mfa_status(&args.access_key).await { + Ok(status) => { + if formatter.is_json() { + formatter.json(&UserMfaStatusOutput { + access_key: status.access_key, + enabled: status.enabled, + activated_at: status.activated_at, + recovery_codes_remaining: status.recovery_codes_remaining, + }); + } else { + formatter.println(&format!( + "{}: two-factor authentication {}", + formatter.style_name(&status.access_key), + if status.enabled { "on" } else { "off" } + )); + if status.enabled { + if let Some(activated) = &status.activated_at { + formatter.println(&format!(" Enabled on: {activated}")); + } + formatter.println(&format!( + " Recovery: {} code(s) remaining", + status.recovery_codes_remaining + )); + } + } + ExitCode::Success + } + Err(error) => { + let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); + formatter.error(&format!("Failed to read two-factor state: {error}")); + code + } + } +} + +async fn execute_user_mfa_reset(args: UserMfaResetArgs, formatter: &Formatter) -> ExitCode { + let client = match get_admin_client(&args.alias, formatter) { + Ok(client) => client, + Err(code) => return code, + }; + + if let Err(error) = confirm_mfa_reset(&args.access_key, args.yes, formatter) { + let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); + formatter.error(&error.to_string()); + return code; + } + + match client.user_mfa_reset(&args.access_key).await { + Ok(()) => { + let message = format!( + "Two-factor authentication cleared for '{}'. The account is now protected by its password alone.", + args.access_key + ); + if formatter.is_json() { + formatter.json(&UserOperationOutput { + success: true, + access_key: args.access_key, + message, + secret_key: None, + }); + } else { + formatter.println(&message); + } + ExitCode::Success + } + Err(error) => { + let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); + formatter.error(&format!( + "Failed to clear two-factor authentication: {error}" + )); + code + } + } +} + +/// Confirm a break-glass reset, naming the target. +/// +/// Removing somebody's second factor is not reversible from the operator's side +/// — the user has to enrol again — so a non-interactive run must say `--yes` +/// rather than have the confirmation silently skipped. +fn confirm_mfa_reset(access_key: &str, yes: bool, formatter: &Formatter) -> rc_core::Result<()> { + use std::io::{BufRead as _, IsTerminal as _, Write as _}; + + if yes { + return Ok(()); + } + if formatter.is_json() || !std::io::stdin().is_terminal() { + return Err(Error::InvalidPath( + "Clearing a user's second factor requires --yes in non-interactive or JSON mode" + .to_string(), + )); + } + + let mut stderr = std::io::stderr().lock(); + write!( + stderr, + "Clear two-factor authentication for '{}'? The account will be protected by its password alone. [y/N] ", + formatter.sanitize_text(access_key) + ) + .map_err(Error::Io)?; + stderr.flush().map_err(Error::Io)?; + + let mut answer = String::new(); + std::io::stdin() + .lock() + .read_line(&mut answer) + .map_err(Error::Io)?; + + if matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") { + Ok(()) + } else { + Err(Error::Interrupted( + "Clearing two-factor authentication was declined".to_string(), + )) } } @@ -377,6 +672,86 @@ async fn execute_disable(args: DisableArgs, formatter: &Formatter) -> ExitCode { #[cfg(test)] mod tests { use super::*; + use clap::Parser; + + #[derive(Debug, Parser)] + struct TestCli { + #[command(subcommand)] + command: UserCommands, + } + + #[test] + fn passwd_parses_a_target_and_an_environment_source() { + let cli = TestCli::parse_from([ + "user", + "passwd", + "local", + "analyst", + "--password-from-env", + "NEW_PW", + ]); + match cli.command { + UserCommands::Passwd(args) => { + assert_eq!(args.alias, "local"); + assert_eq!(args.access_key, "analyst"); + assert_eq!(args.password_from_env.as_deref(), Some("NEW_PW")); + } + other => panic!("unexpected command: {other:?}"), + } + } + + #[test] + fn passwd_requires_a_target_access_key() { + // Without a target this would be ambiguous with the self-service + // `account passwd`, so clap must reject it rather than guess. + assert!(TestCli::try_parse_from(["user", "passwd", "local"]).is_err()); + } + + #[test] + fn mfa_status_parses_a_target() { + let cli = TestCli::parse_from(["user", "mfa", "status", "local", "analyst"]); + match cli.command { + UserCommands::Mfa(UserMfaCommands::Status(args)) => { + assert_eq!(args.access_key, "analyst"); + } + other => panic!("unexpected command: {other:?}"), + } + } + + #[test] + fn mfa_reset_carries_an_explicit_confirmation_flag() { + let cli = TestCli::parse_from(["user", "mfa", "reset", "local", "analyst", "--yes"]); + match cli.command { + UserCommands::Mfa(UserMfaCommands::Reset(args)) => { + assert!(args.yes); + assert_eq!(args.access_key, "analyst"); + } + other => panic!("unexpected command: {other:?}"), + } + } + + #[test] + fn a_json_mode_reset_without_confirmation_is_a_usage_error() { + // Never silently proceed: a scripted break-glass must be deliberate. + let formatter = Formatter::new(crate::output::OutputConfig { + json: true, + ..Default::default() + }); + + let error = confirm_mfa_reset("analyst", false, &formatter).expect_err("must refuse"); + assert!(matches!(error, Error::InvalidPath(_)), "{error:?}"); + assert!(error.to_string().contains("--yes"), "{error}"); + } + + #[test] + fn an_explicit_yes_skips_confirmation_even_in_json_mode() { + let formatter = Formatter::new(crate::output::OutputConfig { + json: true, + ..Default::default() + }); + + confirm_mfa_reset("analyst", true, &formatter).expect("--yes must be honoured"); + } #[test] fn test_user_info_from_user() { diff --git a/crates/cli/src/output/mod.rs b/crates/cli/src/output/mod.rs index 2df659cc..d0fc66a1 100644 --- a/crates/cli/src/output/mod.rs +++ b/crates/cli/src/output/mod.rs @@ -5,6 +5,7 @@ mod formatter; mod progress; +pub mod qr; mod v3; // These exports will be used in Phase 2+ when commands are implemented diff --git a/crates/cli/src/output/qr.rs b/crates/cli/src/output/qr.rs new file mode 100644 index 00000000..eb71f5ec --- /dev/null +++ b/crates/cli/src/output/qr.rs @@ -0,0 +1,78 @@ +//! Printing the server-rendered QR code for TOTP enrollment. +//! +//! The server does the encoding, so `rc` carries no QR library and the console +//! and the CLI show the same symbol from the same source. All that happens here +//! is deciding whether a terminal can display it and writing it out. + +use crate::output::Formatter; + +/// Widest terminal the block art is worth attempting on. +/// +/// A version-2 `otpauth://` QR is 25 modules plus a 4-module quiet zone on each +/// side, so 33 columns is the practical floor. Below that the symbol wraps and +/// becomes unscannable, and printing a broken one is worse than saying so. +const MIN_TERMINAL_COLUMNS: u16 = 33; + +/// A version-2 symbol is 25 modules wide plus a 4-module quiet zone each side. +/// Lowering the floor below that would print a code no phone can read. +const _: () = assert!( + MIN_TERMINAL_COLUMNS >= 25 + 4 + 4, + "the QR floor must still fit a version-2 symbol with its quiet zone" +); + +/// Print the QR, or explain why it was skipped. +/// +/// Returns whether it was printed, so a caller can decide how loudly to point at +/// the manual key. +pub fn print_qr(formatter: &Formatter, qr_utf8: &str, suppressed: bool) -> bool { + if suppressed || qr_utf8.is_empty() { + return false; + } + + if let Some((columns, _)) = terminal_size() + && columns < MIN_TERMINAL_COLUMNS + { + formatter.println(&format!( + "(Terminal is {columns} columns wide; the QR code needs at least {MIN_TERMINAL_COLUMNS}. Use the setup key below.)" + )); + return false; + } + + formatter.println(""); + for line in qr_utf8.lines() { + formatter.println(line); + } + formatter.println(""); + true +} + +fn terminal_size() -> Option<(u16, u16)> { + console::Term::stdout() + .size_checked() + .map(|(rows, columns)| (columns, rows)) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::output::OutputConfig; + + fn formatter() -> Formatter { + Formatter::new(OutputConfig { + no_color: true, + ..Default::default() + }) + } + + #[test] + fn suppressing_the_qr_reports_that_nothing_was_printed() { + assert!(!print_qr(&formatter(), "▀▄█", true)); + } + + #[test] + fn an_empty_payload_prints_nothing() { + // A server that omitted the field must not produce a blank frame the + // user might mistake for an unscannable code. + assert!(!print_qr(&formatter(), "", false)); + } +} diff --git a/crates/cli/src/secret_input.rs b/crates/cli/src/secret_input.rs index e9981d78..ae694b47 100644 --- a/crates/cli/src/secret_input.rs +++ b/crates/cli/src/secret_input.rs @@ -257,3 +257,137 @@ mod tests { assert!(matches!(symlink_error, Error::InvalidPath(_))); } } + +// --------------------------------------------------------------------------- +// Account credentials and second-factor codes +// --------------------------------------------------------------------------- + +/// Where a password or verification code comes from. +/// +/// Every source except an interactive prompt is explicit, because a command that +/// silently prompts is a command that hangs in CI. `--*-from-env` and `--*-file` +/// exist so automation never has to put a secret on the command line, where it +/// would land in the shell history and in `ps` output. +#[derive(Debug, Clone)] +pub(crate) enum SecretSource { + /// Read from a named environment variable. + Environment(String), + /// Read from the first line of a file. + File(PathBuf), + /// Prompt on the terminal, with echo off. + Prompt, +} + +impl SecretSource { + /// Pick a source from the mutually exclusive flags. + /// + /// Interactive prompting is only offered when there is a terminal to prompt + /// on and the output is human-readable; otherwise the caller is told which + /// flag to pass instead of being left to hang. + pub(crate) fn resolve( + from_env: Option, + from_file: Option, + interactive_allowed: bool, + what: &str, + ) -> Result { + match (from_env, from_file) { + (Some(_), Some(_)) => Err(Error::InvalidPath(format!( + "Select either an environment variable or a file for the {what}, not both" + ))), + (Some(name), None) => { + if !valid_environment_name(&name) { + return Err(Error::InvalidPath(format!( + "Environment variable name for the {what} is invalid" + ))); + } + Ok(Self::Environment(name)) + } + (None, Some(path)) => { + if path.as_os_str().is_empty() { + return Err(Error::InvalidPath(format!( + "File path for the {what} cannot be empty" + ))); + } + Ok(Self::File(path)) + } + (None, None) => { + if interactive_allowed { + Ok(Self::Prompt) + } else { + Err(Error::InvalidPath(format!( + "Provide the {what} with --{what}-from-env or --{what}-file when running non-interactively or with --json" + ))) + } + } + } + } + + /// Load the value, prompting with `prompt` when this is [`Self::Prompt`]. + pub(crate) fn load(&self, prompt: &str) -> Result> { + match self { + Self::Environment(name) => { + let value = std::env::var(name).map_err(|_| { + Error::InvalidPath(format!("Environment variable '{name}' is not set")) + })?; + let value = Zeroizing::new(value.trim_end_matches(['\r', '\n']).to_string()); + if value.is_empty() { + return Err(Error::InvalidPath(format!( + "Environment variable '{name}' is empty" + ))); + } + Ok(value) + } + Self::File(path) => { + let bytes = read_protected_key_file(path)?; + let text = String::from_utf8(bytes.to_vec()) + .map_err(|_| Error::InvalidPath("Secret file must be UTF-8".to_string()))?; + // First line only: an editor-written file usually has a trailing + // newline, and a stray second line is more likely a mistake than + // part of the secret. + let value = Zeroizing::new( + text.lines() + .next() + .unwrap_or_default() + .trim_end_matches(['\r', ' ', '\t']) + .to_string(), + ); + if value.is_empty() { + return Err(Error::InvalidPath("Secret file is empty".to_string())); + } + Ok(value) + } + Self::Prompt => { + let term = console::Term::stderr(); + // Prompt on stderr so stdout stays clean for piping. + term.write_str(prompt).map_err(Error::Io)?; + let value = term.read_secure_line().map_err(Error::Io)?; + let value = Zeroizing::new(value); + if value.is_empty() { + return Err(Error::Interrupted("No value was entered".to_string())); + } + Ok(value) + } + } + } +} + +/// Read a verification code, which is not secret enough to hide but is still +/// kept out of the command line by default. +pub(crate) fn read_code_interactive(prompt: &str) -> Result> { + let term = console::Term::stderr(); + term.write_str(prompt).map_err(Error::Io)?; + // Echoed, unlike a password: a TOTP code is short-lived, and hiding it only + // makes transcription errors harder to spot. + let value = term.read_line().map_err(Error::Io)?; + let value = Zeroizing::new(value.trim().to_string()); + if value.is_empty() { + return Err(Error::Interrupted("No code was entered".to_string())); + } + Ok(value) +} + +/// Whether prompting is possible and appropriate. +pub(crate) fn can_prompt(is_json: bool) -> bool { + use std::io::IsTerminal; + !is_json && std::io::stdin().is_terminal() +} diff --git a/docs/reference/rc/admin.md b/docs/reference/rc/admin.md index 9f79adff..de2c81e7 100644 --- a/docs/reference/rc/admin.md +++ b/docs/reference/rc/admin.md @@ -10,6 +10,13 @@ The `rc admin` operation manages the RustFS Admin API, including scanner and sto ```bash rc [GLOBAL OPTIONS] admin +rc admin account info +rc admin account passwd [--current-password-from-env NAME|--current-password-file PATH] [--new-password-from-env NAME|--new-password-file PATH] +rc admin account mfa status +rc admin account mfa enroll [--no-qr] +rc admin account mfa activate [--code CODE|--code-from-env NAME] [--output-file PATH] +rc admin account mfa disable [--code CODE|--code-from-env NAME] [--password-from-env NAME|--password-file PATH] +rc admin account mfa recovery-codes [--code CODE|--code-from-env NAME] [--output-file PATH] rc admin diagnostics rc admin info [OPTIONS] rc admin scanner status @@ -40,6 +47,9 @@ rc admin decommission [OPTIONS] rc admin decommission status [POOL] [OPTIONS] rc admin rebalance rc admin user ... +rc admin user passwd [--password-from-env NAME|--password-file PATH] +rc admin user mfa status +rc admin user mfa reset [--yes] rc admin policy ... rc admin policy detach ... (--user USER | --group GROUP) rc admin policy entities [--user USER]... [--group GROUP]... [--policy POLICY]... @@ -64,6 +74,7 @@ rc admin replicate remove <--all|--site > | Command | Description | | --- | --- | +| `account` | Inspect and manage the identity the alias authenticates as: password and two-factor authentication. | | `diagnostics` | Read bounded authenticated snapshots or run explicitly confirmed bounded probes. | | `info` | Display cluster, server, or disk information. | | `scanner` | Inspect scanner health, freshness, and cycle state. | @@ -75,7 +86,7 @@ rc admin replicate remove <--all|--site > | `expand` | Manage post-expansion data rebalancing. Alias: `scale`. | | `decommission` | Manage server pool decommissioning. Alias: `decom`. | | `rebalance` | Manage post-expansion rebalancing. | -| `user` | Manage IAM users. | +| `user` | Manage IAM users, including password resets and break-glass two-factor clearing. | | `policy` | Manage IAM policies and attachments. | | `access-key` | Inspect individual or bounded pages of secret-free access-key metadata. | | `group` | Manage IAM groups and group membership. | @@ -86,6 +97,96 @@ rc admin replicate remove <--all|--site > | `bucket-metadata` | Export or import validated per-bucket configuration archives. | | `replicate` | Manage site replication across clusters. | +## Account and Two-Factor Workflow + +`rc admin account` acts on the identity the alias authenticates as. It never +takes a target access key, so it cannot modify another account; managing someone +else's credentials is `rc admin user passwd` and `rc admin user mfa`. + +``` +rc admin account info +rc admin account passwd [secret sources] +rc admin account mfa status +rc admin account mfa enroll [--no-qr] +rc admin account mfa activate [--code CODE | --code-from-env NAME] [--output-file PATH] +rc admin account mfa disable [--code ...] [password sources] +rc admin account mfa recovery-codes [--code ...] [--output-file PATH] + +rc admin user passwd [password sources] +rc admin user mfa status +rc admin user mfa reset [--yes] +``` + +### Two-factor authentication does not gate `rc` + +`rc` signs every request with the alias access key. That path is deliberately not +gated by two-factor authentication: gating it would break every script the moment +a human enabled the second factor on their own account, and it would add no +protection, because whoever holds the secret key already has full access without +presenting a code. The second factor guards session minting — the interactive +console login (`AssumeRole`) — which `rc` does not use. + +Enabling two-factor authentication therefore never breaks an existing alias or +automation. Scripts that do call `AssumeRole` can pass the factor through STS's +own `SerialNumber` and `TokenCode` parameters. + +### Secret and code sources + +No command accepts a password on the command line, where it would be captured by +shell history and visible in `ps`. Each password is read from one of: + +- `--*-from-env NAME` — a named environment variable. +- `--*-file PATH` — the first line of a file, which must not be group- or + world-readable. +- an interactive prompt with echo off, offered only when stdin is a terminal and + the output is human-readable. + +Verification codes additionally accept `--code CODE` because a TOTP code is +valid for at most 90 seconds. `--code` and `--code-from-env` are mutually +exclusive. + +In `--json` mode, or when stdin is not a terminal, a command that would need to +prompt exits with `USAGE_ERROR` naming the flag to pass instead. No command +blocks waiting for input it cannot receive. + +### QR rendering + +The server renders the QR code; `rc` prints the Unicode block art it returns. +There is no QR encoder in the CLI, and the console shows the same symbol from +the same source. `--no-qr` prints only the setup key and the `otpauth://` URI, +and the QR is skipped automatically when the terminal is narrower than 33 +columns, since a wrapped symbol cannot be scanned. + +`--json` output omits the QR entirely — both the SVG and the block art — and +carries `secret_base32` and `otpauth_uri` instead. + +### Recovery codes + +Recovery codes are returned in plaintext exactly once, by `mfa activate` and +`mfa recovery-codes`; the server stores only their hashes and cannot show them +again. `--output-file PATH` writes them with mode `0600` and refuses to +overwrite an existing file, because that file may hold the only copy of a +previous set. Without `--output-file` they are printed to stdout. + +Each code works once. Generating a new set invalidates the previous one. + +### Break-glass reset + +`rc admin user mfa reset` clears another identity's second factor, for a user who +lost both their authenticator and their recovery codes. It names the target and +asks for confirmation; `--yes` is required in `--json` mode or when stdin is not +a terminal. The account is left protected by its password alone until the user +enrols again. + +### Root identities + +A root identity provisioned from `RUSTFS_ACCESS_KEY` cannot have its password or +username changed at runtime: the value is fixed for the life of the process and +also derives the internode RPC secret. `rc admin account info` reports +`credentials_source: env` and `password_mutable: false` for such an identity, and +the mutation commands fail with a message naming the environment variable. Use a +built-in IAM user with the `consoleAdmin` policy for day-to-day administration. + ## IAM archive migration `rc admin iam` round-trips RustFS users, groups, policies, mappings, and supported From d0432c6d438c1cca119ec68814a81d33ad12d75a Mon Sep 17 00:00:00 2001 From: Sinan Eldem Date: Wed, 26 Aug 2026 11:48:48 +0300 Subject: [PATCH 3/4] fix(admin): address the account and two-factor review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of these could lose or corrupt a secret. `--*-file` went through the SSE-C key reader, which stops at 33 bytes. A 40-character secret key handed to `--new-password-file` therefore set a password made of its first 33 bytes, with nothing anywhere reporting it. The reader is now parameterized: the read bound, the error wording and the hardening rules come from the caller. The SSE-C path keeps its exact behaviour, including reading 33 so its own "exactly 32 bytes" check still speaks; the account path reports an oversized file instead of returning a prefix, because nothing downstream verifies that length. It also accepts a symlink and a group-readable mode, which is how Kubernetes projects a secret into a container and something an SSE-C key has no reason to allow. Recovery codes could be lost outright. The write happened after the server had already activated or rotated, and refused to clobber an existing file, so a leftover path meant the only copy of the new set was dropped unprinted — with the previous set already invalid. The path is now checked before the request, and if the write fails anyway the codes are printed rather than discarded, with a non-zero exit saying the file was not written. Under `--json` they go to stdout and the error to stderr, so a script gets both. Also, from the same review: - The reason printed for an immutable password was `enrollment_blocked_reason`, which belongs to enrollment: it attributed a two-factor restriction to the password, and printed nothing in the environment-root case where the field is absent. It is now derived from `credentials_source` and `identity_type`. - `print_enrollment` told the user to scan a QR code it had not printed. It now uses `print_qr`'s return value, which exists for this. - Server-controlled text reached the terminal through `println`, which does not escape: the rendered QR above all, but also the setup URI, timestamps and blocked reasons. These go through `sanitize_text` now. JSON output does not, deliberately — `serde_json` escapes correctly and a consumer needs the value the server sent. - `passwd` spent a round-trip on `account_info` for every run and fell back to the alias name, so `--json` could report an alias as an access key. The call moved into the JSON branch and the field is omitted when unknown. - `mfa disable` resolved the code, possibly prompting, before validating the password flags, so a conflicting pair burned a live TOTP code before the usage error appeared. - The local `fail`/`usage_failure` used `Formatter::error`, so the `--json` error envelope carried `code: null` while the rest of the CLI carried the real one. - The account request helpers re-derived the sign, bound and send sequence. They now sit on `request_bounded_bytes`, extracted from `request_bounded_json`, and the user endpoints pass `accessKey` as a query parameter instead of building it into the path. - The `File` secret arm copied out of its zeroizing buffer, leaving two unzeroized copies of the password; the enroll JSON branch cloned what it could move. `crates/cli/tests/admin_account.rs` covers two exit-code scenarios per new command, per the PR checklist. Two of them pin the failures above: that a 40-byte secret arrives whole, and that an occupied output path is refused before any request leaves. --- crates/cli/src/commands/admin/account.rs | 259 ++++++++--- crates/cli/src/commands/admin/user.rs | 32 +- crates/cli/src/output/qr.rs | 5 +- crates/cli/src/secret_input.rs | 227 ++++++++-- crates/cli/tests/admin_account.rs | 542 +++++++++++++++++++++++ crates/s3/src/admin.rs | 133 +++--- docs/reference/rc/admin.md | 19 +- 7 files changed, 1043 insertions(+), 174 deletions(-) create mode 100644 crates/cli/tests/admin_account.rs diff --git a/crates/cli/src/commands/admin/account.rs b/crates/cli/src/commands/admin/account.rs index a6a0f162..7297506a 100644 --- a/crates/cli/src/commands/admin/account.rs +++ b/crates/cli/src/commands/admin/account.rs @@ -21,7 +21,8 @@ use crate::exit_code::ExitCode; use crate::output::{Formatter, qr}; use crate::secret_input::{SecretSource, can_prompt, read_code_interactive}; use rc_core::admin::{ - AccountApi, AccountInfo, AccountMfaApi, MfaEnrollment, MfaStatus, RecoveryCodes, SecretValue, + AccountApi, AccountInfo, AccountMfaApi, CredentialsSource, IdentityType, MfaEnrollment, + MfaStatus, RecoveryCodes, SecretValue, }; use rc_core::{Error, Result}; @@ -211,7 +212,12 @@ impl From for AccountInfoOutput { #[derive(Serialize)] struct PasswordChangeOutput { success: bool, - access_key: String, + /// Absent when the identity could not be read. + /// + /// Omitted rather than filled with the alias name: `"access_key": "prod"` + /// for an alias called `prod` reads as an access key and is not one. + #[serde(skip_serializing_if = "Option::is_none")] + access_key: Option, sessions_revoked: u32, message: String, } @@ -319,17 +325,29 @@ fn print_account_info(info: &AccountInfo, formatter: &Formatter) { "user" } )); - formatter.println(&format!("Status: {}", info.status)); + formatter.println(&format!( + "Status: {}", + formatter.sanitize_text(&info.status) + )); formatter.println(&format!("Credentials: {}", info.credentials_source)); if let Some(session) = &info.session_access_key { - formatter.println(&format!("Session key: {session}")); + formatter.println(&format!( + "Session key: {}", + formatter.sanitize_text(session) + )); } if !info.policies.is_empty() { - formatter.println(&format!("Policies: {}", info.policies.join(", "))); + formatter.println(&format!( + "Policies: {}", + formatter.sanitize_text(&info.policies.join(", ")) + )); } if !info.member_of.is_empty() { - formatter.println(&format!("Groups: {}", info.member_of.join(", "))); + formatter.println(&format!( + "Groups: {}", + formatter.sanitize_text(&info.member_of.join(", ")) + )); } formatter.println(&format!( @@ -348,9 +366,29 @@ fn print_account_info(info: &AccountInfo, formatter: &Formatter) { if !info.mutable.password { formatter.println(""); formatter.println("This identity's password cannot be changed here."); - if let Some(reason) = &info.mfa.enrollment_blocked_reason { - formatter.println(&format!(" {reason}")); + formatter.println(&format!(" {}", password_immutability_hint(info))); + } +} + +/// Explain an immutable password from the fields the server actually sends. +/// +/// There is no per-field reason on the wire: `AccountMutability` is two bools, +/// and `enrollment_blocked_reason` belongs to enrollment. Reading that field +/// here attributed a two-factor restriction to the password, and printed +/// nothing at all in the environment-root case where it is absent — which is +/// the one case a user is most likely to hit. +fn password_immutability_hint(info: &AccountInfo) -> &'static str { + match (info.credentials_source, info.identity_type) { + (CredentialsSource::Env, _) => { + "It is provisioned from the server environment (RUSTFS_ACCESS_KEY / RUSTFS_SECRET_KEY) and cannot be changed while the server is running." + } + (_, IdentityType::Sts) => { + "It is a temporary session credential. Change the password of the identity it was minted from." } + (_, IdentityType::ServiceAccount) => { + "It is a service account. Change the password of its parent identity." + } + _ => "The server reports this identity's secret as read-only.", } } @@ -389,13 +427,6 @@ async fn execute_passwd(args: PasswdArgs, formatter: &Formatter) -> ExitCode { Err(error) => return usage_failure(formatter, error), }; - let access_key = match client.account_info().await { - Ok(info) => info.access_key, - // Reporting the identity is a convenience; failing to read it must not - // block the rotation. - Err(_) => args.alias.clone(), - }; - match client.account_change_password(¤t, &new).await { Ok(result) => { let message = if result.sessions_revoked > 0 { @@ -408,6 +439,11 @@ async fn execute_passwd(args: PasswdArgs, formatter: &Formatter) -> ExitCode { }; if formatter.is_json() { + // Only the JSON shape names the identity, so the human path does + // not pay for the extra round-trip. Reporting the identity is a + // convenience: failing to read it leaves the field out rather + // than blocking a rotation that already succeeded. + let access_key = client.account_info().await.ok().map(|info| info.access_key); formatter.json(&PasswordChangeOutput { success: true, access_key, @@ -454,13 +490,19 @@ async fn execute_mfa_status(args: MfaStatusArgs, formatter: &Formatter) -> ExitC if status.enabled { formatter.println(&format!( " Algorithm: {} / {} digits / {}s period", - status.algorithm, status.digits, status.period_seconds + formatter.sanitize_text(&status.algorithm), + status.digits, + status.period_seconds )); if let Some(activated) = &status.activated_at { - formatter.println(&format!(" Enabled on: {activated}")); + formatter.println(&format!( + " Enabled on: {}", + formatter.sanitize_text(activated) + )); } if let Some(last) = &status.last_verified_at { - formatter.println(&format!(" Last used: {last}")); + formatter + .println(&format!(" Last used: {}", formatter.sanitize_text(last))); } formatter.println(&format!( " Recovery: {} code(s) remaining", @@ -478,7 +520,10 @@ async fn execute_mfa_status(args: MfaStatusArgs, formatter: &Formatter) -> ExitC if !status.enrollment_available && let Some(reason) = &status.enrollment_blocked_reason { - formatter.println(&format!(" Enrollment unavailable: {reason}")); + formatter.println(&format!( + " Enrollment unavailable: {}", + formatter.sanitize_text(reason) + )); } } ExitCode::Success @@ -496,13 +541,16 @@ async fn execute_mfa_enroll(args: MfaEnrollArgs, formatter: &Formatter) -> ExitC match client.account_mfa_enroll().await { Ok(enrollment) => { if formatter.is_json() { + // Moved, not cloned: the branches are exclusive, and the + // secret should not get a second copy in memory just to be + // serialized. formatter.json(&MfaEnrollOutput { - secret_base32: enrollment.secret_base32.clone(), - otpauth_uri: enrollment.otpauth_uri.clone(), - algorithm: enrollment.algorithm.clone(), + secret_base32: enrollment.secret_base32, + otpauth_uri: enrollment.otpauth_uri, + algorithm: enrollment.algorithm, digits: enrollment.digits, period_seconds: enrollment.period_seconds, - expires_at: enrollment.expires_at.clone(), + expires_at: enrollment.expires_at, }); } else { print_enrollment(&enrollment, args.no_qr, formatter); @@ -514,19 +562,37 @@ async fn execute_mfa_enroll(args: MfaEnrollArgs, formatter: &Formatter) -> ExitC } fn print_enrollment(enrollment: &MfaEnrollment, no_qr: bool, formatter: &Formatter) { - formatter.println("Scan this QR code with your authenticator app:"); - qr::print_qr(formatter, &enrollment.qr_utf8, no_qr); + formatter.println("Two-factor enrollment started."); + + // Only claim there is a code to scan once one has been printed. `--no-qr`, a + // terminal too narrow for the symbol, and an empty payload all skip it, and + // telling somebody to scan a code that is not on their screen sends them + // looking for a rendering bug. + if qr::print_qr(formatter, &enrollment.qr_utf8, no_qr) { + formatter + .println("Scan the code above with your authenticator app, or add the key by hand:"); + } else { + formatter.println("Add this account to your authenticator app by hand:"); + } formatter.println(&format!( "Manual setup key: {}", - group_secret(&enrollment.secret_base32) + group_secret(&formatter.sanitize_text(&enrollment.secret_base32)) + )); + formatter.println(&format!( + "Setup URI: {}", + formatter.sanitize_text(&enrollment.otpauth_uri) )); - formatter.println(&format!("Setup URI: {}", enrollment.otpauth_uri)); formatter.println(&format!( "Parameters: {} / {} digits / {}s period", - enrollment.algorithm, enrollment.digits, enrollment.period_seconds + formatter.sanitize_text(&enrollment.algorithm), + enrollment.digits, + enrollment.period_seconds + )); + formatter.println(&format!( + "Expires: {}", + formatter.sanitize_text(&enrollment.expires_at) )); - formatter.println(&format!("Expires: {}", enrollment.expires_at)); formatter.println(""); formatter.println("Then confirm with:"); formatter.println(" rc admin account mfa activate --code <6-digit code>"); @@ -549,6 +615,10 @@ async fn execute_mfa_activate(args: MfaCodeArgs, formatter: &Formatter) -> ExitC Err(code) => return code, }; + if let Err(exit) = ensure_output_path_is_free(args.output_file.as_deref(), formatter) { + return exit; + } + let code = match resolve_code(args.code, args.code_from_env, formatter) { Ok(code) => code, Err(exit) => return exit, @@ -566,6 +636,10 @@ async fn execute_mfa_recovery_codes(args: MfaCodeArgs, formatter: &Formatter) -> Err(code) => return code, }; + if let Err(exit) = ensure_output_path_is_free(args.output_file.as_deref(), formatter) { + return exit; + } + let code = match resolve_code(args.code, args.code_from_env, formatter) { Ok(code) => code, Err(exit) => return exit, @@ -583,11 +657,10 @@ async fn execute_mfa_disable(args: MfaDisableArgs, formatter: &Formatter) -> Exi Err(code) => return code, }; - let code = match resolve_code(args.code, args.code_from_env, formatter) { - Ok(code) => code, - Err(exit) => return exit, - }; - + // Validate every flag before resolving the code. Resolving may prompt, and a + // TOTP code is single-use against a 30-second window: burning one only to + // then report that `--password-from-env` and `--password-file` conflict + // costs the user a wait they did not need. let interactive = can_prompt(formatter.is_json()); let password_source = match SecretSource::resolve( args.password_from_env, @@ -598,6 +671,12 @@ async fn execute_mfa_disable(args: MfaDisableArgs, formatter: &Formatter) -> Exi Ok(source) => source, Err(error) => return usage_failure(formatter, error), }; + + let code = match resolve_code(args.code, args.code_from_env, formatter) { + Ok(code) => code, + Err(exit) => return exit, + }; + let password = match password_source.load("Account password: ") { Ok(value) => SecretValue::new(value.to_string()), Err(error) => return usage_failure(formatter, error), @@ -666,40 +745,95 @@ fn resolve_code( } } +/// Reject an occupied output path *before* the server issues a set. +/// +/// `write_recovery_codes` will not clobber an existing file, and by the time it +/// runs the server has already activated or rotated: the set it refuses to +/// write is the only copy that will ever exist, and the previous set is already +/// invalid. Checking here costs a syscall; checking there costs the codes. +fn ensure_output_path_is_free( + output_file: Option<&std::path::Path>, + formatter: &Formatter, +) -> std::result::Result<(), ExitCode> { + let Some(path) = output_file else { + return Ok(()); + }; + // `symlink_metadata`, not `exists`: a dangling symlink reports absent but + // still makes the `create_new` open fail with `AlreadyExists`. + if std::fs::symlink_metadata(path).is_ok() { + // Through `fail`, not `usage_failure`, so this reports the same exit + // code as `write_recovery_codes` refusing the very same path later. + return Err(fail( + formatter, + "Cannot write the recovery codes", + Error::Conflict(format!( + "{} already exists; choose another path", + path.display() + )), + )); + } + Ok(()) +} + /// Print or write a recovery-code set. /// -/// These exist in plaintext exactly once, so the human-readable path is loud -/// about that and the file path refuses to clobber an existing file rather than -/// destroying a set the user may not have stored yet. +/// These exist in plaintext exactly once. The file path refuses to clobber an +/// existing file, and if the write fails anyway the set is printed instead of +/// being dropped: a file the operator has to re-create is a nuisance, a set +/// nobody ever saw is a locked-out account. fn emit_recovery_codes( codes: RecoveryCodes, output_file: Option, formatter: &Formatter, activated: bool, ) -> ExitCode { - if let Some(path) = output_file { - if let Err(error) = write_recovery_codes(&path, &codes.recovery_codes) { - return fail(formatter, "Failed to write the recovery codes", error); + let Some(path) = output_file else { + return print_recovery_codes(&codes, formatter, activated); + }; + + match write_recovery_codes(&path, &codes.recovery_codes) { + Ok(()) => { + if formatter.is_json() { + formatter.json(&MfaOperationOutput { + success: true, + message: format!("Recovery codes written to {}", path.display()), + }); + } else { + formatter.println(&format!( + "{} recovery code(s) written to {} (mode 0600).", + codes.recovery_codes.len(), + path.display() + )); + } + ExitCode::Success } - if formatter.is_json() { - formatter.json(&MfaOperationOutput { - success: true, - message: format!("Recovery codes written to {}", path.display()), - }); - } else { - formatter.println(&format!( - "{} recovery code(s) written to {} (mode 0600).", - codes.recovery_codes.len(), - path.display() - )); + Err(error) => { + // The server has already issued this set and invalidated any + // previous one, so it exists nowhere but in this process. Print it + // before reporting the failure. In `--json` mode the codes go to + // stdout and the error to stderr, so a script gets both and the + // exit code still says the file was not written. + print_recovery_codes(&codes, formatter, activated); + fail( + formatter, + &format!( + "The codes above were NOT written to {}; store them now", + path.display() + ), + error, + ) } - return ExitCode::Success; } +} +fn print_recovery_codes(codes: &RecoveryCodes, formatter: &Formatter, activated: bool) -> ExitCode { if formatter.is_json() { + // Not sanitized: `serde_json` escapes control characters correctly, and + // a consumer needs the value the server sent. Escaping is a terminal + // concern, so it belongs on the human path below and nowhere else. formatter.json(&RecoveryCodesOutput { - recovery_codes: codes.recovery_codes, - generated_at: codes.generated_at, + recovery_codes: codes.recovery_codes.clone(), + generated_at: codes.generated_at.clone(), }); return ExitCode::Success; } @@ -712,7 +846,11 @@ fn emit_recovery_codes( formatter.println(""); formatter.println("Save these recovery codes. They are shown only once:"); for (index, code) in codes.recovery_codes.iter().enumerate() { - formatter.println(&format!(" {:2}. {code}", index + 1)); + formatter.println(&format!( + " {:2}. {}", + index + 1, + formatter.sanitize_text(code) + )); } formatter.println(""); formatter.println("Each code works once. Store them somewhere only you can reach."); @@ -751,15 +889,18 @@ fn write_recovery_codes(path: &std::path::Path, codes: &[String]) -> Result<()> Ok(()) } +/// Report a failed operation and return its exit code. +/// +/// `Formatter::fail` rather than `error`: the latter builds the descriptor from +/// a bare message, so the `--json` error envelope carried `code: null` while +/// every other command reported the real one. fn fail(formatter: &Formatter, context: &str, error: Error) -> ExitCode { let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); - formatter.error(&format!("{context}: {error}")); - code + formatter.fail(code, &format!("{context}: {error}")) } fn usage_failure(formatter: &Formatter, error: Error) -> ExitCode { - formatter.error(&error.to_string()); - ExitCode::UsageError + formatter.fail(ExitCode::UsageError, &error.to_string()) } #[cfg(test)] diff --git a/crates/cli/src/commands/admin/user.rs b/crates/cli/src/commands/admin/user.rs index bc6703d6..bcabace3 100644 --- a/crates/cli/src/commands/admin/user.rs +++ b/crates/cli/src/commands/admin/user.rs @@ -266,17 +266,11 @@ async fn execute_passwd(args: PasswdArgs, formatter: &Formatter) -> ExitCode { "password", ) { Ok(source) => source, - Err(error) => { - formatter.error(&error.to_string()); - return ExitCode::UsageError; - } + Err(error) => return formatter.fail(ExitCode::UsageError, &error.to_string()), }; let secret = match source.load(&format!("New password for {}: ", args.access_key)) { Ok(value) => SecretValue::new(value.to_string()), - Err(error) => { - formatter.error(&error.to_string()); - return ExitCode::UsageError; - } + Err(error) => return formatter.fail(ExitCode::UsageError, &error.to_string()), }; match client.set_user_secret_key(&args.access_key, &secret).await { @@ -304,8 +298,7 @@ async fn execute_passwd(args: PasswdArgs, formatter: &Formatter) -> ExitCode { } Err(error) => { let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); - formatter.error(&format!("Failed to reset the password: {error}")); - code + formatter.fail(code, &format!("Failed to reset the password: {error}")) } } } @@ -340,7 +333,10 @@ async fn execute_user_mfa_status(args: UserMfaStatusArgs, formatter: &Formatter) )); if status.enabled { if let Some(activated) = &status.activated_at { - formatter.println(&format!(" Enabled on: {activated}")); + formatter.println(&format!( + " Enabled on: {}", + formatter.sanitize_text(activated) + )); } formatter.println(&format!( " Recovery: {} code(s) remaining", @@ -352,8 +348,7 @@ async fn execute_user_mfa_status(args: UserMfaStatusArgs, formatter: &Formatter) } Err(error) => { let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); - formatter.error(&format!("Failed to read two-factor state: {error}")); - code + formatter.fail(code, &format!("Failed to read two-factor state: {error}")) } } } @@ -366,8 +361,7 @@ async fn execute_user_mfa_reset(args: UserMfaResetArgs, formatter: &Formatter) - if let Err(error) = confirm_mfa_reset(&args.access_key, args.yes, formatter) { let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); - formatter.error(&error.to_string()); - return code; + return formatter.fail(code, &error.to_string()); } match client.user_mfa_reset(&args.access_key).await { @@ -390,10 +384,10 @@ async fn execute_user_mfa_reset(args: UserMfaResetArgs, formatter: &Formatter) - } Err(error) => { let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); - formatter.error(&format!( - "Failed to clear two-factor authentication: {error}" - )); - code + formatter.fail( + code, + &format!("Failed to clear two-factor authentication: {error}"), + ) } } } diff --git a/crates/cli/src/output/qr.rs b/crates/cli/src/output/qr.rs index eb71f5ec..c071ab6b 100644 --- a/crates/cli/src/output/qr.rs +++ b/crates/cli/src/output/qr.rs @@ -40,7 +40,10 @@ pub fn print_qr(formatter: &Formatter, qr_utf8: &str, suppressed: bool) -> bool formatter.println(""); for line in qr_utf8.lines() { - formatter.println(line); + // The symbol is server-rendered, so it is escaped like any other server + // text before it reaches a terminal. Block-drawing characters survive + // untouched; an escape sequence smuggled in alongside them does not. + formatter.println(&formatter.sanitize_text(line)); } formatter.println(""); true diff --git a/crates/cli/src/secret_input.rs b/crates/cli/src/secret_input.rs index ae694b47..f05ab93c 100644 --- a/crates/cli/src/secret_input.rs +++ b/crates/cli/src/secret_input.rs @@ -58,7 +58,7 @@ impl SecretLocator { pub(crate) fn load_customer_key(&self) -> Result { self.validate()?; let bytes = match self { - Self::File(path) => read_protected_key_file(path)?, + Self::File(path) => read_protected_file(path, &SSE_C_KEY_FILE)?, Self::Environment(name) => read_environment_key(name)?, }; if bytes.len() != 32 { @@ -76,24 +76,87 @@ fn valid_environment_name(name: &str) -> bool { && bytes.all(|byte| byte.is_ascii_alphanumeric() || byte == b'_') } -fn read_protected_key_file(path: &Path) -> Result>> { - let path_metadata = std::fs::symlink_metadata(path) - .map_err(|_| Error::InvalidPath("Failed to inspect SSE-C key file".to_string()))?; - if path_metadata.file_type().is_symlink() || !path_metadata.is_file() { - return Err(Error::InvalidPath( - "SSE-C key input must be a regular file, not a symlink".to_string(), - )); +/// How a file holding secret material is read. +/// +/// An SSE-C key and an account password are different shapes of secret, and one +/// reader serving both silently truncated the longer one. Everything that +/// differs between them lives here, so a caller cannot inherit a length bound, +/// a hardening rule, or an error message meant for the other. +struct ProtectedFileSpec { + /// Most bytes to read from the file. + read_limit: usize, + /// Report a file longer than `read_limit` instead of returning the prefix. + /// + /// Off for the SSE-C key, whose caller checks for exactly 32 bytes and has + /// its own wording for a file that is not: reading 33 and letting that check + /// speak keeps its message unchanged. On wherever nothing downstream + /// verifies the length, which is where a silent prefix becomes a password + /// the operator does not know. + reject_oversize: bool, + /// Noun used in error messages, so a password failure never mentions SSE-C. + subject: &'static str, + /// Require a regular file, not a symlink, with no group or other permission. + /// + /// On for an SSE-C key: long-lived encryption material, placed by the + /// operator, where a symlink or a readable mode is worth refusing. Off for + /// an account password, which is routinely a Kubernetes secret mount — + /// those are symlinks into `..data/` and are group-readable inside the + /// container by default, so the strict rule rejects an ordinary deployment. + owner_only_regular_file: bool, +} + +/// A 32-byte SSE-C customer key. +/// +/// Reads 33 so `load_customer_key` can distinguish "exactly 32" from "more than +/// that" and report it in the words it always has. +const SSE_C_KEY_FILE: ProtectedFileSpec = ProtectedFileSpec { + read_limit: 33, + reject_oversize: false, + subject: "SSE-C key file", + owner_only_regular_file: true, +}; + +/// A password or secret key. S3 sets no maximum secret-key length, so this is a +/// sanity bound on a file meant to hold a single line, not a protocol limit. +const ACCOUNT_SECRET_FILE: ProtectedFileSpec = ProtectedFileSpec { + read_limit: 4096, + reject_oversize: true, + subject: "secret file", + owner_only_regular_file: false, +}; + +fn read_protected_file(path: &Path, spec: &ProtectedFileSpec) -> Result>> { + let subject = spec.subject; + + // `symlink_metadata` when a symlink is a hard error, plain `metadata` when + // one is allowed: either way the identity check below compares against the + // file that was actually opened. + let path_metadata = if spec.owner_only_regular_file { + std::fs::symlink_metadata(path) + } else { + std::fs::metadata(path) + } + // No article here, unlike its siblings: `tests/sse_customer.rs` pins this + // exact string, and rewording another command's error is not this change's + // business. + .map_err(|_| Error::InvalidPath(format!("Failed to inspect {subject}")))?; + if !path_metadata.is_file() { + return Err(Error::InvalidPath(if spec.owner_only_regular_file { + format!("The {subject} must be a regular file, not a symlink") + } else { + format!("The {subject} must be a regular file") + })); } let file = File::open(path) - .map_err(|_| Error::InvalidPath("Failed to open SSE-C key file".to_string()))?; + .map_err(|_| Error::InvalidPath(format!("Failed to open the {subject}")))?; let file_metadata = file .metadata() - .map_err(|_| Error::InvalidPath("Failed to inspect opened SSE-C key file".to_string()))?; + .map_err(|_| Error::InvalidPath(format!("Failed to inspect the opened {subject}")))?; if !file_metadata.is_file() { - return Err(Error::InvalidPath( - "SSE-C key input must remain a regular file while opening".to_string(), - )); + return Err(Error::InvalidPath(format!( + "The {subject} must remain a regular file while opening" + ))); } #[cfg(unix)] @@ -102,18 +165,18 @@ fn read_protected_key_file(path: &Path) -> Result>> { if path_metadata.dev() != file_metadata.dev() || path_metadata.ino() != file_metadata.ino() { - return Err(Error::InvalidPath( - "SSE-C key file changed while being opened".to_string(), - )); + return Err(Error::InvalidPath(format!( + "The {subject} changed while being opened" + ))); } - if file_metadata.permissions().mode() & 0o077 != 0 { - return Err(Error::InvalidPath( - "SSE-C key file cannot grant group or other permissions".to_string(), - )); + if spec.owner_only_regular_file && file_metadata.permissions().mode() & 0o077 != 0 { + return Err(Error::InvalidPath(format!( + "The {subject} cannot grant group or other permissions" + ))); } } - read_exact_key(file) + read_bounded(file, spec) } fn read_environment_key(name: &str) -> Result>> { @@ -125,12 +188,31 @@ fn read_environment_key(name: &str) -> Result>> { Ok(Zeroizing::new(value.into_bytes())) } -fn read_exact_key(reader: impl Read) -> Result>> { - let mut bytes = Zeroizing::new(Vec::with_capacity(33)); +/// Read at most `read_limit`, and fail rather than truncate when there is more. +/// +/// Truncating is the dangerous outcome: an operator who fed a 40-character +/// secret key to `--new-password-file` would have set a password consisting of +/// its first 33 bytes, with nothing anywhere saying so. +fn read_bounded(reader: impl Read, spec: &ProtectedFileSpec) -> Result>> { + let subject = spec.subject; + // One past the bound when oversize is an error, so a file exactly at the + // bound is not mistaken for one that ran over it. + let probe = if spec.reject_oversize { + spec.read_limit.saturating_add(1) + } else { + spec.read_limit + }; + let mut bytes = Zeroizing::new(Vec::with_capacity(probe)); reader - .take(33) + .take(probe as u64) .read_to_end(&mut bytes) - .map_err(|_| Error::InvalidPath("Failed to read SSE-C key file".to_string()))?; + .map_err(|_| Error::InvalidPath(format!("Failed to read the {subject}")))?; + if spec.reject_oversize && bytes.len() > spec.read_limit { + return Err(Error::InvalidPath(format!( + "The {subject} is larger than {} bytes", + spec.read_limit + ))); + } Ok(bytes) } @@ -338,9 +420,12 @@ impl SecretSource { Ok(value) } Self::File(path) => { - let bytes = read_protected_key_file(path)?; - let text = String::from_utf8(bytes.to_vec()) - .map_err(|_| Error::InvalidPath("Secret file must be UTF-8".to_string()))?; + let bytes = read_protected_file(path, &ACCOUNT_SECRET_FILE)?; + // Borrow the zeroizing buffer rather than copying out of it: + // `String::from_utf8(bytes.to_vec())` would leave two further + // copies of the secret in memory with nothing to wipe them. + let text = std::str::from_utf8(&bytes) + .map_err(|_| Error::InvalidPath("The secret file must be UTF-8".to_string()))?; // First line only: an editor-written file usually has a trailing // newline, and a stray second line is more likely a mistake than // part of the secret. @@ -352,7 +437,7 @@ impl SecretSource { .to_string(), ); if value.is_empty() { - return Err(Error::InvalidPath("Secret file is empty".to_string())); + return Err(Error::InvalidPath("The secret file is empty".to_string())); } Ok(value) } @@ -391,3 +476,87 @@ pub(crate) fn can_prompt(is_json: bool) -> bool { use std::io::IsTerminal; !is_json && std::io::stdin().is_terminal() } + +#[cfg(test)] +mod account_secret_tests { + use super::*; + use std::io::Write as _; + + fn write_secret_file(contents: &[u8]) -> tempfile::NamedTempFile { + let mut file = tempfile::NamedTempFile::new().expect("create secret file"); + file.write_all(contents).expect("write secret file"); + file + } + + #[test] + fn a_secret_longer_than_the_sse_c_bound_is_read_whole() { + // The regression this guards: the reader was shared with SSE-C and + // stopped at 33 bytes, so a 40-character secret key silently became a + // password made of its first 33 bytes. + let secret = "A".repeat(40); + let file = write_secret_file(secret.as_bytes()); + let loaded = SecretSource::File(file.path().to_path_buf()) + .load("") + .expect("a 40-byte secret must load"); + + assert_eq!(loaded.as_str(), secret); + } + + #[test] + fn an_oversized_secret_file_is_rejected_rather_than_truncated() { + let file = write_secret_file(&vec![b'A'; ACCOUNT_SECRET_FILE.read_limit + 1]); + let error = SecretSource::File(file.path().to_path_buf()) + .load("") + .expect_err("an oversized file must fail"); + + assert!(matches!(error, Error::InvalidPath(_)), "{error:?}"); + assert!(error.to_string().contains("larger than"), "{error}"); + // Never the contents, however long. + assert!(!error.to_string().contains("AAAA"), "{error}"); + } + + #[test] + fn secret_file_errors_never_mention_sse_c() { + // Somebody changing a password should not be told about an SSE-C key. + let missing = SecretSource::File(PathBuf::from("no-such-secret-file")) + .load("") + .expect_err("a missing file must fail"); + assert!(!missing.to_string().contains("SSE-C"), "{missing}"); + + let empty = write_secret_file(b""); + let error = SecretSource::File(empty.path().to_path_buf()) + .load("") + .expect_err("an empty file must fail"); + assert!(!error.to_string().contains("SSE-C"), "{error}"); + } + + #[cfg(unix)] + #[test] + fn a_symlinked_group_readable_secret_file_loads() { + // The shape Kubernetes mounts a secret in: a symlink into `..data/`, + // group-readable inside the container. The SSE-C rules reject both. + use std::os::unix::fs::{PermissionsExt as _, symlink}; + + let file = write_secret_file(b"projected-secret\n"); + std::fs::set_permissions(file.path(), std::fs::Permissions::from_mode(0o644)) + .expect("relax permissions the way a projected volume does"); + + let link_dir = tempfile::TempDir::new().expect("create link directory"); + let link = link_dir.path().join("password"); + symlink(file.path(), &link).expect("create the secret symlink"); + + let loaded = SecretSource::File(link) + .load("") + .expect("a projected secret must load"); + assert_eq!(loaded.as_str(), "projected-secret"); + } + + #[test] + fn only_the_first_line_of_a_secret_file_is_used() { + let file = write_secret_file(b"the-secret\nnot-part-of-it\n"); + let loaded = SecretSource::File(file.path().to_path_buf()) + .load("") + .expect("load first line"); + assert_eq!(loaded.as_str(), "the-secret"); + } +} diff --git a/crates/cli/tests/admin_account.rs b/crates/cli/tests/admin_account.rs new file mode 100644 index 00000000..6933daad --- /dev/null +++ b/crates/cli/tests/admin_account.rs @@ -0,0 +1,542 @@ +//! Exit-code scenarios for `rc admin account …` and the `rc admin user` +//! credential commands. +//! +//! Two per command, per the PR checklist in AGENTS.md. Several of these guard a +//! specific way an earlier revision lost data or truncated a secret, so they +//! assert on what reached the wire rather than only on the exit status. +#![cfg(not(windows))] + +mod admin_support; + +use std::fs; +use std::process::Command; +use std::time::Duration; + +use admin_support::{ + rc_binary, rc_host_alias, start_admin_response_test_server, start_admin_test_server, +}; + +/// An alias pointing at a port nothing is listening on. +/// +/// Used by the tests that must prove a command fails *before* any request: if +/// one were sent, the command would report a network error instead of the +/// usage error being asserted. +const UNREACHABLE_ENDPOINT: &str = "http://127.0.0.1:1"; + +const ACCOUNT_INFO_BODY: &str = r#"{"access_key":"admin","identity_type":"root","is_admin":true,"status":"enabled","credentials_source":"env","mutable":{"password":false,"username":false},"mfa":{"enabled":false,"pending":false,"recovery_codes_remaining":0,"enrollment_available":true}}"#; + +const RECOVERY_CODES_BODY: &str = r#"{"recovery_codes":["AAAA1111BBBB2222CCCC","DDDD3333EEEE4444FFFF"],"generated_at":"2026-01-01T00:00:00Z"}"#; + +fn rc() -> Command { + Command::new(rc_binary()) +} + +// --------------------------------------------------------------------------- +// account info +// --------------------------------------------------------------------------- + +#[test] +fn account_info_succeeds_against_a_server_that_answers() { + let config_dir = tempfile::tempdir().expect("create config dir"); + let (endpoint, receiver, handle) = start_admin_test_server(ACCOUNT_INFO_BODY); + + let output = rc() + .args(["--json", "admin", "account", "info", "myalias"]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!( + output.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let request = receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + assert_eq!(request.method, "GET"); + assert_eq!(request.target, "/rustfs/admin/v3/account/info"); + handle.join().expect("admin test server finished"); +} + +#[test] +fn account_info_reports_auth_error_when_the_server_refuses() { + let config_dir = tempfile::tempdir().expect("create config dir"); + let (endpoint, receiver, handle) = start_admin_response_test_server( + "403 Forbidden", + "application/json", + r#"{"Code":"AccessDenied","Message":"not allowed"}"#.to_string(), + ); + + let output = rc() + .args(["--json", "admin", "account", "info", "myalias"]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(4), "expected AuthError"); + receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + handle.join().expect("admin test server finished"); +} + +// --------------------------------------------------------------------------- +// account passwd +// --------------------------------------------------------------------------- + +#[test] +fn account_passwd_rejects_two_sources_for_one_secret_before_connecting() { + let config_dir = tempfile::tempdir().expect("create config dir"); + + let output = rc() + .args([ + "--json", + "admin", + "account", + "passwd", + "myalias", + "--current-password-from-env", + "RC_TEST_CURRENT", + "--current-password-file", + "/dev/null", + "--new-password-from-env", + "RC_TEST_NEW", + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(UNREACHABLE_ENDPOINT)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(2), "expected UsageError"); +} + +#[test] +fn account_passwd_sends_a_secret_longer_than_the_former_thirty_three_byte_bound() { + // The regression: the password file went through the SSE-C reader, which + // stopped at 33 bytes, so a 40-character secret key became a password made + // of its first 33 bytes with nothing reporting it. + let config_dir = tempfile::tempdir().expect("create config dir"); + let secret_dir = tempfile::tempdir().expect("create secret dir"); + let long_secret = "L".repeat(40); + let current = secret_dir.path().join("current"); + let new = secret_dir.path().join("new"); + fs::write(¤t, "old-password\n").expect("write current password"); + fs::write(&new, format!("{long_secret}\n")).expect("write new password"); + + let (endpoint, receiver, handle) = start_admin_test_server(r#"{"sessions_revoked":0}"#); + + let output = rc() + .args([ + "--json", + "admin", + "account", + "passwd", + "myalias", + "--current-password-file", + current.to_str().expect("utf-8 path"), + "--new-password-file", + new.to_str().expect("utf-8 path"), + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!( + output.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let request = receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + let body = String::from_utf8_lossy(&request.body); + assert!( + body.contains(&long_secret), + "the whole 40-byte secret must reach the server, got: {body}" + ); + handle.join().expect("admin test server finished"); +} + +// --------------------------------------------------------------------------- +// account mfa status / enroll +// --------------------------------------------------------------------------- + +#[test] +fn mfa_status_succeeds_against_a_server_that_answers() { + let config_dir = tempfile::tempdir().expect("create config dir"); + let (endpoint, receiver, handle) = start_admin_test_server( + r#"{"enabled":false,"pending":false,"algorithm":"SHA1","digits":6,"period_seconds":30,"recovery_codes_remaining":0,"enrollment_available":true}"#, + ); + + let output = rc() + .args(["--json", "admin", "account", "mfa", "status", "myalias"]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!( + output.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let request = receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + assert_eq!(request.target, "/rustfs/admin/v3/account/mfa"); + handle.join().expect("admin test server finished"); +} + +#[test] +fn mfa_enroll_reports_unsupported_when_the_server_has_no_such_route() { + let config_dir = tempfile::tempdir().expect("create config dir"); + let (endpoint, receiver, handle) = start_admin_response_test_server( + "501 Not Implemented", + "application/json", + r#"{"Code":"NotImplemented","Message":"at-rest protection is not configured"}"#.to_string(), + ); + + let output = rc() + .args(["--json", "admin", "account", "mfa", "enroll", "myalias"]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(7), "expected UnsupportedFeature"); + receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + handle.join().expect("admin test server finished"); +} + +// --------------------------------------------------------------------------- +// account mfa activate / recovery-codes +// --------------------------------------------------------------------------- + +#[test] +fn mfa_activate_refuses_an_occupied_output_path_before_the_server_rotates() { + // The lost-codes case. `write_recovery_codes` will not clobber, and by the + // time it runs the server has already activated: the set it refuses to + // write is the only copy there will ever be. So the path is checked first, + // and no request may leave. + let config_dir = tempfile::tempdir().expect("create config dir"); + let out_dir = tempfile::tempdir().expect("create output dir"); + let occupied = out_dir.path().join("codes.txt"); + fs::write(&occupied, "an earlier set\n").expect("occupy the output path"); + + let output = rc() + .args([ + "--json", + "admin", + "account", + "mfa", + "activate", + "myalias", + "--code", + "123456", + "--output-file", + occupied.to_str().expect("utf-8 path"), + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(UNREACHABLE_ENDPOINT)) + .output() + .expect("run rc command"); + + // Conflict, not a network error: nothing was sent to the unreachable host. + assert_eq!( + output.status.code(), + Some(6), + "expected Conflict, stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + fs::read_to_string(&occupied).expect("read back"), + "an earlier set\n", + "the existing file must be untouched" + ); +} + +#[test] +fn mfa_activate_writes_the_codes_to_a_fresh_owner_only_file() { + let config_dir = tempfile::tempdir().expect("create config dir"); + let out_dir = tempfile::tempdir().expect("create output dir"); + let target = out_dir.path().join("codes.txt"); + let (endpoint, receiver, handle) = start_admin_test_server(RECOVERY_CODES_BODY); + + let output = rc() + .args([ + "--json", + "admin", + "account", + "mfa", + "activate", + "myalias", + "--code", + "123456", + "--output-file", + target.to_str().expect("utf-8 path"), + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!( + output.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let written = fs::read_to_string(&target).expect("read the codes back"); + assert!(written.contains("AAAA1111BBBB2222CCCC"), "got: {written}"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + let mode = fs::metadata(&target) + .expect("metadata") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "codes file must be owner-only"); + } + receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + handle.join().expect("admin test server finished"); +} + +#[test] +fn recovery_codes_prints_the_set_when_the_file_cannot_be_written() { + // The server has already invalidated the previous set, so the new one must + // reach the operator even though the write failed. A mistyped directory + // passes the up-front path check — nothing is there — and then fails at the + // open, which is precisely the window the fallback exists for. + let config_dir = tempfile::tempdir().expect("create config dir"); + let out_dir = tempfile::tempdir().expect("create output dir"); + let target = out_dir.path().join("no-such-directory").join("codes.txt"); + + let (endpoint, receiver, handle) = start_admin_test_server(RECOVERY_CODES_BODY); + + let output = rc() + .args([ + "admin", + "account", + "mfa", + "recovery-codes", + "myalias", + "--code", + "123456", + "--output-file", + target.to_str().expect("utf-8 path"), + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_ne!( + output.status.code(), + Some(0), + "a failed write must not report success" + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("AAAA1111BBBB2222CCCC"), + "the codes must be printed rather than lost, stdout: {stdout}" + ); + receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + handle.join().expect("admin test server finished"); +} + +// --------------------------------------------------------------------------- +// account mfa disable +// --------------------------------------------------------------------------- + +#[test] +fn mfa_disable_reports_conflicting_password_flags_before_consuming_a_code() { + // Resolving the code may prompt, and a TOTP code is single-use inside a + // 30-second window. The flag conflict has to surface first. + let config_dir = tempfile::tempdir().expect("create config dir"); + + let output = rc() + .args([ + "--json", + "admin", + "account", + "mfa", + "disable", + "myalias", + "--code", + "123456", + "--password-from-env", + "RC_TEST_PW", + "--password-file", + "/dev/null", + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(UNREACHABLE_ENDPOINT)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(2), "expected UsageError"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!(stderr.contains("not both"), "stderr: {stderr}"); +} + +#[test] +fn mfa_disable_requires_a_password_source_without_a_terminal() { + let config_dir = tempfile::tempdir().expect("create config dir"); + + let output = rc() + .args([ + "--json", "admin", "account", "mfa", "disable", "myalias", "--code", "123456", + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(UNREACHABLE_ENDPOINT)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(2), "expected UsageError"); +} + +// --------------------------------------------------------------------------- +// user passwd / user mfa +// --------------------------------------------------------------------------- + +#[test] +fn user_passwd_requires_a_password_source_without_a_terminal() { + let config_dir = tempfile::tempdir().expect("create config dir"); + + let output = rc() + .args(["--json", "admin", "user", "passwd", "myalias", "analyst"]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(UNREACHABLE_ENDPOINT)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(2), "expected UsageError"); +} + +#[test] +fn user_passwd_targets_the_named_identity() { + let config_dir = tempfile::tempdir().expect("create config dir"); + let secret_dir = tempfile::tempdir().expect("create secret dir"); + let password = secret_dir.path().join("password"); + fs::write(&password, "a-new-password\n").expect("write password"); + + let (endpoint, receiver, handle) = start_admin_test_server(r#"{"sessions_revoked":2}"#); + + let output = rc() + .args([ + "--json", + "admin", + "user", + "passwd", + "myalias", + "analyst", + "--password-file", + password.to_str().expect("utf-8 path"), + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!( + output.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let request = receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + assert_eq!(request.method, "PUT"); + assert_eq!( + request.target, + "/rustfs/admin/v3/set-user-secret-key?accessKey=analyst" + ); + handle.join().expect("admin test server finished"); +} + +#[test] +fn user_mfa_reset_requires_yes_in_json_mode() { + let config_dir = tempfile::tempdir().expect("create config dir"); + + let output = rc() + .args([ + "--json", "admin", "user", "mfa", "reset", "myalias", "analyst", + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(UNREACHABLE_ENDPOINT)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(2), "expected UsageError"); +} + +#[test] +fn user_mfa_reset_sends_the_delete_when_confirmed() { + let config_dir = tempfile::tempdir().expect("create config dir"); + let (endpoint, receiver, handle) = start_admin_test_server(""); + + let output = rc() + .args([ + "--json", "admin", "user", "mfa", "reset", "myalias", "analyst", "--yes", + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!( + output.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let request = receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + assert_eq!(request.method, "DELETE"); + assert_eq!( + request.target, + "/rustfs/admin/v3/user/mfa?accessKey=analyst" + ); + handle.join().expect("admin test server finished"); +} + +#[test] +fn user_mfa_status_reports_auth_error_when_the_server_refuses() { + let config_dir = tempfile::tempdir().expect("create config dir"); + let (endpoint, receiver, handle) = start_admin_response_test_server( + "403 Forbidden", + "application/json", + r#"{"Code":"AccessDenied","Message":"not allowed"}"#.to_string(), + ); + + let output = rc() + .args([ + "--json", "admin", "user", "mfa", "status", "myalias", "analyst", + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(4), "expected AuthError"); + receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + handle.join().expect("admin test server finished"); +} diff --git a/crates/s3/src/admin.rs b/crates/s3/src/admin.rs index 075ab911..14904bd9 100644 --- a/crates/s3/src/admin.rs +++ b/crates/s3/src/admin.rs @@ -670,6 +670,26 @@ impl AdminClient { body: Option<&[u8]>, response: BoundedJsonResponse, ) -> Result { + let response_body = self + .request_bounded_bytes(method, path, query, body, response) + .await?; + serde_json::from_slice(&response_body).map_err(Error::Json) + } + + /// One signed admin request with a bounded response, returning raw bytes. + /// + /// Every bounded admin family goes through here so the signing input, the + /// query encoding and the response bound cannot drift apart between them: + /// a second copy of this sequence is a second chance for a query parameter + /// to be encoded one way for SigV4 and another way on the wire. + async fn request_bounded_bytes( + &self, + method: Method, + path: &str, + query: Option<&[(&str, &str)]>, + body: Option<&[u8]>, + response: BoundedJsonResponse, + ) -> Result> { let mut url = self.admin_url(path); if let Some(query) = query { let query_string = query @@ -717,51 +737,19 @@ impl AdminClient { )); } - serde_json::from_slice(&response_body).map_err(Error::Json) + Ok(response_body) } - /// One bounded, signed admin request for the account/MFA family, returning - /// the raw response body. + /// The response bound for the account/MFA family. /// - /// Bounded because an MFA response carries a rendered QR and a recovery-code - /// set: generous, but never unbounded. - async fn request_account_bytes( - &self, - method: Method, - path: &str, - body: Option<&[u8]>, - ) -> Result> { - let url = self.admin_url(path); - let body_bytes = body.unwrap_or_default(); - let headers = self.request_headers(body_bytes)?; - let signed_headers = self - .sign_request(&method, &url, &headers, body_bytes) - .await?; - let mut request = self.http_client.request(method, &url); - for (name, value) in &signed_headers { - request = request.header(name, value); - } - if !body_bytes.is_empty() { - request = request.body(body_bytes.to_vec()); - } - - let response = request - .send() - .await - .map_err(|_| Error::Network("Account administration request failed".to_string()))?; - let status = response.status(); - let response_body = read_bounded_response_body( - response, - MAX_ACCOUNT_RESPONSE_BYTES, - "Account administration response", - ) - .await?; - - if !status.is_success() { - return Err(self.map_error(status, &String::from_utf8_lossy(&response_body))); + /// Generous because an MFA response carries a rendered QR and a + /// recovery-code set, but never unbounded. + fn account_response() -> BoundedJsonResponse { + BoundedJsonResponse { + max_bytes: MAX_ACCOUNT_RESPONSE_BYTES, + name: "Account administration response", + error_mapper: |client, status, body| client.map_error(status, body), } - - Ok(response_body) } /// A request whose response is a JSON document. @@ -773,9 +761,12 @@ impl AdminClient { &self, method: Method, path: &str, + query: Option<&[(&str, &str)]>, body: Option<&[u8]>, ) -> Result { - let response_body = self.request_account_bytes(method, path, body).await?; + let response_body = self + .request_bounded_bytes(method, path, query, body, Self::account_response()) + .await?; if response_body.is_empty() { return Err(Error::General( "RustFS returned an empty account administration response".to_string(), @@ -789,9 +780,11 @@ impl AdminClient { &self, method: Method, path: &str, + query: Option<&[(&str, &str)]>, body: Option<&[u8]>, ) -> Result<()> { - self.request_account_bytes(method, path, body).await?; + self.request_bounded_bytes(method, path, query, body, Self::account_response()) + .await?; Ok(()) } @@ -3399,7 +3392,7 @@ impl OidcReadApi for AdminClient { #[async_trait] impl AccountApi for AdminClient { async fn account_info(&self) -> Result { - self.request_account_json(Method::GET, "/account/info", None) + self.request_account_json(Method::GET, "/account/info", None, None) .await } @@ -3420,7 +3413,7 @@ impl AccountApi for AdminClient { })) .map_err(|_| Error::General("Failed to encode the password change request".to_string()))?; - self.request_account_json(Method::POST, "/account/password", Some(&body)) + self.request_account_json(Method::POST, "/account/password", None, Some(&body)) .await } } @@ -3428,20 +3421,20 @@ impl AccountApi for AdminClient { #[async_trait] impl AccountMfaApi for AdminClient { async fn account_mfa_status(&self) -> Result { - self.request_account_json(Method::GET, "/account/mfa", None) + self.request_account_json(Method::GET, "/account/mfa", None, None) .await } async fn account_mfa_enroll(&self) -> Result { // An empty JSON object rather than no body: the endpoint is a POST and // some proxies drop a bodyless one. - self.request_account_json(Method::POST, "/account/mfa/enroll", Some(b"{}")) + self.request_account_json(Method::POST, "/account/mfa/enroll", None, Some(b"{}")) .await } async fn account_mfa_activate(&self, code: &SecretValue) -> Result { let body = encode_code_request(code)?; - self.request_account_json(Method::POST, "/account/mfa/activate", Some(&body)) + self.request_account_json(Method::POST, "/account/mfa/activate", None, Some(&body)) .await } @@ -3463,14 +3456,19 @@ impl AccountMfaApi for AdminClient { })) .map_err(|_| Error::General("Failed to encode the disable request".to_string()))?; - self.request_account_empty(Method::POST, "/account/mfa/disable", Some(&body)) + self.request_account_empty(Method::POST, "/account/mfa/disable", None, Some(&body)) .await } async fn account_mfa_recovery_codes(&self, code: &SecretValue) -> Result { let body = encode_code_request(code)?; - self.request_account_json(Method::POST, "/account/mfa/recovery-codes", Some(&body)) - .await + self.request_account_json( + Method::POST, + "/account/mfa/recovery-codes", + None, + Some(&body), + ) + .await } } @@ -3494,13 +3492,13 @@ impl UserCredentialApi for AdminClient { let body = serde_json::to_vec(&serde_json::json!({ "secret_key": secret_key.expose() })) .map_err(|_| Error::General("Failed to encode the secret key request".to_string()))?; - let path = format!( - "/set-user-secret-key?accessKey={}", - urlencoding::encode(access_key) - ); - - self.request_account_json(Method::PUT, &path, Some(&body)) - .await + self.request_account_json( + Method::PUT, + "/set-user-secret-key", + Some(&[("accessKey", access_key)]), + Some(&body), + ) + .await } async fn user_mfa_status(&self, access_key: &str) -> Result { @@ -3509,8 +3507,13 @@ impl UserCredentialApi for AdminClient { "Access key must not be empty".to_string(), )); } - let path = format!("/user/mfa?accessKey={}", urlencoding::encode(access_key)); - self.request_account_json(Method::GET, &path, None).await + self.request_account_json( + Method::GET, + "/user/mfa", + Some(&[("accessKey", access_key)]), + None, + ) + .await } async fn user_mfa_reset(&self, access_key: &str) -> Result<()> { @@ -3519,9 +3522,13 @@ impl UserCredentialApi for AdminClient { "Access key must not be empty".to_string(), )); } - let path = format!("/user/mfa?accessKey={}", urlencoding::encode(access_key)); - self.request_account_empty(Method::DELETE, &path, None) - .await + self.request_account_empty( + Method::DELETE, + "/user/mfa", + Some(&[("accessKey", access_key)]), + None, + ) + .await } } diff --git a/docs/reference/rc/admin.md b/docs/reference/rc/admin.md index de2c81e7..1810f9bf 100644 --- a/docs/reference/rc/admin.md +++ b/docs/reference/rc/admin.md @@ -136,11 +136,18 @@ No command accepts a password on the command line, where it would be captured by shell history and visible in `ps`. Each password is read from one of: - `--*-from-env NAME` — a named environment variable. -- `--*-file PATH` — the first line of a file, which must not be group- or - world-readable. +- `--*-file PATH` — the first line of a file, up to 4096 bytes. A longer file is + reported rather than truncated, so a secret can never be silently shortened + into a password nobody knows. The file may be a symlink and may be group- or + world-readable, because a Kubernetes projected secret is both. - an interactive prompt with echo off, offered only when stdin is a terminal and the output is human-readable. +The SSE-C key file (`--sse-c-key-file` on the data commands) is stricter: it must +be a regular file of exactly 32 bytes with no group or other permission. That key +is long-lived encryption material an operator places directly, where a symlink or +a readable mode is worth refusing; an account password is neither. + Verification codes additionally accept `--code CODE` because a TOTP code is valid for at most 90 seconds. `--code` and `--code-from-env` are mutually exclusive. @@ -166,7 +173,13 @@ Recovery codes are returned in plaintext exactly once, by `mfa activate` and `mfa recovery-codes`; the server stores only their hashes and cannot show them again. `--output-file PATH` writes them with mode `0600` and refuses to overwrite an existing file, because that file may hold the only copy of a -previous set. Without `--output-file` they are printed to stdout. +previous set. The path is checked before the request is sent: by the time the +write runs the server has already rotated, so refusing then would destroy the +only copy of the new set as well. If the write fails anyway — a mistyped +directory, a full disk — the codes are printed instead of dropped, and the +command still exits non-zero to say the file was not written. In `--json` mode +they go to stdout and the error to stderr, so a script gets both. Without +`--output-file` they are printed to stdout. Each code works once. Generating a new set invalidates the previous one. From 95d0c52e205c75fa160da6ad40814738bde48a0b Mon Sep 17 00:00:00 2001 From: Sinan Eldem Date: Wed, 26 Aug 2026 11:54:57 +0300 Subject: [PATCH 4/4] refactor(cli): share the confirmation prompt and the private-file writer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both had reached a third copy, which is where the review asked for them to be folded together. Deleting an OIDC provider, running a replication check that writes to every configured target, and clearing somebody's second factor each carried their own confirmation. They differed only in three strings, and agreed on the parts that matter: `--yes` skips the question, a run with nobody to ask fails rather than assuming consent, and anything other than `y`/`yes` is a decline. Those are exactly the rules a fourth copy would get subtly wrong, so they now live in `crate::confirm` and each caller supplies its own wording. `admin config export` and the recovery-code output both create a file that only its owner may read. The mechanics — created rather than opened, `0600` set in the open flags so it is never briefly readable by anyone else, never a silent overwrite — move to `crate::private_file`. How the refusal is read stays with the callers, because it genuinely differs: an export that will not clobber a file is an ordinary I/O failure, while an occupied recovery-code path is a conflict the operator has to resolve before there is anywhere to put the only copy of a set the server has already issued. `config.rs` has a test pinning the first of those, and this change keeps it passing. No behaviour changes. The refactor left three `std::io` imports unused, which `-D warnings` rejects, and one `rc_core::Error` import used only from a test module; that assertion now uses the fully qualified path its neighbours in the same file already use. --- crates/cli/src/commands/admin/account.rs | 42 ++++----- crates/cli/src/commands/admin/config.rs | 15 +--- crates/cli/src/commands/admin/idp.rs | 41 +++------ crates/cli/src/commands/admin/user.rs | 47 +++------- crates/cli/src/commands/replicate.rs | 38 ++------ crates/cli/src/confirm.rs | 110 +++++++++++++++++++++++ crates/cli/src/lib.rs | 2 + crates/cli/src/main.rs | 2 + crates/cli/src/private_file.rs | 86 ++++++++++++++++++ 9 files changed, 252 insertions(+), 131 deletions(-) create mode 100644 crates/cli/src/confirm.rs create mode 100644 crates/cli/src/private_file.rs diff --git a/crates/cli/src/commands/admin/account.rs b/crates/cli/src/commands/admin/account.rs index 7297506a..01d2dae5 100644 --- a/crates/cli/src/commands/admin/account.rs +++ b/crates/cli/src/commands/admin/account.rs @@ -19,6 +19,7 @@ use serde::Serialize; use super::get_admin_client; use crate::exit_code::ExitCode; use crate::output::{Formatter, qr}; +use crate::private_file::write_private_file; use crate::secret_input::{SecretSource, can_prompt, read_code_interactive}; use rc_core::admin::{ AccountApi, AccountInfo, AccountMfaApi, CredentialsSource, IdentityType, MfaEnrollment, @@ -858,35 +859,24 @@ fn print_recovery_codes(codes: &RecoveryCodes, formatter: &Formatter, activated: } /// Write recovery codes to a new file with owner-only permissions. +/// +/// The file mechanics are shared with `admin config export`; only the reading of +/// the failure is local. An occupied path is a `Conflict` here rather than a +/// plain I/O error, because what is in the way may be the only copy of a +/// previous set and the operator has to decide what happens to it. fn write_recovery_codes(path: &std::path::Path, codes: &[String]) -> Result<()> { - use std::io::Write as _; - - let mut options = std::fs::OpenOptions::new(); - // `create_new` so an existing file is never silently overwritten: it may - // hold the only copy of a previous set. - options.write(true).create_new(true); - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt as _; - options.mode(0o600); - } - - let mut file = options.open(path).map_err(|error| { - if error.kind() == std::io::ErrorKind::AlreadyExists { - Error::Conflict(format!( - "{} already exists; choose another path", - path.display() - )) - } else { - Error::Io(error) - } - })?; - + let mut contents = String::new(); for code in codes { - writeln!(file, "{code}").map_err(Error::Io)?; + contents.push_str(code); + contents.push('\n'); } - file.flush().map_err(Error::Io)?; - Ok(()) + + write_private_file(path, contents.as_bytes()).map_err(|error| match error { + Error::Io(io) if io.kind() == std::io::ErrorKind::AlreadyExists => Error::Conflict( + format!("{} already exists; choose another path", path.display()), + ), + other => other, + }) } /// Report a failed operation and return its exit code. diff --git a/crates/cli/src/commands/admin/config.rs b/crates/cli/src/commands/admin/config.rs index d19034e9..9a8eb5a6 100644 --- a/crates/cli/src/commands/admin/config.rs +++ b/crates/cli/src/commands/admin/config.rs @@ -17,6 +17,7 @@ use zeroize::Zeroizing; use super::get_admin_client; use crate::exit_code::ExitCode; use crate::output::Formatter; +use crate::private_file::write_private_file; #[derive(Subcommand, Debug)] #[command(disable_help_subcommand = true)] @@ -839,20 +840,6 @@ fn read_protected_value_file(path: &Path) -> rc_core::Result> Ok(Zeroizing::new(value.to_string())) } -fn write_private_file(path: &Path, contents: &[u8]) -> rc_core::Result<()> { - let mut options = std::fs::OpenOptions::new(); - options.write(true).create_new(true); - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - options.mode(0o600); - } - let mut file = options.open(path)?; - use std::io::Write; - file.write_all(contents)?; - Ok(()) -} - fn emit_error(error: &Error, formatter: &Formatter) -> ExitCode { let code = ExitCode::from_i32(error.exit_code()).unwrap_or(ExitCode::GeneralError); if formatter.is_json() { diff --git a/crates/cli/src/commands/admin/idp.rs b/crates/cli/src/commands/admin/idp.rs index 0d35b853..bcccae05 100644 --- a/crates/cli/src/commands/admin/idp.rs +++ b/crates/cli/src/commands/admin/idp.rs @@ -9,11 +9,12 @@ use rc_core::{Error, Result}; use serde::Serialize; use serde_json::Value; use std::fs::File; -use std::io::{BufRead, IsTerminal, Read, Write}; +use std::io::Read; use std::path::{Path, PathBuf}; use zeroize::Zeroizing; use super::{emit_observability_error, get_admin_client}; +use crate::confirm::{Confirmation, confirm}; use crate::exit_code::ExitCode; use crate::output::Formatter; @@ -554,35 +555,19 @@ async fn prepare_and_delete( } fn confirm_delete(provider: &OidcProvider, yes: bool, formatter: &Formatter) -> Result<()> { - if yes { - return Ok(()); - } - if formatter.is_json() || !std::io::stdin().is_terminal() { - return Err(Error::InvalidPath( - "OIDC provider deletion requires --yes in non-interactive or JSON mode".to_string(), - )); - } - - let mut stderr = std::io::stderr().lock(); - write!( - stderr, - "Delete OIDC provider '{}'? [y/N] ", + let prompt = format!( + "Delete OIDC provider '{}'? [y/N]", safe(&provider.provider_id, formatter) + ); + confirm( + &Confirmation { + prompt: &prompt, + requires_yes: "OIDC provider deletion requires --yes in non-interactive or JSON mode", + declined: "OIDC provider deletion was declined", + }, + yes, + formatter, ) - .map_err(Error::Io)?; - stderr.flush().map_err(Error::Io)?; - let mut answer = String::new(); - std::io::stdin() - .lock() - .read_line(&mut answer) - .map_err(Error::Io)?; - if matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") { - Ok(()) - } else { - Err(Error::Interrupted( - "OIDC provider deletion was declined".to_string(), - )) - } } async fn prepare_and_apply_mutation( diff --git a/crates/cli/src/commands/admin/user.rs b/crates/cli/src/commands/admin/user.rs index bcabace3..5f7b3272 100644 --- a/crates/cli/src/commands/admin/user.rs +++ b/crates/cli/src/commands/admin/user.rs @@ -8,10 +8,10 @@ use clap::Subcommand; use serde::Serialize; use super::get_admin_client; +use crate::confirm::{Confirmation, confirm}; use crate::exit_code::ExitCode; use crate::output::Formatter; use crate::secret_input::{SecretSource, can_prompt}; -use rc_core::Error; use rc_core::admin::{AdminApi, SecretValue, User, UserCredentialApi, UserStatus}; const ADD_USER_AFTER_HELP: &str = "\ @@ -398,40 +398,19 @@ async fn execute_user_mfa_reset(args: UserMfaResetArgs, formatter: &Formatter) - /// — the user has to enrol again — so a non-interactive run must say `--yes` /// rather than have the confirmation silently skipped. fn confirm_mfa_reset(access_key: &str, yes: bool, formatter: &Formatter) -> rc_core::Result<()> { - use std::io::{BufRead as _, IsTerminal as _, Write as _}; - - if yes { - return Ok(()); - } - if formatter.is_json() || !std::io::stdin().is_terminal() { - return Err(Error::InvalidPath( - "Clearing a user's second factor requires --yes in non-interactive or JSON mode" - .to_string(), - )); - } - - let mut stderr = std::io::stderr().lock(); - write!( - stderr, - "Clear two-factor authentication for '{}'? The account will be protected by its password alone. [y/N] ", + let prompt = format!( + "Clear two-factor authentication for '{}'? The account will be protected by its password alone. [y/N]", formatter.sanitize_text(access_key) + ); + confirm( + &Confirmation { + prompt: &prompt, + requires_yes: "Clearing a user's second factor requires --yes in non-interactive or JSON mode", + declined: "Clearing two-factor authentication was declined", + }, + yes, + formatter, ) - .map_err(Error::Io)?; - stderr.flush().map_err(Error::Io)?; - - let mut answer = String::new(); - std::io::stdin() - .lock() - .read_line(&mut answer) - .map_err(Error::Io)?; - - if matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") { - Ok(()) - } else { - Err(Error::Interrupted( - "Clearing two-factor authentication was declined".to_string(), - )) - } } async fn execute_list(args: ListArgs, formatter: &Formatter) -> ExitCode { @@ -733,7 +712,7 @@ mod tests { }); let error = confirm_mfa_reset("analyst", false, &formatter).expect_err("must refuse"); - assert!(matches!(error, Error::InvalidPath(_)), "{error:?}"); + assert!(matches!(error, rc_core::Error::InvalidPath(_)), "{error:?}"); assert!(error.to_string().contains("--yes"), "{error}"); } diff --git a/crates/cli/src/commands/replicate.rs b/crates/cli/src/commands/replicate.rs index b93a4c26..f6836172 100644 --- a/crates/cli/src/commands/replicate.rs +++ b/crates/cli/src/commands/replicate.rs @@ -20,9 +20,9 @@ use rc_core::{AliasManager, Error, ObjectStore as _}; use rc_s3::{AdminClient, S3Client}; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, BTreeSet, HashMap}; -use std::io::{BufRead as _, IsTerminal as _, Write as _}; use std::path::{Path, PathBuf}; +use crate::confirm::{Confirmation, confirm}; use crate::exit_code::ExitCode; use crate::output::{Formatter, OutputConfig}; @@ -1988,35 +1988,15 @@ async fn execute_check(args: CheckArgs, output_config: OutputConfig) -> ExitCode } fn confirm_replication_check(yes: bool, formatter: &Formatter) -> rc_core::Result<()> { - if yes { - return Ok(()); - } - if formatter.is_json() || !std::io::stdin().is_terminal() { - return Err(Error::InvalidPath( - "Replication check performs temporary remote writes and deletes; pass --yes in non-interactive or JSON mode" - .to_string(), - )); - } - - let mut stderr = std::io::stderr().lock(); - write!( - stderr, - "Replication check writes and deletes a temporary object on every configured target. Continue? [y/N] " + confirm( + &Confirmation { + prompt: "Replication check writes and deletes a temporary object on every configured target. Continue? [y/N]", + requires_yes: "Replication check performs temporary remote writes and deletes; pass --yes in non-interactive or JSON mode", + declined: "Replication check was declined", + }, + yes, + formatter, ) - .map_err(Error::Io)?; - stderr.flush().map_err(Error::Io)?; - let mut answer = String::new(); - std::io::stdin() - .lock() - .read_line(&mut answer) - .map_err(Error::Io)?; - if matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") { - Ok(()) - } else { - Err(Error::Interrupted( - "Replication check was declined".to_string(), - )) - } } fn output_replication_check( diff --git a/crates/cli/src/confirm.rs b/crates/cli/src/confirm.rs new file mode 100644 index 00000000..4ff89e11 --- /dev/null +++ b/crates/cli/src/confirm.rs @@ -0,0 +1,110 @@ +//! The yes/no prompt for an action worth asking about twice. +//! +//! Three commands had grown their own copy of this: deleting an OIDC provider, +//! running a replication check that writes to every target, and clearing +//! somebody's second factor. They differed only in their three strings, while +//! agreeing on the parts that matter — that `--yes` skips the question, that a +//! run with nobody to ask fails instead of assuming consent, and that anything +//! other than `y`/`yes` is a decline rather than a default. +//! +//! Those are the rules a fourth copy would be most likely to get subtly wrong, +//! so they live here once. + +use std::io::{BufRead as _, IsTerminal as _, Write as _}; + +use rc_core::{Error, Result}; + +use crate::output::Formatter; + +/// What to say while confirming one particular action. +pub(crate) struct Confirmation<'a> { + /// The question, ending in `[y/N]`. + /// + /// Callers that interpolate a name are responsible for passing it through + /// [`Formatter::sanitize_text`] first: it reaches a terminal from here with + /// no further escaping. + pub(crate) prompt: &'a str, + /// Why `--yes` is required when there is no terminal to ask on. + pub(crate) requires_yes: &'a str, + /// Reported when the answer is anything but yes. + pub(crate) declined: &'a str, +} + +/// Ask, unless `yes` was passed. +/// +/// `Ok(())` means go ahead. A refusal is [`Error::Interrupted`]; a run that +/// could not ask at all is [`Error::InvalidPath`], which is a usage problem and +/// exits as one. +pub(crate) fn confirm(request: &Confirmation<'_>, yes: bool, formatter: &Formatter) -> Result<()> { + if yes { + return Ok(()); + } + // Refuse rather than proceed: a machine-readable run has nobody to answer, + // and treating silence as consent is how a destructive command becomes a + // surprise in someone's CI log. + if formatter.is_json() || !std::io::stdin().is_terminal() { + return Err(Error::InvalidPath(request.requires_yes.to_string())); + } + + // The question goes to stderr so stdout stays usable in a pipeline. + let mut stderr = std::io::stderr().lock(); + write!(stderr, "{} ", request.prompt).map_err(Error::Io)?; + stderr.flush().map_err(Error::Io)?; + + let mut answer = String::new(); + std::io::stdin() + .lock() + .read_line(&mut answer) + .map_err(Error::Io)?; + + if matches!(answer.trim().to_ascii_lowercase().as_str(), "y" | "yes") { + Ok(()) + } else { + Err(Error::Interrupted(request.declined.to_string())) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::output::OutputConfig; + + fn request() -> Confirmation<'static> { + Confirmation { + prompt: "Delete everything? [y/N]", + requires_yes: "Deleting everything requires --yes in non-interactive or JSON mode", + declined: "Deleting everything was declined", + } + } + + fn formatter(json: bool) -> Formatter { + Formatter::new(OutputConfig { + json, + no_color: true, + ..Default::default() + }) + } + + #[test] + fn yes_skips_the_question_entirely() { + // True even in JSON mode, where there would be nobody to ask. + confirm(&request(), true, &formatter(true)).expect("--yes must be honoured"); + } + + #[test] + fn json_mode_refuses_instead_of_assuming_consent() { + let error = confirm(&request(), false, &formatter(true)).expect_err("must refuse"); + + assert!(matches!(error, Error::InvalidPath(_)), "{error:?}"); + assert_eq!(error.exit_code(), 2, "a missing --yes is a usage error"); + assert!(error.to_string().contains("--yes"), "{error}"); + } + + #[test] + fn a_declined_answer_is_reported_as_an_interruption() { + // Not asserted through the prompt, which needs a terminal: this pins the + // contract the callers rely on for their exit code. + let error = Error::Interrupted(request().declined.to_string()); + assert_eq!(error.exit_code(), 130); + } +} diff --git a/crates/cli/src/lib.rs b/crates/cli/src/lib.rs index 26cdf06b..f0f9efd5 100644 --- a/crates/cli/src/lib.rs +++ b/crates/cli/src/lib.rs @@ -3,6 +3,8 @@ //! This module exports the CLI components for use in integration tests. pub mod commands; +pub(crate) mod confirm; pub mod exit_code; pub mod output; +pub(crate) mod private_file; pub(crate) mod secret_input; diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index 24c41af6..e74dffe1 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -7,8 +7,10 @@ use clap::Parser; use tracing_subscriber::{EnvFilter, fmt, prelude::*}; mod commands; +mod confirm; mod exit_code; mod output; +mod private_file; mod secret_input; use commands::Cli; diff --git a/crates/cli/src/private_file.rs b/crates/cli/src/private_file.rs new file mode 100644 index 00000000..57820b80 --- /dev/null +++ b/crates/cli/src/private_file.rs @@ -0,0 +1,86 @@ +//! Writing a file that holds something only its owner should read. +//! +//! Two commands write one: `admin config export` and the recovery-code output of +//! `admin account mfa`. Both need the same three properties — created rather +//! than opened, mode `0600` from the moment it exists, and never a silent +//! overwrite — and had them written out twice. +//! +//! What the callers do *not* share is how they classify the refusal, so that +//! stays with them: an export that will not clobber a file is an ordinary +//! failure, while a recovery-code set that cannot be written is a conflict the +//! operator has to resolve before there is anywhere to put the only copy. + +use std::fs::OpenOptions; +use std::io::Write as _; +use std::path::Path; + +use rc_core::Result; + +/// Create `path` with owner-only permissions and write `contents`. +/// +/// `create_new`, so an existing file is never truncated: the caller cannot know +/// that what is already there is expendable. The mode is set in the open flags +/// rather than afterwards, so the file is never briefly readable by anyone else. +/// +/// Returns the underlying [`std::io::Error`] as-is — including +/// [`std::io::ErrorKind::AlreadyExists`] — for the caller to classify. +pub(crate) fn write_private_file(path: &Path, contents: &[u8]) -> Result<()> { + let mut options = OpenOptions::new(); + options.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(0o600); + } + + let mut file = options.open(path)?; + file.write_all(contents)?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn an_existing_file_is_left_alone() { + let directory = tempfile::tempdir().expect("create temp directory"); + let path = directory.path().join("private.txt"); + std::fs::write(&path, "existing").expect("create existing file"); + + let error = + write_private_file(&path, b"replacement").expect_err("must not overwrite the file"); + + assert_eq!( + std::fs::read_to_string(&path).expect("read existing file"), + "existing" + ); + // The kind is the caller's to interpret, so it must survive the trip. + assert!( + matches!( + error, + rc_core::Error::Io(ref io) + if io.kind() == std::io::ErrorKind::AlreadyExists + ), + "{error:?}" + ); + } + + #[cfg(unix)] + #[test] + fn a_new_file_is_owner_only() { + use std::os::unix::fs::PermissionsExt as _; + + let directory = tempfile::tempdir().expect("create temp directory"); + let path = directory.path().join("private.txt"); + write_private_file(&path, b"secret").expect("write the file"); + + let mode = std::fs::metadata(&path) + .expect("metadata") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600); + assert_eq!(std::fs::read_to_string(&path).expect("read back"), "secret"); + } +}