-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OauthBearer validation filter #1195
OauthBearer validation filter #1195
Conversation
3c099bf
to
ae95b4a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like great progress, thank you!
@@ -742,7 +747,7 @@ else if (offsetFetchRequestData.topics() != null) { | |||
.build(apiVersion); | |||
break; | |||
case UPDATE_METADATA: | |||
req = new UpdateFeaturesRequest((UpdateFeaturesRequestData) reqBody, apiVersion); | |||
req = new UpdateFeaturesRequest((UpdateFeaturesRequestData) reqBody, apiVersion); // TODO: check why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean, "why it's not using the builder", I'm not sure there is a reason. Happy to switch to using the builder if it doesn't fail the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first read it, I didn't understand why the UPDATE_METADATA
returns an UpdateFeaturesRequest
rather than an UpdateMetadataRequest
? Is it voluntary ?
5b2c6fe
to
24f7a35
Compare
...ylicious-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearer.java
Outdated
Show resolved
Hide resolved
...ylicious-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearer.java
Outdated
Show resolved
Hide resolved
...ious-oauthbearer/src/test/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerTest.java
Outdated
Show resolved
Hide resolved
...ylicious-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearer.java
Outdated
Show resolved
Hide resolved
...us-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerFilter.java
Outdated
Show resolved
Hide resolved
...us-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerFilter.java
Outdated
Show resolved
Hide resolved
...ious-oauthbearer/src/test/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerTest.java
Outdated
Show resolved
Hide resolved
This looks really useful to me. I know in a past life, I'd have loved to been able to protect my Kafka Cluster from clients hammering it with duff tokens. |
...us-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerFilter.java
Outdated
Show resolved
Hide resolved
...ylicious-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearer.java
Outdated
Show resolved
Hide resolved
9ff79d4
to
2cfe932
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @callaertanthony, I took a closer look and left a few comments. I don't think this is very far from being mergeable.
...us-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerFilter.java
Outdated
Show resolved
Hide resolved
...icious-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/SharedContext.java
Outdated
Show resolved
Hide resolved
<id>analyze</id> | ||
<configuration> | ||
<ignoredUnusedDeclaredDependencies> | ||
<ignoredUnusedDeclaredDependency>org.bitbucket.b_c:jose4j</ignoredUnusedDeclaredDependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain how this is actually being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency is used by kafka on the token validation, but it is not found by kroxylicious on classpath. We must add it as dependency in the project but we don't directly use it. It's used by the kafka OAuthBearerValidatorCallbackHandler
...us-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerFilter.java
Outdated
Show resolved
Hide resolved
...us-oauthbearer/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerFilter.java
Outdated
Show resolved
Hide resolved
2cfe932
to
c3047aa
Compare
...validation/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerValidation.java
Outdated
Show resolved
Hide resolved
...tion/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerValidationFilter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment left. There's also one Sonar issue. But otherwise code lgtm.
Can you add an entry into the CHANGLOG?
ab80548
to
651f569
Compare
651f569
to
18192c7
Compare
...tion/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerValidationFilter.java
Outdated
Show resolved
Hide resolved
I ran up a test case using auth0.com as the oauth server. I notice a problem, the token validation is failing (it is an audience issue but the precise detail of the error is unimportant) but the filter is still relaying the SaslAuthenticationRequest to the broker. I'm seeing this is the logs:
examing the stack trace I see that OAuthBearerSaslServer.process is going doing the token == null path, which causes a error response to be returned to the caller. The filter isn't treating this an an exception, so the Sasl Auth Request is being fowarded. You see a warn written to Kroxylicious's logs, but the connection between client/broker is still setup. |
To work with auth0, I found I need to pass through the SASL_OAUTHBEARER_EXPECTED_AUDIENCE as a config parameter. With that in place, the happy path was good for my test case. I then tried deliberately poisoning the token presented by the kafka client I hit the same problem as above, the Sasl Auth Request is being fowarded despite the duff token. I have a rough fix which I share with you as a PR (no tests, no docs for the new property). https://github.com/callaertanthony/kroxylicious/pull/1/files |
thanks @k-wall , I added tests, doc and the issuer property |
ad3a3ca
to
f50f3bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @callaertanthony, I found a few more minor things worth discussing.
...tion/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerValidationFilter.java
Show resolved
Hide resolved
return operation.get(); | ||
} | ||
CompletableFuture<A> future = new CompletableFuture<>(); | ||
executorService.schedule(() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By discarding the returned ScheduledFuture
you're basically allowing the client to send very many SaslAuthenticateRequests
, each with its own scheduled completion. Other clients are sharing the same event loop (which is what the executorService
here actually is), so with enough scheduled events you might get an obserable noisy neighbour effect.
We should probably hold the last scheduled operation's ScheduledFuture
in a field. That would allow us to implement a one-scheduler-slot-per-client policy by cancelling the outstanding operation. We'd need to decide what the right thing to do with the two in-flight requests is. I guess respond to the first with some error code, and allow the second to be scheduled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we cancel the first request, a response will be sent to the client no ? How this behavior will prevent the client to start new requests ? We can effectively limit the number of scheduled operations to not , but I don't see how this helps to prevent too many requests ?
Maybe we can refuse new requests from client when another one is already scheduled to fast short circuit ?
...tion/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerValidationFilter.java
Outdated
Show resolved
Hide resolved
...tion/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerValidationFilter.java
Outdated
Show resolved
Hide resolved
...tion/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerValidationFilter.java
Show resolved
Hide resolved
...tion/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerValidationFilter.java
Outdated
Show resolved
Hide resolved
2aab4f6
to
684af8d
Compare
...tion/src/main/java/io/kroxylicious/proxy/filter/oauthbearer/OauthBearerValidationFilter.java
Outdated
Show resolved
Hide resolved
@callaertanthony I made a couple of suggestions in callaertanthony#2 |
e8bb1b3
to
0a8c7f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. @tombentley would you be able to re-review? I think this is good to merge.
thank you for your help |
0a8c7f9
to
e5ed5e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @callaertanthony!
e5ed5e2
to
b1b9845
Compare
Quality Gate passedIssues Measures |
Type of change
Description
This pull request has the intention to introduce OauthBearer management into Kroxylicious.
Two ideas for this :
OauthBearerFilter
to check if a token is valid before sending it to upstream.AuthnHandler
and addOAUTHBEARER
.Additional Context
OauthBearerFilter
is simple and really useful for a Kafka proxy, it helps relieve the upstream kafka when downstream tried too many invalid authentications.If token is valid, it just forward the SASL request to upstream, letting upstream kafka do it again and self manage RBAC.It requires to have RBAC management I think, otherwise, the use case is just to proxy
OAUTHBEARER
toANY
connection, loosing theOAUTHBEARER
principal user and using another one from the upstream connection (or no one if no authentication configured.AuthnHandler
is more tricky, at this time, it doesn't give the possibility to handle differentOAUTHBEARER
configs byVirtualCluster
, so I think a refactor is required on this part.Edit : I moved the SASL connection part to another PR : handle OAUTHBEARER SASL connection #1206
Checklist
Please go through this checklist and make sure all applicable tasks have been done