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

Improvements to crypto-password. #1

Closed
wants to merge 2 commits into from
Closed

Conversation

ghoseb
Copy link
Contributor

@ghoseb ghoseb commented Mar 26, 2014

Following the advice @abedra gave at Clojure/West '14, I have done the following things:

  • Increased default iterations of PBKDF2 to 100,000
  • Implemented support for HMAC (supports MD5, SHA1-512 with SHA1 being the default)

Let's all work on this together and make crypto-password better.

Suggestions welcome.

ghoseb added 2 commits March 26, 2014 11:50
20,000 is too low to be a sane default in 2014. According to OWASP the
rule of thumb is to tune the work-factor to make sure the process takes
around 250ms to 1s.

100,000 iterations takes around 241ms on my mid-2012 MacBook Air, so
given modern hardware 100,000 seems to be a fairly sane default.
Supported algorithms: HMACMD5, HMACSHA1, HMACSHA256 (default),
HMACSHA384 & HMACSHA512.
@weavejester
Copy link
Owner

Could you split this pull request up? Increasing the number of iterations by default seems sensible, but I'm puzzled by the HMAC commit. This is a library of key derivation functions.

@ghoseb
Copy link
Contributor Author

ghoseb commented Mar 26, 2014

Ok, sent a fresh pull request. Before I close this one, could you please tell me what's wrong with the hmac commit? As far as I can see HMAC is useful in a password management context.

@weavejester
Copy link
Owner

HMACs are not key derivation functions, and this project is a library of key derivation functions. Maybe the name could have been better chosen, but "crypto-password" seemed snappier than "crypto-key-derivation-functions".

Your code is good, I just think it belongs in a separate "crypto-hmac" library. Since I use similar code in Ring, would you be interested in collaborating on such a library?

@ghoseb
Copy link
Contributor Author

ghoseb commented Mar 26, 2014

Oh absolutely @weavejester. The key idea behind this commit is inspired by the talk @abedra gave at Clojure/West this week. He was talking about the state of security in the Clojure ecosystem and discussed a bunch of related Clojure libraries. He considered crypto-password as a password management library and pointed out that it lacks support for HMAC. He also suggested that the Clojure ecosystem should standardize on this library for all password related work and avoid reinventing.

You should probably watch the talk for more context: https://www.youtube.com/watch?v=CBL59w7fXw4&index=6&list=PLZdCLR02grLp__wRg5OTavVj4wefg69hM

May be we should rename this library and widen its scope to deal with all aspects of password management a la what Austin does to authentication & authorization? Just a suggestion.

@weavejester
Copy link
Owner

I'd rather not widen the scope of the library. Tightly scoped libraries are generally more useful, easier to maintain, and also easier to secure. I like the idea of libraries that focus on doing one thing.

That's not to say we couldn't have another library that builds on crypto-password, but code for generating HMACs doesn't belong in a library of key derivation functions, IMO.

I've been slowly working my way through the Clojure West talks. I agree with almost every @abedra says on the state of Clojure web security. However, he appears to have assumed that crypto-password was constructed as a generic password management library. Granted, I can see why one might assume that from the name, but the description of the project is pretty specific:

A Clojure library for securing user passwords using a key derivation function.

It's not a library for dealing with every aspect of managing and securing passwords. It's a library specifically to do with key derivation functions.

@abedra
Copy link

abedra commented Mar 26, 2014

Understood. If that is in fact the case, then maybe we create a new project for doing just that and leave the PBKDF2 default iteration changes here. HMAC support and migration path support are certainly part of a password management library.

@abedra
Copy link

abedra commented Mar 26, 2014

Also, thanks @ghoseb for responding to the talk with action!

@weavejester
Copy link
Owner

Thank you both. I picked up the default PBKDF2 from some older recommendations, but Moore's Law has obviously rendered that advice out of date. I'll release a new version with those changes.

My thought is to produce two new libraries:

  • crypto-codec - encoding for Base64, Base58 and Hex
  • crypto-hmac - Clojure wrapper for the Java HMAC libs

Those could also be used for things like the cookie sessions in Ring, as well as in a password management library. I'll setup the repos for them tonight, after work, and invite you as contributors, if you like.

@abedra
Copy link

abedra commented Mar 26, 2014

Since we already have data.codec (https://github.com/clojure/data.codec), I would rather just work from there if we want to add things. But for the HMAC stuff, sounds great!

@weavejester
Copy link
Owner

There's also ring-codec. I don't think it's necessarily a bad idea to have different codec libraries that are a little specialised. Percent-encoding is fairly specific to the web, and in a crypto encoding library it might be useful to include the Base58Check encoding, which has little place in a general-purpose codec library.

That said, all we really need for now is support for hex encoding, which seems a reasonable thing to add to data.codec. My only reservation is that dealing with the contrib libraries is a lot of hassle, even for those of us who've signed a contributor's agreement.

@abedra
Copy link

abedra commented Mar 26, 2014

Good point. I have a little easier time with committing to contrib libraries, so I don't often think about that :)

@ghoseb
Copy link
Contributor Author

ghoseb commented Mar 26, 2014

@weavejester Thanks! I would love to contribute.
@abedra Thanks for the talk. Consider this my way of participating in Clojure/West '14 from 8K miles away :-)

@cemerick
Copy link

@weavejester Hi! :-) What is the status of the proposed crypto-hmac library? I am interested in connection with cemerick/friend#108.

@weavejester
Copy link
Owner

Well, I'm about 80% of the way through updating data.codec to support hex encoding. The only thing left to do is add some namespaces for performance-testing the new code. Unfortunately that work got put on the back-burner for about a month, but I'm going to pick it up again soon.

The reason that work's important is because I didn't want to add encoding directly to crypto-hmac, since it's a fairly generic piece of functionality. However, there's nothing stopping me making crypto-hmac concurrently, I guess. It'll likely be a pretty slim library.

Incidentally, @cemerick, have you thought about using crypto-password in Friend?

@cemerick
Copy link

@weavejester Yes, see cemerick/friend#110. I haven't looked at it in detail, but I presume it will be straightforward; probably just a HOF for producing a Friend credential function given a KDF from crypto-password (or similar that implements analogous contracts).

@ghoseb
Copy link
Contributor Author

ghoseb commented May 28, 2014

👍

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.

4 participants