feat: Perform Schema Registry permissions checks #3323

spena · 2019-09-10T19:55:08Z

Description

This change makes KSQL perform Schema access permissions checks on the statement to execute in order to provide better authorization error messages when the User and/or the KSQL service principal do not have access to the Schema.

First, when SR is configured with Authorization, the following invalid error was displayed on KSQL when it cannot access the schema which was not helping users to recognize it as an access denied aerror:

io.confluent.kafka.schemaregistry.client.rest.exceptions.RestClientException: Unrecognized token 'User': was expecting ('true', 'false' or 'null')
 at [Source: (sun.net.www.protocol.http.HttpURLConnection$HttpInputStream); line: 1, column: 6]; error code: 50005

This PR changes the message to like:

ksql> create stream s10 with (kafka_topic='s10', value_format='avro');
Authorization denied to Read on Schema Registry subject(s): [s10-value]

It also perform the check for the KSQL service principal, and the error will look like:

ksql> create stream s10 with (kafka_topic='s10', value_format='avro');
The KSQL server is not permitted to execute the command
Caused by: Authorization denied to Read on Schema Registry subject(s):
	[s10-value]

SR checks are done in KsqlAuthorizationValidatorImpl (where Topic permissions checks are done). This class checks access to the schema of some statements that use AVRO topics only.

How to review it?

Commit DO NOT MERGE! This is just for review purposes only #1 added a KsqlSchemaAuthorizationException which will be used throw the desired error message.
Commit Add basic .gitignore #2 calls the KsqlSchemaAuthorizationException on places where SR is denying access + when doing permissions checks on KsqlAuthorizationValidatorImpl

Testing done

Unit tests
Verified manually on CLI

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

spena · 2019-09-10T19:56:19Z

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

