Check create topic permission on topic creation using pulsar proto clients #17411

KannarFr · 2022-09-01T17:29:17Z

Fixes #17406

Motivation

Enable CREATE_TOPIC permission check on topic auto creation.

Modifications

Introduce AuthAction.create_topic for the PulsarAuthorizationProvider

Verifying this change

Make sure that the change passes the CI checks.

This change updated tests and can be verified as follows:

updated test and give create_topic permission on the namespace

Does this pull request potentially affect one of the following parts:

it affects permissions as currently, CREATE_TOPIC wasn't checked on the default authZ provider.
doc-required

github-actions · 2022-09-01T17:43:00Z

@KannarFr Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

gaoran10 · 2022-09-02T14:36:39Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

+                CompletableFuture<Boolean> isAuthorizedToCreateTopicFuture = isNamespaceOperationAllowed(
+                        topicName.getNamespaceObject(),
+                        NamespaceOperation.CREATE_TOPIC


Maybe we can add this before line 1026, and use this method isAuthorizedFuture.thenCombine(isAuthorizedToCreateTopicFuture, (isAuthorized, isAuthorizedToCreateTopic).

This will prevent produce/consume of topic if it already exists but create_topic operation not authorized, nope?

Oh nvm, got it.

@gaoran10 done.

...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java

KannarFr · 2022-09-19T14:29:37Z

@gaoran10 the failing tests are due to CI flakyness, nope?

michaeljmarshall

This looks like a breaking change for some current use cases. @KannarFr - what do you think about making this feature an alternative to auto topic creation? Then, users can run without auto topic creation and give certain roles permission to create topics.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

michaeljmarshall

Do we also need to update the logic for ServerCnx#handleProducer?

KannarFr · 2022-10-17T14:46:46Z

@michaeljmarshall are you ok with the way I've done it in handleProducer?

michaeljmarshall

Sorry for the my delayed review @KannarFr. This is looking good, but I think it still needs some changes. Thanks!

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

michaeljmarshall · 2022-10-18T04:40:46Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

+                                    consumers.remove(consumerId, consumerFuture);
+                                    ctx.writeAndFlush(Commands.newError(requestId, ServerError.AuthorizationError,
+                                            msg));
+                                    return null;


I don't think returning null here will give the desired behavior unless we modify the thenAccept block. It seems like the current paradigm in this section of the code is to throw an exception, so I think it would be appropriate to do that here.

Did you see https://github.com/apache/pulsar/pull/17411/files#diff-1e0e8195fb5ec5a6d79acbc7d859c025a9b711f94e6ab37c94439e99b3202e84L1187-L1198 lines? It seems we are returning null and writing errors to the channel. Nope?

If we return null here, we will go to the thenAccept block and will call consumerFuture.complete(consumer). We don't want to complete the future there. I think we wnat to run the lines 1187 to 1198 that you reference. Returning a failed future ensures that we jump to the exceptionally block on 1169 that will send the error response.

Ok, does e34524a LGTY?

Hi @michaeljmarshall

Could you retake a look? Thanks

I just rebased it from master.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

KannarFr · 2023-04-18T20:35:35Z

@poorbarcode, @michaeljmarshall looks not available, can you ping someone else? It would be cool to merge it upstream asap :D.

michaeljmarshall

After reviewing this, here are a collection of thoughts on Pulsar's Authorization.

At it's core, I think the problem described by #17406 is that allowAutoTopicCreation is a configuration about permission/authorization.

In my view, the allowAutoTopicCreation=true says "a role with permission to produce/consume to a topic also has permission to create that topic".

This change proposes that allowAutoTopicCreation=true and produce/consumer permissions are insufficient, and that a role must also have explicit permission to create a topic.

michaeljmarshall · 2023-04-19T06:05:27Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

+        CompletableFuture<Boolean> isAuthorizedToCreateTopicFuture = isNamespaceOperationAllowed(
+                topicName.getNamespaceObject(),
+                NamespaceOperation.CREATE_TOPIC
+        );


As I mentioned before, this does not look backwards/forwards compatible. I don't think we can add required AuthActions without finding a way to make them compatible. The idea is that users should be able to upgrade then downgrade and things should still work. Can you confirm @KannarFr?

That would be nice, but as I answered you am I not sure we can really do it as CREATE_TOPIC check is already performed in HTTP admin checks.

While I agree that the current design is inconsistent, I don't think we should add logic that breaks existing use cases.

KannarFr · 2023-04-19T10:05:21Z

After reviewing this, here are a collection of thoughts on Pulsar's Authorization.

At it's core, I think the problem described by #17406 is that allowAutoTopicCreation is a configuration about permission/authorization.

Yes?

In my view, the allowAutoTopicCreation=true says "a role with permission to produce/consume to a topic also has permission to create that topic".

The HTTP admin API does not comply with this sentence. The CREATE_TOPIC operation is defined and used by HTTP admin API authz checks. I agree that this is introducing breaking changes in the permissions system and this is a problem, but there is authZ plugin provider providing this operation check and does not verify it during producer/consumer. So, I have no idea what the best answer is, but we can't stay and need to find a solution or make a decision here.

This change proposes that allowAutoTopicCreation=true and produce/consumer permissions are insufficient, and that a role must also have explicit permission to create a topic.

Well, this is the default behavior of every configurations keys in pulsar. Like namespace policies or tiered storage configurations, there is a default value that can be overridden by custom namespace policies.

michaeljmarshall · 2023-04-19T15:55:45Z

The HTTP admin API does not comply with this sentence.

Fair point. I hadn't considered the pulsar and http endpoints together when writing that generalization.

I agree that this is introducing breaking changes in the permissions system and this is a problem, but there is authZ plugin provider providing this operation check and does not verify it during producer/consumer.

Is there another way to achieve this? Perhaps we can introduce an additional check that calls the authorization service and then we'll implement the default PulsarAuthorizationProvider in such a way that there are no breaking changes.

I think it might make sense to discuss the underlying inconsistency on the pulsar mailing list.

KannarFr · 2023-04-19T17:41:35Z

Perhaps we can introduce an additional check that calls the authorization service and then we'll implement the default PulsarAuthorizationProvider in such a way that there are no breaking change

This LGTM. Can you drive this discussion on the ML if you think it's required? Shall I do it?

michaeljmarshall · 2023-04-19T22:56:57Z

Perhaps we can introduce an additional check that calls the authorization service and then we'll implement the default PulsarAuthorizationProvider in such a way that there are no breaking change

This LGTM. Can you drive this discussion on the ML if you think it's required? Shall I do it?

I won't be able to drive this discussion. Let me know if you have any questions though.

nodece

Your expectations seem reasonable, but I don't suggest we continue to add elements to AuthAction, I think we should deprecate the AuthAction.

We have NamespaceOperation and TopicOperation provides fine-grained permission control, but the provider doesn't support granting these operations to the user, perhaps we should move in this direction.

Let me know your thoughts!

github-actions · 2023-06-02T02:06:48Z

The pr had no activity for 30 days, mark with Stale label.

KannarFr · 2023-06-02T08:52:17Z

I sent

Hi,

CREATE_TOPIC authorization check is not performed when trying to PRODUCE/CONSUME a topic, it has been referenced: #17406.

I opened a PR to fix it #17411, but Michael reported issues about backward compatibility (which is totally correct). Adding support of CREATE_TOPIC authorization as-is will break current authorization system. I noticed that HTTP Admin API verifies the CREATE_TOPIC right when creating topic, so we have inconsistencies between pulsar binary protocol and the HTTP admin API on this.

Also, the AuthorizationProvider is an interface exposing the CREATE_TOPIC feature for authZ plugins. But it is inconsistent too.

Michael suggested to fix this interface to support the CREATE_TOPIC check and adapt the pulsar's DefaultAuthzProvider to continue as-is.

I'd like to know what do you think?

Thanks,

Kannar

On the ML the 20/04/2023 and still have no answers @michaeljmarshall @nodece.

Nowadays · 2023-07-31T15:34:12Z

Is there any progression on this issue ?

KannarFr · 2023-07-31T16:30:23Z

Unfortunately, no. I don't know why this is stuck @michaeljmarshall?

KannarFr · 2023-07-31T16:30:40Z

@gaoran10 @MarvinCai

KannarFr · 2023-10-04T23:14:02Z

@michaeljmarshall @gaoran10 @MarvinCai, it makes sense to rebase it..?

Nowadays · 2023-10-05T07:26:16Z

Is there any way to move forward with this PR ?

publishing/consuming and creating/deleting a topic are clearly different actions, and thus should require different authorization.

Why would there be an api to create/delete topics if that was not the case ?

KannarFr · 2024-03-22T12:39:09Z

Any bumps?

github-actions bot added the doc-label-missing label Sep 1, 2022

KannarFr changed the title ~~Check create topic on subscription creation~~ Check create topic permission on topic creation using pulsar proto clients Sep 2, 2022

KannarFr force-pushed the checkCreateTopicOnSubscriptionCreation branch from 241ccc0 to 48fe841 Compare September 2, 2022 13:38

gaoran10 reviewed Sep 2, 2022

View reviewed changes

KannarFr force-pushed the checkCreateTopicOnSubscriptionCreation branch 3 times, most recently from ebfd946 to 4903905 Compare September 5, 2022 12:39

MarvinCai requested a review from gaoran10 September 8, 2022 13:04

MarvinCai reviewed Sep 8, 2022

View reviewed changes

...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated Show resolved Hide resolved

KannarFr force-pushed the checkCreateTopicOnSubscriptionCreation branch 5 times, most recently from 80dd5a0 to 364e212 Compare September 14, 2022 13:33

michaeljmarshall requested changes Sep 20, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Outdated Show resolved Hide resolved

michaeljmarshall reviewed Sep 22, 2022

View reviewed changes

KannarFr force-pushed the checkCreateTopicOnSubscriptionCreation branch from ce5d4c1 to e9f004d Compare September 23, 2022 11:59

KannarFr force-pushed the checkCreateTopicOnSubscriptionCreation branch from e9f004d to 8fd1301 Compare October 17, 2022 12:21

michaeljmarshall requested changes Oct 18, 2022

View reviewed changes

michaeljmarshall requested review from eolivelli, zymap and codelipenghui October 18, 2022 04:47

michaeljmarshall added type/feature The PR added a new feature or issue requested a new feature doc-required Your PR changes impact docs and you will update later. component/authorization and removed doc-label-missing labels Oct 18, 2022

michaeljmarshall added this to the 2.12.0 milestone Oct 18, 2022

poorbarcode added this to the 3.1.0 milestone Apr 10, 2023

KannarFr force-pushed the checkCreateTopicOnSubscriptionCreation branch from 512d053 to 4d743bb Compare April 10, 2023 17:47

github-actions bot removed the Stale label Apr 19, 2023

michaeljmarshall reviewed Apr 19, 2023

View reviewed changes

nodece requested changes Apr 23, 2023

View reviewed changes

github-actions bot added the Stale label Jun 2, 2023

github-actions bot removed the Stale label Jun 4, 2023

Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023

Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023

coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024

check CREATE_TOPIC operation permission on topic auto creation

da5205f

KannarFr force-pushed the checkCreateTopicOnSubscriptionCreation branch from 4d743bb to da5205f Compare October 3, 2024 11:30

lhotari modified the milestones: 4.0.0, 4.1.0 Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check create topic permission on topic creation using pulsar proto clients #17411

Check create topic permission on topic creation using pulsar proto clients #17411

KannarFr commented Sep 1, 2022 •

edited

Loading

github-actions bot commented Sep 1, 2022

gaoran10 Sep 2, 2022

KannarFr Sep 5, 2022 •

edited

Loading

KannarFr Sep 5, 2022

KannarFr Sep 5, 2022

KannarFr commented Sep 19, 2022

michaeljmarshall left a comment

michaeljmarshall left a comment

KannarFr commented Oct 17, 2022

michaeljmarshall left a comment

michaeljmarshall Oct 18, 2022

KannarFr Oct 18, 2022

michaeljmarshall Oct 19, 2022

KannarFr Oct 19, 2022

poorbarcode Apr 10, 2023

KannarFr Apr 10, 2023

KannarFr commented Apr 18, 2023

michaeljmarshall left a comment

michaeljmarshall Apr 19, 2023

KannarFr Apr 19, 2023 •

edited

Loading

michaeljmarshall Apr 19, 2023

KannarFr commented Apr 19, 2023

michaeljmarshall commented Apr 19, 2023 •

edited

Loading

KannarFr commented Apr 19, 2023 •

edited

Loading

michaeljmarshall commented Apr 19, 2023

nodece left a comment

github-actions bot commented Jun 2, 2023

KannarFr commented Jun 2, 2023

Nowadays commented Jul 31, 2023

KannarFr commented Jul 31, 2023

KannarFr commented Jul 31, 2023

KannarFr commented Oct 4, 2023

Nowadays commented Oct 5, 2023

KannarFr commented Mar 22, 2024

Check create topic permission on topic creation using pulsar proto clients #17411

Are you sure you want to change the base?

Check create topic permission on topic creation using pulsar proto clients #17411

Conversation

KannarFr commented Sep 1, 2022 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

github-actions bot commented Sep 1, 2022

Choose a reason for hiding this comment

KannarFr Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KannarFr commented Sep 19, 2022

michaeljmarshall left a comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

KannarFr commented Oct 17, 2022

michaeljmarshall 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

KannarFr commented Apr 18, 2023

michaeljmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KannarFr Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KannarFr commented Apr 19, 2023

michaeljmarshall commented Apr 19, 2023 • edited Loading

KannarFr commented Apr 19, 2023 • edited Loading

michaeljmarshall commented Apr 19, 2023

nodece left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 2, 2023

KannarFr commented Jun 2, 2023

Nowadays commented Jul 31, 2023

KannarFr commented Jul 31, 2023

KannarFr commented Jul 31, 2023

KannarFr commented Oct 4, 2023

Nowadays commented Oct 5, 2023

KannarFr commented Mar 22, 2024

KannarFr commented Sep 1, 2022 •

edited

Loading

KannarFr Sep 5, 2022 •

edited

Loading

KannarFr Apr 19, 2023 •

edited

Loading

michaeljmarshall commented Apr 19, 2023 •

edited

Loading

KannarFr commented Apr 19, 2023 •

edited

Loading