Skip to content

fix: Parse full certificate chain for client certificates#2222

Merged
thjaeckle merged 1 commit intoeclipse-ditto:masterfrom
ATNoG:fix-client-cert-parsing
Sep 16, 2025
Merged

fix: Parse full certificate chain for client certificates#2222
thjaeckle merged 1 commit intoeclipse-ditto:masterfrom
ATNoG:fix-client-cert-parsing

Conversation

@JCapucho
Copy link
Contributor

@JCapucho JCapucho commented Sep 10, 2025

Previously the code for parsing the certificate chain for use in connection's client certificate based authentication only parsed the very first certificate of what the user submitted (the client certificate itself), and disregarded all other certificates.

This works when the validation path doesn't include any intermediate certificates. However, as soon as an intermediate certificate is introduced everything stops working, because when the server requests a certificate for it's configured client certificate authority, no path can be made and client certificate authentication fails.

This PR fixes this issue by properly parsing the full certificate chain.

@JCapucho JCapucho mentioned this pull request Sep 10, 2025
Merged
@JCapucho JCapucho force-pushed the fix-client-cert-parsing branch from 8d9ca48 to d1f4311 Compare September 10, 2025 14:04
@thjaeckle
Copy link
Member

thjaeckle commented Sep 10, 2025

Started a systemTests run at: https://github.com/eclipse-ditto/ditto/actions/runs/17616067613

@JCapucho
fix: Parse full certificate chain for client certificates
d56247f 
Previously the code for parsing the certificate chain for use in
connection's client certificate based authentication only parsed the
very first certificate of what the user submitted (the client
certificate itself), and disregarded all other certificates.

This works when the validation path doesn't include any intermediate
certificates. However, as soon as an intermediate certificate is
introduced everything stops working, because when the server requests a
certificate for it's configured client certificate authority, no path
can be made and client certificate authentication fails.

This commit fixes this issue by properly parsing the full certificate
chain.
@JCapucho JCapucho force-pushed the fix-client-cert-parsing branch from d1f4311 to d56247f Compare September 10, 2025 14:15
@JCapucho
Copy link
Contributor Author

JCapucho commented Sep 10, 2025

Sorry just noticed that there was a problem with the code where it will accept empty certificates chains, should be fixed now.

@thjaeckle
Copy link
Member

thjaeckle commented Sep 10, 2025

One relevant System-Test failed: org.eclipse.ditto.testing.system.connectivity.amqp10.Amqp10SaslIT

We must look into this before being able to accept the PR.
Test code is located here: https://github.com/eclipse-ditto/ditto-testing/blob/main/system/src/test/java/org/eclipse/ditto/testing/system/connectivity/amqp10/Amqp10SaslIT.java

@JCapucho
Copy link
Contributor Author

JCapucho commented Sep 10, 2025

One relevant System-Test failed: org.eclipse.ditto.testing.system.connectivity.amqp10.Amqp10SaslIT

We must look into this before being able to accept the PR. Test code is located here: https://github.com/eclipse-ditto/ditto-testing/blob/main/system/src/test/java/org/eclipse/ditto/testing/system/connectivity/amqp10/Amqp10SaslIT.java

I've had a quick look and this test seems to be using hmac credentials and there doesn't seem to be anything involving client certificates (or even amqps) and the timeout error seems a bit suspicious.

I'll try to look deeper as soon as I can find the time.

@JCapucho
Copy link
Contributor Author

JCapucho commented Sep 15, 2025

@thjaeckle I've just checked locally and the test passes, seeing as these changes shouldn't affect the test and the error seems to be a timeout I think it's fair to assume it's flakiness on the part of the tests.

@thjaeckle
Copy link
Member

thjaeckle commented Sep 16, 2025

Ran another test-run: https://github.com/eclipse-ditto/ditto/actions/runs/17738343683

The test failure/error is still there - and it is not on a run against the "master" branch .. I'll look into it once I find some time.

@JCapucho
Copy link
Contributor Author

JCapucho commented Sep 16, 2025

@thjaeckle I didn't notice at first but the tests include logs

15:49:13.697 [ERROR] [Time-limited test] o.e.d.t.s.c.a.Amqp10TestServer - Could not listen on AMQP 1.0 server port 5887, remaining attempts: 1.
java.net.BindException: Address already in use
	at java.base/sun.nio.ch.Net.bind0(Native Method)
	at java.base/sun.nio.ch.Net.bind(Net.java:565)
	at java.base/sun.nio.ch.ServerSocketChannelImpl.netBind(ServerSocketChannelImpl.java:344)
	at java.base/sun.nio.ch.ServerSocketChannelImpl.bind(ServerSocketChannelImpl.java:301)
	at io.netty.channel.socket.nio.NioServerSocketChannel.doBind(NioServerSocketChannel.java:141)
	at io.netty.channel.AbstractChannel$AbstractUnsafe.bind(AbstractChannel.java:561)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.bind(DefaultChannelPipeline.java:1281)
	at io.netty.channel.AbstractChannelHandlerContext.invokeBind(AbstractChannelHandlerContext.java:600)
	at io.netty.channel.AbstractChannelHandlerContext.bind(AbstractChannelHandlerContext.java:579)
	at io.netty.channel.DefaultChannelPipeline.bind(DefaultChannelPipeline.java:922)
	at io.netty.channel.AbstractChannel.bind(AbstractChannel.java:259)
	at io.netty.bootstrap.AbstractBootstrap$2.run(AbstractBootstrap.java:380)
	at io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:173)
	at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:166)
	at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:472)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:569)
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:998)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:1583)
15:50:10.580 [INFO ] [Time-limited test] o.e.d.t.c.IntegrationTest - Removing all dynamic connections for solution username <user-e8df3fdb-c228-4a18-b182-8ad88ddaef73>

This is the relevant logs for the test. Seems like the test can't start the test amqp server because the port is already taken.

@thjaeckle thjaeckle added this to the 3.8.0 milestone Sep 16, 2025
@thjaeckle
Copy link
Member

thjaeckle commented Sep 16, 2025

@JCapucho thanks, good catch 👍
I saw that 2 system-tests (which could run in parallel) used the same port for the Amqp10Server (used for tests).
I adjusted the one the Amqp10SaslIT.java uses .. and will re-run the tests now .. fingers crossed :)

Run: https://github.com/eclipse-ditto/ditto/actions/runs/17769628910

thjaeckle
thjaeckle approved these changes Sep 16, 2025
View reviewed changes
Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the last test run was a success .. so good for me to be merged.

Thanks a lot @JCapucho for the contribution 🥳

@thjaeckle thjaeckle merged commit 3abc595 into eclipse-ditto:master Sep 16, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thjaeckle thjaeckle thjaeckle approved these changes

Assignees

@JCapucho JCapucho

Labels

None yet

Projects

Ditto Planning
Status: Done

Milestone

3.8.0

Development

Successfully merging this pull request may close these issues.

2 participants

@JCapucho @thjaeckle