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.

Sign up for GitHub

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

Jump to bottom

(MODULES-1737) Add pw_hash() function #408

Merged
merged 1 commit into from
Apr 9, 2015
Merged

(MODULES-1737) Add pw_hash() function #408

merged 1 commit into from
Apr 9, 2015

Conversation

elyscape
Copy link
Contributor

@elyscape elyscape commented Feb 3, 2015

As per MODULES-1737, this PR provides a method of generating password hashes. Tests coming.

@elyscape elyscape force-pushed the feature/pw_hash branch 13 times, most recently from 25dcef9 to 3a75f94 Compare February 3, 2015 02:17
@elyscape
Copy link
Contributor Author

elyscape commented Feb 3, 2015

Tests added.

@elyscape
Copy link
Contributor Author

Rebased onto master, so tests should pass soon.

@cyberious
Copy link
Contributor

Please update to have graceful failure on unsupported platforms for this function. They way it is currently written if attempted on Windows it will fail hard.

hunner
hunner reviewed Feb 19, 2015
View reviewed changes
lib/puppet/parser/functions/pw_hash.rb

# work around JRuby bug in String::crypt for JRuby < 1.7.17
if RUBY_PLATFORM == 'java' and 'test'.crypt('$1$1') != '$1$1$Bp8CU9Oujr9SSEw53WV6G.'
def password.crypt(salt)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not just module String def crypt ... or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could change it to that. It was based on some code I found elsewhere to work around this issue.

@elyscape
Copy link
Contributor Author

@cyberious I'm not sure what you mean. Ruby provides a basic implementation of String#crypt() on Windows, albeit one that only offers the default DES hashing scheme.

daenney
daenney reviewed Mar 5, 2015
View reviewed changes
lib/puppet/parser/functions/pw_hash.rb
return nil if password.empty?

# work around JRuby bug in String#crypt for JRuby < 1.7.17
if RUBY_PLATFORM == 'java' and 'test'.crypt('$1$1') != '$1$1$Bp8CU9Oujr9SSEw53WV6G.'
Copy link

Choose a reason for hiding this comment

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

Why are we only salting on the JVM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We salt everywhere. See line 42. This just checks for a faulty String#crypt implementation that doesn't support extended hashes, which is known to exist in older versions of JRuby, and overrides it with a proper Java implementation of crypt(). This weak implementation also exists on MRI on Windows, but we don't have a proper Java implementation under that scenario. I could change this to fail if the implementation is weak and we're not on Java, though. That seems like it would probably be a good idea.

@elyscape
Copy link
Contributor Author

elyscape commented Mar 5, 2015

Documentation and checking for unsupported platforms has been added. Working on a spec test for unsupported platforms now.

@elyscape
Copy link
Contributor Author

elyscape commented Mar 5, 2015

Spec test is in. Pinging @daenney, @cyberious, and @hunner for review. If the review is positive, I'll squash the commits together.

igalic
igalic reviewed Mar 6, 2015
View reviewed changes
README.markdown
|---------|---------------------|
|1 |MD5 |
|5 |SHA-256 |
|6 |SHA-512 (recommended)|
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we even implement hashes which we don't recommend?
why not make this the third, optional argument?

Copy link

Choose a reason for hiding this comment

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

There's a difference between "you shouldn't use this" and "we recommend you to use the strongest one".

@elyscape
Copy link
Contributor Author

elyscape commented Apr 1, 2015

Rebased onto master and squashed.

daenney
daenney reviewed Apr 4, 2015
View reviewed changes
lib/puppet/parser/functions/pw_hash.rb
are compatible before using this function.") do |args|
raise ArgumentError, "pw_hash(): wrong number of arguments (#{args.size} for 3)" if args.size != 3
raise ArgumentError, "pw_hash(): first argument must be a string" unless args[0].is_a? String
raise ArgumentError, "pw_hash(): #{args[1]} is not a valid hash type" unless ['1', '5', '6'].include? args[1].to_s
Copy link

Choose a reason for hiding this comment

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

Can we do this the other way around?

User-understanding wise I much prefer: pw_hash('lalalala', 'sha-256', 'oseatnoirsentaiore') over pw_hash('lalalala', '5', 'oseatnoirsentaiore').

The first case I can immediately see what's going on. The second case I need to look up some implementation detail around specifiers to understand what type of hash this function call will be computing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Give me a few minutes to make the change.

@elyscape
Copy link
Contributor Author

elyscape commented Apr 4, 2015

@daenney How's it look now?

@daenney
Copy link

daenney commented Apr 5, 2015

Much better but the tests are red.

the pw_hash function
should return nil if the first argument is empty (FAILED - 1)
should raise an ArgumentError if the second argument is an invalid hash type (FAILED - 2)
should raise an ArgumentError if the third argument is not a string (FAILED - 3)
should raise an ArgumentError if the third argument is empty (FAILED - 4)
should raise an ArgumentError if the third argument has invalid characters (FAILED - 5)
should fail on platforms with weak implementations of String#crypt (FAILED - 6)
should return a hashed password (FAILED - 7)
should use the specified salt (FAILED - 8)
should use the specified hash type (FAILED - 9)
should generate a valid hash (FAILED - 10)

@elyscape elyscape force-pushed the feature/pw_hash branch 2 times, most recently from 101a48b to 23be402 Compare April 6, 2015 00:21
@elyscape
Copy link
Contributor Author

elyscape commented Apr 6, 2015

@daenney I knew I forgot to do something! Fixed.

@daenney
Copy link

daenney commented Apr 6, 2015

@hunner @cyberious I'm okay with this. Up to you now.

hunner added a commit that referenced this pull request Apr 9, 2015
@hunner
Merge pull request #408 from elyscape/feature/pw_hash
487e8d4 
(MODULES-1737) Add pw_hash() function
@hunner hunner merged commit 487e8d4 into puppetlabs:master Apr 9, 2015
@elyscape elyscape deleted the feature/pw_hash branch April 9, 2015 18:00
@bmjen bmjen mentioned this pull request Apr 10, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@elyscape @cyberious @daenney @hunner @igalic @jordanbreen28