-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add support for SRP authentication to the manager #312
base: master
Are you sure you want to change the base?
Conversation
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.
@maffoo , just some minor suggestions that might help with readability. I don't understand security, so no comment there.
|
||
def int_to_bytes(n): | ||
"""Convert the given int to a byte string in big-endian byte order.""" | ||
bs = [] |
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.
bs -> byte_string
untouched. | ||
""" | ||
m = hash_class() | ||
for a in args: |
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.
a -> argument, or a -> arg
converted to bytes using int_to_bytes, while byte strings are left | ||
untouched. | ||
""" | ||
m = hash_class() |
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.
m -> hash
m.update(challenge) | ||
if isinstance(credential.password, bytes): | ||
m.update(credential.password) | ||
if 'srp' in self.manager_features: |
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 does "srp" stand for?
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.
@jayich Secure Random Password -- see https://tools.ietf.org/html/rfc5054
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.
@@ -580,6 +585,15 @@ def ping(p): | |||
manager_features = set() | |||
returnValue(manager_features) | |||
|
|||
if tls_mode == 'on': | |||
tls_options = crypto.tls_options(host) | |||
p = yield _factory.connectSSL(host, port, tls_options, timeout=C.TIMEOUT) |
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.
p is a port? possibly p -> port
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.
Aside from some comments re: exceptions and constants, this LGTM :)
"""Convert the given int to a byte string in big-endian byte order.""" | ||
bs = [] | ||
while n > 0: | ||
b, n = n & 0xFF, n >> 8 |
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.
For readability, I'd break apart the parallel assignment here--
b = n & 0xFF
n = n >> 8
0846851D F9AB4819 5DED7EA1 B1D510BD 7EE74D73 FAF36BC3 1ECFA268 | ||
359046F4 EB879F92 4009438B 481C6CD7 889A002E D5EE382B C9190DA6 | ||
FC026E47 9558E447 5677E9AA 9E3050E2 765694DF C81F56E8 80B96E71 | ||
60C980DD 98EDD3DF FFFFFFFF FFFFFFFF |
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.
Likewise here, I think we can have a common file that can be read in to store these numbers. Then both Scala & python can read them in.
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.
And, actually, we can use MoultingYAML in Scala and Python's YAML format to stuff these into a YAML file keyed on the bit size.
return hash_all(hash_class, *args) | ||
|
||
def PAD(x): | ||
return int_to_bytes(x).rjust(len(N_bytes), chr(0)) |
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 would suggest renaming PAD
-> pad
to keep naming consistent (even though this looks like it's mimicking a C macro)
u = bytes_to_int(H(PAD(A), PAD(B))) | ||
if u == 0: | ||
raise Exception("auth failed: incompatible server and client " | ||
"challenges") |
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 recommend making an AuthException
class and raising that instead, in case we need it in the future (or using the existing LoginFailedError
exception)...
m.update(challenge) | ||
if isinstance(credential.password, bytes): | ||
m.update(credential.password) | ||
if 'srp' in self.manager_features: |
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.
@jayich Secure Random Password -- see https://tools.ietf.org/html/rfc5054
A_bytes, M1 = srp.process_server_challenge(salt, B_bytes) | ||
try: | ||
resp, M2 = yield self._sendManagerRequest(11, (A_bytes, M1)) | ||
except Exception: |
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 realize that labrad will propagate the stack trace to the client, which should hopefully explain where the exception is being raised, but I'm still hesitant to use this outside of generic code. Any chance we can be more specific in this exception? My experience with except Exception
is that it can cause serious debugging issues trying to isolate root causes.
m.update(credential.password.encode('UTF-8')) | ||
try: | ||
resp = yield self._sendManagerRequest(0, m.digest()) | ||
except Exception: |
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.
Ditto here
No description provided.