refactor(admin): route plugin handler auth through authorize_admin_request - #6247
Merged
Merged
Conversation
…quest The plugin/extension admin family carried seven byte-near copies of the admin auth preamble (extract credentials, check_key_valid, read RemoteAddr out of the extensions, call validate_admin_request). Each copy is a place the gate can drift, which is exactly the review surface rustfs/backlog#1829 tracks. Every one of the seven is a per-file wrapper with no resource scope and no audit seam, so it folds onto the shared `authorize_admin_request` gate without changing the decision. The wrappers keep their own missing- credentials pre-check, following the pattern established in `kms_management.rs`: the shared gate reports "get cred failed", while these endpoints have always reported "authentication required" (six sites) and "missing credentials" (object_data_cache), and that response must stay byte-identical. New tests pin each message. Action sets, `deny_only=false`, the RemoteAddr lookup, and the wrapper signatures are unchanged, so authenticated-but-unauthorized and wrong-credential responses are unchanged too.
Contributor
|
CLA requirements are satisfied for this pull request. |
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.
Related Issues
rustfs/backlog#1829 (T3 batch P1 — plugin/extension family). Follows PR #6020 (T1, shared gate landed) and PR #6194 (T2, KMS management group).
Summary of Changes
The plugin/extension admin family carried seven byte-near copies of the admin auth preamble — extract
req.credentials,check_key_valid(get_session_token(...)), digRemoteAddrout ofreq.extensions, callvalidate_admin_request. Every copy is a place the gate can drift, and a drifted copy is an endpoint with different authorization than its siblings.All seven are per-file
authorize_*wrappers with no resource scope and no audit seam, so they fold onto the sharedauthorize_admin_requestgate (rustfs/src/admin/auth.rs:302) without changing the decision. Only the wrapper bodies change; every handler call site is untouched.Sites converted (7/7 of the batch classified in rustfs/backlog#1829):
handlers/extensions.rsauthorize_extension_catalog_requestServerInfoAdminActionInvalidRequest "authentication required"handlers/extensions.rsauthorize_extension_instance_requestGetBucketTargetActionInvalidRequest "authentication required"handlers/plugins_catalog.rsauthorize_plugin_catalog_requestServerInfoAdminActionInvalidRequest "authentication required"handlers/plugins_instances.rsauthorize_plugin_instance_requestGetBucketTargetActionInvalidRequest "authentication required"handlers/plugins_instances.rsauthorize_plugin_instance_write_requestSetBucketTargetActionInvalidRequest "authentication required"handlers/object_data_cache.rsauthorize(parameterized)AdminActionInvalidRequest "missing credentials"handlers/cluster_snapshot.rsauthorize_cluster_snapshot_requestServerInfoAdminActionInvalidRequest "authentication required"The shared gate reports
InvalidRequest "get cred failed"for a credential-less request, which differs from what all seven endpoints have always returned. Each wrapper therefore keeps an explicitif req.credentials.is_none()pre-check returning its own historical error, the pattern established by T2 inkms_management.rs:75-81. Five new tests pin those messages byte-for-byte.Per-site response-equivalence argument
Three request classes, per site:
req.credentials == None). Before:s3_error!(InvalidRequest, "<site message>"). After: the pre-check returns that same expression before the shared gate is reached. Identical code, identical message. Pinned by the five new*_keeps_its_missing_credentials_message/*_keep_their_missing_credentials_messagetests.check_key_validfails). Before:check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &input_cred.access_key).await?. After:authorize_admin_request→authenticate_request(&req.headers, &req.uri, input_cred)(auth.rs:256), whose body ischeck_key_valid(get_session_token(uri, headers).unwrap_or_default(), &credentials.access_key).awaitand which returnsresultunmodified. Same token source, same access key, sameS3Errorpropagated through?. The only difference is thatauthenticate_requestemits adebug!/warn!trace event — log output, not response bytes.validate_admin_request(&req.headers, &cred, owner, false, vec![Action::AdminAction(X)], req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0))). After: the gate callsvalidate_admin_request(&req.headers, &cred, owner, false, actions, remote_addr)withremote_addrcomputed by the identical expression. Same six arguments, samedeny_only = false, same action vector per site, same emptybucket/objectresource slots.validate_admin_requestreturnsS3Result<()>; the gate's extraOk(cred)is dropped by.await?; Ok(())in the wrapper, so the wrapper signature and its success/error values are unchanged.No site gained or lost an action, changed an action, gained a resource scope, or changed
deny_only. No status code changes: all three classes return exactly the error they returned before.Sites deliberately excluded
None from this batch — all seven classified sites converted. For the record, the wider issue's excluded classes are untouched here: key/bucket-scoped gates (
kms_keys,kms_key_lifecycle,kms_key_metadata,quota), audit-wrapped gates (KmsAdminAudit::gate/gate_admin),deny_only-dynamic sites (user.rs:228/628,sts.rs:195), and the E-class one-offs. Batches P2–P6 of the inventory remain to be done.Verification
cargo check -p rustfs— cleancargo clippy -p rustfs --lib --tests -- -D warnings— cleancargo nextest run -p rustfs -E 'test(/admin/)'— 1435 passed, 0 failedcargo nextest run -p rustfs -E 'binary(rustfs) and test(/admin::handlers::(plugins_catalog|plugins_instances|extensions|cluster_snapshot|object_data_cache)::/)'— 57 passedcargo fmt --all --checkscripts/check_layer_dependencies.sh,scripts/check_architecture_migration_rules.sh,scripts/check_unsafe_code_allowances.sh,scripts/check_logging_guardrails.sh,scripts/check_no_planning_docs.sh— all passinclude_str!source-text assertions (plugin-contract-guard)All five touched files are read by their own
include_str!contract tests. Every one was re-run and passes unmodified — the assertions target handler call sites (unchanged) and theAdminAction::*constant inside each wrapper body (retained):extensions::tests::extension_handlers_require_admin_authorization_contractplugins_catalog::tests::plugin_catalog_handlers_require_admin_authorization_contractplugins_instances::tests::plugin_instance_handlers_require_admin_authorization_contractobject_data_cache::tests::stats_handler_requires_server_info_actioncluster_snapshot::tests::cluster_snapshot_handler_requires_server_info_admin_permissionNo source-text assertion needed updating.
Impact
None user-facing. Authorization decisions, HTTP status codes, error codes, and error messages are unchanged for every one of the seven endpoints. The only observable difference is two additional
tracingevents (debug!on authentication attempt/success,warn!on authentication failure) emitted by the shared gate, matching the six admin files already routed through it. No API, config, deployment, or on-disk impact.Additional Notes
Adversarial Validation — Standard tier + security reviewer
deny_onlyvalue, theRemoteAddrextension lookup (present, absent, andSome(None)), the wrapper return type (S3Result<()>vs the gate'sS3Result<Credentials>), and gate-vs-body ordering (all seven wrappers are called at the top ofcallbeforereq.inputis consumed, and stay there). No break found.Nonearm — it cannot without changing 7 responses. No break found.AdminAction::*constant against the operation its handler performs: catalogs and cluster snapshot are reads underServerInfoAdminAction, instance reads underGetBucketTargetAction, instance writes underSetBucketTargetAction, matching theplugin-contract-guardread/write split; the object-data-cache stats/flush pair keeps itsServerInfoAdminAction/ConfigUpdateAdminActionasymmetry. No action set widened, nodeny_onlyflipped, no scope dropped, no read endpoint downgraded to a credentials-exist check, and theAccessDenied → InvalidRequeststatus-code hazard flagged in the inventory does not arise here (none of the seven usedAccessDenied). No break found.include_str!contract tests; a dropped gate call is caught by the same contract tests. Residual risk: the authenticated-but-unauthorized path has no unit test here becausevalidate_admin_requestrequires a ready IAM handle — it is covered by argument-identity inspection above and byauth::tests::authorize_admin_request_without_credentials_is_rejectedplus the gate's own allow/deny unit suite inauth.rs. No break found.