-
Notifications
You must be signed in to change notification settings - Fork 224
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
Add Custom Auth Provider with support for gRPC, plus tests and exception #5578
base: common-server-builder-and-auth-module
Are you sure you want to change the base?
Conversation
…ion handling Signed-off-by: Siqi Ding <[email protected]>
c1db494
to
e918e31
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 for providing this change. I am trying to understand, how this change relates to the gRPC authentication documentation. Can you explain this a little bit. IMHO the name CustomAuthentication
is too generic. It should be more specific on a fixed token authentication.
) | ||
public class CustomGrpcAuthenticationProvider implements GrpcAuthenticationProvider { | ||
private final String token; | ||
private static final String AUTH_HEADER = "authentication"; |
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 make the header name configurable?
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.
Made the header name configurable via pipeline configuration
Metadata headers, | ||
ServerCallHandler<ReqT, RespT> next) { | ||
|
||
String auth = headers.get(Metadata.Key.of("authentication", Metadata.ASCII_STRING_MARSHALLER)); |
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.
Shouldn't this at least use the constant AUTH_HEADER
?
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.
You are right. I have made the change.
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.
Not in this PR, yet.
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.
Made the change
if (auth == null || !auth.equals(token)) { | ||
call.close(Status.UNAUTHENTICATED.withDescription("Invalid token"), new Metadata()); | ||
return new ServerCall.Listener<>() {}; | ||
} |
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.
What if I have a different kind of token, a JWT for example?
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 for the suggestion! For now, this implementation is meant for simple token validation. JWT support can be considered in future iterations.
final String auth = req.headers().get(AUTH_HEADER); | ||
if (auth == null || !auth.equals(token)) { | ||
return HttpResponse.of( | ||
HttpStatus.UNAUTHORIZED, | ||
MediaType.PLAIN_TEXT_UTF_8, | ||
"Unauthorized: Invalid or missing token" | ||
); | ||
} |
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.
Isn't this essentially the same code as ll. 49-54. Why are there differences? Can't this be a single implementation?
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.
Thank you for pointing out. These two similar logic: one for gRPC headers using Armeria -> Metadata; another one for HTTP headers using Armeria's HttpRequest. Both perform Header extraction, Token validation, and Returning unauthorized response if token mismatches. But the APIs are different.
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 for pointing this out. I recommend extracting the condition (auth == null || !auth.equals(token))
into a private method isValid(auth)
or similar. After all, this is the main logic of the class and is currently duplicated.
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! I’ve refactored the duplicated logic into a private isValid(auth) method as suggested.
import java.time.Duration; | ||
import java.util.concurrent.TimeoutException; | ||
|
||
public class CustomAuthenticationExceptionHandler implements GoogleGrpcExceptionHandlerFunction { |
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.
Why do we need an additional exception handler and where is this class actually used? It contains a lot of duplicated code, too.
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 for the feedback! That exception handler was only used in one test. Since it's no longer needed, I’ve safely removed it to avoid duplication and keep the codebase clean.
Thank you for the feedback! You're right, the current name CustomAuthentication is too broad. The purpose of this plugin is to simulate SigV4-style custom authorization by using a fixed token passed in the gRPC metadata header. This allows us to test custom gRPC authentication flows locally without depending on the actual SigV4 implementation (which requires AWS deployment). I can rename it to something more descriptive like FixedTokenGrpcAuthenticationProvider to make its purpose clearer. Let me know what you think. |
import java.util.Optional; | ||
import java.util.function.Function; | ||
|
||
public interface CustomAuthenticationProvider { |
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.
I believe this is intended to be a test class, right?
Move this into the src/test/
directory and rename it to something like TestCustomAuthenticationProvider
.
Do the same with the other classes you created in src/main
.
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.
Quick question: implementing CustomGrpcAuthenticationProvider is to allow Data Prepper to support token-based custom authentication in production, as a simulation of SigV4. If we more classes into src/test. can we still able to load it during integration tests or real pipelines?
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.
The difference between src/main
and src/test
is what is on the classpath during production and during integration tests.
The Data Prepper plugin framework just needs the plugin to be in the Java classpath and in the specified Java package. Moving these to src/test
will allow you to use them in integration testing. But, you cannot run them in a real Data Prepper pipeline. I think this is what we generally want. Now, there could be value in running them for testing purposes. But, I'd think we want to isolate them somehow before doing that. We could do that by using a dedicated Java package.
I'd say for now we just support running these in integration tests.
…ogic, rename test classes, and move test into a new package 'testcustomauth' Signed-off-by: Siqi Ding <[email protected]>
1ef6fce
to
be19db6
Compare
name = GrpcAuthenticationProvider.UNAUTHENTICATED_PLUGIN_NAME, | ||
pluginType = GrpcAuthenticationProvider.class | ||
) | ||
public class UnauthenticatedCustomGrpcAuthenticationProvider implements GrpcAuthenticationProvider { |
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.
We already have support for this here:
Is there anything different about this plugin from that one?
Alternatively, should this be moved into src/test
similar to the other plugins?
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.
You're right. The purpose of writing UnauthenticatedCustomGrpcAuthenticationProvider
was to simulate the behavior of UnauthenticatedGrpcAuthenticationProvider
, specifically for testing custom authentication scenarios.
It overlaps with the existing plugin and isn't needed in production code.
Moving this version and its tests into src/test
.
…test for test-only use Signed-off-by: Siqi Ding <[email protected]>
…test for test-only use Signed-off-by: Siqi Ding <[email protected]>
Description
This PR introduces a new gRPC authentication provider, GrpcCustomAuthenticationProvider, which uses a token-based authentication mechanism. It is intended to simulate the behavior of SigV4.
Issues Resolved
Related to #5423
Scope
Testing
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.