-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
HBASE-28970 Get asyncfs working with custom SASL mechanisms #6507
base: master
Are you sure you want to change the base?
Conversation
This should be good, but I'm waiting for the Hadoop side fixes before landing this one. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
The standard HBase SASL code already works like this. |
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.
Quick question.
// Treat null negotiated QOP as "auth" for the purpose of verification | ||
// Code elsewhere does the same implicitly | ||
if (negotiatedQop == null) { | ||
negotiatedQop = "auth"; | ||
} |
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.
I don't see why is this necessary here. It's only effective within this private method and doesn't make any difference in the verification at line 517, while it hides that there was no negotiated QoP with the client.
Since rest of the code handles "auth" and null equally, it'd make sense to return "auth" by the getNegotiatedQop()
if null was negotiated effectively making sure that negotiatedQop will never be null. That would probably make some of the code in this class simpler, but still not strictly required for this patch.
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.
That's not true.
The Hadoop code always requests some kind of QOP, at least "auth".
SASL mechanisms that don't support QOP at all, like SCRAM, will ignore the requested QOP and always return null negotiated qop.
Without this if, we could not use SCRAM at all, as ["auth"] does not contain null.
The rest of the code does not check the negotiated QOP against the requested one, so a null check is fine there.
This is the simplest way I can think of to handle non-QOP capable SASL mechanisms.
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.
So, basically in case of SCRAM requestedQop is ["auth"], but negotiatedQop is NULL?
Do you put this hack here to pretend "auth" was negotiated?
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.
Do you have any real use cases?
IIRC we have done the same fix in hbase's own rpc implementation, but I can not recall the details...
No description provided.