numinnex commented on code in PR #4550:
URL: https://github.com/apache/iggy/pull/4550#discussion_r4286018567
##########
core/metadata/src/stm/authz.rs:
##########
@@ -78,7 +87,33 @@ where
if let Some(denied) = authorize(&prepare, mux.users(), mux.streams()) {
return Ok(denied);
}
- mux.update(prepare)
+ if let Some(rejected) = reject_dangling_grant(&prepare, mux.users(),
mux.streams()) {
+ return Ok(rejected);
+ }
+ let header = prepare.header();
+ let timestamp = IggyTimestamp::from(header.timestamp);
+ let context = ApplyContext::from(header);
+ let reply = mux.update(prepare)?;
+ if let Some(deleted) = reply.deleted_resource {
+ // Publish the pruned grants before a later op can reuse the resource
ID.
+ // Peer shards may see the deletion first, but cannot see its
replacement yet.
+ mux.users().remove_grants(deleted, timestamp, context);
+ }
+ Ok(reply)
+}
+
+/// An admitted request can commit after its user was deleted or disabled.
+/// Check the replicated session table before resolving the reused user ID.
+/// Partition completions use a synthetic session without a client-table entry.
+pub(crate) fn reject_stale_session(
+ header: &PrepareHeader,
+ client_table: &ClientTable,
+) -> Option<ApplyReply> {
+ if message_bus::is_server_internal_client(header.client) {
Review Comment:
This exemption trusts an id the client picks.
`handle_login_register_request` takes `request.header().client` verbatim
(`core/server/src/dispatch/session_ops.rs:1101`), `validate_request_fields`
only refuses 0 (`core/binary_protocol/src/consensus/header.rs:507`), and every
later op is stamped with that id (`core/server/src/dispatch/mod.rs:968-971`).
`ConsensusSession::with_client_id` is public, so a client can log in with an id
in `u128::MAX << 64`.
Such a session skips this check on every op, which reopens the takeover this
PR closes: alice's `CreatePersonalAccessToken` queued behind
`DeleteUser(alice)` and a `CreateUser(bob)` that reuses her id commits as bob
(`core/metadata/src/stm/user.rs:836-858`), and alice gets back a token that
logs in as bob.
The previous allowlist (`CompleteConsumerGroupRevocation`,
`CompleteLifecycle`) could not be forged, since clients cannot send either op
(`core/metadata/src/impls/metadata.rs:4010`). My earlier comment here suggested
the client-id exemption; that was wrong without an ingress guard, sorry.
Fix: refuse internal-range ids at every Register entry (see
`register_preflight`), and exempt only an explicit list of server-only
operations. "Not client-allowed" is not enough, `TruncatePartition` is admitted
from clients with the caller's session. A regression test for a Register plus a
stale op from an internal-range id would pin it.
##########
core/consensus/src/metadata_helpers.rs:
##########
@@ -258,11 +260,14 @@ where
if !is_caught_up_primary(consensus) {
return PreflightOutcome::NotReady;
}
- let Ok(verifier) = verifier_body.try_into() else {
+ let Ok(registration) = RegisterRequest::decode_from(verifier_body) else {
return PreflightOutcome::Reject(IggyError::InvalidCommand.as_code());
};
+ if !account_is_active(registration.user_created_at) {
Review Comment:
This is one of the places to refuse
`message_bus::is_server_internal_client(client_id)` with a terminal error,
together with the login handler
(`core/server/src/dispatch/session_ops.rs:1101`) and
`submit_register_in_process` (`core/metadata/src/impls/metadata.rs:2414`).
Every later op inherits the id from the bound session, so a Register gate
covers them all at no per-op cost.
Not in `validate_request_fields`: the server builds its own request frames
with `AUTO_COMMIT_CLIENT_ID`, and those go through it too.
The same gate also closes two holes that are on master already. A login with
`u128::MAX` is treated as the auto-commit client and skips the consumer group
owner/session match (`core/partitions/src/iggy_partition.rs:1092-1096`), so it
can store or delete offsets of groups it is not a member of, within its own
grants. A login with the exact `server_internal_client_id(key)` of a completion
keeps `pipeline_has_message_from_client` true and stalls that completion with
`InProgress` (`core/metadata/src/impls/metadata.rs:2987`).
SDKs that mint random ids (C#, Java, Node, Go) should then clear the top bit
so an honest client never hits the refusal; Rust, Python and C++ use UUIDv4 and
are already outside the range.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
