feat: Add REST and Websocket authorization hooks and interface #3000

spena · 2019-06-21T14:19:28Z

Description

Add hooks and an interface to provide authorization to KSQL REST and Websocket endpoints. KSQL does not provide any authorization provider by default. An external authorizer provider (returned by the KSQL security extension) is used when checking for endpoint access.

How to review?

Look first at the KsqlAuthorizationFilter. This filter is called every time a user wants to access a REST endpoint. The filter does not do any check. It is up to the authorization provider to validate the request parameters. The WSQueryEndpodpoint is the Websocket part that calls the authorization provider.
Then look at the KsqlSecurityExtension and KsqlAuthorizationProvider interfaces. Easy to understand. These will be the interfaces to implement by external authorization applications.
Go to KsqlRestApplication to see how the KsqlAuthorizationFilter is registered, and the security extension is initialized.
Finally, look at the AuthorizationFunctionalTest which has a couple of validation tests that allow and deny access to a KSQL request. I plan to add more tests in a PR for impersonation.

Notes

I plan to add better user context impersonation hooks in a follow-up PR.
I plan to remove the KsqlDefaultSecurityExtension and make it Optional

Testing done

Added united tests
Added integration tests on AuthorizationFunctionalTest
Run mvn test

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 #")

vcrfxia

Thanks @spena ! LGTM minus my comment about the new integration test inline.

vcrfxia · 2019-06-22T00:17:07Z

...est-app/src/main/java/io/confluent/ksql/rest/server/resources/streaming/WSQueryEndpoint.java

+          try {
+            provider.checkEndpointAccess(user, method, path);
+          } catch (final Throwable t) {
+            log.warn(String.format("User:%s is denied access \"%s %s\"", user, method, path), t);


Will this error message be confusing to users of the websocket endpoint? I'm no websocket expert but I think talking about POST-ing to a websocket endpoint is strange.

Right. I changed it.

vcrfxia · 2019-06-22T00:24:19Z

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

+    );
+
+    // When:
+    makeKsqlRequest(VALID_USER1, "SHOW TOPICS;");


nit: can we rename VALID_USER1 to simply USER1? When reading this test I was expecting the request to succeed at first. Plus there are no invalid users in this test.

Changed it.

vcrfxia · 2019-06-22T00:28:08Z

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

+  }
+
+  private static void assertSuccessful(final List<KsqlEntity> results) {
+    results.stream()


Is this check doing anything? I don't think SHOW TOPICS; returns a CommandStatusEntity. By having the check filter for CommandStatusEntities and only perform checks on the results that match, the test could pass without performing any checks. It would be better to directly assert what we expect about the results, for example, that results.size() is 1, and that one entity is of type KafkaTopicsList, etc.

I copied this method from another integration test. I didn't notice the results weren't correct.
I changed it to verify a topic was returned.

vcrfxia

Thanks @spena -- LGTM!

vcrfxia · 2019-06-24T15:37:08Z

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


    // Then:
-    assertSuccessful(results);
+    final List<KafkaTopicInfo> topics = ((KafkaTopicsList)results.get(0)).getTopics();


To be extra thorough we could also assert that results.size() is 1, but it's not a huge deal.

rodesai

Thanks @spena, left some comments inline

rodesai · 2019-06-25T04:23:24Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/KsqlRestApplication.java

+        ac -> config.register(new KsqlAuthorizationFilter(ac))
+    );
+
+    // Registers any other security filters (i.e. user context impersonation)


nit: this comment isn't really helpful, and is a bit confusing (since the impersonation stuff is really a detail of the plugin)

Right. I was actually going to remove this call in another PR. My plan is to wrap the impersonation implementation into a call like extension.getUserContext(principal), and make that call from the REST and WS endpoints to get the Kafka and SR clients for that user.

This PR is only authorization so I didn't want to add more functionality.

rodesai · 2019-06-25T04:39:54Z

...rest-app/src/main/java/io/confluent/ksql/rest/server/security/KsqlAuthorizationProvider.java

