security: enforce AMQPS + fail fast on multiple AEM validation bindings#223
Open
rjayasinghe wants to merge 6 commits into
Open
security: enforce AMQPS + fail fast on multiple AEM validation bindings#223rjayasinghe wants to merge 6 commits into
rjayasinghe wants to merge 6 commits into
Conversation
… present Previously, the first aem-validation-service binding returned by the runtime was selected via findFirst() and reused for every AEM messaging binding. In deployments with more than one such binding this made the validation context non-deterministic and could validate an AEM endpoint against a mismatched validation tenant. Reject configurations with more than one aem-validation-service binding at startup with a clear ServiceException listing the offending binding names. The empty and single-binding cases are unchanged.
The AMQP URI is taken from the service binding without local checks and then used as the destination URI for a JmsConnectionFactory that receives an OAuth bearer token as its password override. A binding advertising 'amqp://' would therefore send the bearer token over an unencrypted transport, and a binding whose AMQP host does not match the management endpoint host would send the bearer to a broker outside the validated tenant. Validate the AMQP URI before building the destination: require scheme 'amqps' (case insensitive), and, when the management URI is present in the same binding, require the AMQP host to equal the management host. Malformed AMQP URIs are rejected; a malformed management URI is tolerated because getServiceUri() will fail explicitly later.
Contributor
SummaryThe following content is AI-generated and provides a summary of the pull request: Security: Enforce AMQPS and Fail Fast on Multiple AEM Validation BindingsSecurity🔒 Addresses two security-compliance findings by enforcing encrypted broker connections and preventing ambiguous validation-binding configurations at startup. Finding #7 – AMQP scheme enforcement + broker consistency:
Finding #10 – Validation-binding fan-out prevention:
Changes
PR Bot InformationVersion:
|
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
The PR adds meaningful security hardening (AMQPS enforcement and duplicate-binding fail-fast), but there are two substantive gaps in validateAmqpUri: a silent bypass when either parsed URI yields a null host, and a silent skip with no observability when the management URI is malformed. The Collectors comment I posted is minor and can be disregarded if team style prefers Collectors.joining. Please address the logic/security issues before merging.
PR Bot Information
Version: 1.26.11
- File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened - Correlation ID:
518ccebe-0cff-4b83-8291-70a38bb01beb
Resolved additive conflicts in test files: - AemMessagingServiceConfigurationTest: kept both selectValidationBinding tests (security branch) and testServiceConfigurationByKind_* tests (main). - AemMessagingConnectionProviderTest: merged the mocked-provider test setup from main with the validateAmqpUri tests from the security branch, using main's Mockito-based fixtures for getToken/fetchToken alongside the new validateAmqpUri static-method tests.
Address PR review feedback on validateAmqpUri: - Fail fast when the AMQP URI parses but has no host component (e.g., opaque URIs like 'amqps:foo'), rather than silently skipping the cross-host check. - Fail fast when the management URI parses but has no host component. - When the management URI is malformed and cannot be parsed, log a warning naming the binding instead of silently swallowing. Also drop the java.util.stream.Collectors import from AemMessagingServiceConfiguration in favour of String.join with Stream.toList(), which is already the idiom used elsewhere in the file.
Contributor
There was a problem hiding this comment.
The PR introduces sensible fail-fast guards for both the AMQPS scheme enforcement and the multiple-validation-binding fan-out case, and the test coverage is thorough. Two correctness issues were raised: the early return on a malformed management URI creates a security bypass path, and the URI mutation after validation could silently corrupt path-bearing AMQP URIs before connection establishment.
PR Bot Information
Version: 1.26.11
- Correlation ID:
451d6096-9dc2-4973-8d7c-455bf24271a1 - File Content Strategy: Full file content
- Event Trigger:
issue_comment.created - LLM:
anthropic--claude-4.6-sonnet
…ure/messaging/aem/jms/AemMessagingConnectionProvider.java Co-authored-by: hyperspace-insights[bot] <209611008+hyperspace-insights[bot]@users.noreply.github.com>
Commit 061a643 accepted a review suggestion in the GitHub web UI. The suggestion replaced the malformed-mgmt-URI 'return' with 'mgmt = null' plus a null-guarded check, but the web UI insertion left the old block dangling outside the class body — producing 'class, interface, enum, or record expected' compilation errors at lines 178, 179, 182, and 192 and turning PR #223's build red. Drop the orphaned trailing block. The new logic (mgmt = null on parse failure, then null-guarded host check) is preserved unchanged; 72/72 tests pass locally.
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
1 participant
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.
Addresses security-compliance#18 findings #7 and #10.
Deferred from that issue: #6 (secret rotation), #8 (mutable release
actions), #9 (host-only validation payload).