Skip to content

Conversation

tombentley
Copy link
Member

It's been suggested to break #71 into separate proposals to simplify review etc. This PR adds a "proposal" for exposing the client and server TLS information to filters.

@tombentley tombentley requested a review from a team as a code owner July 25, 2025 03:46
Signed-off-by: Tom Bentley <[email protected]>
@tombentley
Copy link
Member Author

@robobario updated, if you wanted to take another pass.

@tombentley tombentley mentioned this pull request Jul 29, 2025
@tombentley
Copy link
Member Author

@k-wall @SamBarker please can you take a look at this. I'm very aware that we've already implemented half of it. It would be good to keep this repo aligned with (or at least not behind) what's actually implemented.

Copy link
Member

@k-wall k-wall left a comment

Choose a reason for hiding this comment

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

I agree with @robobario's comment, but otherwise LGTM

Copy link
Member

@SamBarker SamBarker left a comment

Choose a reason for hiding this comment

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

I'm 100% happy with the gist of the proposal. My only objection as usual is the use of client and server, they should be downstream and upstream

import java.security.cert.X509Certificate;
import java.util.Optional;

public interface ServerTlsContext {
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering about changing server to broker here but then remembered we want to retain the idea we can proxy controllers as well so we are back to the usual upstream vs server argument.

I still think upstream is clearer and entirely unambiguous where as server is not.

import java.security.cert.X509Certificate;
import java.util.Optional;

public interface ClientTlsContext {
Copy link
Member

Choose a reason for hiding this comment

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

While I have fewer issues with calling this ClientTlsContext than I do with ServerTlsContext I think this is better named DownstreamTlsContext.

Comment on lines +96 to +99
/**
* @return the TLS server certificate was presented by the Kafka server to the proxy during TLS handshake.
*/
X509Certificate serverCertificate();
Copy link
Member

Choose a reason for hiding this comment

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

Upstream clearly conveys that it is the remote certificate and avoids needing to talk about Kafka server which is not normal parlance when discussing Kafka.

Suggested change
/**
* @return the TLS server certificate was presented by the Kafka server to the proxy during TLS handshake.
*/
X509Certificate serverCertificate();
/**
* @return the TLS server certificate was presented by the remote Kafka process to the proxy during TLS handshake.
*/
X509Certificate upstreamCertificate();

Signed-off-by: Tom Bentley <[email protected]>
@tombentley tombentley merged commit cf722ed into kroxylicious:main Aug 7, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants