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-1715) Add FQDN-based random string generator #405

Merged
merged 1 commit into from
Apr 9, 2015
Merged

(MODULES-1715) Add FQDN-based random string generator #405

merged 1 commit into from
Apr 9, 2015

Conversation

elyscape
Copy link
Contributor

As requested in MODULES-1715, this PR provides fqdn_rand_string(), which generates random alphanumeric strings of the desired length. It also provides fqdn_rand_base64(), which generates random base64 strings of the desired length.

@elyscape
Copy link
Contributor Author

As a side note, the performance of these (especially fqdn_rand_string()) could be improved by bringing the contents of fqdn_rand() and, more importantly, Puppet::Util.deterministic_rand() inline, as that would allow reuse of the same random generator for successive characters/integers, but that would decrease maintainability and I wasn't sure if the tradeoff was worth it. If you think that it would be a worthwhile change, let me know and I'll make it happen.

@elyscape
Copy link
Contributor Author

elyscape commented Feb 2, 2015

I have added acceptance tests for the new functions. Let me know if I should squash some/all of these commits.

@elyscape elyscape force-pushed the feature/fqdn_rand_strings branch 2 times, most recently from f0d23c0 to cafdad5 Compare February 2, 2015 20:55
@elyscape
Copy link
Contributor Author

elyscape commented Feb 2, 2015

It occurred to me that I didn't document the functions in the README, so I fixed that. I squashed it into the first commit and squashed the two test commits together as well.

@elyscape elyscape force-pushed the feature/fqdn_rand_strings branch 2 times, most recently from 7b4d900 to 2c9da0f Compare February 6, 2015 16:47
@elyscape
Copy link
Contributor Author

The test failures are due to the release of rspec 3.2.0, which breaks some things.

@elyscape elyscape force-pushed the feature/fqdn_rand_strings branch 2 times, most recently from 93406c7 to d670665 Compare February 12, 2015 22:04
@elyscape
Copy link
Contributor Author

Rebased onto master, so tests should pass soon.

@elyscape
Copy link
Contributor Author

I went ahead and squashed this down into one commit.

@hunner
Copy link
Contributor

hunner commented Feb 19, 2015

I can see random numbers and strings... but why base64?

@elyscape
Copy link
Contributor Author

Among other things, it can be used to generate password salts. The POSIX spec says that the valid character set for password hash salts is a slightly-modified base64 (+ replaced with .).

@daenney
Copy link

daenney commented Mar 5, 2015

But now you're asking us to include a base64 implementation which cannot be used for the salts according to the POSIX spec. You're countering your own point here.

@hunner
Copy link
Contributor

hunner commented Mar 5, 2015

I'd actually rather have fqdn_rand_string() take an argument of the character set from which to generate a random string, then base64, or base64 with . instead of +, or random UTF-8 emoticons... etc. Something generic.

@elyscape
Copy link
Contributor Author

elyscape commented Mar 5, 2015

Good points. I'll modify fqdn_rand_string() to accept a charset as a string.

@elyscape elyscape force-pushed the feature/fqdn_rand_strings branch 3 times, most recently from c6a0f1b to c00d221 Compare March 5, 2015 23:21
@elyscape
Copy link
Contributor Author

elyscape commented Mar 5, 2015

Custom character sets added. Pinging @daenney and @hunner for review. If the review is positive, I'll squash it down.

@elyscape elyscape changed the title (MODULES-1715) Add FQDN-based random string and base64 generators (MODULES-1715) Add FQDN-based random string generator Mar 6, 2015
@elyscape
Copy link
Contributor Author

elyscape commented Apr 1, 2015

Rebased onto master and squashed. Pinging @daenney and @hunner for review.

@elyscape elyscape mentioned this pull request Apr 1, 2015
Closed
hunner added a commit that referenced this pull request Apr 9, 2015
@hunner
Merge pull request #405 from elyscape/feature/fqdn_rand_strings
8fba5c0 
(MODULES-1715) Add FQDN-based random string generator
@hunner hunner merged commit 8fba5c0 into puppetlabs:master Apr 9, 2015
@elyscape elyscape deleted the feature/fqdn_rand_strings branch April 9, 2015 17:59
cmurphy pushed a commit to cmurphy/puppetlabs-stdlib that referenced this pull request Apr 10, 2015
Fix acceptance tests for puppetlabs#405
6731f1a 
This fixes the acceptance tests by:
 - Ensuring the fqdn_rand spec is passed undef as the second parameter
   so that the seed is not used as the charset
 - Ensuring the pw_hash spec is passed the key specifying the type of
   hash, rather than the value that will be used to generate the
   password
 - Removing the fqdn_rand_base64 test because there is no such function
cmurphy pushed a commit to cmurphy/puppetlabs-stdlib that referenced this pull request Apr 10, 2015
Fix acceptance tests for puppetlabs#405
65116da 
This fixes the acceptance tests by:
 - Ensuring the fqdn_rand_string spec is passed undef as the second
   parameter so that the seed is not used as the charset
 - Ensuring the pw_hash spec is passed the key specifying the type of
   hash, rather than the value that will be used to generate the
   password
 - Expecting puppet to report nil instead of empty string for undef
   passwords
 - Removing the fqdn_rand_base64 test because there is no such function
bmjen added a commit that referenced this pull request Apr 10, 2015
@bmjen
Merge pull request #433 from cmurphy/fix_acceptance
cf7dbef 
Fix acceptance tests for #405
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.
4 participants
@elyscape @hunner @daenney @jordanbreen28