You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by hg...@apache.org on 2026/10/18 15:34:26 UTC
(iggy) 08/23: fix(server-ng): verify and strip current_password on change
This is an automated email from the ASF dual-hosted git repository.
hubcio pushed a commit to branch http-listener
in repository https://gitbox.apache.org/repos/asf/iggy.git
commit e596a5c1d271a445b5c4f6ad136da10d32c35625
Author: Hubert Gruszecki <h....@gmail.com>
AuthorDate: Thu Jul 2 18:19:59 2026 +0200
fix(server-ng): verify and strip current_password on change
ChangePassword neither verified current_password nor removed it
before replication: any session holding the change_password
permission could rotate a password blind, and both supplied
passwords rode the replicated prepare into the WAL in plaintext on
every replica. Legacy verifies against the stored hash and strips
the field before writing state.
Restore that shape on the shard-0 pre-submit rewrite shared by all
transports: the rewrite now always hashes new_password and empties
current_password, and verifies current_password against the target's
stored hash whenever the target resolves locally, rejecting with
InvalidCredentials on mismatch. An unresolved target still commits a
stripped no-op that apply answers with UserNotFound; the narrow
window where a target materialises between rewrite and apply skips
only the verify and stays permission-gated. Denials are typed on
both transports: HTTP maps to 400, TCP answers through the reply
status field, which also replaces the silent drop on malformed
bodies that previously wedged the connection until the client
timeout.
---
core/integration/tests/server/http_rbac.rs | 126 +++++++++++++++++++++++++
core/server-ng/src/dispatch.rs | 26 +++++-
core/server-ng/src/http.rs | 21 +++--
core/server-ng/src/users.rs | 142 +++++++++++++++++++++++++++--
4 files changed, 298 insertions(+), 17 deletions(-)
diff --git a/core/integration/tests/server/http_rbac.rs b/core/integration/tests/server/http_rbac.rs
index 7fcd89f89..d8e5df831 100644
--- a/core/integration/tests/server/http_rbac.rs
+++ b/core/integration/tests/server/http_rbac.rs
@@ -55,12 +55,17 @@ const CONSUMER_ID: u32 = 1;
// `IggyError` discriminants (== the numeric `id` in the JSON error body).
const UNAUTHORIZED_ID: u64 = 41;
+const INVALID_CREDENTIALS_ID: u64 = 42;
const CANNOT_DELETE_USER_ID: u64 = 48;
const CANNOT_CHANGE_PERMISSIONS_ID: u64 = 49;
+// The apply's `ChangePasswordResult::UserNotFound` maps to `ResourceNotFound`.
+const RESOURCE_NOT_FOUND_ID: u64 = 20;
// Snake-case `IggyError` names (== the `code` string in the JSON error body).
const UNAUTHORIZED_CODE: &str = "unauthorized";
+const INVALID_CREDENTIALS_CODE: &str = "invalid_credentials";
const CANNOT_DELETE_USER_CODE: &str = "cannot_delete_user";
const CANNOT_CHANGE_PERMISSIONS_CODE: &str = "cannot_change_permissions";
+const RESOURCE_NOT_FOUND_CODE: &str = "resource_not_found";
/// Root is the first user (slab id 0) and is undeletable / permission-locked.
const ROOT_USER_PATH: &str = "0";
@@ -270,6 +275,26 @@ impl Client {
.await
}
+ /// Change a user's password via `PUT /users/{user_id}/password`.
+ async fn change_password(&self, user_id: &str, current: &str, new: &str) -> Response {
+ self.put_json(
+ &format!("/users/{user_id}/password"),
+ &json!({ "current_password": current, "new_password": new }),
+ )
+ .await
+ }
+
+ /// Anonymous login attempt returning the raw response, for exercising a
+ /// rejected login (the asserting [`Client::login`] cannot express failure).
+ async fn login_attempt(&self, username: &str, password: &str) -> Response {
+ self.http
+ .post(self.url("/users/login"))
+ .json(&json!({ "username": username, "password": password }))
+ .send()
+ .await
+ .expect("login request")
+ }
+
async fn produce(
&self,
stream: &str,
@@ -914,3 +939,104 @@ async fn given_non_root_tcp_session_when_polling_denied_should_surface_typed_una
"non-root TCP poll must surface Err(Unauthorized) via the reply status peek, got {result:?}"
);
}
+
+/// The current-password check gates a self-service rotation: a wrong current
+/// password is rejected pre-consensus as 400 `InvalidCredentials` (never
+/// reaching the replicated apply), and only the correct one rotates the
+/// credential so the old password stops working and the new one starts.
+#[iggy_harness]
+async fn given_own_password_when_current_wrong_should_reject_and_correct_should_rotate(
+ harness: &TestHarness,
+) {
+ let root = Client::login_root(harness).await;
+ root.create_user("carol", "carol-pass-1", no_permissions())
+ .await;
+ let carol = root.login("carol", "carol-pass-1").await;
+
+ let wrong = carol
+ .change_password("carol", "not-my-password", "carol-pass-2")
+ .await;
+ assert_eq!(
+ wrong.status(),
+ StatusCode::BAD_REQUEST,
+ "wrong current password must be 400"
+ );
+ assert_error_body(
+ wrong,
+ INVALID_CREDENTIALS_ID,
+ INVALID_CREDENTIALS_CODE,
+ "carol change-password wrong current",
+ )
+ .await;
+
+ let correct = carol
+ .change_password("carol", "carol-pass-1", "carol-pass-2")
+ .await;
+ assert_eq!(
+ correct.status(),
+ StatusCode::NO_CONTENT,
+ "correct current password must be 204"
+ );
+
+ assert!(
+ !carol
+ .login_attempt("carol", "carol-pass-1")
+ .await
+ .status()
+ .is_success(),
+ "old password must stop working after a successful rotation"
+ );
+ assert!(
+ carol
+ .login_attempt("carol", "carol-pass-2")
+ .await
+ .status()
+ .is_success(),
+ "new password must work after a successful rotation"
+ );
+}
+
+/// Admin-initiated change (target != caller): the current password is checked
+/// against the TARGET's stored hash, so a wrong one is 400 `InvalidCredentials`;
+/// a target that does not resolve is left to the replicated apply, which
+/// answers 404 `UserNotFound` (the plaintext is still stripped + hashed first).
+#[iggy_harness]
+async fn given_admin_change_password_when_target_current_wrong_or_missing_should_reject(
+ harness: &TestHarness,
+) {
+ let root = Client::login_root(harness).await;
+ root.create_user("dave", "dave-pass", no_permissions())
+ .await;
+
+ let wrong = root
+ .change_password("dave", "not-daves-password", "new-dave-pass")
+ .await;
+ assert_eq!(
+ wrong.status(),
+ StatusCode::BAD_REQUEST,
+ "admin change with the wrong target current password must be 400"
+ );
+ assert_error_body(
+ wrong,
+ INVALID_CREDENTIALS_ID,
+ INVALID_CREDENTIALS_CODE,
+ "admin change dave wrong current",
+ )
+ .await;
+
+ let missing = root
+ .change_password("ghost-does-not-exist", "whatever", "whatever-else")
+ .await;
+ assert_eq!(
+ missing.status(),
+ StatusCode::NOT_FOUND,
+ "change for an unknown target must be 404 from the apply"
+ );
+ assert_error_body(
+ missing,
+ RESOURCE_NOT_FOUND_ID,
+ RESOURCE_NOT_FOUND_CODE,
+ "admin change missing target",
+ )
+ .await;
+}
diff --git a/core/server-ng/src/dispatch.rs b/core/server-ng/src/dispatch.rs
index e27f27ceb..9f628a710 100644
--- a/core/server-ng/src/dispatch.rs
+++ b/core/server-ng/src/dispatch.rs
@@ -598,17 +598,35 @@ async fn handle_client_request(
return;
}
};
- // Hash raw passwords on the primary before replication (CreateUser /
- // ChangePassword); see `crate::users`. Replicas store the hash directly.
- let request = match maybe_rewrite_user_password_request(request) {
+ // Hash raw passwords and verify ChangePassword's current password on the
+ // primary before replication (CreateUser / ChangePassword); see
+ // `crate::users`. Replicas store the hash directly.
+ let request = match maybe_rewrite_user_password_request(shard, request) {
Ok(rewritten) => rewritten,
Err(error) => {
+ // Deny fast with the typed code (InvalidCredentials on a failed
+ // current-password check, InvalidCommand on a malformed body): a
+ // silent drop would wedge every later request on the connection
+ // until the socket read timeout.
warn!(
transport_client_id,
error = %error,
operation = ?header.operation,
- "dropping user request with invalid password payload"
+ "denying user password request"
);
+ let commit = current_metadata_commit(shard);
+ let reply = build_deny_reply(&header, transport_client_id, 0, commit, error.as_code());
+ if let Err(send_error) = shard
+ .bus
+ .send_to_client(transport_client_id, reply.into_generic().into_frozen())
+ .await
+ {
+ warn!(
+ transport_client_id,
+ error = %send_error,
+ "failed to send password-deny reply"
+ );
+ }
return;
}
};
diff --git a/core/server-ng/src/http.rs b/core/server-ng/src/http.rs
index 7c8c45f3f..53931ca46 100644
--- a/core/server-ng/src/http.rs
+++ b/core/server-ng/src/http.rs
@@ -2126,8 +2126,11 @@ async fn delete_user(
/// `PUT /users/{user_id}/password`: change a user's password. Returns 204.
///
-/// Like [`create_user`], the new password rides the JSON body in plaintext and
-/// is hashed by [`submit_write`] on shard 0 before replication.
+/// Both passwords ride the JSON body in plaintext. On shard 0, before the op
+/// enters consensus, [`maybe_rewrite_user_password_request`] hashes the new
+/// password and strips the current one (so neither plaintext is ever
+/// replicated), and verifies `current_password` against the target's stored
+/// hash -- a wrong one rejects as 400 `InvalidCredentials`.
async fn change_password(
State(state): State<HttpState>,
identity: Authenticated,
@@ -2311,10 +2314,13 @@ async fn submit_committed(
///
/// Both shard-0 request rewrites run here before consensus, mirroring the TCP
/// dispatch path: the PAT rewrite mints a raw token and replicates only its hash
-/// (`CreatePersonalAccessToken`), and the user-password rewrite hashes plaintext
-/// (`CreateUser` / `ChangePassword`). Both are no-ops for every other operation,
-/// so plaintext secrets never enter consensus on any write. On either rewrite's
-/// decode failure the gate is released without advancing, leaving the id free.
+/// (`CreatePersonalAccessToken`), and the user-password rewrite hashes the new
+/// password and, for `ChangePassword`, strips the current one and verifies it
+/// against the stored hash (`CreateUser` / `ChangePassword`). Both are no-ops
+/// for every other operation, so plaintext secrets never enter consensus on any
+/// write. On either rewrite's rejection (a malformed body, or a failed
+/// current-password check) the gate is released without advancing, leaving the
+/// id free.
async fn submit_gated(
shard: &Rc<ServerNgShard>,
session: &HttpSession,
@@ -2331,7 +2337,8 @@ async fn submit_gated(
);
let (message, raw_token) =
rewrite_pat_request_for_user(session.user_id, message).map_err(WriteError::Rejected)?;
- let message = maybe_rewrite_user_password_request(message).map_err(WriteError::Rejected)?;
+ let message =
+ maybe_rewrite_user_password_request(shard, message).map_err(WriteError::Rejected)?;
let request_header = *message.header();
let reply = submit_client_request_on_owner(shard, message).await;
let Some(reply) = reply else {
diff --git a/core/server-ng/src/users.rs b/core/server-ng/src/users.rs
index 524512cc7..5367ab7b6 100644
--- a/core/server-ng/src/users.rs
+++ b/core/server-ng/src/users.rs
@@ -25,20 +25,37 @@
//! replica and diverge state (and a raw password would never parse,
//! panicking `verify_password`). So the primary hashes once here and
//! replicates the hash, mirroring the PAT mint in [`crate::pat`].
+//!
+//! `ChangePassword` also verifies the caller-supplied `current_password`
+//! against the target's stored hash before the op replicates (the legacy
+//! server does this for self- and admin-initiated changes alike), and it
+//! always empties that field and hashes the new password, so no plaintext
+//! credential ever rides the replicated prepare/WAL.
+use crate::bootstrap::ServerNgShard;
use crate::wire::{request_body, rewrite_request_body};
+use bytes::Bytes;
+use consensus::MetadataHandle;
use iggy_binary_protocol::codec::{WireDecode, WireEncode};
use iggy_binary_protocol::requests::users::{ChangePasswordRequest, CreateUserRequest};
-use iggy_binary_protocol::{Operation, RequestHeader};
+use iggy_binary_protocol::{Operation, RequestHeader, WireIdentifier};
use iggy_common::IggyError;
+use metadata::impls::metadata::StreamsFrontend;
use server::streaming::utils::crypto;
use server_common::Message;
+use std::rc::Rc;
+use std::sync::Arc;
-/// Replace a raw wire password with its Argon2 hash before replication.
+/// Rewrite a raw wire password request before replication.
///
-/// Only touches `CreateUser` and `ChangePassword`; every other operation
-/// passes through unchanged.
+/// `CreateUser` hashes the new password. `ChangePassword` hashes the new
+/// password and strips the current one unconditionally (so neither plaintext
+/// enters consensus), and additionally verifies the supplied `current_password`
+/// against the target's stored hash when the target resolves. Every other
+/// operation passes through unchanged. Returns [`IggyError::InvalidCredentials`]
+/// when the current password does not match, matching the legacy server.
pub(crate) fn maybe_rewrite_user_password_request(
+ shard: &Rc<ServerNgShard>,
request: Message<RequestHeader>,
) -> Result<Message<RequestHeader>, IggyError> {
let operation = request.header().operation;
@@ -53,11 +70,124 @@ pub(crate) fn maybe_rewrite_user_password_request(
Operation::ChangePassword => {
let mut wire =
ChangePasswordRequest::decode_from(body).map_err(|_| IggyError::InvalidCommand)?;
- wire.new_password = crypto::hash_password(&wire.new_password);
- wire.to_bytes()
+ let stored_hash = resolve_password_hash(shard, &wire.user_id);
+ verify_and_rewrite_change_password(&mut wire, stored_hash.as_deref())?
}
_ => return Ok(request),
};
rewrite_request_body(&request, &rewritten)
}
+
+/// Hash the new password and strip the current one, always, then return the
+/// re-encoded body. When the target resolves (`stored_hash` is `Some`) the
+/// supplied `current_password` is verified against it first, returning
+/// [`IggyError::InvalidCredentials`] on mismatch (matching the legacy server).
+///
+/// The hash + strip are unconditional so no plaintext credential ever enters
+/// consensus, even for an unresolved target (which the replicated apply rejects
+/// with `UserNotFound`). ACCEPTED RESIDUAL: if the target is created in the
+/// narrow window between this rewrite and the apply, the current-password check
+/// is skipped for that one op; RBAC still gates cross-user changes.
+fn verify_and_rewrite_change_password(
+ wire: &mut ChangePasswordRequest,
+ stored_hash: Option<&str>,
+) -> Result<Bytes, IggyError> {
+ if let Some(stored_hash) = stored_hash
+ && !crypto::verify_password(&wire.current_password, stored_hash)
+ {
+ return Err(IggyError::InvalidCredentials);
+ }
+ wire.current_password = String::new();
+ wire.new_password = crypto::hash_password(&wire.new_password);
+ Ok(wire.to_bytes())
+}
+
+/// Stored Argon2 hash of the user named by `user_id`, read from the local
+/// metadata STM (the same replica login verifies against). `None` when the
+/// user does not resolve.
+fn resolve_password_hash(shard: &Rc<ServerNgShard>, user_id: &WireIdentifier) -> Option<Arc<str>> {
+ shard.plane.metadata().mux_stm.users().read(|users| {
+ let resolved = match user_id {
+ WireIdentifier::Numeric(id) => {
+ let id = *id as usize;
+ users.items.contains(id).then_some(id)
+ }
+ WireIdentifier::String(name) => users.index.get(name.as_str()).map(|&id| id as usize),
+ };
+ resolved
+ .and_then(|id| users.items.get(id))
+ .map(|user| Arc::clone(&user.password_hash))
+ })
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ const CURRENT_PASSWORD: &str = "current-password";
+ const NEW_PASSWORD: &str = "brand-new-password";
+
+ fn change_password_request() -> ChangePasswordRequest {
+ ChangePasswordRequest {
+ user_id: WireIdentifier::numeric(7),
+ current_password: CURRENT_PASSWORD.to_owned(),
+ new_password: NEW_PASSWORD.to_owned(),
+ }
+ }
+
+ #[test]
+ fn given_wrong_current_password_when_rewrite_should_reject() {
+ let stored_hash = crypto::hash_password("some-other-password");
+ let mut wire = change_password_request();
+
+ let result = verify_and_rewrite_change_password(&mut wire, Some(&stored_hash));
+
+ assert!(matches!(result, Err(IggyError::InvalidCredentials)));
+ }
+
+ #[test]
+ fn given_correct_current_password_when_rewrite_should_strip_and_hash() {
+ let stored_hash = crypto::hash_password(CURRENT_PASSWORD);
+ let mut wire = change_password_request();
+
+ let body = verify_and_rewrite_change_password(&mut wire, Some(&stored_hash))
+ .expect("verification succeeds and re-encodes a body");
+
+ let decoded =
+ ChangePasswordRequest::decode_from(&body).expect("re-encoded body round-trips");
+ assert!(
+ decoded.current_password.is_empty(),
+ "current password must be stripped before replication"
+ );
+ assert_ne!(
+ decoded.new_password, NEW_PASSWORD,
+ "new password must be hashed, not replicated raw"
+ );
+ assert!(
+ crypto::verify_password(NEW_PASSWORD, &decoded.new_password),
+ "hashed new password must verify against the raw input"
+ );
+ }
+
+ #[test]
+ fn given_unresolved_target_when_rewrite_should_hash_and_strip_without_verify() {
+ let mut wire = change_password_request();
+
+ // No stored hash: nothing to verify against, but the plaintext must
+ // still never reach consensus, so the body is rewritten regardless.
+ let body = verify_and_rewrite_change_password(&mut wire, None)
+ .expect("unresolved target is not an error");
+
+ let decoded =
+ ChangePasswordRequest::decode_from(&body).expect("re-encoded body round-trips");
+ assert!(
+ decoded.current_password.is_empty(),
+ "current password must be stripped even for an unresolved target"
+ );
+ assert!(
+ crypto::verify_password(NEW_PASSWORD, &decoded.new_password),
+ "new password must still be hashed for an unresolved target"
+ );
+ }
+}