fix: Internal Server Error for /healthcheck endpoint in RBAC-enabled #6482

spena · 2020-10-21T17:15:15Z

Description

Context:
There are endpoints in KsqlServerEndpoints that do not require a security context initialized. Some of these endpoints do not require authentication, so a security context should not be initialized because there is no Principal authenticated.

The current Vert.x code in KsqlServerEndpoints attempted to initialize the security context on endpoints where the user was not authenticated. This caused an error when calling these endpoints even with valid credentials. The fix was just to prevent initializing the security contexts, which is unnecessary.

Testing done

Added unit tests.
Verified manually with Confluent RBAC library.

User:ksql has valid credentials and has authorization to the server.
User:user1 has valid credentials but not authorization to the server.

Endpoints that required authentication only

curl   http://localhost:8088/info | jq .
{
  "@type": "generic_error",
  "error_code": 40100,
  "message": "Failed authentication"
}

curl -u user1:user1  http://localhost:8088/info | jq .
{
  "KsqlServerInfo": {
    "version": "6.1.0-0",
    "kafkaClusterId": "Bj_7MIRSQ36T97mtEimh-A",
    "ksqlServiceId": "default_",
    "serverStatus": "RUNNING"
  }
}

Endpoints that do not require authentication

 curl http://localhost:8088/v1/metadata/id 2>/dev/null | jq .
{
  "scope": {
    "path": [],
    "clusters": {
      "kafka-cluster": "Bj_7MIRSQ36T97mtEimh-A",
      "ksql-cluster": "default_"
    }
  },
  "id": ""
}

Endpoints that require authentication and authorization

$ curl http://localhost:8088/info 2>/dev/null | jq .
{
  "@type": "generic_error",
  "error_code": 40100,
  "message": "Failed authentication"
}

$ curl -u user1:user1 http://localhost:8088/status 2>/dev/null | jq .
{
  "@type": "generic_error",
  "error_code": 40300,
  "message": "You are forbidden from using this cluster."
}

$ curl -u ksql:ksql http://localhost:8088/status 2>/dev/null | jq .
{
  "commandStatuses": {
    "stream/`KSQL_PROCESSING_LOG`/create": "SUCCESS"
  }
}

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

Hey @spena , I don't think this is the right way to address this bug. This PR fixes the issue for the default unauthenticated paths, but it doesn't solve the problem more generally. For example, if a user chooses to make the /ksql endpoint unauthenticated (not sure why anyone would do that but bear with me 😆), then they would again hit the bug in the Github issue. I'd need to dig into the code more in order to figure out the proper way to fix this. It might involve changes around ApiSecurityContext. Ping me tomorrow if you want to brainstorm together offline?

vcrfxia · 2020-10-23T02:18:46Z

ksqldb-rest-app/src/test/java/io/confluent/ksql/api/AuthTest.java

@@ -539,7 +612,7 @@ private void shouldNotAllowAccessIfPermissionCheckThrowsException(
    void run() throws Exception;
  }

