spetz commented on code in PR #4550:
URL: https://github.com/apache/iggy/pull/4550#discussion_r4282883265
##########
core/server/src/dispatch/session_ops.rs:
##########
@@ -141,17 +141,19 @@ where
S: 'static,
SB: SuperblockStore + 'static,
{
- verify_pat_credentials_with_expiry(shard, token).map(|(user_id, _)|
user_id)
+ verify_pat_credentials_with_expiry(shard, token).map(|(user_id, _, _)|
user_id)
}
-/// Like [`verify_pat_credentials`] but also surfaces the token's expiry (unix
-/// seconds, `u64::MAX` when the PAT never expires). The HTTP extractor keys a
-/// per-token VSR session table on this expiry for lazy eviction; the wire and
-/// login paths only need the user id and go through
[`verify_pat_credentials`].
+/// Like [`verify_pat_credentials`] but also surfaces the owner's `created_at`,
+/// from the same read so it names the token's owner, and the token's expiry
+/// (unix seconds, `u64::MAX` when the PAT never expires). The HTTP extractor
+/// keys a per-token VSR session table on this expiry for lazy eviction; the
+/// wire and login paths only need the user id and go through
Review Comment:
The wire login needs more than the user id, because that bare id is what
lets a deleted user's login bind to the account that reuses it.
`verify_login_credentials` (`core/server/src/dispatch/session_ops.rs:129`) and
`verify_pat_credentials` return only the id, read from the locally applied
users with no frontier wait (`core/server/src/dispatch/session_ops.rs:1140`).
The `Register` apply binds `header.user_id` with no account check and returns
before `reject_stale_session` runs (`core/metadata/src/impls/metadata.rs:773`),
and `complete_session_binding` only checks that the id is active
(`core/server/src/dispatch/mod.rs:1135`). If `DeleteUser(alice)` and
`CreateUser(bob)` commit between alice's credential check and her `Register`,
for example while she logs in on a lagging backup, she gets a session as bob
with bob's grants. Only the delete has to land first. The client table entry
her `Register` leaves at the freed id is never ended, and `BIND_SESSION` is
accepted on an unbound con
nection with session 0, so she can rebind with her own client id and bind
secret whenever a later `CreateUser` takes the id, with no timing on the
create. Two unit tests in `dispatch::tests` at a60f74a9 reproduce it:
`complete_session_binding` after the delete and the create returns `Ok((1,
4))`, and a rebind after a later create returns `Ok((1, 3))`. The new recovery
test registers its client before the delete
(`core/metadata/src/impls/recovery.rs:1784`), so it does not cover this order.
##########
core/server/src/http/handlers.rs:
##########
@@ -1344,6 +1368,7 @@ pub(in crate::http) async fn poll_messages(
},
))
.await;
+ authorize_read(&state, &identity, consistency, rule)?;
Review Comment:
This recheck passes whenever the path stops resolving. `rule` re-resolves
the stream and topic from the URL and maps a `resolve_gate_topic_ids` miss to
`Ok(())` (`core/server/src/http/handlers.rs:1296`), instead of checking the
`namespace` the owner just read. If the stream is deleted while the read waits
on the owner, the delete drops the user's grant and this check still passes, so
the batch goes out. A rename does the same to a revocation that lands in that
window. The binary twin checks `namespace.stream_id()` and
`namespace.topic_id()` after its wait
(`core/server/src/dispatch/partition.rs:705`) and refuses it.
`get_consumer_offset` has the same shape at
`core/server/src/http/handlers.rs:1486`.
##########
core/server/src/dispatch/partition.rs:
##########
@@ -694,11 +696,23 @@ where
consumer,
args,
attachment: ConsumerAttachment {
- session: attachment,
+ session: attachment.clone(),
metadata,
},
};
- match shard.partition_read(namespace, read).await {
+ let reply = shard.partition_read(namespace, read).await;
+ let users = shard.plane.metadata().mux_stm.users();
+ authorize_attached(Some(attachment), || {
Review Comment:
This recheck runs after the owner has already moved the offset. The owner
checks the session and the poll grant (`core/shard/src/poll.rs:421`,
`core/shard/src/poll.rs:432`), and `complete_poll` stores the `auto_commit`
offset (`core/shard/src/poll.rs:451`) before it replies. If the session ends or
the grant is revoked during the hop back, this refusal drops a batch the owner
has already committed past, and no consumer receives it. Before this commit the
batch reached the consumer the owner authorized. The HTTP poll does the same at
`core/server/src/http/handlers.rs:1371`, and there the owner stores the offset
with no grant check at all, because `UnboundPollOnPrimary` carries no identity.
--
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]
