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 allowing Filters to make use of SASL.

@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 it. It would be good to keep this repo aligned with (or at least not behind) what's actually implemented.

Copy link
Member

@robobario robobario left a comment

Choose a reason for hiding this comment

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

LGTM with nit

/**
* Returns the SASL context for the client connection, or empty if the client
* has not successfully authenticated using SASL.
* Filters should use {@link #clientPrincipal()} in preference to this method, unless they require SASL-specific functionality.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the clientPrincipal reference should go from this doc, it's not defined here or in 004

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.

lgtm

Signed-off-by: Tom Bentley <[email protected]>
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 think the proposal is sensible but wonder if there is an alternative API thats worth exploring.

@tombentley tombentley merged commit 57b308e 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