Repository navigation
CAMEL-25941: camel-as2 - support sending through an HTTP proxy - #28265
Conversation
Add proxyHost, proxyPort, proxyAuthUsername and proxyAuthPassword options to the AS2 client. Plain http is sent to the proxy in absolute form; https goes through a CONNECT tunnel, with TLS and hostname verification against the target. Proxy credentials are sent preemptively to the proxy only, and a proxy refusal (407) fails the send. Asynchronous MDN delivery is not affected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 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 707 tested, 25 compile-only — current: 9 all testedMaveniverse Scalpel detected 9 affected modules (current approach: 9). Skip-tests mode would test 9 modules (4 direct + 7 downstream), skip tests for 25 (generated code, meta-modules) Modules Scalpel would test (9)
Modules with tests skipped (25)
All tested modules (36 modules, 5m 56s total)Total reactor time: 5m 56s
Top 20 slowest modules:
|
|
there is a test error: |
The YAML DSL validator checks every YAML example in the component documentation; a bare "to" step is not a valid route. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132kd6FojYDmowiTV4jkrvW
|
Thanks @Croway! Fixed in f226e50: the YAML proxy example in the AS2 docs was a bare to step; it's now a complete route (from: direct:sendToPartner → to: as2://client/send), in the component doc and the catalog copy. EipDocExamplesTest passes locally (4/4). Could you approve the CI workflows again when you have a moment? |
|
CI is green and the YAML doc example is fixed. @gnodet @davsclaus, could you take a look when you have time? |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Request changes: the proxy support is well put together, but a refused CONNECT leaves an untunnelled connection in the pool. The next send then skips the tunnel and TLS.
Blockers
AS2ClientConnection.java:240-246: whenCONNECTor the TLS upgrade fails, the connection goes back to the pool and still counts as connected.
Other findings: 2 non-blocking, 1 suggestion. See below.
Thanks @seshuthota, this is a careful piece of work. Some things I liked:
- The option names, labels and
security = "secret"match camel-http, so users of both components see the same options. - On the tunnel path,
Proxy-Authorizationgoes on theCONNECTonly, hostname verification checks the partner's name, and both are tested. - The generated files and the catalog copies are regenerated. The new doc section is clear, and it says plainly that asynchronous MDN delivery is out of scope.
CI is green.
Blocking
1. A failed CONNECT or TLS upgrade puts the connection back in the pool as connected (AS2ClientConnection.java:240-246)
When the proxy refuses the tunnel, createTunnel throws and response stays null. The finally block then releases the endpoint with TimeValue.ZERO_MILLISECONDS (line 270). In HttpClient 5, PoolingHttpClientConnectionManager.release() closes the connection only when keepAlive is null. With ZERO, an open and consistent connection is put back as reusable, and PoolEntry.updateExpiry() makes it never expire: Deadline.calculate() returns MAX_VALUE for a non-positive value. The IOHelper.close(endpoint) after the release does nothing, because the endpoint is already detached.
An unread 407 body on the CONNECT response does not make the connection inconsistent, since isConsistent() only tracks the request entity. isConnected() only checks that a socket is bound. So the next send() on that pool:
- gets a connection that counts as connected
- skips
createTunnelandupgrade - writes the AS2 request in origin form, in plain text, on the proxy connection. It then parses the leftover 407 body as its response.
A wrong proxyAuthPassword makes this happen on every second send, and so does any proxy that keeps the connection open after refusing a CONNECT. The lease check does not help. It only looks for stale connections after 2s of idle time (the default validateAfterInactivity), and a stale check cannot tell a refused tunnel from a good one anyway.
A failed TLS upgrade has the same shape. AbstractClientTlsStrategy closes the SSL socket, but the plain socket is still bound to the connection, so it stays "connected". The next send then fails with a socket-closed error instead of making a fresh CONNECT and handshake.
The tests do not catch this. TestHttpProxy answers 407 with Connection: close, and each refusal test sends only once.
Suggested fix, inline: close the connection whenever connect, tunnel or upgrade fails (it needs import org.apache.hc.core5.io.CloseMode;). Separately, the comment at lines 268-270 says "do NOT keep this connection around", but ZERO keeps it. Passing null there is what actually closes it. That finally block is older than this PR and also covers the direct path, so fixing it here is optional.
Non-blocking
2. 407 on the plain-http path (AS2ClientConnection.java:281-286): the check runs after the finally block has already released the connection to the pool with the keep-alive from the strategy. If the proxy keeps that connection open, the 407 body may still be unread when the next request uses it. One fix is to do the check inside the try and close the endpoint before throwing. Another is to release with a null keep-alive for a proxy 407.
3. Tests for a second send after a refusal (inline on AS2ClientProxyTest.java:207): send twice through the wrong-password tunnel route, and expect a 407 both times and two CONNECT lines at the proxy. This fails today and passes with the fix. Optionally, TestHttpProxy could also keep the client connection open after a 407, as real proxies often do. A second send after verifiesTheTargetHostNameThroughTheTunnel should likewise fail with an SSLException again.
Suggestions
4. #28274 (CAMEL-25918) also touches AS2Configuration, as2.json, as2-component.adoc and AS2EndpointBuilderFactory. Whichever PR merges second needs a rebase, and the generated files need regenerating.
Claude Code on behalf of @davsclaus. This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
… instead of pooling it A refused CONNECT or a failed TLS upgrade released the connection to the pool with a zero keep-alive, which HttpClient keeps open with no expiry. The next send then skipped CONNECT and TLS and wrote the AS2 request in plain text to the proxy. Close the connection when connect, tunnel or upgrade fails, and when the proxy answers 407 on the plain-http path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132kd6FojYDmowiTV4jkrvW
|
Thanks for the thorough review @davsclaus! All three points are addressed in d6423fe, details in the threads. On #28274: no conflict with main for now. If it merges first, I'll rebase and regenerate. Could a committer approve the CI workflows for the new commit? Claude Code on behalf of @seshuthota |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks @seshuthota, all three points are addressed in d6423fe.
- Failed tunnel or upgrade:
AS2ClientConnection.java:241-254now closes the endpoint withCloseMode.IMMEDIATEwhen connect,CONNECTor the TLS upgrade fails. The entry goes back to the pool without a connection, so the next send makes a freshCONNECTand handshake. - Plain-http 407: the check now runs inside the
try(lines 268-276) and closes the endpoint before throwing, so an unread 407 body cannot reach the next request. - Tests:
refusedTunnelIsNotReusedandrefusedHttpConnectionIsNotReused, with their keep-alive variants, assert two 407s, two request lines and two proxy connections, and would fail without the fix. Sending twice inverifiesTheTargetHostNameThroughTheTunnelcovers the failed handshake too. The keep-alive mode ofTestHttpProxyis a nice touch.
Could you link the JIRA for the Connection: close pooling issue on the direct path once it is filed? That follow-up is also a good place to change the ZERO_MILLISECONDS release in the finally (lines 284-290) to null: its comment says the connection is not kept, but with ZERO it is.
LGTM once CI is green.
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.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132kd6FojYDmowiTV4jkrvW
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Solid proxy implementation — correct HTTP/HTTPS proxy semantics, proper connection cleanup on failure, and thorough test coverage. One minor usability note below.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…ssword, or the reverse Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132kd6FojYDmowiTV4jkrvW
|
Addressed the note from the automated review in 5f9144e: setting only one of the proxy credentials now fails the endpoint at startup (details in the thread). Could a committer approve the CI workflows for the new commit? Claude Code on behalf of @seshuthota |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
✅ Approval — CAMEL-25941: camel-as2 proxy support
The implementation is clean and well-tested:
AS2ClientConnectioncorrectly validates thatproxyAuthUsernameandproxyAuthPasswordare always set together, throwingIllegalArgumentExceptionwith a clear message- The new
AS2ClientProxyTestcovers proxy auth validation, tunnel failures, connection reuse, and concurrency (15+ tests) - The catch block in the connection setup properly re-throws after closing the endpoint — no silent swallowing
No issues found.
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
The new commit is correct: setting only one proxy credential now fails when the endpoint is created, with a clear message. Good to merge once CI is green.
Blockers: None
Other findings: 0
Thanks @seshuthota, this answers the point from the automated review well.
AS2ClientConnection.java:128-131: the check runs before the connection pool is built, so nothing is left open when it throws. It applies only whenproxyHostis set, and the older constructors passnullfor both credentials, so existing users see no change.AS2Endpointcreates the client connection inafterConfigureProperties, so the route fails at startup, as the new sentence inas2-component.adocsays. The catalog copy is in sync.- The three new tests in
AS2ClientProxyTestcover each credential alone and the endpoint path. - The merge of main changed nothing in this PR's own files, apart from the regenerated
as2.jsonrows that now carry the option descriptions from CAMEL-25918. The merge also brings httpclient 5.6.5, so the CI run that is starting now is the first one of the proxy tests on it.
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.
Claude Code on behalf of seshuthota
JIRA: CAMEL-25941
Description
The AS2 client can't send through an HTTP proxy today.
AS2ClientConnectiondrivesPoolingHttpClientConnectionManagerdirectly withnew HttpRoute(targetHost), so HttpClient's route planning never runs, the JVMhttp(s).proxyHostproperties are ignored, and the startup test connection opens a raw socket straight to the partner.This PR adds four options to the AS2 client endpoint, with the same names and meaning as camel-http (same HttpClient 5 library):
proxyHostproxyPortproxyAuthUsernamesecurity = "secret", optional Basic proxy auth: set both credentials or neitherproxyAuthPasswordsecurity = "secret"Behaviour:
Proxy-Authorizationheader when credentials are set.upgrade(), using the endpoint'ssslContext. Hostname verification checks the partner's name, not the proxy's.Proxy-Authorizationgoes on the CONNECT request only and never reaches the partner.proxyAuthUsername/proxyAuthPasswordfails the endpoint at startup with anIllegalArgumentException, like a missingproxyPort, instead of skipping authentication and failing later with a 407.AS2ClientConnectionconstructors keep their behaviour; a new constructor takes the proxy settings.Left out on purpose (see the ticket): asynchronous MDN delivery stays direct, because its address is chosen by the remote sender and routing it through the operator's proxy deserves its own discussion. Also not included: Digest/NTLM,
nonProxyHosts, and CamelContext global proxy options.Documented in a new "Sending through an HTTP proxy" section of the AS2 component page. Generated files (configurers,
as2.json, catalog copies,AS2EndpointBuilderFactory) are regenerated. BecauseAS2Configurationis shared, the options also appear in the generated configurers of the server endpoint configurations, labelledproducer,proxy, as other client-only options do.Tests
AS2ClientProxyTest(17 tests) with a small in-test forward/CONNECT proxy (TestHttpProxy), which can close the connection after a 407 or keep it open:Proxy-Authorizationreaches the partnerproxyPortgives a clear errorAS2ClientConnectionand when the endpoint is created (the 3 tests fail without the check)AS2MessageTestcases (signed, encrypted, compressed, combined) over http and https🤖 Generated with Claude Code
https://claude.ai/code/session_0132kd6FojYDmowiTV4jkrvW