Repository navigation
Conversation
…(P3-T002 PR-14a part 1)
The auth module of design P3-T002 (owner-approved) and ADR 0046:
Authenticator trait and ServerAuth; Role (viewer < operator < admin);
Credentials, Authenticated and AuthFailure with Spark's two
UNAUTHENTICATED messages. Static tokens
krishiv.auth.tokens.<name>.{secret,principal,kind,role} are held as
SHA-256 digests compared in constant time against every entry: at least
32 bytes, no shared secrets, env:/file: references only, and a file
readable by group or others is refused. spark.connect.authenticate.token
is one admin token named spark, with a startup warning. Failures are
throttled per peer address (krishiv.auth.failureThrottle.*) and each one
writes one krishiv::auth record and one (surface, method) counter.
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
… IPv6 /64 keys, malformed headers) From the Opus review of p3/t002-auth-core: - two tokens may not give one principal different roles (ADR 0046 §2); - throttle window and block are 1s to 24h and the deadline cannot overflow; a full table is swept at most once a second; - throttle keys are the IPv4 address or the IPv6 /64 (IPv4-mapped addresses canonicalised); - a present but unusable Authorization header is an invalid token (Spark's message), counted and throttled; - the secret file is checked and read through one handle; tokens over 16 KiB are refused unhashed; throttled requests count as method `throttle`; a test that failure logs never contain a token. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…/Manage/Impersonate (P3-T002 PR-14b-roles) - krishiv-plan: Privilege gains Browse, Operate, Manage and Impersonate (additive, non_exhaustive). authorize asks Select when a policy does not allow Browse, so providers that know only Select keep working. - krishiv-sql: SHOW TABLES/VIEWS/FUNCTIONS/COLUMNS/DATABASES/CREATE TABLE/ TBLPROPERTIES and plain DESCRIBE need Browse; SHOW TABLE EXTENDED, SHOW PARTITIONS and DESCRIBE EXTENDED|FORMATTED keep Select (statistics and partition values are data); CLEAR CACHE, REFRESH and ADD JAR need Manage on the cluster. - krishiv-session: ADMIN-class statements need Manage on the cluster (was Monitor); view metadata statements check Browse on the view. RolePolicy (auth/roles.rs) answers the matrix: viewer metadata and own system rows, operator data, client code at krishiv.auth.udf.minRole (admin until T014b), others' work (Operate) and Monitor, admin writes, DDL and Manage; Impersonate never; no role denies everything. - tests: roles_matrix over the statement classes for three roles, no_role_is_denied_even_metadata, a_denial_reveals_no_reason_or_existence, unit tests of the matrix; access_policy expectations updated. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
… Select, viewer system tables, AllowWith browses) From the Opus review of p3/t002-roles: - SHOW CREATE TABLE of a view and DESCRIBE with EXTENDED/FORMATTED or a PARTITION spec need Select (the view check follows the statement); - a viewer selects only system.queries, capabilities and metrics, names compared ignoring case; - AllowWith satisfies Browse, directly and on its Select fallback; - tests: select_implies_browse, a_filtered_reader_may_browse, the view Browse/Select split, roles_matrix rows for views, cache, refresh, system tables and path reads. The stricter readings are recorded in ADR 0046's 2026-11-12 update. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…t regular before opening them (Codex review) Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
… Select (Codex review) Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…roles Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…sed before the break is removed (Codex review) Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…read-only viewer keeps its metadata access (Codex review) Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…roles Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
… mTLS principal rule over a bounded DER walk, role lists, rustls ServerTls with reload (feature tls), listener coherence checks, cert_names fuzz target Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…ature, no client auth without a CA, format characters and constructed names refused, undecodable CN only refuses the cn rule, bounded certificate reads); CI runs the tls handshake tests Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…owse, ADR 0046 §3) Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…-rest Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…s not depend on the checkout's umask Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…rs, JWK set cache and fetch thread, OIDC role lists, jwt_parse fuzz target
- jose.rs: strict JWS/JWK set/discovery parsers (16 KiB tokens, canonical base64url, duplicate members
refused, RS256/384/512 and ES256/384 only, crit refused, header key members never used), claim checks.
- oidc.rs (feature oidc): signatures on aws-lc-rs with no none or HMAC path; JwksCache (one on-demand
fetch a minute, the first request waits, a token without kid needs exactly one fitting key, failed or
empty fetches keep the keys); HttpKeySource (no redirects, 1 MiB cap, one 10 s deadline per fetch).
- oidc_config.rs: krishiv.auth.oidc.* including roles.{viewer,operator,admin} for role-claim values,
separate from the certificate names of krishiv.auth.roles.*; https locations with a host, no userinfo.
- server.rs: JWS-shaped bearers go to OIDC; a JWT beside a client certificate fails as unavailable or
expired rather than as a mismatch; only invalid tokens count toward the throttle.
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…Policy allows object-store prefixes by store and whole key segments; from_config reads krishiv.io.local.allowedRoots and krishiv.io.objectStore.allowedPrefixes (startup only, default empty, every role) Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…zure prefixes name their account, listings never follow a link out of the allowed roots Opus diff review ops/reviews/2026-11-13-opus-A-t002-path-confinement-diff.md: - B1: an Azure prefix must carry its account host (abfss://c@acct.dfs.core.windows.net/p), so an account-less path, whose account would come from the client's fs.azure.account.name, never matches. Azure paths arrive typed as URLs with credentials (container@host); the policy now compares them (Azure only; container:secret@ is refused by the parser), where before every Azure path was denied. - B2: SettledLocalFileSystem::confined(roots): a glob or directory listing that meets a link (directory, partition directory or file) out of every root fails. The runtime uses it whenever krishiv.io.local.allowedRoots is set. - M2: prefixes with glob characters are refused at startup. - N2/N3: a bad root is KRISHIV_INVALID_CONFIG_VALUE naming the key; roots must be absolute existing directories (allowed_roots, shared by policy and runtime). - N1, N5, N6: edge tests (s3a/s3, scheme case, gs bucket names, blank entries), error text, rustdoc wrap. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…efaults (P3-T067b, PR-14b)
- A store endpoint (krishiv_store::ENDPOINT_KEYS: fs.s3a.endpoint,
fs.gs.storage.root.url, fs.gs.endpoint, fs.azure.endpoint, fs.hf.endpoint)
needs Manage on System("cluster"), admin only in server mode: per read
(bind_files, before anything is listed), per write (authorize_write_path,
before anything is written), on SQL SET (before the value is stored) and,
for spark.conf.set, on every query while the session's value differs from
the one it was built with (analyze_in).
- Ambient credentials and http(s):// locations reach only
krishiv.io.objectStore.allowedPrefixes (the T002 confinement); listing an
http(s) prefix is the enable.
- tests/server_egress.rs: one test per default; embedded use unchanged.
- Config docs: the allowedPrefixes and allowedRoots entries describe both.
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…onfig reference for the path keys (review M3, M4) Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…n inline credential is admin-only, table read options checked, S3 regions validated Opus review ops/reviews/2026-11-13-opus-A-t067b-egress-diff.md: - B1: krishiv_store::is_server_scoped_option replaces the five endpoint keys: every fs.* key except inline credentials (keys, secrets, tokens, SAS, the inline GCS key, the HF token) needs Manage on the cluster in server mode. That covers regions, accounts, proxies and key files (M1). S3 regions are validated before object_store formats them into the S3/STS host names. - B2: a table read's options (FROM t WITH (...), Connect table reads) are checked in bind (Found::StoreOptions); CREATE TABLE OPTIONS and SET SERDEPROPERTIES are checked before they are stored (readers are not asked again for an admin's stored options). - M3 (part): the link refusal no longer names the link. Its 42501 class and audit record, M2 (descriptor walk), M4 (Azure paths end to end, pre-existing) and M5 (one config for policy and runtime) are recorded on the T067b plan row. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…server-scoped, an admin's stored table options serve INSERTs Verification of b4c150c (ops/reviews/2026-11-13-opus-A-t067b-egress-diff.md): - 1 (blocking): fs.gs.auth.service.account.json is server-scoped too; object_store follows the JSON's gcs_base_url, so a non-admin could make the server fetch any HTTPS host. - 2: INSERT into a table whose stored options hold a server-scoped key no longer asks for Manage: the writer marks the table's own options (DataFrameWriter::options_are_the_tables) and the write check skips an option equal to a stored one. A different value is still checked. - Nit: the config doc says tuning keys are admin-only too, and that stored options are not asked about again. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…th and the ADR 0046 roles (P3-T002 PR-14, HTTP side) A Guard seam in krishiv-server: every guarded admin route (/metrics, the UI's /api/internal/*) asks one Guard what a request may do. TokenGuard keeps the printed start-up token of a server without authentication; SessionGuard (feature session) runs the server's ServerAuth and the ADR 0046 matrix through RolePolicy: /metrics needs Monitor on System(metrics) (every role), UI reads Monitor on System(system) and a cancel Operate on System(queries) (operator). Failures are 401 with WWW-Authenticate: Bearer, a throttled peer 503, a denial 403 with one krishiv::audit record. serve_capped records the peer address for the throttle; a guard without one fails closed. krishiv server builds one ServerAuth from krishiv.auth.* (never passed into the engine configuration) and uses it when tokens are configured. The non-loopback refusal stays until the Connect listener is wired. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…back under 2,000 lines after merging main) Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…onnect warning, feature-gated auth tests in the PR gate - krishiv.server.metrics.public opens /metrics only without authentication (S2) - krishiv server warns that Spark Connect does not check tokens yet (S1) - ci: krishiv-server (session, metrics-prometheus) and the CLI server tests run in the PR gate (S6) - tests: exact cancel status, a query-string token on POST is 401, the CLI UI check per feature Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…t002-connect-grpc Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com> # Conflicts: # .github/workflows/ci.yml
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com> # Conflicts: # Cargo.toml # crates/krishiv-session/CLAUDE.md # crates/krishiv-session/src/auth/mod.rs # crates/krishiv-session/src/auth/server.rs # crates/krishiv-session/src/command.rs # crates/krishiv-session/src/session/ops.rs # crates/krishiv-session/tests/server_egress.rs
…answered as unknown and writes an audit record (ADR 0046 §3b) ReattachExecute, the session lookups, a create of a foreign id and a release of one write one krishiv::audit record naming the caller and the kind of id, never the owner; unknown ids write none. Not counted in krishiv_auth_denied_total, which counts 42501 refusals only (§8). Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…e; 0s leaves a stalled handshake holding its slot Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…ecord a foreign operation id, the record names the caller's role; SET of an admin key's Spark alias needs Manage too Review ops/reviews/2026-11-14-opus-A-t002-foreign-id-audit.md M1, N1, N3, N5. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
PR-15a (#742) added the replay buffer, ReattachExecute and abandoned tombstones on the same files. Resolved: abandoned-operation keys use the session owner (is_abandoned takes the Authenticated caller); a reattach of an unknown, unabandoned id still writes the foreign-operation audit record; the server keeps both its TLS/auth and its response budget and abandon task; tests/reattach.rs looks sessions up by the local caller; config reference regenerated. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
… N2, N6) N2: the race after a session is built (another owner creates the id meanwhile) is refused as unknown and writes exactly one record; a ReattachExecute of another principal's operation, end to end over the authenticated listener, is OPERATION_NOT_FOUND like an unknown id and writes one record naming the caller only. N6: the session's denial record carries the client's reported user_id as client_user_id_unverified, and omits it when none was reported. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com> # Conflicts: # crates/krishiv-session/CLAUDE.md
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6161092068
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
command.rs is at 2,000 lines on main; the RESET check now lives in session/ops.rs so the file stays under the file-size limit. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Merging this PR will not alter performance1 warning
Comparing 
Footnotes
|
…keTimeout) Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…SET does A Config RPC refusal came from the role check alone: no audit record, no krishiv_auth_denied_total count, and server-scoped store keys (fs.*) were not checked at all (Codex review on #769). Set and unset now call Session::authorize_set_key (made public) before check_settable; a silent set still turns the refusal into a warning. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…ure-gated runs Three test runs of this branch died linking krishiv-server (ld Bus error, then "No space left on device" in the runner itself): the feature-gated steps rebuild test executables beside the workspace ones, which have already run. Libraries stay, so nothing is compiled again. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Author
|
@coderabbitai review |
Action not completed
|
…icy alone (review M1, N1-N4) The listener's own role check (check_settable, role_of) could refuse with 42501 unaudited and uncounted when a policy and the role disagreed (ADR 0046 §8 and its rejected listener-local check). A Startup key stays refused for everyone (check_static, looked up as authorize_set_key does); every other key is Session::authorize_set_key, as SQL SET. Tests: an admin key named by its Spark alias is refused and counted; the admin's value reads back (1min), and the refused-set check now compares the normalized value, so it no longer passes trivially. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
…ig (re-check R1) With authentication off the base session's policy allows everything: admin keys by name and by Spark alias and fs.* store keys are set, read back and unset. The removed role_of(local) == Admin assertion was the only test of it. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
Contributor
Author
|
Diff review passed (Opus, through head 7cb082f). Reviewed 487bff2..8f28f65 (RESET authorization in Session::unset_key, the Config set/unset policy check, the config reference regeneration, the CI test-executable cleanup): approve with nits. Fixed in 55716b0:
The re-check of 55716b0 approved. Its one nit, R1 (no test of the auth-off local user's admin rights), is fixed in 7cb082f. |
techgopal
added a commit
that referenced
this pull request
Nov 15, 2026
dtolnay/rust-toolchain's `stable` branch was rewritten on 2026-11-15 02:32 UTC. The pinned commit b725eda5 no longer has history in that repository, so zizmor's impostor-commit audit now fails on every PR that touches a workflow (#769, #774). Repin the four uses (setup-rust action, binaries, docs-site) to the current `stable` head 39008827. Its action.yml is byte-identical to the old pin's, so the steps behave the same. Signed-off-by: gopal tathe <44522021+techgopal@users.noreply.github.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tier U. P3-T002 PR-14, the gRPC side of server authentication, plus the listener TLS wiring and the review
items left after #733 and #736. With authentication configured,
krishiv servernow authenticates every SparkConnect RPC, binds each session to its caller, enforces the role matrix on Connect requests, serves TLS on both
ports, and counts every refusal on
/metrics.What changes
krishiv-connect-server):AuthLayer, a tower layer that authenticates each RPC fromits headers (and over TLS its verified client certificate) before the body is decoded, using the server's one
ServerAuth. A caller with no role is refused withPERMISSION_DENIED.check_rpc/check_commandgive everyRPC and
Commandarm a request class, with no wildcard arm. A command's class is checked before its refusal, onExecutePlan and AnalyzePlan alike (
request_plan).SessionOwner(method, kind, tenant, name) keys the session manager. A callerwhose role changed gets a new session. Each session is
PathPrefixPolicy(RolePolicy(role))(role_sessions).The client's
UserContext.user_idis never the identity: it is recorded, marked unverified, insystem.sessionsand in denial records. Viewers can read their own
system.sessionsrows.Configtakes the caller's role. SQLSET/RESETof a key settable byadmin(or its Spark alias) needsManageon the cluster through the session's policy.krishiv.server.tls.certFile/keyFile(rustls throughtokio-rustls; ALPNh2for Connect,
http/1.1for the admin port). Each handshake runs on its own task within the handshake timeout.Only certificates rustls verified reach the authenticator. On Unix,
SIGHUPreloads the certificate and key; openconnections keep theirs, and a pair that fails to load leaves the old one serving.
check_listenerreplaces theold non-loopback refusal: a non-loopback address needs an authentication method, tokens or OIDC there need TLS,
and client certificates need TLS.
krishiv_auth_failed_total{surface, method}andkrishiv_auth_denied_total{surface, privilege}on/metrics. Denials are counted by the guards, the no-role refusal and every caller's session.still answered as unknown, and now also writes one
krishiv::auditrecord naming the caller, its role and thekind of id, never the owner. This covers lookups, creates (including a racing create), a release (a no-op) and the
operation ids of ReattachExecute, ReleaseExecute and Interrupt. Unknown ids write no record. These records are not
counted in
krishiv_auth_denied_total, which countsKRISHIV_INSUFFICIENT_PERMISSIONSrefusals only (ADR 0046update 2026-11-14).
krishiv.connect.handshakeTimeoutalso bounds the TLS handshake, and0sleaves a stalled handshakeholding its connection slot. The deploy guide covers authentication, roles, TLS and the counters, and says that logs are not an
audit trail.
Owner question (in the owner queue)
The session key leaves groups out: method, kind, tenant and name. A caller whose role changed gets its session
replaced: the old one is closed, and a client that saw it gets
INVALID_HANDLE.SESSION_CHANGED. Recommended:confirm both.
Dependency note (ADR 0028)
tokio-rustls0.26.6 becomes a direct dependency ofkrishiv-connect-serverand ofkrishiv-server(featuretls). It was already inCargo.lockthrough reqwest, so the crate count is unchanged. License MIT/Apache-2.0;it uses the workspace's rustls 0.23 on aws-lc-rs (no OpenSSL).
hyper-util(already in the lock) is adev-dependency only. The alternative was tonic's own TLS, which cannot take a reloadable certificate resolver
or hand the verified client certificate to our authenticator.
Reviews
Opus-reviewed before opening: Connect authentication (changes required, then approve with nits), TLS wiring
(approve with nits; the two mediums fixed), auth counters (approve with nits; I1, N1-N3 and N5 fixed). Written after those reviews and not yet reviewed: the
client_user_idrecording and viewer access tosystem.sessions(a2145b3), the admin-keySETgate (c31bfc0), and the merges with #733, #736 and #738, which put AnalyzePlan, Config and Interrupt behind authentication. A later Opus review (ops/reviews/2026-11-14-opus-A-t002-foreign-id-audit.md, approve with nits) covered a2145b3, c31bfc0 and the foreign-id audit; M1, M2, N1, N3 and N5 fixed; N2 and N6 are covered by tests in 44048cb (the racing create, ReattachExecute of a foreign operation end to end,client_user_id_unverifiedin the session's denial record). Merged with main after #742 (PR-15a, replay buffer and reattach) in cd1249d: abandoned-operation tombstones are keyed by the session owner, and a reattach of an unknown, unabandoned id still writes the foreign-operation record.Tests
cargo nextest run -p krishiv-connect-server,-p krishiv-server --features tls,session,metrics-prometheus,-p krishiv-cli --features server,ui --test server(TLS port and SIGHUP reload end to end),-p krishiv-session --all-features,-p krishiv-exec metrics;cargo xtask metrics-gate; clippy with all features.