spetz commented on code in PR #4550:
URL: https://github.com/apache/iggy/pull/4550#discussion_r4276910068
##########
core/metadata/src/stm/user.rs:
##########
@@ -478,6 +546,9 @@ impl StateHandler for CreateUserRequest {
}
state.index.insert(username_arc, id as UserId);
+ // A token create admitted before the delete of the last holder of this
+ // id can commit after it. Its tokens must not resolve to the new
holder.
+ state.remove_personal_access_tokens(id as UserId);
Review Comment:
This sweep covers only a token create that commits before the `CreateUser`
that takes the freed id. The opposite commit order is also valid. With two
admin connections, alice's `CreatePersonalAccessToken` and another admin's
`CreateUser` can both be prepared while `DeleteUser(alice)` is still in the
pipeline, because admission checks only the client table session and that
session ends at the delete's commit
(`core/metadata/src/impls/metadata.rs:858`).
`CreatePersonalAccessTokenRequest::apply` (`core/metadata/src/stm/user.rs:802`)
then inserts the hash under the reused id without any owner check,
`core/server/src/dispatch/mod.rs:1024` splices the raw token into alice's
reply, and PAT login resolves it to the new holder and returns that holder's
`created_at` (`core/server/src/dispatch/session_ops.rs:200`), so the
`created_at` binding this PR adds does not catch it. Alice ends up with a
working token for bob's account. A unit test in this module that applies
`DeleteUser(alice)`, `Cre
ateUser(bob)` at the freed id and then alice's `CreatePersonalAccessToken`
gets code 0, and the token index resolves the hash to bob. The token is one
case of a wider gap. `authorize` at apply reads only `header.user_id`
(`core/metadata/src/stm/authz.rs:156`), so any control-plane op alice had
admitted in that window, for example a `CreateStream` she holds no grant for,
commits under bob's grants. The comment above says such ops must not resolve to
the new holder, but only the token case in one commit order is handled. Both
orders existed before this PR.
##########
core/server/src/dispatch/authz.rs:
##########
@@ -49,6 +50,22 @@ use server_common::Message;
use crate::shell::{ShellBus, ShellShard};
+/// Run a read's `rule` only while the connection's session is still attached.
+///
+/// The funnel resolves the acting user before the read starts, and the session
+/// can end during any await of the read. A read runs this before its first
+/// await and again after its last one, before it builds the reply.
+pub(in crate::dispatch) fn authorize_attached(
Review Comment:
Binary `POLL_MESSAGES` never goes through `authorize_attached`, although
this doc and the PR description say binary reads run the permission rule again
after each wait. `handle_poll_messages` checks the poll rule once
(`core/server/src/dispatch/partition.rs:552`), then `read_polled_messages`
awaits the owner shard (`core/server/src/dispatch/partition.rs:703`) and builds
the reply with no second check. The owner completion at
`core/shard/src/poll.rs:421` checks the session and the metadata incarnation
but not grants. A consumer whose poll grant is revoked during that wait still
receives the messages, and with `auto_commit` the owner has already stored the
offset by then. `shard_waiting_reads` (`core/server/src/dispatch/mod.rs:1446`)
leaves out `POLL_MESSAGES_CODE`, so no test covers the poll.
##########
core/server/src/http/handlers.rs:
##########
@@ -1331,6 +1354,7 @@ pub(in crate::http) async fn poll_messages(
{
return Ok(response);
}
+ authorize_read(&state, &identity, consistency, rule)?;
Review Comment:
This is the last `authorize_read` in `poll_messages`, and it runs before the
`partition_read` await below, not after it. The PR description says HTTP reads
run the rule again after their last wait, and the `authorize_read` doc
(`core/server/src/http/reads.rs:54`) says the account can change during any
await. `partition_read` always crosses a shard channel and waits up to
`PARTITION_READ_TIMEOUT` (`core/shard/src/lib.rs:2184`), so a `DeleteUser` or
`UpdatePermissions` can commit during it, and `build_polled_messages_body`
still returns the messages to the revoked account.
##########
core/server/src/http/handlers.rs:
##########
@@ -1445,6 +1471,7 @@ pub(in crate::http) async fn get_consumer_offset(
{
return Ok(response);
}
+ authorize_read(&state, &identity, consistency, rule)?;
Review Comment:
`get_consumer_offset` has the same gap as `poll_messages`. It runs
`authorize_read` before the `partition_read` await at line 1484 and builds the
reply after it with no further check. The binary twin checks again after its
read (`core/server/src/dispatch/partition.rs:858`), so a replaced or revoked
account is refused over TCP but still gets the offset over HTTP.
--
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]
