[improve][authentication] Pass the authorization when user lookup transactionCoordinator topic #22744
Conversation
TakaHiR07
changed the title
|
@nodece @michaeljmarshall could you take a look of this problem. thx |
TakaHiR07
force-pushed
the
improve_tc_lookup_auth
branch
from
3b969e2 to
c1f37d2
Compare
|
The |
| @@ -155,6 +156,9 @@ public CompletableFuture<Boolean> canConsumeAsync(TopicName topicName, String ro | |||
| @Override | |||
| public CompletableFuture<Boolean> canLookupAsync(TopicName topicName, String role, | |||
| AuthenticationDataSource authenticationData) { | |||
| if (SystemTopicNames.isTransactionCoordinatorAssign(topicName)) { | |||
There was a problem hiding this comment.
You should write this code in the org.apache.pulsar.broker.authentication.AuthenticationService.
That looks like an important change, could you discuss this in the mailing list?
There was a problem hiding this comment.
Why should we implement it in AuthenticationService?
thetumbled
requested review from
congbobo184,
liangyepianzhou,
dao-jun and
poorbarcode
There was a problem hiding this comment.
LGTM, PTAL. @lhotari @congbobo184 @liangyepianzhou @BewareMyPower @poorbarcode
| @@ -155,6 +156,9 @@ public CompletableFuture<Boolean> canConsumeAsync(TopicName topicName, String ro | |||
| @Override | |||
| public CompletableFuture<Boolean> canLookupAsync(TopicName topicName, String role, | |||
| AuthenticationDataSource authenticationData) { | |||
| if (SystemTopicNames.isTransactionCoordinatorAssign(topicName)) { | |||
There was a problem hiding this comment.
how do we control user can use transaction?
It seem like a good way to control user to use transaction, if user have the produce role of TC system topic, then can send transaction message and can do lookup for tc topic. this only a idea. WDYT? @thetumbled @TakaHiR07
There was a problem hiding this comment.
Do we need to restrict user to use transaction feature by authorizing the lookup permission of TC system topic?
It is weird as there is no official document pointing out this.
There was a problem hiding this comment.
This is just a thought, as we currently have no means to restrict users from using transactions.
Motivation
As seen in the AuthenticatedTransactionProducerConsumerTest, if we enable authorization, and want to produce/consume to a normal topic by transaction, we not only need to grant permission on normal topic, but also need to grant permission on system namespace.
It looks unreasonable and very dangerous.
Normal users just want to produce/consume to a normal topic by transaction, but super user need to grant the whole system namespace permission to them. I think the reasonable way is to make normal user unable to produce/consume system namespace directly, instead, make them able to lookup the transactionCoordinator topic.
Modifications
When do canLookupAsync(), if the topic is tc topic, pass the authorization
Verifying this change
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: TakaHiR07#20