Add support for escaped . in authz validation requests#161
Draft
sdewhitt wants to merge 6 commits into
Draft
Conversation
…back-only test flag
Follow-ups from a post-merge review, stacked without individual issues.
- aws_auth_validate_ldap: express the LDAP SSRF range policy as
?LDAP_DENIED_V4_CIDRS/?LDAP_DENIED_V6_CIDRS consumed by the shared
aws_auth_validate_net:in_any_cidr/2, replacing the hand-coded guard
clauses. LDAP keeps its own broader list (all RFC1918/CGNAT/reserved,
not just broker infra) but both backends now enumerate ranges in ONE
vocabulary, so a future range or segment-math fix cannot silently apply
to only one classifier. Mirrors the amazon-mq#153 embedded_v4 unification, one
layer up (F3).
- aws_auth_validate_ldap: default an omitted port to 389, matching
rabbit_auth_backend_ldap's application-env default (the broker does not
bump to 636 for TLS). A config the live broker would accept no longer
fails input_invalid for parity's sake (F5).
- aws_auth_validate_ldap: the test-only auth_validation_allow_private_networks
flag now relaxes ONLY loopback (via is_denied_ip/1, mirroring
aws_auth_validate_net:classify_denied/1) instead of fully bypassing both
SSRF checks. IMDS/link-local/RFC1918/CGNAT stay denied even under the
flag, so the LDAP backend no longer gets a broader test-mode bypass than
http/oauth. Both the pre-connect and post-connect peer checks always run
(F7).
- tests: cover the loopback-only relaxation (loopback reachable; IMDS/
RFC1918/CGNAT still denied on both the pre-connect and post-connect
checks) and the port default (absent -> 389, supplied honoured,
out-of-range/non-integer still rejected). Export parse_port/2 under
-ifdef(TEST). All 205 eunit tests pass.
…ity (amazon-mq#157, amazon-mq#158) - aws_auth_validate.js: add body_too_large to AWS_AUTH_VALIDATE_CATEGORY. The backend returns {error:"body_too_large"} at HTTP 400 and 4ee6ff7 added it to status_for_category/1, but the UI status map never got the matching key, so an oversized config fell through to the generic "Unexpected response (400)" banner instead of an actionable message. It was the sole backend category missing from the map (amazon-mq#157). - aws_auth_validate_oauth: allow a pre-existing query string on jwks_uri (both the directly-supplied one and the one discovered from an OIDC document). jwks_uri is fetched VERBATIM -- the live broker's uaa_jwks:get/2 fetches it as-is -- so rejecting a query string input_invalid diverged too-strict from the broker, which fetches it fine. issuer keeps rejecting a query (OIDC discovery appends the well-known path, so a query there is ambiguous). Threaded via a QueryPolicy (allow_query | reject_query) arg on parse_url/2; parse_url/1 keeps the reject_query default (amazon-mq#158). - tests: jwks_uri-with-query is now accepted in the pure phase (asserted via parse_input/1, no network) and issuer-with-query is still rejected, replacing the old url_with_query_rejected_test. All 475 auth-validate eunit tests pass.
The CI production build (make app, warnings-as-errors, no -ifdef(TEST) exports) failed with "function parse_url/1 is unused". The prior commit moved both production call sites to parse_url/2 but left the parse_url/1 wrapper, which only survived the local eunit build because parse_url/1 was in the -ifdef(TEST) export list (so it counted as exported there, never unused). Remove the dead wrapper and update the test export to parse_url/2. Both call sites already pass an explicit query policy (allow_query for jwks_uri, reject_query for issuer), so no behavior change. Verified: clean `make app` (production) build passes; 304 auth-validate eunit tests pass.
- Add authz_dotted_additional_scopes_key_parity_pin_test_ exercising an
STS-style dotted claim key (https://sts.amazonaws.com/tags) as
additional_scopes_key. Runs the real upstream normalize_token_scope ->
get_expanded_scopes -> resource_access path (not mocks), asserting the
current authz_unverified/"no effective scopes" behavior: split_path/1
mis-splits the dotted key on '.', so the flat claim is never found.
- Flips to a grant when upstream resolves rabbitmq/rabbitmq-server
discussion #16947; the pin surfaces that drift as a test failure and
forces a coordinated broker-dependency bump.
- Update the aws_auth_validate_oauth over-trust caveat comment: scope
authorization is now checkable via the optional authz_check block
(delegated to aws_auth_validate_oauth_authz), noting the remaining
resource_server-field overlay parity gap.
- Route the request's additional_scopes_key through the broker's own
rabbit_oauth2_schema:tokenize_additional_scopes_key/1 when available, so
the endpoint splits the key IDENTICALLY to auth_oauth2.additional_scopes_key
in rabbitmq.conf: an escaped "\." stays a literal dot in a flat claim name
(the STS/OIDC case behind rabbitmq/rabbitmq-server #16947), while an
unescaped "." remains a nesting separator. The record's list-form clause
consumes the tokenized [[binary()]] without re-splitting.
- Probe with code:ensure_loaded before function_exported so escaped-dot
support does not depend on load order (the trap available/0 documents).
- Fallback: on a pre-#16947 broker series the tokenizer is absent, so we pass
the raw binary through unchanged and normalize_token_scope/2's own
split_path/1 applies -- the endpoint behaves exactly as before. The feature
stays available on old brokers; it just lacks escaped-dot support there,
matching what those brokers themselves can do. Request contract unchanged
(still a plain string).
- Add authz_escaped_dotted_additional_scopes_key_test_ (escaped key -> grant),
double-gated on maybe_skip_authz + tokenizer presence so it skips cleanly on
a pre-fix broker rather than failing. Correct the parity-pin comment: the fix
adds an escaped form, it does not make unescaped dots resolve a flat key.
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.
This PR lands after both #159, #160, and rabbitmq-server 16985 merge.
rabbitmq-server 16947 brought to light issues with how "." splitting is handled, and rabbitmq-server 16985 adds support for escaping dots (like
\.). Since said PR introduces such a change to the upstream OAuth backend, there needs to be proper support on the validation endpoint to maintain parity.Background
The upstream OAuth backend resolves
additional_scopes_keyby splitting the key on.into a nested-map path (rabbit_auth_backend_oauth2:split_path/1). A flat claim key that itself contains dots — e.g. an STS/OIDC key likehttps://sts.amazonaws.com/tags— is therefore split into["https://sts", "amazonaws", "com/tags"]and never found. rabbitmq-server#16985 fixes this by:\.(a literal dot in a segment) viarabbit_oauth2_schema:tokenize_additional_scopes_key/1, which produces a pre-tokenized[[binary()]](list of paths, each a list of segments); andadditional_scopes_keyto accept that list form directly (a newis_list/1clause onextract_token_value/4/extract_scopes_from_additional_scopes_key/2), so a pre-split key is consumed without being re-split.Because the validation endpoint replays the broker's real scope-resolution code, it must interpret
additional_scopes_keyidentically — otherwise an escaped key that the live broker would resolve would be reported asauthz_unverified, breaking parity in the safe-but-misleading direction.This PR adds said support by:
rabbit_oauth2_schema:tokenize_additional_scopes_key/1rather than reimplementing the escape logic. This keeps the same "execute the broker's code, don't mirror it" guarantee the authz layer already relies on: the endpoint splitsadditional_scopes_keybyte-for-byte the wayauth_oauth2.additional_scopes_keydoes inrabbitmq.conf, so an escaped\.stays a literal dot in a flat claim name and an unescaped.remains a nesting separator.aws_auth_validate_oauth_authz:evaluate/3(a newtokenize_additional_scopes_key/1helper that the record overlay routes through). The pure parse phase inaws_auth_validate_oauthis untouched — it still validatesadditional_scopes_keyas a plain binary and never depends on the OAuth backend being loaded.additional_scopes_keyas a plain string;allowed_fields/0, the shape validation, and the response categories are all unchanged. This is purely additive — no new fields, no new statuses.rabbit_oauth2_schema:tokenize_additional_scopes_key/1is not exported (a broker without rabbitmq-server#16985), the helper passes the raw binary through unchanged, sonormalize_token_scope/2takes its existing binary/split_path/1clause and the endpoint behaves exactly as it does today. The feature stays available on old brokers; it simply lacks escaped-dot support there, matching what those brokers themselves can do. No change to theHAVE_OAUTH2_RESOURCE_SERVERcompile gate oravailable/0.code:ensure_loaded/1beforeerlang:function_exported/3(a barefunction_exportedreturnsfalsefor a beam that is on the path but not yet loaded, which would make escaped-dot support depend on load order — the same trapavailable/0already documents).authz_escaped_dotted_additional_scopes_key_test_(new) — an escaped keyhttps://sts\.amazonaws\.com/tagsresolves the flat claim and grants (ok). Double-gated onmaybe_skip_authz/1(the arity-4 scope API) and tokenizer presence, so it skips cleanly on a pre-fix broker rather than failing.authz_dotted_additional_scopes_key_parity_pin_test_(from OAuth Authz validation parity test enhancements #160) — an unescaped key still splits and does not resolve a flat dotted claim (authz_unverified). Its comment is corrected to state that the fix adds an escaped form; it does not make unescaped dots resolve a flat key.Verification
gmake eunit t=aws_auth_validate_oauth_tests— all tests pass; escaped→okand unescaped→authz_unverifiedconfirmed executing (not skipped) with the fixed OAuth backend on the code path.PUT /api/aws/auth/validate/oauthagainst a broker built with rabbitmq-server#16985: same token carrying a flat dotted claim, escapedadditional_scopes_key→ 204, unescaped → 422 authz_unverified.erlfmt -cclean.Notes for reviewers
mainshows both the parity-pin commit (OAuth Authz validation parity test enhancements #160) and the escaped-dot commit. Land order is Hardening/auth validation parity followups #159 → OAuth Authz validation parity test enhancements #160 → (rabbitmq-server#16985 upstream) → this PR.rabbit_oauth2_schema:tokenize_additional_scopes_key/1) only exists once rabbitmq-server#16985 merges. Its signature/return shape should be re-confirmed before this PR leaves draft, since the upstream implementation may still change.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.