Repository navigation
CAMEL-25645: camel-milo - let the OPC UA client validate the server certificate against a trust list - #27824
Merged
Conversation
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
reviewed
Oct 14, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks @oscerd. Closing this gap in the client is the right thing to do, and keeping the old behaviour behind an explicit trustAllCertificates flag with security = "insecure:ssl" is the right shape.
What I verified against Eclipse Milo 1.1.8 (the version in parent/pom.xml, sources from Maven Central):
- The claim holds.
OpcUaClientConfigBuilderdefaults toInsecureCertificateValidator, and the two-argumentUsernameProviderconstructor uses it too. - The validator set with
cfg.setCertificateValidatoris the one Milo uses in the secure channel handshake (UascClientMessageHandler, policy notNone) and onCreateSession(SessionFsmFactory).UsernameProvideronly calls its validator when the endpoint policy isNone(or HTTPS), so passing the same validator there matters for aNoneendpoint with a trust list. - Trust list lifecycle: it is created lazily under the lock and reused across reconnects.
dispose()closes it, and theclosedflag stops a racing reconnect from creating a new one.FileBasedTrustListManagerstarts a watcher thread, so closing it matters.FileBasedCertificateQuarantineholds no resources. IfcreateAndInitializethrows, nothing has been created yet that needs closing. trustAllCertificatescarries"security": "insecure:ssl"in the generated JSON, andSecurityUtilsis regenerated. The catalog JSON and doc copies match the sources for bothmilo-clientandmilo-browse. The DSL factories are regenerated.ServerCertificateValidationTestruns end to end against a server that only offersBasic256Sha256, and the user name from the URI also exercises the identity token path.
1. The doc gets the optional checks wrong (confirmed). new DefaultClientCertificateValidator(manager, quarantine) uses ValidationCheck.NO_OPTIONAL_CHECKS, which in Milo is Set.of(APPLICATION_URI):
- The application URI check is enforced. A trusted server certificate whose SAN URI does not match the server's application URI is refused with
Bad_CertificateUriInvalid. It is also not written torejected/certs, because the quarantine only sees chain building failures. The end-to-end test only passes because it setsserver.setApplicationUri("urn:eclipse:milo:test")to matchTestCertificateFactory. - Revocation is not enforced.
OpcUaCertificateRevocationCheckeronly logscheck suppressed: certificate failed revocation checkunlessREVOCATIONis in the set. The page tells users to put CRLs intrusted/crlandissuer/crl, so they will expect a revoked server certificate to be refused. Expiry (VALIDITY) is only logged too.
I would build the trust list validator with at least REVOCATION (for example EnumSet.of(APPLICATION_URI, REVOCATION), and VALIDITY too if you agree). Without REVOCATION_LISTS Milo uses SOFT_FAIL, so a trust list without CRLs behaves as it does now. Whatever set you pick, please describe it exactly in the doc and say that a certificate failing the application URI check is not written to rejected/certs.
2. How a failure shows to the user. A missing trust configuration and a rejected certificate both end in LOG.info("Failed to connect", e) in performAndEvalConnect, followed by a reconnect every 10 s. A producer gets IllegalStateException("No connected") and a consumer receives nothing. So the clear message from validatorFor and Milo's Bad_CertificateUntrusted only show at INFO, with a stack trace every 10 s. JIRA asks for a clear failure. I would log these at WARN: the IllegalStateException from validatorFor, and a UaException with a certificate or security status code. If you can, also carry the last connect failure as the cause of the "No connected" exception, so a producer sees the reason. The upgrade guide should then name the symptom ("the client logs a WARN ... and does not connect").
3. The negative end-to-end cases pass for any failure. notConfigured and emptyTrustList assert that no message arrives within 5 s. That also holds if the client fails for an unrelated reason, such as a wrong key store password. emptyTrustList is backed by the rejected/certs check. notConfigured has nothing behind it apart from the unit test. This is the same gap as the mina mismatch test on #27770. If you do the "No connected" cause from point 2, a producer to the secured server can assert the message. Otherwise a log capture works.
Nit: trustListRejectsAndKeepsUntrustedServerCertificate could assert StatusCodes.Bad_CertificateUntrusted on the UaException, not just the type.
CI is still running.
Checked against the project conventions in CLAUDE.md. This is a rules-and-conventions review and does not replace CodeRabbit, Sourcery or SonarCloud.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
gnodet-bot
reviewed
Oct 14, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Security fix for MITM vulnerability in camel-milo — solid work overall. The concurrency model in ServerCertificateTrust is sound, test coverage across all five trust configurations is thorough, and the upgrade guide entry clearly describes the breaking change and migration path.
One medium-severity concern on the cache ID implementation worth discussing before merge.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 581 of 705 tested, 24 compile-only — current: 581 all testedMaveniverse Scalpel detected 581 affected modules (current approach: 581). Skip-tests mode would test 581 modules (6 direct + 579 downstream), skip tests for 24 (generated code, meta-modules) Modules Scalpel would test (581)
Modules with tests skipped (24)
Build reactor — dependencies compiled but only changed modules were tested (6 modules, 1m 53s total)Total reactor time: 1m 53s
Top 20 slowest modules:
|
oscerd
force-pushed
the
fix/CAMEL-25645
branch
from
October 15, 2026 09:16
487e7dd to
0b7c25a
Compare
Contributor
Author
|
@davsclaus on the nit about Claude Code on behalf of Andrea Cosentino |
gnodet-bot
reviewed
Oct 15, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Following up on my earlier review — all five previous findings have been addressed.
- Finding 1 (cache ID collision risk) — ✅ addressed. Cache ID now uses
className@identityHashCode, matching Java'sObject.toString()convention. The option Javadoc explicitly states that endpoints share a connection only when they use the same validator instance, so the semantic expectation is clear. - Finding A (doc + optional checks) — ✅ addressed.
TRUST_LIST_CHECKSnow includesAPPLICATION_URI,VALIDITY, andREVOCATION. The doc accurately lists what is enforced vs. only logged, and five new unit tests exercise each check with specificStatusCodeassertions (Bad_CertificateRevoked,Bad_CertificateRevocationUnknown,Bad_CertificateTimeInvalid,Bad_CertificateUriInvalid). - Finding B (WARN log + cause chain) — ✅ addressed.
NotConfiguredExceptionandUaExceptionwith certificate status codes log at WARN vialogConnectFailure(). The last connect failure is now carried as the cause of theIllegalStateException("No connected"). - Finding C (negative test reason assertions) — ✅ addressed.
assertConnectionFailure()asserts both"No connected"and the expected reason string in the cause chain. - Finding D (upgrade guide symptom) — ✅ addressed. The guide now names the WARN message text, retry interval, and producer failure with cause.
One minor observation: handleConnectionFailure() (the subscription reconnect path) still logs at INFO unconditionally, while logConnectFailure() (the initial connect path) uses WARN for certificate failures. This is acceptable — subscription reconnect failures are operational events where the root cause is typically the lost connection, not a configuration error. The initial connect already emitted the WARN. Not a blocker.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
Author
|
@gnodet-bot thanks for the follow-up. On the Claude Code on behalf of Andrea Cosentino |
davsclaus
reviewed
Oct 15, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks @oscerd. I compared 487e7dd..0b7c25a (the rebase left the first commit unchanged, so the new work is in 0b7c25a) and went through my earlier points:
- Optional checks: fixed in the code. The trust list validator uses
APPLICATION_URI,VALIDITYandREVOCATION, and the new unit tests assert the exact status codes. I checked Milo 1.1.8: only a chain-building failure goes torejected/certs, so that part of the doc is right. One sentence is still wrong, see below. - WARN and cause: fixed. A missing trust configuration and a certificate or security status code are logged at WARN as one line, with the stack trace at DEBUG. The last connect failure is the cause of
No connectedand is cleared after a successful connect. - Negative end-to-end cases: fixed.
assertConnectionFailurechecksNo connectedand the reason for the not-configured and empty-trust-list clients. - Upgrade guide symptom: fixed, it names the WARN text, the 10 s retry and the producer failure.
Bad_CertificateUntrustednit: answered. Milo 1.1.8 throwsBad_SecurityChecksFailedwhen no chain can be built.- @gnodet-bot's cache id point: fixed. The id uses the class name, and the javadoc says connections are shared only for the same validator instance.
New finding: the doc and the upgrade guide disagree with the code about a CA without a CRL. milo-client-component.adoc says that without a revocation list for an issuer the revocation status is not checked. Your test trustListRefusesServerCertificateOfCaWithoutRevocationList, the TRUST_LIST_CHECKS javadoc and your reply on the thread say the opposite: a CA-issued certificate is refused with Bad_CertificateRevocationUnknown. The upgrade guide does not mention it, and someone upgrading with a CA-issued server certificate will hit it. (Milo's own revocation checker, which applies SOFT_FAIL, needs reflective access to sun.security.provider.certpath; on JDK 17+ without --add-opens Milo falls back to the JDK checker without SOFT_FAIL, which is probably where the sentence came from.) Requiring the CRL is fine with me, as OPC UA asks for it. Please describe it in the doc, the trustListDirectory javadoc and the upgrade guide, and regenerate the catalog copy, the JSON and the DSL (suggestions inline).
CI is green.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
oscerd
force-pushed
the
fix/CAMEL-25645
branch
from
October 15, 2026 11:12
0b7c25a to
999fd74
Compare
davsclaus
approved these changes
Oct 15, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Thanks @oscerd. I compared 0b7c25a..999fd74 (the rebase left both earlier commits unchanged, so the new work is in 999fd74) and went through my earlier points:
- The revocation sentence in
milo-client-component.adocis fixed. It now says that a CA-issued certificate without a CRL of its CA is refused withBad_CertificateRevocationUnknown(astrustListRefusesServerCertificateOfCaWithoutRevocationListchecks), that an empty CRL is enough (trustListAcceptsServerCertificateOfCaWithRevocationList), and that a self-signed certificate of the trust list is not checked (trustListAcceptsTrustedServerCertificate). - The
setTrustListDirectoryjavadoc says the same, and the JSON, the catalog doc copy and the DSL factories are regenerated. - The 4.24 upgrade guide tells users with a CA-issued server certificate to add the CRL of the CA to
trusted/crlorissuer/crl. - The earlier checks (
APPLICATION_URI,VALIDITY,REVOCATION) are still in place.
No new findings. One thing before merge: #27780 (CAMEL-25566) has since added options to milo-client on main, so the branch now conflicts in the generated milo-client.json (component and catalog) and MiloClientEndpointUriFactory.java. Please rebase and regenerate.
CI is green on 999fd74.
Claude Code on behalf of @davsclaus.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…ertificate against a trust list milo-client and milo-browse kept the default certificate validator of the Eclipse Milo client configuration, which accepts any server certificate, and had no option to configure trust material. The client now validates the server certificate whenever the selected endpoint uses a security policy other than None, with the trust list in trustListDirectory or the validator in certificateValidator, and does not connect when neither is configured. The new trustAllCertificates option, marked insecure:ssl, accepts any server certificate. The same validator checks the certificate that the user identity token is encrypted with. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
The trust list validator checks the application URI, the validity period and the revocation status of the server certificate. As OPC UA requires, a certificate issued by a CA needs a revocation list of that CA in the trust list. The documentation describes these checks exactly. A missing trust configuration and a failed certificate or security check are logged at WARN, and the last connection failure is the cause of the "No connected" exception that a producer gets. The end-to-end test asserts that reason, and the unit tests cover revoked, expired, CA without revocation list and application URI cases. The connection cache id uses the class name of a custom validator. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
… the trust list A server certificate issued by a CA is refused with Bad_CertificateRevocationUnknown when the trust list holds no revocation list of that CA. The component documentation, the option description and the upgrade guide said the opposite, and now describe this requirement. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
The milo-client endpoint metadata now holds both the options added by CAMEL-25566 and the server certificate trust options. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Andrea Cosentino <ancosen@gmail.com>
oscerd
force-pushed
the
fix/CAMEL-25645
branch
from
October 15, 2026 16:31
999fd74 to
31d9e32
Compare
davsclaus
approved these changes
Oct 15, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Claude Code on behalf of davsclaus
Thanks @oscerd for the rebase. I compared what the PR changes before (999fd74 on 6463f13) and after (31d9e32 on a16cccb), file by file:
- 24 of the 27 files change exactly as before, including
SubscriptionManager,ServerCertificateTrust,MiloClientConfiguration, the tests, the docs and the 4.24 upgrade guide. - Only the three files that conflicted with #27780 (CAMEL-25566) differ:
milo-client.json(component and catalog) andMiloClientEndpointUriFactory.java. They now hold the options from both PRs. The indexes are renumbered,trustAllCertificatesstill carries"security": "insecure:ssl", and the catalog JSON and doc copy match the sources.monitorNodesand the trust list options are both in the endpoint DSL and the configurer. - In
SubscriptionManager, the trust list changes sit next to thePendingValuedelivery from #27780 without overlapping. Connect and validator wiring are as I approved them.
All earlier threads are resolved. No new findings. CI was still running on 31d9e32 when I looked, so please merge once it is green.
Checked against the project conventions in CLAUDE.md. This is a rules-and-conventions review and does not replace CodeRabbit, Sourcery or SonarCloud.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Contributor
|
@oscerd In the tests changed by #27908 I start such routes after the first write ( Claude Code on behalf of etzel-mes |
This was referenced Oct 17, 2026
This was referenced Oct 18, 2026
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.
Summary
CAMEL-25645
milo-clientandmilo-browsebuilt the Eclipse Milo client configuration without a certificate validator, so they kept Milo's default,CertificateValidator.InsecureCertificateValidator, and the certificate of a server offering a Sign or SignAndEncrypt endpoint was not checked. The client had no option to configure trust material.This PR:
adds three client options, at component and endpoint level, shared by
milo-clientandmilo-browse:trustListDirectory: a trust list in Eclipse Milo's file based layout (trusted/certs,trusted/crl,issuer/certs,issuer/crl).certificateValidator: a customCertificateValidatorbean, used instead of the trust list. It is not autowired.trustAllCertificates: accept any server certificate, markedsecurity = "insecure:ssl".validates the server certificate whenever the selected endpoint uses a security policy other than
None. When neithertrustListDirectorynorcertificateValidatoris set andtrustAllCertificatesis false, the client does not connect.builds the trust list validator with
APPLICATION_URI,VALIDITYandREVOCATION. It refuses a server certificate that:As OPC UA requires, a CA-issued certificate needs a CRL of its CA in the trust list (an empty one is enough). A self-signed certificate of the trust list is not checked for revocation. Only a certificate without a chain up to a trusted certificate is written to
rejected/certs.uses the same validator for the secure channel, the session and the
UsernameProviderthat encrypts the user name and password.logs a missing trust configuration, or a
UaExceptionwith a certificate or security status code, at WARN as a single line (stack trace at DEBUG). The client keeps retrying every 10 seconds, so a certificate moved totrusted/certsis used at the next attempt. The last connect failure is the cause of theNo connectedexception that a producer gets.creates the file based trust list once per connection and closes it when the connection is disposed, since it watches its directories.
adds the trust settings to the connection cache id (the validator's class name and identity hash), so endpoints that validate the server differently do not share a connection.
documents the options and the checks in
milo-client-component.adoc(new "Validating the server certificate" section), recommends restrictingallowedSecurityPolicies, and adds a 4.24 upgrade guide entry that names the log message users will see.Endpoints that use the security policy
Noneare not affected.Tests
ServerCertificateTrustTest(13): the choice of validator, a trusted certificate accepted, an untrusted one refused (Bad_SecurityChecksFailed) and written torejected/certs. Certificates issued with Bouncy Castle (TestPki) cover:Bad_CertificateRevoked;Bad_CertificateRevocationUnknown;Bad_CertificateTimeInvalid;Bad_CertificateUriInvalid.None of these is written to
rejected/certs. The class also checks the classification of connection failures.MiloClientConfigurationTest: the cache id is unchanged without trust settings and differs with them; copy and clone keep the settings.ServerCertificateValidationTest: a server that only offers aBasic256Sha256endpoint.trustAllCertificates=true, or with acertificateValidatorbean receive the monitored value.No connected, caused by the missing trust configuration or byBad_SecurityChecksFailed, and the rejected certificate is written to the trust list.camel-milo module: 72 tests, 0 failures, 1 skipped (
MonitorItemMultiConnectionsCertTest, already@Disabled), including the tests of CAMEL-25566 (CAMEL-25566: camel-milo - monitor several nodes with one consumer #27780), which this branch is rebased on. A full reactor build regenerated the catalog, the DSL factories andSecurityUtils.Revert checks:
No connected, the end-to-end test fails on the reason assertion.MonitorItemMultiConnectionsCertTestis still@Disabled, as before. The new test sets up the secured server through the component'scertificateManageroption.Upgrade
Clients that use a secured security policy need
trustListDirectoryorcertificateValidatorafter upgrading.trustAllCertificates=truerestores the previous behaviour, which is insecure.Claude Code on behalf of Andrea Cosentino
🤖 Generated with Claude Code