-
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 forward auth data #1169
Proxy forward auth data #1169
Conversation
…into ProxyForwardAuthData
…lesEnforcement
…into ProxyForwardAuthData
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.
Overall looks ok, few comments.
@@ -101,7 +105,9 @@ | |||
private int nonPersistentPendingMessages = 0; | |||
private final int MaxNonPersistentPendingMessages; | |||
private String originalPrincipal = null; | |||
|
|||
private Set<String> proxyRoles = Sets.newHashSet(); |
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 it's initialized in constructor, why create a new set here?
@@ -180,6 +188,19 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) throws E | |||
ctx.close(); | |||
} | |||
|
|||
private boolean validateOriginalPrincipal(String originalPrincipal, ByteBuf errorResponse, String topicName, |
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.
Can you add comment explaining the logic?
connect.hasOriginalAuthData() ? connect.getOriginalAuthData() : null, | ||
connect.hasOriginalAuthMethod() ? connect.getOriginalAuthMethod() : null, | ||
connect.hasOriginalPrincipal() ? connect.getOriginalPrincipal() : null, | ||
sslSession); |
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.
What happens if sslSession is null ?
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.
is sslSession is null AuthDataCommand.hasDataFromTls will return false
|
||
// Original auth role and auth Method that was passed | ||
// to the proxy. In this case the auth info above | ||
// will the the auth of the proxy itself |
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.
"will be the"?
Same for the remaining comments as well.
@saandrews - have addressed your comments |
retest this please |
@@ -197,6 +197,10 @@ | |||
// role as proxyRoles - it will demand to see the original client role or certificate. | |||
private Set<String> proxyRoles = Sets.newTreeSet(); | |||
|
|||
// If this flag is set then the broker authenticates the original Auth data | |||
// else it just accepts the originalPrincipal and authorizes it (if required). | |||
private boolean authenticateOriginalAuthData = false; |
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.
Config options need to be also added into conf/broker.conf
and conf/standalone.conf
, also check if there is a suitable name prefix this can be aggregated into.
@@ -213,8 +221,19 @@ protected void handleLookup(CommandLookupTopic lookup) { | |||
|
|||
final Semaphore lookupSemaphore = service.getLookupRequestSemaphore(); | |||
if (lookupSemaphore.tryAcquire()) { | |||
final String originalPrincipal = lookup.hasOriginalPrincipal() ? lookup.getOriginalPrincipal() | |||
: this.originalPrincipal; | |||
String originalPrincipal; |
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.
This check can be done before acquiring the semaphore
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.
As with similar changes, try to put common logic in a single method
|
||
// Forward client authData to Broker for re authorization | ||
// make sure authentication is enabled for this to take effect | ||
private boolean forwardAuthData = false; |
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.
Should be added to proxy.conf
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.
Rename to something like authorizationForwardCredentials
or similar
@merlimat @rdhabalia @saandrews - can I merge this or do you want to take one last look. |
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.
overall LGTM..
lookup); | ||
|
||
if (authenticateOriginalAuthData && lookup.hasOriginalAuthData() && originalPrincipal == null) { | ||
return; |
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.
this condition seems little tricky. if we fail to validate original principal then validateOriginalPrincipal()
will send failure response to the client and then thread should not process anything further. but then with this condition there is a possibility to move forward and thread will do further processing if authenticateOriginalAuthData && lookup.hasOriginalAuthData() =is false
.??
@rdhabalia - handled your comments - hope to get a +1 from you |
======
Currently the proxy extracts and forwards the client Principal to the broker. Client Principal is a modifiable string i.e can be changed or manufactured by the Proxy.
What we want to do instead is to send the clientAuthData to the broker - which is in most cases digitally signed. The broker will extract the client principal from the clientAuthData and reauthenticate the client.
In order to enforce this behavior we have introduced two new flags:-
======
@msb-at-yahoo