@@ -137,8 +137,7 @@ private static SchemaResult notFound(final String topicName) {
            + "\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: "
-            + topicName + KsqlConstants.SCHEMA_REGISTRY_VALUE_SUFFIX


Removed this because the subject name is already displayed on the full message.

big-andy-coates

Hey @spena

The change makes sense. I've noted a few nits.

However, this change sees similar logic peppered through any use of the SchemaRegistryClient in the code base. Any new use of the client will also need to have the same exception conversion logic, which people are likely to forget.

Would a better approach not implement our own SchemaRegistryClient that internally handles the exception conversion, and ensure we wrap the true client in ours. Such wrapping should be done in one place, e.g. SchemaRegistryClientFactory.

This approach would tidy up the code, removing the duplication and would be more future proof

What do you think?

big-andy-coates · 2019-09-11T16:25:46Z

ksql-engine/src/main/java/io/confluent/ksql/engine/InsertValuesExecutor.java

+    }
+  }
+
+  private boolean isAvroSchema(final DataSource dataSource) {


nit: static

big-andy-coates · 2019-09-11T16:26:05Z

ksql-engine/src/main/java/io/confluent/ksql/engine/InsertValuesExecutor.java

+    return dataSource.getKsqlTopic().getValueFormat().getFormat() == Format.AVRO;
+  }
+
+  private boolean isSchemaAccessUnauthorized(final Exception e) {


nit: static

big-andy-coates · 2019-09-11T16:29:13Z

ksql-engine/src/main/java/io/confluent/ksql/util/AvroUtil.java

-        errorMessage = String.format(
-            "Not authorized to access Schema Registry subject: [%s]",
-            topicName + KsqlConstants.SCHEMA_REGISTRY_VALUE_SUFFIX
+        throw new KsqlSchemaAuthorizationException(


Not sure how the ACLs work on SR, but we should correctly set the ACL in this exception so the user knows what acls they need to set.

big-andy-coates · 2019-09-11T16:30:00Z

ksql-engine/src/main/java/io/confluent/ksql/exception/KsqlSchemaAuthorizationException.java

+import org.apache.kafka.common.acl.AclOperation;
+
+public class KsqlSchemaAuthorizationException extends RuntimeException {
+  public KsqlSchemaAuthorizationException(final Set<String> unauthorizedSchema) {


Can we change this to:

Suggested change

public KsqlSchemaAuthorizationException(final Set<String> unauthorizedSchema) {

public KsqlSchemaAuthorizationException(final String unauthorizedSchema) {

As it looks to be always called with a single schema

big-andy-coates · 2019-09-11T16:30:16Z

ksql-engine/src/main/java/io/confluent/ksql/exception/KsqlSchemaAuthorizationException.java

+
+  public KsqlSchemaAuthorizationException(
+      final AclOperation operation,
+      final Set<String> unauthorizedSchema


Suggested change

final Set<String> unauthorizedSchema

final String unauthorizedSchema

Again, it looks to be always called with a single schema

big-andy-coates · 2019-09-11T16:31:33Z

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

@@ -137,8 +137,7 @@ private static SchemaResult notFound(final String topicName) {
            + "\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: "
-            + topicName + KsqlConstants.SCHEMA_REGISTRY_VALUE_SUFFIX
+            + "- You do not have permissions to access the Schema Registry.Subject"


Suggested change

+ "- You do not have permissions to access the Schema Registry.Subject"

+ "- You do not have permissions to access the Schema Registry."

spena · 2019-09-16T22:31:58Z

@big-andy-coates Thanks for the review. Your idea of wrapping the exception in our own SchemaRegistryClient makes sense. I wasn't sure at the beginning because SR does not tell you what permission failed in the server, but I guess the SR client method name will help use, like schemaRegistryClient.deleteSubject(subject) is a DELETE operation.

I'll submit another PR with those changes.

spena · 2019-09-16T23:05:39Z

@big-andy-coates Btw, I created a JIRA for the SchemaRegistry team to fix the output error of the SchemaRegistryClient some time ago to avoid making these changes, but seems they have other priorities, thus I had to submit this PR.

I was looking on how to create our own SchemaRegistryClient. I am planning to use the LimitedProxyBuilder to forward the desired methods ot ours. Does it sound right?

Anyway, I wonder if we should wait for the SR fix instead? I am pushing the team to commit those changes to in 5.4.

Regardless the changes on the SR side or on our side, the ksqlAuthorizationValidator still needs to perform SR permissions checks to validate KSQL will have access to the schema after is written to the command topic. Should we follow-up this on another PR? or would you prefer to fix it here?

big-andy-coates · 2019-09-20T09:16:58Z

@big-andy-coates Thanks for the review. Your idea of wrapping the exception in our own SchemaRegistryClient makes sense.

To be clear, my suggestion is to wrap the SR client and handle the exceptions in one place, rather than throughout the code.

@big-andy-coates Btw, I created a JIRA for the SchemaRegistry team to fix the output error of the SchemaRegistryClient some time ago to avoid making these changes, but seems they have other priorities, thus I had to submit this PR.

You could raise a suitable PR yourself against the SR project

I was looking on how to create our own SchemaRegistryClient. I am planning to use the LimitedProxyBuilder to forward the desired methods ot ours. Does it sound right?

Yeah, that'll work. There's a definite performance hit for using a proxy, so you might want to check if anything is on the critical path. The alternative is to implement the SchemaRegistryClient interface ourselves in a concrete class but, as we know, such things tend to see the build break due to upstream changes that add new methods. Hence the proxy approach is preferable, as long as its not hitting performance.

rodesai · 2019-09-24T04:59:14Z

ksql-engine/src/main/java/io/confluent/ksql/exception/KsqlSchemaAuthorizationException.java

+public class KsqlSchemaAuthorizationException extends RuntimeException {
+  public KsqlSchemaAuthorizationException(final String unauthorizedSchema) {
+    super(
+        "Authorization denied to access Schema Registry subject(s): " + unauthorizedSchema


nit: "Not authorized to access" or "Denied access to"

rodesai · 2019-09-24T05:06:09Z

ksql-engine/src/main/java/io/confluent/ksql/schema/registry/SchemaRegistryUtil.java

-    ExecutorUtil.executeWithRetries(() -> schemaRegistryClient.deleteSubject(subject), ALWAYS);
+    ExecutorUtil.executeWithRetries(() -> {
+      try {
+        schemaRegistryClient.deleteSubject(subject);


Kind of a small point, but we could make this endpoint snappier if we just return immediately if we get a 403 or 401 (or any 4xx for that matter). Retries certainly won't help in those cases. We could either have the function return an Optional and throw if we get something back. Or change executeWithRetries to support a whitelist of exceptions to not retry on.

rodesai · 2019-09-24T05:07:56Z

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

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


Shouldn't we also check for read access on the schema in this case?

rodesai · 2019-09-24T05:11:52Z

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

+    final String subject = topicName + KsqlConstants.SCHEMA_REGISTRY_VALUE_SUFFIX;
+
+    try {
+      schemaRegistryClient.getLatestSchemaMetadata(subject);


Does this succeeding imply that any operation (READ/WRITE/etc) will work?

spena · 2021-07-08T15:48:18Z

Closing this PR. Working on this other #7773

spena requested a review from a team September 10, 2019 19:55

spena self-assigned this Sep 10, 2019

spena commented Sep 10, 2019

View reviewed changes

big-andy-coates reviewed Sep 11, 2019

View reviewed changes

big-andy-coates requested a review from a team September 11, 2019 16:36

spena added 3 commits September 16, 2019 16:56

refactor: use KsqlSchemaAuthorizationException on SR auth messages

af81f1d

feat: perform SR permissions checks

c3fe320

fix: address Andy's feedback

80560cd

spena force-pushed the sr_auth_messages branch from bec0518 to 80560cd Compare September 16, 2019 23:09

rodesai reviewed Sep 24, 2019

View reviewed changes

spena removed their assignment Jan 14, 2020

spena mentioned this pull request Jul 8, 2021

feat: make SchemaRegistry permission validations on KSQL requests #7773

Merged

2 tasks

spena closed this Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Perform Schema Registry permissions checks #3323

feat: Perform Schema Registry permissions checks #3323

spena commented Sep 10, 2019

spena Sep 10, 2019

big-andy-coates left a comment

big-andy-coates Sep 11, 2019

big-andy-coates Sep 11, 2019

big-andy-coates Sep 11, 2019

big-andy-coates Sep 11, 2019

big-andy-coates Sep 11, 2019

big-andy-coates Sep 11, 2019

spena commented Sep 16, 2019

spena commented Sep 16, 2019

big-andy-coates commented Sep 20, 2019

rodesai Sep 24, 2019

rodesai Sep 24, 2019

rodesai Sep 24, 2019

rodesai Sep 24, 2019

spena commented Jul 8, 2021

	public KsqlSchemaAuthorizationException(final Set<String> unauthorizedSchema) {
	public KsqlSchemaAuthorizationException(final String unauthorizedSchema) {

	final Set<String> unauthorizedSchema
	final String unauthorizedSchema

	+ "- You do not have permissions to access the Schema Registry.Subject"
	+ "- You do not have permissions to access the Schema Registry."

feat: Perform Schema Registry permissions checks #3323

feat: Perform Schema Registry permissions checks #3323

Conversation

spena commented Sep 10, 2019

Description

Testing done

Reviewer checklist

Choose a reason for hiding this comment

big-andy-coates 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

spena commented Sep 16, 2019

spena commented Sep 16, 2019

big-andy-coates commented Sep 20, 2019

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 Jul 8, 2021