-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proxy roles enforcement #1168
Proxy roles enforcement #1168
Conversation
Forgot to mention: This enhancement is configurable - if you don't populate the proxyRoles then the behavior falls back to default. |
@@ -196,6 +202,16 @@ protected void handleLookup(CommandLookupTopic lookup) { | |||
if (lookupSemaphore.tryAcquire()) { | |||
final String originalPrincipal = lookup.hasOriginalPrincipal() ? lookup.getOriginalPrincipal() | |||
: this.originalPrincipal; | |||
if (service.isAuthorizationEnabled() && proxyRoles.contains(authRole)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have the 3 similar blocks of code in a single method
retest this please |
@merlimat @rdhabalia @saandrews @msb-at-yahoo - please review this when you find the time. |
// Step 1: Create Admin Client | ||
createAdminClient(); | ||
final String proxyServiceUrl = "pulsar://localhost:" + servicePort; | ||
// create a client which connects to proxy over tls and pass authData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the test is not using tls, please remove the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@jai1 Can you also update the deployment documentation for the proxy with all the instructions and the different options on how to enable auth with proxy?
retest this please |
@@ -196,6 +215,13 @@ protected void handleLookup(CommandLookupTopic lookup) { | |||
if (lookupSemaphore.tryAcquire()) { | |||
final String originalPrincipal = lookup.hasOriginalPrincipal() ? lookup.getOriginalPrincipal() | |||
: this.originalPrincipal; | |||
if (!validateOriginalPrincipal(originalPrincipal, | |||
newLookupErrorResponse(ServerError.AuthorizationError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot create the buffer in all cases, otherwise this is being leaked when there is no error.
The broker should be able to distinguish between a proxy and a client so that no compromised proxy can impersonate a client.
To be specific - once a proxy host is compromised it can choose to send originalPrincipal as null which will make the broker treat it as a normal client and authenticate/authorize the proxy as a normal client rather than demanding to authorize the originalPrincipal.
So for example:-
A client using topic T1 with roleToken R1 going through proxy with roleToken R3 will require granting AuthAction produce/consume on both R1 and R3 (proxy).
Similarly, a client using topic T2 with roleToken R2 going through the same proxy will require granting AuthAction produce/consume on both R1 and R3 (proxy).
Now with my current code if proxy is compromised it can access topics T1 and T2 without requiring client roleToken (R1 and R2) at all by passing originalPrincipal as null.
If I add a broker side setting "proxyRoles=R3, R4", then as soon as I see that the roleToken (R3) extracted from the incoming cert is in proxyRoles (R3, R4) then I can return an error if originalPrincipal is null.
Thanks to @msb-at-yahoo for suggesting this.