Add Custom Auth Provider with support for gRPC, plus tests and exception by Davidding4718 · Pull Request #5578 · opensearch-project/data-prepper

Davidding4718 · 2025-04-02T15:34:23Z

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

Only affects gRPC sources (otel_traces, otel_metrics, otel_logs)
Does not apply to HTTP sources, as no HTTP decorator for the custom auth has been implemented

Testing

Added unit tests for GrpcCustomAuthenticationProvider
Tested token handling via grpcurl and WebClient in test cases
Verified behavior with and without valid/invalid tokens
Confirmed health check is restricted based on authentication configuration

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
New functionality has javadoc added
Commits are signed with a real name per the DCO

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.

…ion handling Signed-off-by: Siqi Ding <dingdd@amazon.com>

KarstenSchnitter

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.

KarstenSchnitter · 2025-04-02T18:22:20Z

...ommon/src/main/java/org/opensearch/dataprepper/plugins/CustomGrpcAuthenticationProvider.java

+)
+public class CustomGrpcAuthenticationProvider implements GrpcAuthenticationProvider {
+    private final String token;
+    private static final String AUTH_HEADER = "authentication";


Can you make the header name configurable?

Made the header name configurable via pipeline configuration

KarstenSchnitter · 2025-04-02T18:23:17Z

...ommon/src/main/java/org/opensearch/dataprepper/plugins/CustomGrpcAuthenticationProvider.java

+                    Metadata headers,
+                    ServerCallHandler<ReqT, RespT> next) {
+
+                String auth = headers.get(Metadata.Key.of("authentication", Metadata.ASCII_STRING_MARSHALLER));


Shouldn't this at least use the constant AUTH_HEADER?

You are right. I have made the change.

Not in this PR, yet.

Made the change

KarstenSchnitter · 2025-04-02T18:23:50Z

...ommon/src/main/java/org/opensearch/dataprepper/plugins/CustomGrpcAuthenticationProvider.java

+                if (auth == null || !auth.equals(token)) {
+                    call.close(Status.UNAUTHENTICATED.withDescription("Invalid token"), new Metadata());
+                    return new ServerCall.Listener<>() {};
+                }


What if I have a different kind of token, a JWT for example?

Thanks for the suggestion! For now, this implementation is meant for simple token validation. JWT support can be considered in future iterations.

KarstenSchnitter · 2025-04-02T18:26:31Z

...ommon/src/main/java/org/opensearch/dataprepper/plugins/CustomGrpcAuthenticationProvider.java

+            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"
+                );
+            }


Isn't this essentially the same code as ll. 49-54. Why are there differences? Can't this be a single implementation?

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.

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.

Thanks! I’ve refactored the duplicated logic into a private isValid(auth) method as suggested.

KarstenSchnitter · 2025-04-02T18:34:24Z

...ia-common/src/main/java/org/opensearch/dataprepper/CustomAuthenticationExceptionHandler.java

+import java.time.Duration;
+import java.util.concurrent.TimeoutException;
+
+public class CustomAuthenticationExceptionHandler implements GoogleGrpcExceptionHandlerFunction {


Why do we need an additional exception handler and where is this class actually used? It contains a lot of duplicated code, too.

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.

Davidding4718 · 2025-04-02T20:32:08Z

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.

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.

dlvenable · 2025-04-04T15:58:25Z

...ain/java/org/opensearch/dataprepper/armeria/authentication/CustomAuthenticationProvider.java

+import java.util.Optional;
+import java.util.function.Function;
+
+public interface CustomAuthenticationProvider {


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.

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?

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 <dingdd@amazon.com>

dlvenable · 2025-04-09T00:50:04Z

...java/org/opensearch/dataprepper/plugins/UnauthenticatedCustomGrpcAuthenticationProvider.java

+        name = GrpcAuthenticationProvider.UNAUTHENTICATED_PLUGIN_NAME,
+        pluginType = GrpcAuthenticationProvider.class
+)
+public class UnauthenticatedCustomGrpcAuthenticationProvider implements GrpcAuthenticationProvider {


We already have support for this here:

https://github.com/opensearch-project/data-prepper/blob/main/data-prepper-plugins/armeria-common/src/main/java/org/opensearch/dataprepper/plugins/UnauthenticatedGrpcAuthenticationProvider.java

Is there anything different about this plugin from that one?

Alternatively, should this be moved into src/test similar to the other plugins?

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 <dingdd@amazon.com>

…ion (opensearch-project#5578) Add Custom Auth Provider with support for gRPC, plus tests and exception handling Signed-off-by: Siqi Ding <109874435+Davidding4718@users.noreply.github.com>

Davidding4718 requested review from KarstenSchnitter, chenqi0805, dinujoh, dlvenable, engechas, graytaylor0, kkondaka, oeyh, san81, sb2k16 and srikanthjg as code owners April 2, 2025 15:34

Add Custom Auth Provider with support for gRPC, plus tests and except…

e918e31

…ion handling Signed-off-by: Siqi Ding <dingdd@amazon.com>

Davidding4718 force-pushed the common-server-builder-and-auth-module branch from c1db494 to e918e31 Compare April 2, 2025 16:09

KarstenSchnitter reviewed Apr 2, 2025

View reviewed changes

dlvenable requested changes Apr 4, 2025

View reviewed changes

Making change on code based on comments. Refactor token avalidation l…

be19db6

…ogic, rename test classes, and move test into a new package 'testcustomauth' Signed-off-by: Siqi Ding <dingdd@amazon.com>

Davidding4718 force-pushed the common-server-builder-and-auth-module branch from 1ef6fce to be19db6 Compare April 8, 2025 17:13

dlvenable requested changes Apr 9, 2025

View reviewed changes

Davidding4718 added 2 commits April 9, 2025 10:22

Move UnauthenticatedCustomGrpcAuthenticationProvider and test to src/…

741d81a

…test for test-only use Signed-off-by: Siqi Ding <dingdd@amazon.com>

Move UnauthenticatedCustomGrpcAuthenticationProvider and test to src/…

57232dd

…test for test-only use Signed-off-by: Siqi Ding <dingdd@amazon.com>

dlvenable approved these changes Apr 21, 2025

View reviewed changes

dlvenable merged commit ee3b447 into opensearch-project:common-server-builder-and-auth-module Apr 23, 2025
45 of 47 checks passed

Conversation

Davidding4718 commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Scope

Testing

Check List

Uh oh!

KarstenSchnitter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Davidding4718 commented Apr 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Davidding4718 commented Apr 2, 2025 •

edited

Loading