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

Scram sha auth #156

Merged
merged 4 commits into from Feb 26, 2018
Merged

Scram sha auth #156

merged 4 commits into from Feb 26, 2018

Conversation

seriyps
Copy link
Member

@seriyps seriyps commented Feb 15, 2018

See #142 for details.

https://github.com/sfackler/rust-postgres/blob/master/postgres-protocol/src/authentication/sasl.rs and https://github.com/npgsql/npgsql/pull/1769/files were used as references.

Code that implements authentication was reworked so it should be easier to add any new auth methods.

scram-sha-256 auth method was added to Postgresql 10 and not available on earlier postgresql releases.
However, it seems that travis-ci doesn't support PG10 yet travis-ci/travis-ci#8537, so, we can't test this on travis, but I checked it locally.

* Add typespecs
* Fix tests
* Add server proof validation
* Change the way how server version is handled
Copy link
Member

@davidw davidw left a comment

Choose a reason for hiding this comment

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

Looks ok, but I'm not much of a crypto guy - might not be a bad idea to have a few more people look at it?

@seriyps
Copy link
Member Author

seriyps commented Feb 16, 2018

@davidw thanks! Yep, me too. I just implemented it from specification with simplification for normalize function (because full implementation of that is huge). I may send a mail to our mail list to call for some more reviewers.

@mmzeeman
Copy link

It looks ok, with one remark.

Implementations of crypto protocols often go sideways with presumably simple things like generating and validating nonce's.

People often just generate a random number, in this case an 80 bit random number. This means, because of the birthday paradox, that you have to watch 2**40 login sessions to actually see a nonce used twice!

Nonce's don't have to be random, they have to be unique. The combination of the nonce, with (in this case) the password should be used once and only once (in the entire system).

Maybe 2**40 is probably enough uniqueness here, but maybe there is a simple way to improve this by mixing in a timestamp of some sorts.

Sometimes validating the uniqueness of incoming nonce's is also important. I really don't know if that is the case for this protocol.

@seriyps
Copy link
Member Author

seriyps commented Feb 21, 2018

Thanks, @mmzeeman ! That was very educational comment.

I added unique part to the nonce (it uses different mechanisms for erlang before 18 and >=18) and set random part size to 16 bytes.

I don't think we should add validation of uniqueness of the server's nonce... It might be too complex.

* Nonce should be unique
* Make normalize/1 stricter
* add handling of 'unknown' from auth functions
@seriyps
Copy link
Member Author

seriyps commented Feb 26, 2018

@davidw do you think this can be merged now?

@davidw
Copy link
Member

davidw commented Feb 26, 2018

Looks good!

@seriyps seriyps merged commit fa56fa7 into epgsql:devel Feb 26, 2018
@seriyps
Copy link
Member Author

seriyps commented Feb 26, 2018

Thanks everybody

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

4 participants