danielsomerfield / go-strong-auth-plugin

A file-based auth plugin that uses configurable strong hash algorithm and salts.
Apache License 2.0
5 stars 1 forks source link

Feedback on GoCD's file based authentication plugin #3

Open ketan opened 7 years ago

ketan commented 7 years ago

I'm one of the developers on the core GoCD team, and wanted to let you know that we've addressed some of the reasons why this plugin was authored, and I was hoping that you'd have some feedback for us.

For some context:

GoCD's in-built password file authentication mechanism historically supported only (insecure) SHA1 hashes. We've extracted the in built authentication functionality and replaced it with a new file based authentication plugin. This plugin now ships by default with GoCD and replaces the in-built password file authentication mechanism.

This plugin currently supports bcrypt hashing and SHA1. (SHA1 support will be removed in a few releases). We expect to add support for PBE in time for the next release.

We are hoping that between bcrypt and PBE, we should be covered. We would love to hear what you have to say.

/cc @maheshp @arvindsv @bdpiparva

ketan commented 7 years ago

Corresponding issue: https://github.com/gocd/filebased-authentication-plugin/issues/13

danielsomerfield commented 7 years ago

I'll take a look. My general concerns are:

I'll take a look and see what I think.

On Thu, Jun 22, 2017 at 2:06 AM Ketan Padegaonkar notifications@github.com wrote:

Corresponding issue: gocd/filebased-authentication-plugin#13 https://github.com/gocd/filebased-authentication-plugin/issues/13

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/danielsomerfield/go-strong-auth-plugin/issues/3#issuecomment-310322133, or mute the thread https://github.com/notifications/unsubscribe-auth/AFvbXD6T2U2GaXHrtLoYlmblzIHCHD2eks5sGi6cgaJpZM4OCAhh .

danielsomerfield commented 7 years ago

Looks good in general, but I do have a couple of thoughts:

dudadornelles commented 7 years ago

htpasswd -B does bcrypt, didn't try but I'm guessing it should do it.

ketan commented 7 years ago

Is there any reason to use PBE or PBKDF2 at all? I had it in my plugin mostly to make sure my interface was complete, but bcrypt seems like the best option these days.

It's a NIST recommendation, so we think it's worth the (small) effort to add it in.

could you provide a tool (like the one with the plugin) or at least instructions on generating bcrypt entries?

As @dudadornelles mentions, htpasswd -B does the trick, we're building a CLI (https://github.com/gocd/filebased-authentication-plugin/pull/18) to allow you to effectively alias htpasswd="java -jar .." and perform bcrypt and PBKDF2 using a (mostly) drop-in replacement.

danielsomerfield commented 7 years ago

My understandings is that NIST doesn't recommend PBKDF2 for password hashing, but for key derivation which I think is subtly different. From what I see, the current NIST recommendations http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-175B.pdf are SHA3, although word is that SHA3 is still not ideal.

I'm honestly not sure what the "perfect" strategy is any more given so much contradictory advice floating around. I recently talking to a security engineer I trust who said NOT to use Argon (he and his company uses bcrypt), but OWASP says to consider Argon, bcrypt, or scrypt.

Long and the short: as long as other people can write plugins and the default of either/both bcrypt and PBKDF2 are there, it seems fine.

Ah yes. The htpasswd approach looks good. I guess I would just suggestion calling out the -B flag more explicitly. My general advice is that if someone copy/pasted the instructions, they would do the most secure thing. Right now I think the instructions https://github.com/gocd/filebased-authentication-plugin leave too many choices. If I can find a little time, I can do a pull request for the readme with the approach I'd suggest and you can take it or leave it.

I know when I tried to get the plugin adopted as a core feature, Aravind had some concern about having a sound approach to migration. Is that no longer an issue?

Daniel

On Mon, Jun 26, 2017 at 11:51 PM Ketan Padegaonkar notifications@github.com wrote:

Is there any reason to use PBE or PBKDF2 at all? I had it in my plugin mostly to make sure my interface was complete, but bcrypt seems like the best option these days.

It's a NIST recommendation, so we think it's worth the (small) effort to add it in.

could you provide a tool (like the one with the plugin) or at least instructions on generating bcrypt entries?

As @dudadornelles https://github.com/dudadornelles mentions, htpasswd -B does the trick, we're building a CLI ( gocd/filebased-authentication-plugin#18 https://github.com/gocd/filebased-authentication-plugin/pull/18) to allow you to effectively alias htpasswd="java -jar .." and perform bcrypt and PBKDF2 using a (mostly) drop-in replacement.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/danielsomerfield/go-strong-auth-plugin/issues/3#issuecomment-311269183, or mute the thread https://github.com/notifications/unsubscribe-auth/AFvbXHJ8gayUfWdS5ZUXDcJgxpfUaIB1ks5sIKZ1gaJpZM4OCAhh .

arvindsv commented 7 years ago

@danielsomerfield - We're probably going to try a more forceful approach, with some warnings in the app at some point soon. No migration, but will need rekeying. Is that your understanding too, @ketan?

ketan commented 7 years ago

@danielsomerfield - We're probably going to try a more forceful approach, with some warnings in the app at some point soon. No migration, but will need rekeying. Is that your understanding too, @ketan?

Yes, that's my understanding. Not sure about the pushback and things breaking when passwords just stop working...

danielsomerfield commented 7 years ago

I think that's the right call, despite potential customer pushback. You can do migrations, but then you have to draw a hard line at some point and prompt the user for a new password since they're all potentially compromised. Hard to do if you don't have the UI for password changes.

I think it's better to risk pushback than to risk compromise.

On Tue, Jun 27, 2017 at 8:37 AM Aravind SV notifications@github.com wrote:

@danielsomerfield https://github.com/danielsomerfield - We're probably going to try a more forceful approach, with some warnings in the app at some point soon. No migration, but will need rekeying. Is that your understanding too, @ketan https://github.com/ketan?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/danielsomerfield/go-strong-auth-plugin/issues/3#issuecomment-311397273, or mute the thread https://github.com/notifications/unsubscribe-auth/AFvbXJQik5sPB_nI0_oOMOidQjzK35nFks5sISGagaJpZM4OCAhh .

arvindsv commented 7 years ago

I think in-app warnings are key - with a reasonable schedule before being cut off. We've talked about having the plugin manage the file as well (and I guess a UI or API for password changes). But, that won't happen soon, I feel. So, warning and making it easy to rekey will help. If someone skips versions though, it's a problem.