-  private static class StringPrincipal implements Principal {
+  public static class StringPrincipal implements Principal {


Why can't this still be private?

Left from another approach I was taking. Fixed.

vcrfxia · 2020-10-23T02:19:33Z

ksqldb-rest-app/src/main/java/io/confluent/ksql/api/auth/KsqlAuthorizationProviderHandler.java

@@ -32,7 +32,7 @@
 */
 public class KsqlAuthorizationProviderHandler implements Handler<RoutingContext> {

-  public static final Set<String> PATHS_WITHOUT_AUTHORIZATION = ImmutableSet
+  public static final Set<String> KSQL_AUTHENTICATION_SKIP_PATHS = ImmutableSet


Can we move this into AuthenticationPluginHandler if we're going to rename it? It looks out of place here.

vcrfxia · 2020-10-23T02:19:54Z

ksqldb-rest-app/src/test/java/io/confluent/ksql/api/AuthTest.java

+  protected HttpResponse<Buffer> sendGetRequest(final String uri) throws Exception {
+    return sendGetRequestWithCreds(client, uri, USER_WITH_ACCESS, USER_WITH_ACCESS_PWD);
+  }
+
  @Override
  protected HttpResponse<Buffer> sendRequest(final WebClient client, final String uri,


nit: can we rename this to sendPostRequest() in light of the new method above?

vcrfxia · 2020-10-23T02:20:05Z

ksqldb-rest-app/src/test/java/io/confluent/ksql/api/AuthTest.java

+    request.send(requestFuture);
+    return requestFuture.get();
+  }
+
  private HttpResponse<Buffer> sendRequestWithCreds(


nit: can we rename this to sendPostRequestWithCreds() in light of the new method above?

spena · 2020-10-23T16:17:49Z

@vcrfxia You're right. I was expecting that whenever an endpoint required a service context, then the authentication would require a user to create one. It's just lucky that we don't have an endpoint that requires a service context with no authentication.

I fixed the issue, and now I create a default service context when there is a missing user. I left a comment in the code about why there's no problem about it.

I added tests to the DefaultKsqlServiceContextProvider, I copied the tests we had for this class (https://github.com/confluentinc/ksql/blob/5.5.x/ksqldb-rest-app/src/test/java/io/confluent/ksql/rest/server/context/KsqlSecurityContextBinderFactoryTest.java), and added just one when the user missing.

I also noticed that the AuthTest just tests Authentication and Authorization. It does not test the path when building the user context. So I removed the unnecessary code, and rely now On the DefaultKsqlServiceContextProviderTest to return the right service context without failing.

I verified the endpoints work manually again.

vcrfxia

Thanks @spena -- LGTM with a suggested update to clarify the missing user case.

...db-rest-app/src/main/java/io/confluent/ksql/api/impl/DefaultKsqlSecurityContextProvider.java

Co-authored-by: Victoria Xia <victoria.f.xia281@gmail.com>

fix: Internal Server Error for /healthcheck endpoint in RBAC-enabled

13172b9

spena added bug P0 Denotes must-have for a given milestone labels Oct 21, 2020

spena added this to the 0.14.0 milestone Oct 21, 2020

spena requested a review from a team October 21, 2020 17:15

apurvam assigned spena Oct 21, 2020

test: add tests to verify paths without authentication

f49fbf2

spena force-pushed the fix_unauth_paths branch 2 times, most recently from 6403832 to 1202d41 Compare October 22, 2020 12:58

chore: remove unused method

807260d

spena force-pushed the fix_unauth_paths branch from 1202d41 to 807260d Compare October 22, 2020 17:26

vcrfxia reviewed Oct 23, 2020

View reviewed changes

spena added 2 commits October 23, 2020 09:37

refactor: rename sendRequest() to sendPostRequest()

3558b6b

refactor: move KSQL_AUTHENTICATINO_SKIPT_PATHS and other minor fixes

5ea80d5

fix: return a default ServiceContext when User is missing

553fc18

spena force-pushed the fix_unauth_paths branch from ffca842 to 553fc18 Compare October 23, 2020 20:13

vcrfxia approved these changes Oct 26, 2020

View reviewed changes

...db-rest-app/src/main/java/io/confluent/ksql/api/impl/DefaultKsqlSecurityContextProvider.java Outdated Show resolved Hide resolved

Update comment

50cbb75

Co-authored-by: Victoria Xia <victoria.f.xia281@gmail.com>

spena force-pushed the fix_unauth_paths branch from e93c035 to 50cbb75 Compare October 26, 2020 15:12

spena merged commit ebee5ec into confluentinc:6.0.x Oct 26, 2020

spena deleted the fix_unauth_paths branch October 26, 2020 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Internal Server Error for /healthcheck endpoint in RBAC-enabled #6482

fix: Internal Server Error for /healthcheck endpoint in RBAC-enabled #6482

spena commented Oct 21, 2020

vcrfxia left a comment

vcrfxia Oct 23, 2020

spena Oct 23, 2020

vcrfxia Oct 23, 2020

spena Oct 23, 2020

vcrfxia Oct 23, 2020

spena Oct 23, 2020

vcrfxia Oct 23, 2020

spena Oct 23, 2020

spena commented Oct 23, 2020

vcrfxia left a comment

fix: Internal Server Error for /healthcheck endpoint in RBAC-enabled #6482

fix: Internal Server Error for /healthcheck endpoint in RBAC-enabled #6482

Conversation

spena commented Oct 21, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spena commented Oct 23, 2020

vcrfxia left a comment

Choose a reason for hiding this comment