You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "hubcio (via GitHub)" <gi...@apache.org> on 2026/10/14 21:46:36 UTC
[PR] fix(server)!: bind grants and HTTP tokens to the objects they name [iggy]
hubcio opened a new pull request, #4550:
URL: https://github.com/apache/iggy/pull/4550
Stream, topic and user IDs are reused. Grants and HTTP credentials
must stay bound to the original resource or account.
Drop resource grants in the replicated delete path and reject grants
on absent resources. Rebuild the permission index from the pruned
users once per delete.
Bind self-issued JWTs and HTTP read identities to the account
creation timestamp. Recheck the account after read barriers. Resolve
trusted issuer usernames per request and include the account in
session keys. Drop personal access tokens left at a user ID when a
new user takes it.
Reads also checked their caller only before they waited. Binary
reads now carry the session attachment, refuse with StaleClient once
it is invalid, and run the permission rule again after each wait.
HTTP reads run it again after their last wait.
Trusted issuer entries take username instead of user_id, and the
server refuses to start while one is empty. Tokens issued before this
change are rejected, so HTTP clients must log in again. Older data
directories need --fresh, because their log entries replay
differently under these grant rules, and old and new replicas do not
form a cluster.
Cover account replacement during both read waits and preserve grants
across users and snapshot restore.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server)!: bind grants and HTTP tokens to the objects they name [iggy]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #4550:
URL: https://github.com/apache/iggy/pull/4550#discussion_r4286586443
##########
core/server/src/http/handlers.rs:
##########
@@ -2061,11 +2101,17 @@ pub(in crate::http) async fn delete_pat(
Ok(StatusCode::NO_CONTENT)
}
-/// Issue a fresh access token for `user_id` and wrap it in the exact
-/// `IdentityInfo` shape the SDKs pin: numeric `user_id` plus an `access_token`
-/// carrying the token string and its unix-seconds expiry.
-fn issue_identity(inner: &HttpInner, user_id: u32) -> Result<Json<IdentityInfo>, CustomError> {
- let generated = inner.jwt.generate(user_id)?;
+/// Mint a token bound to the active account at `user_id`. A refresh passes the
+/// `created_at` its token names, so it never re-issues for a newer account that
+/// took a deleted user's id. A login passes `None`, because it just verified
Review Comment:
nit: The comment says that a login passes `None`, but `login_user` and `login_with_personal_access_token` both pass `Some(created_at)`. Rewrite the sentence to match, since every caller passes a verified creation stamp.
##########
core/server/config.toml:
##########
@@ -165,9 +165,9 @@ use_base64_secret = false
# Trusted issuers for A2A (Application-to-Application) authentication. Opt-in:
# with none configured the listener accepts only self-issued HMAC tokens.
-# `issuer`, `audience` and `jwks_url` are required per entry; `user_id` is
-# optional but defaults to 0 (root), which is rejected - set it to the non-zero
-# iggy user every token from that issuer is remapped onto.
+# `issuer`, `audience`, `jwks_url` and `username` are required per entry. Every
Review Comment:
nit: `HttpJwt::build` rejects an empty `issuer`, `jwks_url` or `username` at startup, but not an empty `audience`, so the comment above overstates the check. Add the matching check: an empty audience cannot match a real token's `aud` claim, so the misconfiguration surfaces as 401s instead of a startup error.
##########
core/metadata/src/stm/result.rs:
##########
@@ -216,6 +231,8 @@ result_enum!(TruncatePartitionResult {
result_enum!(CreateUserResult {
InvalidUsername = 43,
UserAlreadyExists = 46,
+ StreamNotFound = 1009,
Review Comment:
nit: `CreateUserResult` and `UpdatePermissionsResult` gained `StreamNotFound` and `TopicNotFound`, but the test that claims to pin every discriminant omits them. Add the four assertions, so a renumber still fails here.
##########
core/server/src/http/handlers.rs:
##########
@@ -2061,11 +2101,17 @@ pub(in crate::http) async fn delete_pat(
Ok(StatusCode::NO_CONTENT)
}
-/// Issue a fresh access token for `user_id` and wrap it in the exact
-/// `IdentityInfo` shape the SDKs pin: numeric `user_id` plus an `access_token`
-/// carrying the token string and its unix-seconds expiry.
-fn issue_identity(inner: &HttpInner, user_id: u32) -> Result<Json<IdentityInfo>, CustomError> {
- let generated = inner.jwt.generate(user_id)?;
+/// Mint a token bound to the active account at `user_id`. A refresh passes the
+/// `created_at` its token names, so it never re-issues for a newer account that
+/// took a deleted user's id. A login passes `None`, because it just verified
+/// the account's own credentials.
+fn issue_identity(
+ inner: &HttpInner,
+ user_id: u32,
+ expected_created_at: Option<IggyTimestamp>,
Review Comment:
simplification: All three callers pass `Some`, so the `Option` on `issue_identity`'s `expected_created_at` only carries an always-present value. Take `IggyTimestamp` directly and drop the option.
##########
core/simulator/src/client.rs:
##########
@@ -241,12 +241,19 @@ impl SimClient {
#[allow(clippy::cast_possible_truncation)]
pub fn register(&self) -> Message<RoutedRequestHeader> {
let header_size = std::mem::size_of::<RoutedRequestHeader>();
- let verifier =
- consensus::client_table::bind_verifier(self.client_id, 0, &self.bind_secret());
+ let registration = iggy_binary_protocol::requests::users::login_register::RegisterRequest {
+ user_created_at: 1,
Review Comment:
nit: This register hard-codes `user_created_at: 1`, which must equal the fixed stamp `ensure_root_user` gives the root user, or the commit gate refuses it. Reuse one named sentinel here, so a change to the root bootstrap cannot break the simulator.
##########
core/shard/src/poll.rs:
##########
@@ -418,16 +446,40 @@ where
if reply.is_disconnected() {
return;
}
- if attachment.is_some_and(|attachment| {
- !attachment.session.is_valid()
- || !attachment
- .metadata
- .is_valid(self.plane.metadata().mux_stm.streams(), namespace)
- }) {
- let _ = reply.try_send(PartitionReadReply::Rejected(
- IggyError::TransientNotAccepted,
- ));
- return;
+ if let Some(attachment) = &attachment {
+ let users = self.plane.metadata().mux_stm.users();
+ let (user_id, metadata, account_valid) = match attachment {
+ PollAttachment::Bound(attachment) => (
+ attachment.session.user_id(),
+ &attachment.metadata,
+ attachment.session.is_valid(),
+ ),
+ PollAttachment::Unbound {
+ user_id,
+ user_created_at,
+ metadata,
+ } => (
+ *user_id,
+ metadata,
+ users.is_active_account(*user_id, *user_created_at),
Review Comment:
simplification: The unbound poll completion reads the users metadata twice, once for `is_active_account` and once for `authorize`. Fold both checks into one read closure, so one snapshot supplies the account and its grants. Also at `core/server/src/http/reads.rs:66`.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server)!: bind grants and HTTP tokens to the objects they name [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio merged PR #4550:
URL: https://github.com/apache/iggy/pull/4550
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server)!: bind grants and HTTP tokens to the objects they name [iggy]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #4550:
URL: https://github.com/apache/iggy/pull/4550#issuecomment-6179874026
## [Codecov](https://app.codecov.io/gh/apache/iggy/pull/4550?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `97.42698%` with `37 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 89.04%. Comparing base ([`c70c286`](https://app.codecov.io/gh/apache/iggy/commit/c70c286cd892a95eb81ca192908e9ce43aa89c5d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4bb7f40`](https://app.codecov.io/gh/apache/iggy/commit/4bb7f40bbe7676dc39d496138d74e88bb59b02e4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/4550?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [core/metadata/src/stm/user.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fmetadata%2Fsrc%2Fstm%2Fuser.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9tZXRhZGF0YS9zcmMvc3RtL3VzZXIucnM=) | 96.23% | [2 Missing and 7 partials :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/metadata/src/stm/authz.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fmetadata%2Fsrc%2Fstm%2Fauthz.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9tZXRhZGF0YS9zcmMvc3RtL2F1dGh6LnJz) | 98.04% | [0 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/server/src/http/reads.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fhttp%2Freads.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2h0dHAvcmVhZHMucnM=) | 98.32% | [4 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/server/src/http/jwt.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fhttp%2Fjwt.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2h0dHAvand0LnJz) | 95.55% | [2 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/server/src/dispatch/mod.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fdispatch%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2Rpc3BhdGNoL21vZC5ycw==) | 98.53% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/server/src/dispatch/reads.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fdispatch%2Freads.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2Rpc3BhdGNoL3JlYWRzLnJz) | 96.80% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/server/src/http/state.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fhttp%2Fstate.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2h0dHAvc3RhdGUucnM=) | 96.00% | [0 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/server/src/http/handlers.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fhttp%2Fhandlers.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2h0dHAvaGFuZGxlcnMucnM=) | 96.00% | [1 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/server/src/dispatch/authz.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fdispatch%2Fauthz.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2Rpc3BhdGNoL2F1dGh6LnJz) | 88.88% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [core/simulator/src/workload/ops/create\_user.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fsimulator%2Fsrc%2Fworkload%2Fops%2Fcreate_user.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zaW11bGF0b3Ivc3JjL3dvcmtsb2FkL29wcy9jcmVhdGVfdXNlci5ycw==) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| ... and [1 more](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #4550 +/- ##
============================================
+ Coverage 88.99% 89.04% +0.05%
+ Complexity 1735 1734 -1
============================================
Files 1329 1329
Lines 283994 285553 +1559
Branches 242523 244084 +1561
============================================
+ Hits 252742 254282 +1540
+ Misses 25622 25578 -44
- Partials 5630 5693 +63
```
| [Components](https://app.codecov.io/gh/apache/iggy/pull/4550/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [Rust Core](https://app.codecov.io/gh/apache/iggy/pull/4550/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.76% <97.42%> (+0.07%)` | :arrow_up: |
| [Java SDK](https://app.codecov.io/gh/apache/iggy/pull/4550/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.23% <ø> (-0.02%)` | :arrow_down: |
| [C# SDK](https://app.codecov.io/gh/apache/iggy/pull/4550/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `66.08% <ø> (-0.35%)` | :arrow_down: |
| [Python SDK](https://app.codecov.io/gh/apache/iggy/pull/4550/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.43% <ø> (ø)` | |
| [PHP SDK](https://app.codecov.io/gh/apache/iggy/pull/4550/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.84% <ø> (ø)` | |
| [Node SDK](https://app.codecov.io/gh/apache/iggy/pull/4550/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `95.04% <ø> (-0.10%)` | :arrow_down: |
| [Go SDK](https://app.codecov.io/gh/apache/iggy/pull/4550/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.91% <ø> (+0.02%)` | :arrow_up: |
| [C++ SDK](https://app.codecov.io/gh/apache/iggy/pull/4550/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `84.44% <ø> (ø)` | |
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/4550?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...common/src/types/permissions/permissions\_global.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fcommon%2Fsrc%2Ftypes%2Fpermissions%2Fpermissions_global.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jb21tb24vc3JjL3R5cGVzL3Blcm1pc3Npb25zL3Blcm1pc3Npb25zX2dsb2JhbC5ycw==) | `54.95% <ø> (ø)` | |
| [core/configs/src/common/http.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fconfigs%2Fsrc%2Fcommon%2Fhttp.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jb25maWdzL3NyYy9jb21tb24vaHR0cC5ycw==) | `90.00% <ø> (ø)` | |
| [core/metadata/src/stm/result.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fmetadata%2Fsrc%2Fstm%2Fresult.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9tZXRhZGF0YS9zcmMvc3RtL3Jlc3VsdC5ycw==) | `99.68% <100.00%> (+<0.01%)` | :arrow_up: |
| [core/metadata/src/stm/stream.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fmetadata%2Fsrc%2Fstm%2Fstream.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9tZXRhZGF0YS9zcmMvc3RtL3N0cmVhbS5ycw==) | `95.13% <100.00%> (+0.01%)` | :arrow_up: |
| [core/server/src/boot/mod.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fboot%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2Jvb3QvbW9kLnJz) | `86.84% <100.00%> (+0.01%)` | :arrow_up: |
| [core/server/src/dispatch/partition.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fdispatch%2Fpartition.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2Rpc3BhdGNoL3BhcnRpdGlvbi5ycw==) | `95.87% <100.00%> (+0.05%)` | :arrow_up: |
| [core/server/src/dispatch/session\_ops.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fdispatch%2Fsession_ops.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2Rpc3BhdGNoL3Nlc3Npb25fb3BzLnJz) | `91.92% <100.00%> (-0.17%)` | :arrow_down: |
| [core/server/src/dispatch/test\_support.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fdispatch%2Ftest_support.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2Rpc3BhdGNoL3Rlc3Rfc3VwcG9ydC5ycw==) | `80.14% <100.00%> (+1.21%)` | :arrow_up: |
| [core/server/src/http/extractor.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fhttp%2Fextractor.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2h0dHAvZXh0cmFjdG9yLnJz) | `97.10% <100.00%> (+8.39%)` | :arrow_up: |
| [core/server/src/http/forward.rs](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree&filepath=core%2Fserver%2Fsrc%2Fhttp%2Fforward.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zZXJ2ZXIvc3JjL2h0dHAvZm9yd2FyZC5ycw==) | `86.34% <100.00%> (+0.25%)` | :arrow_up: |
| ... and [13 more](https://app.codecov.io/gh/apache/iggy/pull/4550?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [52 files with indirect coverage changes](https://app.codecov.io/gh/apache/iggy/pull/4550/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server)!: bind grants and HTTP tokens to the objects they name [iggy]
Posted by "numinnex (via GitHub)" <gi...@apache.org>.
numinnex commented on code in PR #4550:
URL: https://github.com/apache/iggy/pull/4550#discussion_r4282814790
##########
core/metadata/src/impls/metadata.rs:
##########
@@ -849,12 +851,14 @@ where
// Normal op: apply, build the reply. `Err` is decode/corruption only; a
// business rejection commits as a deterministic no-op whose code rides
// the reply body, replayed on retry.
- let apply = gated_apply(mux_stm, prepare).unwrap_or_else(|err| {
- panic!(
- "apply_committed_prepare: committed metadata op={} failed to apply: {err}",
- header.op
- );
- });
+ let apply = reject_stale_session(&header, &client_table.borrow())
Review Comment:
`Register` escapes this check: `apply_committed_prepare` returns for `Operation::Register` above, and `register_preflight` (`core/consensus/src/metadata_helpers.rs:246-291`) reads only the client table.
A login admitted while `DeleteUser(alice)` is in the pipeline therefore commits a live entry for the freed id after `end_user_sessions` ran, and `complete_session_binding` (`core/server/src/dispatch/mod.rs:1131-1141`) only checks that `items[N]` is active.
Once bob takes id N, alice is bound as bob and her ops pass this check and `authorize` under bob's grants. Without bob, the ghost entry survives in the checkpointed table and `BIND_SESSION` (`core/server/src/dispatch/reads.rs:640-671`) binds it to whoever takes N later.
HTTP closes the same race with `ensure_active_user(created_at)` after Register (`core/server/src/http/state.rs:555`). Binary needs the same: carry the verified `created_at` in the Register body, refuse at commit when `items[N].created_at` differs, and compare it in `complete_session_binding`.
That also covers `CreatePersonalAccessTokenRequest::apply` (`core/metadata/src/stm/user.rs:801-808`), which inserts under `user_id` with no `items` check.
##########
core/shard/src/poll.rs:
##########
@@ -429,6 +429,23 @@ where
));
return;
}
+ if let Some(attachment) = &attachment
Review Comment:
An HTTP poll arrives as `UnboundPollOnPrimary` with no attachment (`core/server/src/http/handlers.rs:1360-1368`), so this grant check never runs for it. `complete_poll` then stores the `auto_commit` offset, and the gateway's `authorize_read` after `partition_read` (`handlers.rs:1371`) drops the batch on a revoke that landed during the read.
Binary has the same shape with a smaller window between this check and `core/server/src/dispatch/partition.rs:701-711`.
Nit, since `auto_commit` is already at-most-once (`poll.rs:392-396`). Still, carry `(user_id, created_at)` in `UnboundPollOnPrimary` and gate here before `complete_poll`. Do not deliver after a deny; that reopens the earlier HTTP poll leak.
##########
core/metadata/src/stm/authz.rs:
##########
@@ -78,7 +87,36 @@ 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 {
+ 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 matches!(
+ header.operation,
+ Operation::CompleteConsumerGroupRevocation | Operation::CompleteLifecycle
Review Comment:
Exemption by `Operation` allowlist. `submit_partition_completion` stamps `session = 1` (`core/metadata/src/impls/metadata.rs:4471`), so the next server-internal op that is not on this list commits as a silent `StaleClient` no-op on every replica.
Exempt by `message_bus::is_server_internal_client(header.client)` instead, as `metadata.rs:3501` does.
##########
core/metadata/src/stm/authz.rs:
##########
@@ -78,7 +87,36 @@ 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 {
+ mux.users().remove_grants(deleted, timestamp, context);
Review Comment:
This is a second `Users` publish after the `Streams` publish from `mux.update`, so a peer-shard reader can observe the stream gone with the grant still present for one op. Harmless today, the id cannot be reused before a later op, but correct only by writer program order. Worth a comment here so a future reorder does not break it.
##########
core/metadata/src/stm/user.rs:
##########
@@ -655,6 +707,82 @@ impl StateHandler for UpdatePermissionsRequest {
}
}
+/// Server-originated request that drops every grant on a deleted resource.
+///
+/// Never decoded from the wire: `gated_apply` applies it right after the
+/// delete, so every replica and WAL replay remove the same grants.
+#[derive(Debug, Clone)]
+pub struct RemoveGrantsRequest {
+ pub deleted: DeletedResource,
+}
+
+impl StateHandler for RemoveGrantsRequest {
+ type State = UsersInner;
+ fn apply(&self, state: &mut UsersInner, _timestamp: IggyTimestamp) -> ApplyReply {
+ let mut pruned_any = false;
+ for (_, user) in &mut state.items {
+ let Some(pruned) = user
+ .permissions
+ .as_deref()
+ .and_then(|permissions| without_grant(permissions, self.deleted))
+ else {
+ continue;
+ };
+ user.permissions = Some(Arc::new(pruned));
+ pruned_any = true;
+ }
+ if pruned_any {
+ state.permissioner = permissioner_of(&state.items);
Review Comment:
Any hit rebuilds the whole `Permissioner` and deep-clones every user's `Permissions`, O(total grants), and left_right runs the apply twice (`core/metadata/src/stm/mod.rs:559-565`), so a stream or topic delete under many users stalls the shard 0 apply loop twice.
Collect the pruned user ids and patch the index for those users only; keep the full rebuild for `from_snapshot`.
##########
core/server/src/session_manager.rs:
##########
@@ -199,6 +204,10 @@ impl SessionManager {
};
Some(ConnectionContext {
bound,
+ attachment: conn
+ .consumer_session
+ .as_ref()
+ .map(|(_, attachment)| attachment.clone()),
Review Comment:
This clones the `SessionAttachment` (atomic weak inc + dec) on every inbound frame, `SendMessages` included, while only the `NonReplicatedRead` arm reads it (`core/server/src/dispatch/mod.rs:817`). Clone it only for that class.
##########
core/metadata/src/stm/authz.rs:
##########
@@ -78,7 +87,36 @@ 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 {
+ 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 matches!(
+ header.operation,
+ Operation::CompleteConsumerGroupRevocation | Operation::CompleteLifecycle
+ ) {
+ return None;
+ }
+ (header.session != 0
+ && (client_table.get_epoch(header.client) != Some(header.session)
+ || client_table.get_user_id(header.client) != Some(header.user_id)))
Review Comment:
Dead clause. An epoch match implies a live entry, and `user_id` is immutable per epoch and stamped from that same entry in `prepare_request` (`core/metadata/src/impls/metadata.rs:3990-4010`), so `get_user_id` can never disagree once `get_epoch` matched.
Drop it; that also removes the second slot lookup per committed op.
##########
core/server/src/http/state.rs:
##########
@@ -633,11 +677,29 @@ pub(in crate::http) fn insert_view_header(state: &HttpInner, mut response: Respo
response
}
+/// `(id, created_at)` of `user` when a trusted issuer may act as it: active and
+/// not root. A deleted user's id goes to the next user created, so the issuer
+/// is matched by username, and an external issuer never holds root.
+fn trusted_issuer_account(user: Option<&User>) -> Option<(u32, IggyTimestamp)> {
+ let user = user?;
+ if user.id == DEFAULT_ROOT_USER_ID {
+ error!(
+ username = %user.username,
+ "a trusted issuer maps onto the root user, refusing its token"
Review Comment:
`error!` on every request once an issuer's username resolves to root, so one token holder floods the log, while the missing or inactive case in `trusted_issuer_user` is `debug!` only and a misspelled `username` is invisible at the default level. One policy for both: `warn!` once per issuer.
##########
core/server/src/dispatch/partition.rs:
##########
@@ -694,11 +696,23 @@ where
consumer,
args,
attachment: ConsumerAttachment {
- session: attachment,
+ session: attachment.clone(),
Review Comment:
Three attachment clones per poll now: the `ConnectionContext` clone, the second `consumer_session()` borrow and clone in `reads.rs`, and this one, which was a move before. Derive the consumer session from the context, move it in here, and keep a copied `user_id` for the recheck.
##########
core/server/src/http/jwt.rs:
##########
@@ -288,48 +284,66 @@ impl JwtManager {
validation.set_audience(std::slice::from_ref(&config.audience));
validation.validate_nbf = true;
validation.leeway = self.validation.leeway;
- let mut claims = decode::<JwtClaims>(token, &decoding_key, &validation)
+ let claims = decode::<JwtClaims>(token, &decoding_key, &validation)
.map(|data| data.claims)
.map_err(|error| {
error!(%error, "trusted-issuer JWT verification failed");
IggyError::Unauthenticated
})?;
- // Remap the external subject onto the fixed configured iggy user; the
- // raw external `sub` is discarded (no auto-provisioning).
- claims.sub = config.user_id.to_string();
- Ok(claims)
+ // The raw external `sub` is discarded (no auto-provisioning): every
+ // token from this issuer acts as the configured iggy user.
+ Ok(VerifiedToken {
+ jti: claims.jti,
+ exp: claims.exp,
+ subject: TokenSubject::Username(config.username.clone()),
Review Comment:
A `String` allocation per trusted-issuer request. An `Arc<str>` in the manager's issuer map avoids it.
##########
core/common/src/traits/user_client.rs:
##########
@@ -35,6 +35,12 @@ pub trait UserClient {
/// Create a new user.
///
/// Authentication is required, and the permission to manage the users.
+ ///
+ /// Every stream and topic that `permissions` grants on must exist. If one
+ /// does not, a binary transport returns `IggyError::StreamIdNotFound` or
Review Comment:
The refusal reaches the client through `IggyError::from_code`, so it decodes to `StreamIdNotFound(Identifier::default())` and prints "Stream with ID: 0 was not found." (`core/server/src/http/reply.rs:145`, `core/sdk/src/vsr.rs:380`). Stream 0 is a real id, and a multi-grant request gives no hint which grant failed.
Same placeholder as every existing 1009 refusal, so a nit. Carry the missing id in the reply body, or at least document the placeholder here.
The Rust `HttpClient` also drops the 1009/2010 id on 404 (`core/sdk/src/http/http_client.rs:367`), unlike the C# client, so the HTTP branch documented here is coarser than it reads.
##########
core/server/src/http/extractor.rs:
##########
@@ -163,37 +169,56 @@ pub(in crate::http) fn bearer_token(headers: &HeaderMap) -> Result<&str, IggyErr
.ok_or(IggyError::AccessTokenMissing)
}
-/// Map a bearer credential to its `(session key, user id, expiry secs)`.
+/// Map a bearer credential to its `(session key, user id, user created_at,
+/// expiry secs)`.
///
-/// A JWT is the primary credential, keyed by its `jti`. A raw PAT presented as
-/// the bearer is the documented fallback, keyed by the same 256-bit BLAKE3
-/// digest (`PersonalAccessToken::hash_token`) Iggy already indexes PATs by, so
-/// the key is stable and collision-free while the raw secret never enters the
-/// table. The JWT verify is `async` and `!Send` (a trusted-issuer token may
-/// fetch the issuer's JWKS on a cache miss), so both call sites drive it inside
-/// a `SendWrapper`; the PAT check is local and synchronous. Also the forward
-/// middleware's verify-only gate: a follower proves the bearer before relaying
-/// a request, without minting a session.
+/// A JWT is the primary credential, keyed by its account and `jti`. A raw PAT
+/// presented as the bearer is the documented fallback, keyed by the same
+/// 256-bit BLAKE3 digest (`PersonalAccessToken::hash_token`) Iggy already
+/// indexes PATs by, so the key is stable and collision-free while the raw
+/// secret never enters the table. The JWT verify is `async` and `!Send` (a
+/// trusted-issuer token may fetch the issuer's JWKS on a cache miss), so both
+/// call sites drive it inside a `SendWrapper`; the PAT check is local and
+/// synchronous. Also the forward middleware's verify-only gate: a follower
+/// proves the bearer before relaying a request, without minting a session.
+///
+/// The `created_at` names the account behind the credential, because a deleted
+/// user's id goes to the next user created. A self-issued JWT must name the
+/// account that holds its id now, and a trusted-issuer JWT resolves the
+/// username its issuer config names.
pub(in crate::http) async fn resolve_credential(
state: &HttpState,
bearer: &str,
-) -> Result<(String, u32, u64), AuthError> {
- if let Ok(claims) = state.jwt.decode(bearer).await
- && let Ok(user_id) = claims.sub.parse::<u32>()
- {
- state.ensure_active_user(user_id)?;
- return Ok((
- format!("{JWT_KEY_PREFIX}{}", claims.jti),
- user_id,
- claims.exp,
- ));
+) -> Result<(String, u32, IggyTimestamp, u64), AuthError> {
+ if let Ok(token) = state.jwt.decode(bearer).await {
+ let (user_id, user_created_at) = match token.subject {
+ TokenSubject::Account {
+ user_id,
+ created_at,
+ } => (
+ user_id,
+ state.ensure_active_user(user_id, Some(created_at))?,
+ ),
+ TokenSubject::Username(username) => state.trusted_issuer_user(&username)?,
+ };
+ // A trusted-issuer token resolves its username on every request, so one
+ // `jti` can name another account after a rename or a delete, and the
+ // issuer picks its own `jti`. The account in the key keeps a cached
+ // session from writing as any account but the one resolved here.
+ let key = format!(
+ "{JWT_KEY_PREFIX}{user_id}:{}:{}",
Review Comment:
The key is built on every request and discarded by the `Identity` extractor below. Build it only in `Authenticated`, which is the sole consumer. Pre-existing shape, but this PR made the string longer.
##########
core/server/README.md:
##########
@@ -44,7 +44,7 @@ Command-line arguments include `--replica-id`, `--fresh` (`-f`), `--with-default
Deploy matching server and SDK releases together. Rolling upgrades and mixed
server versions are unsupported. Partition control frames from older servers
carry incarnation zero and are rejected, so a mixed-version cluster cannot
-complete partition view changes. The `IGGY-OWNERSHIP-HISTORY-5` storage gate also
+complete partition view changes. The `IGGY-BOUND-GRANTS-1` storage gate also
Review Comment:
The upgrade note names only the storage gate. Two more things break for operators: every pre-upgrade JWT, access and refresh, is refused (`core/server/src/http/jwt.rs:340`), so HTTP clients and the web UI must log in again; and `[[http.jwt.trusted_issuers]] user_id` became `username`, with the env var `IGGY_HTTP_JWT_TRUSTED_ISSUERS_<n>_USER_ID` becoming `..._USERNAME`. Both are in the commit body only.
##########
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 local gate for the routed case, but the `route_partition` reply above returns before it, so a relayed poll is never re-gated after its await. Safe, the target node re-verifies the same bearer and `created_at`, but `reads.rs:55-58` lists partition routing among the rechecked routes. Either reword that doc or gate the routed reply too. Same in `get_consumer_offset`.
##########
core/server/src/dispatch/reads.rs:
##########
@@ -708,6 +749,7 @@ where
else {
return Err(IggyError::TransientNotAccepted);
};
+ authorize_attached(attachment, authorize)?;
Review Comment:
The re-gate re-resolves by name, but the reply still carries the `incarnation` captured before the `partition_read` await. A delete and recreate under the same name during the wait returns an incarnation the client then refreshes away as `HistoryUnavailable`. Fails closed, so a nit: re-read the incarnation after the gate, or return `TransientNotAccepted` on mismatch.
##########
foreign/cpp/tests/e2e/client.cpp:
##########
@@ -354,8 +354,27 @@ TEST_F(E2E_Client, CreateUserPreservesNestedPermissionsInCreateAndGetResponses)
RecordProperty("description",
"Creates a user with global and per-resource permissions, then verifies create_user and get_user "
"return the same flags and numeric stream/topic IDs.");
- auto client = GetLoggedInHighLevelClient();
- const std::string username = GetRandomName(50);
+ auto client = GetLoggedInHighLevelClient();
+ const std::string username = GetRandomName(50);
+ const std::string first_stream_name = GetRandomName();
+ const std::string second_stream_name = GetRandomName();
+
+ // The server refuses a grant on a stream or topic that does not exist.
+ std::uint32_t first_stream_id = 0;
+ std::uint32_t second_stream_id = 0;
+ std::uint32_t first_topic_id = 0;
+ std::uint32_t second_topic_id = 0;
+ std::uint32_t third_topic_id = 0;
+ ASSERT_NO_THROW({
+ first_stream_id = client.CreateStream(first_stream_name).Id();
+ TrackStream(first_stream_name);
+ second_stream_id = client.CreateStream(second_stream_name).Id();
+ TrackStream(second_stream_name);
+ first_topic_id = client.CreateTopic(iggy::Identifier::Numeric(first_stream_id), GetRandomName()).Id();
+ second_topic_id = client.CreateTopic(iggy::Identifier::Numeric(first_stream_id), GetRandomName()).Id();
+ third_topic_id = client.CreateTopic(iggy::Identifier::Numeric(second_stream_id), GetRandomName()).Id();
Review Comment:
`first_topic_id` and `third_topic_id` are both 0 (first topic in each stream), so a mix-up between the two streams' topic maps still passes the assertions below. Create a filler topic in `second_stream` first, or assert the ids are distinct.
##########
core/integration/tests/server/scenarios/permissions_scenario.rs:
##########
@@ -2626,6 +2628,168 @@ async fn test_live_permission_revocation(harness: &TestHarness, root_client: &Ig
delete_test_user(root_client, USER).await;
}
+// =============================================================================
+// Test: A deleted stream's grant does not reach the stream that reuses its id
+// =============================================================================
+
+async fn test_deleted_stream_grant_not_inherited(harness: &TestHarness, root_client: &IggyClient) {
+ const USER: &str = "deleted-stream-grant-user";
+ let topic_id = Identifier::named(TOPIC_1).unwrap();
+ let deleted = create_stream_with_topic(root_client, "perm-deleted-stream").await;
+ let stream_id = Identifier::numeric(deleted.id).unwrap();
+ let granted_stream_id = deleted.id as usize;
+ create_test_user(
+ root_client,
+ USER,
+ Some(Permissions {
+ global: GlobalPermissions::default(),
+ streams: Some(BTreeMap::from([(
+ granted_stream_id,
+ StreamPermissions {
+ send_messages: true,
+ ..Default::default()
+ },
+ )])),
+ }),
+ )
+ .await;
+ let client = login_user(harness, USER).await;
+
+ let mut msgs = test_messages();
+ client
+ .send_messages(
+ &stream_id,
+ &topic_id,
+ &Partitioning::partition_id(0),
+ &mut msgs,
+ )
+ .await
+ .expect("send must work on the granted stream");
+
+ root_client
+ .delete_stream(&stream_id)
+ .await
+ .expect("delete the granted stream");
+ let user = root_client
+ .get_user(&Identifier::named(USER).unwrap())
+ .await
+ .expect("get user")
+ .expect("user exists");
+ assert!(
+ user.permissions
+ .and_then(|permissions| permissions.streams)
+ .is_none_or(|streams| !streams.contains_key(&granted_stream_id)),
+ "the user record must drop the grant on the deleted stream"
+ );
+
+ let reusing = create_stream_with_topic(root_client, "perm-reusing-stream").await;
+ assert_eq!(
+ reusing.id, deleted.id,
+ "the new stream must reuse the freed id"
+ );
+ let mut msgs = test_messages();
+ assert_error_code(
+ client
+ .send_messages(
+ &stream_id,
+ &topic_id,
+ &Partitioning::partition_id(0),
+ &mut msgs,
+ )
+ .await,
+ IggyError::Unauthorized,
+ "send to the stream that reuses a deleted stream's id",
+ );
+
+ let _ = root_client.delete_stream(&stream_id).await;
+ delete_test_user(root_client, USER).await;
+}
+
+async fn create_stream_with_topic(root_client: &IggyClient, name: &str) -> StreamDetails {
+ let stream = root_client
+ .create_stream(name)
+ .await
+ .expect("create stream");
+ root_client
+ .create_topic(
+ &Identifier::numeric(stream.id).unwrap(),
+ TOPIC_1,
+ &TopicCreateOptions {
+ partitions_count: Some(PARTITIONS),
+ message_expiry: Some(IggyExpiry::NeverExpire),
+ ..TopicCreateOptions::default()
+ },
+ )
+ .await
+ .expect("create topic");
+ stream
+}
+
+// =============================================================================
+// Test: A grant on a stream or topic that does not exist is refused. Ids are
+// handed out lowest free, so such a grant would reach whatever is created at
+// that id later.
+// =============================================================================
+
+async fn test_grant_on_missing_resource_refused(root_client: &IggyClient) {
Review Comment:
This and `http_rbac.rs` are the only coverage of the new contract. Nothing in `bdd/` or the foreign SDK e2e suites asserts the 1009/2010 refusal or that a grant is dropped when its stream or topic is deleted, although every SDK surfaces both. One cross-SDK feature, or a follow-up issue.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server)!: bind grants and HTTP tokens to the objects they name [iggy]
Posted by "spetz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server)!: bind grants and HTTP tokens to the objects they name [iggy]
Posted by "hubcio (via GitHub)" <gi...@apache.org>.
hubcio commented on PR #4550:
URL: https://github.com/apache/iggy/pull/4550#issuecomment-6197200493
/skill team-review-slim
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server)!: bind grants and HTTP tokens to the objects they name [iggy]
Posted by "spetz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server)!: bind grants and HTTP tokens to the objects they name [iggy]
Posted by "numinnex (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org