feat: make SchemaRegistry permission validations on KSQL requests #7773

spena · 2021-07-08T15:47:44Z

Description

KSQL has already validations for Kafka permissions when a new query, DDL or DML statement is executed by a user. This PR adds another validation for Schema Registry permissions.

The idea behind this PR is to let users know exactly what's causing a query or DDL operation to fail and also produce a more generic authorization error message similar to the one we have with Kafka. Currently, some statements show the SR authorization error returned by SR, but queries, for instance, never shown anything. Queries run and do not show any error and users always wonder why there's no data streamed (reason was a SR denied operation).

NOTE
This PR is based on an very old PR (#3323). It was easier to push a new PR with the updates. Also, the comments in that PR suggested we change the SR API to produce the authorization error we want, but that requires more work in SR and KSQL. Also, SR may change error messages in the future. The idea of this PR is to have our own authorization message error, for both SR and Kafka, and no depend from other services when we implement better authorization alternatives (i.e. check against RBAC instead of SR and Kafka).

To help the reviewer understand the workflow of validations.

Start on the KsqlAuthorizationValidatorImpl. This validator is called on every KSQL request to check a user has permissions to access a topic (and now a SR subject). It is also called by the DistributedExecutor before writing a command to the command topic to check if the KSQL server will have permissions to access the topic (and now a SR subject) later when the command is processed. If the user or the KSQL server do not have permissions, then it throws an authorization exception. No all perms checks are done in this class. Other perms are checked when calling the SR client directly, which should return the right authorization exception.
The new authorization exception for SR is called KsqlSchemaAuthorizationException that produces an error message with the denied operation. This is the message the users will see in the CLI. The message is similar to the KsqlTopicAuthorizationException.
The KsqlAuthorizationValidatorImpl calls the internal backed validation KsqlBackendAccessValidator, which now has a SR validation. The SR validation only supports READ operations. WRITE operations are not possible yet (need to figure out how). We might need to make changes in SR to have a new API to check permissions, but this may require a design doc for SR to add new REST API (will follow-up later). Other option is to check the permission directly in RBAC, but requires changes in the security plugins (will follow-up in another PR).
The KsqlCacheAccessValidator is also updated to cache SR permissions checks. If you ask why a cache?, this cache was implemented in the past to make pull queries perform better.
The rest of the changes are places where the SR client is called to fetch or register a schema. Those places just make sure to return the KsqlSchemaAuthorizationException in case the returned SR error is 403 or 404.

Testing done

Added some unit tests and verified in an environment with RBAC + SR.

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

jzaralim

lgtm, just one question

jzaralim · 2021-07-08T17:36:15Z

...src/main/java/io/confluent/ksql/schema/ksql/inference/SchemaRegistryTopicSchemaSupplier.java

@@ -196,8 +197,7 @@ private static SchemaResult notFound(final String topicName, final boolean isKey
            + "\t-> Use the REST API to list available subjects"
            + "\t" + DocumentationLinks.SR_REST_GETSUBJECTS_DOC_URL
            + System.lineSeparator()
-            + "- You do not have permissions to access the Schema Registry.Subject: " + subject
-            + System.lineSeparator()
+            + "- You do not have permissions to access the Schema Registry."


Is it possible to tell if the exception was due to an auth error? If so, would it be possible to return a separate exception for auth errors so that the error messages can have more specific information?

Not sure why this was added this way. I will look at it in a follow-up PR.

Will re-review after more permissions checks are added

guozhangwang

I did not find the class KsqlSchemaAuthorizationException, is it missing in this PR?

guozhangwang · 2021-07-21T19:13:30Z

ksqldb-engine/src/main/java/io/confluent/ksql/security/KsqlProvidedAccessValidator.java

+ * An implementation of {@link KsqlAccessValidator} that provides authorization checks
+ * from a external authorization provider.
+ */
+public class KsqlProvidedAccessValidator implements KsqlAccessValidator {


nit: why not just name it KsqlAccessValidatorImpl? KsqlProvidedAccessValidator sounds a bit weird.

There are 3 classes that extend from this:

KsqlProvidedAccessValidator KsqlCacheAccessValidator KsqlBackendAccessValidator

I used Provided... because it is a validator provided by the user on the ksql-server.properties. I wasn't sure if use this os KsqlExternalAccessValidator. Any ideas?

guozhangwang · 2021-07-21T19:16:29Z

ksqldb-engine/src/main/java/io/confluent/ksql/security/KsqlAuthorizationValidatorImpl.java

-    extractor.process(query, null);
-    for (String kafkaTopic : extractor.getSourceTopics()) {
-      accessValidator.checkAccess(securityContext, kafkaTopic, AclOperation.READ);
+    for (KsqlTopic ksqlTopic : extractQueryTopics(query, metaStore)) {


This is not a comment but a question for my own understanding: it seems KsqlAuthorizationValidator is wrapping a KsqlAccessValidator here, can we just merge these two interfaces? I guess that's boiled down to:

What are the conceptual differences of these two?

Would KsqlAccessValidator ever need to be leveraged directly without the KsqlAuthorizationValidator?

KsqlAuthorizationValidator validates the user can execute a specified statement. This class gets information about that, such as topics and schemas; and then validates each resource based on the type of statement (query, create, insert, ...).

Initially, the validation was part of this KsqlAuthorizationValidator, but I split it when I introduced an authorization cache, which lives in KsqlCacheAccessValidator. The cache wraps another validation if the cache is enabled. The current validation, besides caching, is the KsqlBackendAccessValidator, which just checks with the backend service (i.e. Kafka) to check for ACLs. The new one is KsqlProvidedAccessValidator, which checks with an external service, like RBAC. SR permissions will be checked by the KSQL confluent security plugins which can either use RBAC or call the SR /permissions endpoint to check for ACLs (both part of the confluent security plugins).

spena · 2021-07-22T19:40:22Z

@guozhangwang The KsqlSchemaAuthorizationException is part of the KSQL code already. I split this PR into another that added that. Sorry I didn't update the PR description.

jzaralim · 2021-07-25T02:29:35Z

ksqldb-engine/src/main/java/io/confluent/ksql/security/KsqlAuthorizationValidatorFactory.java

+  ) {
+    if (externalAuthorizationProvider.isPresent()) {
+      return Optional.of(new KsqlProvidedAccessValidator(externalAuthorizationProvider.get()));
+    } else if (isTopicAccessValidatorEnabled(ksqlConfig, serviceContext)) {


Should we also have a config for enabling SR auth?

…uthorizationException (confluentinc#7783)

spena requested a review from a team July 8, 2021 15:47

spena mentioned this pull request Jul 8, 2021

feat: Perform Schema Registry permissions checks #3323

Closed

2 tasks

jzaralim previously approved these changes Jul 8, 2021

View reviewed changes

spena mentioned this pull request Jul 12, 2021

refactor: wrap SchemaRegistry authorization errors into a KsqlSchemaAuthorizationException #7783

Merged

2 tasks

spena force-pushed the sr_perm_checks branch 2 times, most recently from 8f0d6de to aac0500 Compare July 21, 2021 18:26

guozhangwang reviewed Jul 21, 2021

View reviewed changes

spena force-pushed the sr_perm_checks branch from aac0500 to 2ccacfe Compare July 22, 2021 19:49

jzaralim approved these changes Jul 25, 2021

View reviewed changes

spena force-pushed the sr_perm_checks branch from 2ccacfe to c2cb35c Compare July 26, 2021 14:45

spena and others added 2 commits July 26, 2021 14:54

refactor: wrap SchemaRegistry authorization errors into a KsqlSchemaA…

818e0d3

…uthorizationException (confluentinc#7783)

in progress

0e222e1

spena force-pushed the sr_perm_checks branch from c2cb35c to 0e222e1 Compare July 26, 2021 19:55

spena merged commit ad01b72 into confluentinc:master Jul 27, 2021

spena deleted the sr_perm_checks branch July 27, 2021 19:12

spena restored the sr_perm_checks branch July 27, 2021 19:12

spena deleted the sr_perm_checks branch July 27, 2021 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make SchemaRegistry permission validations on KSQL requests #7773

feat: make SchemaRegistry permission validations on KSQL requests #7773

spena commented Jul 8, 2021

jzaralim left a comment

jzaralim Jul 8, 2021

spena Jul 12, 2021

guozhangwang left a comment

guozhangwang Jul 21, 2021

spena Jul 22, 2021

guozhangwang Jul 21, 2021

spena Jul 22, 2021

spena commented Jul 22, 2021

jzaralim Jul 25, 2021

feat: make SchemaRegistry permission validations on KSQL requests #7773

feat: make SchemaRegistry permission validations on KSQL requests #7773

Conversation

spena commented Jul 8, 2021

Description

Testing done

Reviewer checklist

jzaralim left a comment

Choose a reason for hiding this comment

jzaralim Jul 8, 2021

Choose a reason for hiding this comment

spena Jul 12, 2021

Choose a reason for hiding this comment

guozhangwang left a comment

Choose a reason for hiding this comment

guozhangwang Jul 21, 2021

Choose a reason for hiding this comment

spena Jul 22, 2021

Choose a reason for hiding this comment

guozhangwang Jul 21, 2021

Choose a reason for hiding this comment

spena Jul 22, 2021

Choose a reason for hiding this comment

spena commented Jul 22, 2021

jzaralim Jul 25, 2021

Choose a reason for hiding this comment