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.

Sign up for GitHub

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

Jump to bottom

fix: also skip basic authentication for authentication.skip.paths #9669

Merged

lucasbru merged 4 commits into confluentinc:master from lucasbru:fix_skippaths

Nov 2, 2022

Member

lucasbru commented Oct 26, 2022

Description

There is the KSQL parameter authentication.skip.paths that disables authentication for specific paths. Currently, the parameter is ignored for basic authentication, which is probably a bug or at least very confusing to users.

This change moves the handling of authentication.skip.paths from specific auth handlers to the AuthHandlers class, which allows us to skip any authentication/authorization for paths that match authentication.skip.paths.

This change introduces a new Provider called SKIP, that works very similar to the existing SYSTEM Provider - it means that all other authentication / authorization provides. It is enabled for authentication.skip.paths and for 3 hard-coded paths. We also slightly simplify the handling of the SYSTEM authorization to bring it in line with SKIP.

Testing done

We port the existing, provider-specic unit tests to AuthHandlers.

I also performed manual testing with the settings

authentication.skip.paths=/skippath
authentication.method=BASIC
authentication.realm=KsqlServer-Props
authentication.roles=users

and the same JAAS configuration in the ticket #9206
and checking

curl localhost:8088/skippath # not found
curl localhost:8088/v1/metadata # works
curl localhost:8088/noskippath # unauthorized
curl -u "my_user:my_password" localhost:8088/skippath # not found
curl -u "my_user:my_password" localhost:8088/v1/metadata # works
curl -u "my_user:my_password" localhost:8088/noskippath # not found

Fixes #9206

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")


          fix: also skip basic authentication for authentication.skip.paths

a1e25d7

lucasbru requested a review from a team as a code owner

October 26, 2022 11:50

lucasbru self-assigned this

lucasbru requested a review from spena

October 26, 2022 11:52

lucasbru commented

View reviewed changes

...db-rest-app/src/test/java/io/confluent/ksql/rest/integration/PauseResumeIntegrationTest.java

@@ @@ -236,7 +236,6 @@ private void createQuery(final String suffix) { @@
                   TEST_HARNESS.getKafkaCluster().waitForTopicsToBePresent(SINK_TOPIC + suffix);
                 }
-                @NotNull

Member Author

lucasbru Oct 26, 2022

For some reason, spotbugs kept thinking this can return null. Looks to me like a false alarm, not why I'm getting it.

Member Author

lucasbru commented Oct 26, 2022

@spena and/or @AlanConfluent could you maybe have a look?

lucasbru requested a review from AlanConfluent

October 26, 2022 11:58

AlanConfluent reviewed

View reviewed changes

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/server/AuthHandlers.java

-                                new KsqlAuthorizationProviderHandler(server, ksqlAuthorizationProvider)));
+                                selectiveHandler(
+                                        new KsqlAuthorizationProviderHandler(server, ksqlAuthorizationProvider),
+                                        provider -> provider == Provider.JAAS || provider == Provider.PLUGIN

Member

AlanConfluent Oct 31, 2022

Did we just implicitly only fall through to here if it was jaas or plugin and now you're just formally checking that?

Member Author

lucasbru Nov 1, 2022

Yes, mostly. For SYSTEM there was a check inside KsqlAuthorizationProviderHandler that skipped authorization. The other two options were JAAS and PLUGIN, otherwise the selectHandler fails.

I moved those checks here to at least have the selection logic in one file and not spread across the various concrete implementations.

Member

AlanConfluent Nov 1, 2022

That makes sense. It's better to have it centralized here.

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/server/AuthHandlers.java

    
                ) {

                  return rc -> {

                    if (rc.data().get(PROVIDER_KEY) != provider) {

                    if (!providerTest.test((Provider)rc.data().get(PROVIDER_KEY))) {

Member

AlanConfluent Oct 31, 2022

I know you didn't change this part of the logic, but this looks slightly scary that it skips this handler if the particular provider is different from what we expect. It just seems a little error prone that we decide which auth to use and invoke a handler in two different places. Maybe this is because we have to statically install all the handers, but only invoke one of them at request-time? Just a general comment -- not sure if you have any ideas.

Member Author

lucasbru Nov 1, 2022

Yes, I totally agree. This could be a vertx-ish architectural style, but I'm not sure if it is achieving anything in our setting. It should be possible to have a single wrapping handler to call the sub-handlers -- essentially, merge selectHandler and selectiveHandler into one, so that you can use plain old control-flow for handler selection instead of passing around PROVIDER values. I didn't want to make this too large of a refactoring, but if you want to go down this road with me, I could try.

Member

AlanConfluent Nov 1, 2022

No need for anything in this PR, more just thoughts while reading surrounding code. We can consider that as a followup if you do more changes in this area.

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/server/AuthHandlers.java Outdated

@@ @@ -130,6 +150,11 @@ private static void selectHandler( @@
                     context.next();
                     return;
                   }
+                  if (skipPathsPattern.matcher(context.normalizedPath()).matches()) {

Member

AlanConfluent Oct 31, 2022

Nit: Maybe put this first to match the previous order of checks in the plugin. Shouldn't make any difference, but just to be safe.

Member Author

lucasbru Nov 1, 2022

done

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/server/AuthHandlers.java

		@@ -58,28 +68,36 @@ static void setupAuthHandlers(final Server server, final Router router,
		systemAuthenticationHandler.ifPresent(handler -> registerAuthHandler(router, handler));

Member

AlanConfluent Oct 31, 2022

I don't think it should matter whether they're considered system authenticated before you check the path because it only has the effect of skipping any subsequent checks if they are.

Member Author

lucasbru Nov 1, 2022

ack

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/server/AuthHandlers.java Outdated

@@ @@ -120,8 +138,10 @@ private static Handler<RoutingContext> wrappedHandler( @@
                  * In order of preference, we see if the user is the system user, then we check for
                  * JAAS configurations, and finally we check to see if there's a plugin we can use
                  */
-                private static void selectHandler(
+                // default visibility for testing

Member

AlanConfluent Oct 31, 2022

@VisibleForTesting

Member Author

lucasbru Nov 1, 2022

neat! done

lucasbru added 2 commits

November 1, 2022 09:40


          revise: Alans comments

909f8d1


          Merge remote-tracking branch 'upstream/master' into fix_skippaths

071a853

AlanConfluent approved these changes

View reviewed changes

spena approved these changes

View reviewed changes

Member

spena left a comment

Just one minor comment.
LGTM

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/server/AuthHandlers.java Outdated

		@@ -120,11 +139,18 @@ private static Handler<RoutingContext> wrappedHandler(
		* In order of preference, we see if the user is the system user, then we check for

Member

spena Nov 1, 2022

Can you modify this comment for the order of preference? Btw, wouldn't check for SYSTEM be the step 1, then check for SKIP path?

Member Author

lucasbru Nov 2, 2022

Yeah, see comment from Alan above. I would think SYSTEM goes first as well, but the original order in the code is SKIP first, and since it shouldn't really make a difference, the idea is to not risk breaking some existing behavior unknowingly.


          revise: Sergios comments

fb01cab

lucasbru merged commit 05c7123 into confluentinc:master

lucasbru deleted the fix_skippaths branch

November 2, 2022 11:13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet