Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Jan 19, 2017

No description provided.

@maffoo maffoo requested a review from kunalq January 19, 2017 23:04
Copy link
Member

@jayich jayich left a 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 = []
Copy link
Member

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:
Copy link
Member

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()
Copy link
Member

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:
Copy link
Member

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?

Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kunalq , thank you.

@maffoo , space permitting consider srp -> secure_random_password, or secure_rnd_passwd.

@@ -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)
Copy link
Member

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

Copy link
Contributor

@kunalq kunalq left a 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
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor

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))
Copy link
Contributor

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")
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants