Repository navigation
CAMEL-26322: camel-as2 - scope the async MDN allow-list to host and port, and add an option to require https - #28527
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. Solid security-hardening PR — the host+port authority matching and asyncMdnRequireHttps option are correctly implemented, properly threaded through all assembly paths, and well tested.
One minor Javadoc gap: the updated asyncMdnAllowedHosts description documents host:port form but doesn't mention IPv6 bracket notation, which is non-obvious. See inline comment.
httpIsRefusedWhenHttpsIsRequired and httpIsAcceptedWhenHttpsIsNotRequired tests directly exercise the requireHttps && !https branch introduced by the fix. The assertion e.getMessage().contains("asyncMdnRequireHttps") would only trigger if that branch fires, confirming the tests exercise the corrected code path.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 9 of 708 tested, 24 compile-only — current: 9 all testedMaveniverse Scalpel detected 9 affected modules (current approach: 9). Skip-tests mode would test 9 modules (5 direct + 7 downstream), skip tests for 24 (generated code, meta-modules) Modules Scalpel would test (9)
Modules with tests skipped (24)
All tested modules (36 modules, 5m 21s total)Total reactor time: 5m 21s
Top 20 slowest modules:
|
332eefa to
30837e1
Compare
…ort, and add an option to require https The asyncMdnAllowedHosts allow-list now matches the full authority - host and port - of the asynchronous MDN delivery address, instead of the host alone. Each entry is a host with an optional port in host:port form; a bare-host entry matches only the default port for the scheme (80 or 443), so a non-default port is listed explicitly as host:port. This gates both the delivery connection and the attachment of the configured MDN credentials on the port as well as the host. The option description and the upgrade guide are updated for the default-port behaviour. A new opt-in option asyncMdnRequireHttps (default false) is added: when set, an asynchronous MDN whose delivery address does not use https is refused before the connection is opened and before the credentials are attached, so the configured MDN credentials are never sent in cleartext to a sender-chosen address. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
30837e1 to
55686c0
Compare
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Comment — scoping the async MDN allow-list to host and port, and the new asyncMdnRequireHttps, are correct and fail closed: one authorityIsAllowed decision gates both the connection and the credentials, and the https check runs before the socket opens. Generated files and the upgrade guide entry are in place.
Blockers: None.
Other findings: 1 nit (open gnodet-bot thread), 3 suggestions, 1 stale comment — see below.
Findings
-
The IPv6 bracket form is not documented (gnodet-bot thread on
AS2Configuration.java:763, still open). The matcher handles[::1]and[::1]:8443(AS2AsynchronousMDNManagerisAllowedAuthority), andanIpv6AllowListEntryIsMatchedOnHostAndPorttests it, but theasyncMdnAllowedHostsjavadoc (the catalog description) and the upgrade guide only sayhost:port. Applying the suggestion needs the catalog, component JSON and endpoint DSL regenerated. -
The WARN hint can point to an entry that does not match.
AS2AsynchronousMDNManager.java:254-256logs only the host: "credentials not sent to sender-supplied host {}: set asyncMdnAllowedHosts to authorise it". A bare-host entry now matches only port 80/443, so an operator who copies that host while the partner uses e.g. 4080 gets every MDN refused. Logginghost + ":" + portgives them the entry to write. -
The wiring of
asyncMdnRequireHttpsis not tested. It passes fromAS2Consumer/AS2ConnectionHelperthroughAS2ConsumerConfigurationandAS2ServerConnectioninto the manager, and every construction site passes it, but no test checksisAsyncMdnRequireHttps()or that the shorter constructors default it tofalse.AS2ConsumerConfigurationMdnSettingsTestalready covers the other MDN settings that way. -
aBareHostEntryMatchesTheDefaultPortreaches the network. The authority is allowed, sosend()opensnew Socket("partner.example", 80)with the test credentials attached. It passes because.examplefails DNS quickly; behind a resolver that rewrites failed lookups, or a firewall that drops traffic, it waits for the OS connect timeout (no connect timeout is set). TestingisAllowedAuthoritydirectly for the port-80 case avoids that. -
Stale comment:
AS2AsynchronousMDNManager.java:200still says "Plain http is always allowed", which is no longer true whenasyncMdnRequireHttpsis set (line 211).
CI: the previous build (21, false) failures were AuthoringToolsLenientTest in camel-jbang-core, a break on main fixed by 57b17bd; the branch is rebased onto it and CI is running again.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying. It does not replace specialized review tools or static analysis.
…ty matching - Document IPv6 bracket notation in AS2Configuration and upgrade guide - Log both host and port in async MDN authority warning - Add unit tests for asyncMdnRequireHttps wiring in consumer configuration - Test bare-host authority matching directly without opening network sockets Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
Addressed review feedback:
Claude Code on behalf of oscerd |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the previous finding (IPv6 bracket notation in asyncMdnAllowedHosts Javadoc) has been addressed. The davsclaus review findings from the prior iteration are also all fixed in this update: host:port authority in the WARN log, asyncMdnRequireHttps wiring tests, direct isAllowedAuthority() call in the bare-host port test, and the stale "Plain http is always allowed" comment.
One remaining inconsistency: the generated catalog/component JSON and endpoint DSL Javadoc for asyncMdnAllowedHosts are missing the IPv6 bracket notation detail that was added to AS2Configuration.java. The setter Javadoc at line 764 now reads:
Each entry is a host with an optional port in host:port form (IPv6 literals use bracket notation, for example
[::1]or[::1]:8443); a bare-host entry…
But the generated catalog/camel-catalog/…/as2.json, camel-as2-component/…/as2.json, and AS2EndpointBuilderFactory.java all say:
Each entry is a host with an optional port in host:port form; a bare-host entry…
The IPv6 parenthetical is absent. This means the codegen was run against an earlier version of the Javadoc and the metadata wasn't regenerated after the IPv6 annotation was added. Operators configuring asyncMdnAllowedHosts via IDE tooling or the component catalog will see incomplete format documentation. Running the Camel code generator again after the Javadoc update will fix this.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
✅ Generated files are up to dateAn earlier CI run reported uncommitted generated changes; the latest run no longer does. |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — 4 of the 5 previous findings have been addressed in this update.
Addressed ✅:
davsclausfinding 2 — WARN log now includes both host and port ({}:{}).davsclausfinding 3 —asyncMdnRequireHttpswiring is now tested inAS2ConsumerConfigurationMdnSettingsTest.davsclausfinding 4 —aBareHostEntryMatchesTheDefaultPortnow callsisAllowedAuthority()directly, no longer touches the network.davsclausfinding 5 — Stale "Plain http is always allowed" comment replaced with accurate text.
Still open
- The generated catalog JSON and endpoint DSL Javadoc show
::1 or ::1:8443(brackets stripped by codegen) instead of[::1] or [::1]:8443. The source Javadoc inAS2Configuration.javaand the upgrade guide are correct. See inline comment.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…n catalog and endpoint dsl
- Wrap IPv6 bracket examples in {@code} in AS2Configuration javadoc
- Restore [::1] and [::1]:8443 in as2.json and AS2EndpointBuilderFactory
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all previous findings are now addressed.
The last open item — IPv6 bracket notation stripped by codegen in the catalog JSON and endpoint DSL — is fixed in commit e0003b4 by wrapping the IPv6 examples in {@code} tags in the source Javadoc, which correctly preserves [::1] and [::1]:8443 through the code generator into the catalog and DSL artifacts. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the new commit (0d22bbb) is a pure codegen refresh: it regenerates the catalog JSON and endpoint DSL Javadoc with the {code ::1} fix from the prior source commit (e0003b4). No logic changes. All previous findings remain addressed.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Verdict: Nearly there: four of my five earlier points are fixed, and the matcher, wiring and tests look good. One documentation item came back in the last regen.
Blockers: None
Other findings: 1 non-blocking (the IPv6 example in the generated catalog/DSL reads {code ::1}), 1 suggestion.
Status of my earlier points:
- IPv6 bracket form: partially. The
AS2Configurationjavadoc and the upgrade guide are right; the generated text is not (below). - WARN names the authority: fixed (
{}:{}). asyncMdnRequireHttpswiring tests: fixed (configurationWithRequireHttpsplus theassertFalsechecks).- Bare-host test without network: fixed (direct
isAllowedAuthoritycall). - Stale "Plain http is always allowed" comment: fixed.
Non-blocking: the IPv6 example in the catalog and Endpoint DSL now reads {code ::1} or {code ::1:8443} (both as2.json files and AS2EndpointBuilderFactory.java:669-670, :1939-1940). The bracketed text in e0003b4 was a hand edit to generated files, and the following Regen (0d22bbb) replaced it with the generator output. JavadocHelper.sanitizeJavaDoc (camel-tooling-util) keeps only identifier characters and .,-='/\!&%():;#${}, so [, ] and @ are dropped and {@code [::1]} cannot survive. Since ::1:8443 is itself a valid IPv6 address and an unbracketed entry is taken as a host, an operator copying the catalog example would have every MDN refused. A hint: describe the form without bracket characters in the javadoc, e.g. "an IPv6 literal is written in square brackets, as in a URL, optionally followed by :port", keep the literal [::1] / [::1]:8443 examples in the upgrade guide (and as2-component.adoc), then regenerate. The earlier gnodet-bot thread on this was resolved, but the text is not fixed at the head.
Suggestion: for an IPv6 destination the WARN prints ::1:8443, because normalizeHost stripped the brackets, which is the form the allow-list does not accept. Wrapping the host in brackets when it contains : would give operators an entry they can copy.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…in words, and bracket an IPv6 host in the warning
The catalog generator keeps only identifier characters and a few
punctuation characters in an option description, so the bracketed IPv6
examples in the asyncMdnAllowedHosts javadoc came out as "{code ::1}"
in the catalog and the Endpoint DSL. Describe the IPv6 form in words
instead and regenerate. The literal [::1] and [::1]:8443 examples stay
in the upgrade guide, and as2-component.adoc gets a short section with
the entry forms.
The warning logged when the credentials are not sent named an IPv6
destination as ::1:8443, which the allow-list reads as another IPv6
address. Write the host in square brackets ([::1]:8443), so the warning
names an entry the operator can copy. The matching logic is unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
|
Thanks for the follow-up review. Both points are addressed in "CAMEL-26322: camel-as2 - write the IPv6 form of asyncMdnAllowedHosts in words, and bracket an IPv6 host in the warning". 1. The IPv6 example in the catalog and Endpoint DSL Confirmed. The option description goes through
2. The warning prints Applied. The warning now writes the authority through a small package-private helper, The new test Tests (local, JDK 21): camel-as2-api 119 tests; camel-as2-component 45 unit tests (1 manual test skipped) and 112 integration tests in 11 classes; no failures. The full reactor ( Claude Code on behalf of oscerd |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all previous findings are now addressed.
The last remaining issue — the catalog JSON and endpoint DSL describing IPv6 allow-list entries with {code ::1} / {code ::1:8443} (brackets stripped by codegen, giving operators a silently-broken example to copy) — is fixed in this commit by switching to prose in the source Javadoc: "an IPv6 literal is written in square brackets, as in a URL". The generated artifacts now carry correct, readable descriptions. The new as2-component.adoc section includes the concrete [::1] and [::1]:8443 forms directly, which survive adoc rendering correctly.
Additionally, the new formatAuthority(host, int) method ensures the WARN log names an asyncMdnAllowedHosts entry the operator can copy verbatim: an IPv6 host is bracketed back ([::1]:8443), making the entry well-formed in both the log message and the allow-list. The roundtrip test theWarningNamesTheAuthorityAsAnAllowListEntry verifies the format→match cycle for both regular and IPv6 authorities. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Reviewed the changes since my last review: the points I raised are addressed, and CI is green on the current head.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
When camel-as2 requests an asynchronous MDN (Message Disposition Notification),
AS2AsynchronousMDNManager.send()resolves the delivery address from the sender-suppliedReceipt-Delivery-OptionURI and opens a connection to it. TheasyncMdnAllowedHostsoption onAS2Configurationgoverns whether the MDN is delivered to the resolved address and whether theconfigured MDN credentials (
mdnUserName/mdnPassword/mdnAccessToken) are attached.send()resolves both the host and the port of the delivery address (the port viauri.getPort(), defaulted to 80 or 443 by scheme), but the allow-list match compared only thehost. The resolved port was used to open the socket but was never matched against the list, so
every port on a listed host was treated the same — both for delivery and for attaching the
credentials.
Change
AS2AsynchronousMDNManagernow matches the full authority — host and port — of the resolveddelivery address. The single allow decision that gates the connection and the attachment of the
credentials now accounts for the port.
asyncMdnAllowedHostsentry may carry an optional port inhost:portform. A bare-hostentry (for example
partner.example) matches only the default port for the scheme (80 forhttp, 443 forhttps); a non-default port is listed explicitly (for examplepartner.example:8443). IPv6 literals are supported ([::1]or[::1]:8443).asyncMdnAllowedHostsdescription onAS2Configurationis updated to document thehost[:port]form, the default-port rule and the IPv6 form, and the catalog / component /endpoint-dsl metadata is regenerated. The IPv6 form is described in words ("an IPv6 literal is
written in square brackets, as in a URL, optionally followed by :port") because the catalog
generator drops bracket characters from option descriptions; the literal
[::1]/[::1]:8443examples are in the component documentation and the upgrade guide.
asyncMdnAllowedHostsset), thewarning names the delivery authority as an entry that can be copied to the list:
host:port, withan IPv6 host in square brackets (for example
[::1]:8443).asyncMdnRequireHttps(defaultfalse) is added. When set, an asynchronousMDN whose delivery address does not use
httpsis refused before the connection is opened andbefore the credentials are attached, so the configured MDN credentials are never sent in cleartext
to a sender-chosen address. The option applies per consumer endpoint, like
asyncMdnAllowedHosts.Tests
New and updated JUnit methods in
AS2AsynchronousMDNManagerDeliveryAddressTest:opening a connection);
asyncMdnRequireHttpsset, a plain-http delivery address is refused and an https address isaccepted; by default (option off) http is still accepted;
host:port, with an IPv6 host in square brackets, and oncelisted it matches the authority it was written for.
AS2ConsumerConfigurationMdnSettingsTestchecks thatasyncMdnRequireHttpsis carried by theconsumer configuration, and that the constructors without it default it to
false.The existing
camel-as2-componenttests that deliver an asynchronous MDN to a random local port(
AS2ServerBasicAuthHeaderTest,AS2ServerBearerAuthHeaderTest,AS2AsyncMdnBasicAuthHeaderTest,AS2AsyncMdnTokenAuthHeaderTest,AS2AsyncMdnContextReuseTestandAS2ServerTwoConsumerAsyncMdnSettingsTest) now listhost:portinasyncMdnAllowedHostsinstead of a barehost, as a bare-host entry matches only the default port.
Documentation
mainindocs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_24.adoc.as2-component.adocgets a short "Delivering asynchronous MDNs" section with the entry forms,including the bracketed IPv6 examples.
Claude Code on behalf of oscerd
🤖 Generated with Claude Code