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

Implemented a SCRAM server, with testing #3

Merged
merged 1 commit into from
Mar 14, 2017

Conversation

dyule
Copy link

@dyule dyule commented Mar 12, 2017

As the message says, I implemented a SCRAM server that uses a trait to provide password information.

I did end up modifying a little bit of the client code, but only to move some code that I was also using to a utils module.

I think I've followed the pattern established in the client module pretty well, and I've updated the documentation to reflect how the server works. I also added some tests in tests/client_server.rs that verify they work properly together.

I'm happy to adjust anything you like, or explain any decisions I made.

@dyule
Copy link
Author

dyule commented Mar 12, 2017

I'm not sure why it's failing on the nightly build, since it builds fine on nightly for me, and cargo fails on --version on Travis

@tomprogrammer
Copy link
Owner

The build failed due to an error in cargo (rust-lang/cargo#3819) which was just fixed.

I'll review your server implementation tomorrow, but at first glance it seems pretty straightforward. Thanks for your PR!

Copy link
Owner

@tomprogrammer tomprogrammer left a comment

Choose a reason for hiding this comment

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

I suggest to use borrowed values instead of owned values in PasswordInfo. I've explained my motivation in the comment.

Thanks for the server implementation, the code is very clean and well documented. Great work!

/// algorithm
pub struct PasswordInfo {
hashed_password: Vec<u8>,
salt: Vec<u8>,
Copy link
Owner

@tomprogrammer tomprogrammer Mar 13, 2017

Choose a reason for hiding this comment

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

I think that this should store bytes slices instead of vectors. I have two arguments for this change:

  • The ownership of these values is often held by other data structures. Taking a slice avoids copies (like in tests/client_server.rs line 27).
  • Salt and hashed password can be stored together and sliced individually. Short example:
const SALT_LENGTH: usize = 20;
let (hashed_password, iterations) = password_db.get(&username).unwrap();
let password_info = PasswordInfo::new(&hashed_password[SALT_LENGTH..], 
                                      iterations,
                                      &hashed_password[..SALT_LENGTH]);

Copy link
Author

Choose a reason for hiding this comment

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

I originally had this as a borrowed value, but switched it to owned after implementing TestProvider. The issue with borrowing the password is that the object which owns it is the database query, which drops out of scope when the lookup method ends, as opposed to the database object itself, which lives longer. It's possible with some memory trickery to switch ownership of the password to the database object, but that's hardly ideal.

As long as the password database is entirely stored in memory, then borrowed values work. If there is any part which isn't, memory management becomes a nightmare, which is why I switched to owned, and took the small hit on copying.

However, there might be something obvious I'm missing here, so I'm perfectly happy to switch it to borrowed if that's the case.

Copy link
Owner

Choose a reason for hiding this comment

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

I get your arguments and agree that the ownership of the password (query result) shouldn't be linked to the database connection. I think that the API can be refined later, when the library gets some usage.

/// Parses a part of a SCRAM message, after it has been split on commas.
/// Checks to make sure there's a key, and then verifies its the right key.
/// Returns everything after the first '='.
/// Returns a ExpectedField error when one of the above conditions fails.
Copy link
Owner

Choose a reason for hiding this comment

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

That's nice! 👍

@tomprogrammer tomprogrammer merged commit 9e0f07d into tomprogrammer:master Mar 14, 2017
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.

3 participants