+   * @param path The endpoint path to access, i.e. "/ksql", "/ksql/terminate", "/query"
+   * @throws KsqlException for access denied or any other authorization error
+   */
+  void checkEndpointAccess(Principal user, String method, String path) throws KsqlException;


I think access denied errors merit a more specific exception type.

I can use the Kafka AuthorizationException for it, what do you think? or should I create our own KsqlAuthorizationException?

Also, I want to let users know that other exceptions can be thrown for other errors, perhaps having both Authorization and KsqlException? I don't want KsqlException because I don't want to link the error to a KSQL error. The exception thrown should be the external library exception.

rodesai · 2019-06-25T04:44:31Z

Not exactly specific to this PR, but we should add some a docs page about how to write a security extension. It's not obvious for example how the impersonation credentials get passed around.

spena · 2019-06-25T17:26:51Z

Not exactly specific to this PR, but we should add some a docs page about how to write a security extension. It's not obvious for example how the impersonation credentials get passed around.

I will do it in another PR once I complete the hooks for impersonation as well.

spena added the security label Jun 21, 2019

spena added this to the 5.4 milestone Jun 21, 2019

spena requested a review from a team June 21, 2019 14:19

spena self-assigned this Jun 21, 2019

vcrfxia reviewed Jun 22, 2019

View reviewed changes

vcrfxia requested a review from a team June 22, 2019 00:38

spena force-pushed the ksql_authorization branch from 1b002a3 to 346b9f0 Compare June 24, 2019 13:21

vcrfxia approved these changes Jun 24, 2019

View reviewed changes

vcrfxia requested a review from a team June 24, 2019 15:37

spena force-pushed the ksql_authorization branch 2 times, most recently from c5cb2d3 to 7ff9dff Compare June 24, 2019 21:29

spena changed the title ~~Add REST and Websocket authorization hooks and interface~~ feat: Add REST and Websocket authorization hooks and interface Jun 25, 2019

rodesai reviewed Jun 25, 2019

View reviewed changes

spena force-pushed the ksql_authorization branch from 9f7c1a3 to 1d4fe5e Compare June 26, 2019 20:19

spena added 5 commits June 27, 2019 08:50

Add authorization interface and hooks to REST and WS endpoints

1ad549a

Add authorization integration tests

d870469

Fix tests

4cb522c

Address Victoria's feedback

b67f1c7

Remove KsqlException from KsqlAuthorizationProvider interface

8944526

spena force-pushed the ksql_authorization branch from 1d4fe5e to 8944526 Compare June 27, 2019 13:50

spena added 2 commits June 27, 2019 11:05

refactor: authorization functional tests

407155a

test: remove test case causing spotbugs failures

3616535

spena merged commit 39af991 into confluentinc:master Jun 27, 2019

spena deleted the ksql_authorization branch June 27, 2019 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add REST and Websocket authorization hooks and interface #3000

feat: Add REST and Websocket authorization hooks and interface #3000

spena commented Jun 21, 2019

vcrfxia left a comment

vcrfxia Jun 22, 2019

spena Jun 24, 2019

vcrfxia Jun 22, 2019

spena Jun 24, 2019

vcrfxia Jun 22, 2019

spena Jun 24, 2019

vcrfxia left a comment

vcrfxia Jun 24, 2019

rodesai left a comment

rodesai Jun 25, 2019

spena Jun 25, 2019

rodesai Jun 25, 2019

spena Jun 25, 2019 •

edited

rodesai commented Jun 25, 2019

spena commented Jun 25, 2019

feat: Add REST and Websocket authorization hooks and interface #3000

feat: Add REST and Websocket authorization hooks and interface #3000

Conversation

spena commented Jun 21, 2019

Description

Testing done

Reviewer checklist

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodesai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spena Jun 25, 2019 • edited

Choose a reason for hiding this comment

rodesai commented Jun 25, 2019

spena commented Jun 25, 2019

spena Jun 25, 2019 •

edited