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

Feature/master/pup 1840 tunable digest algorithm #2537

Merged
merged 14 commits into from Apr 30, 2014

Conversation

Projects
None yet
4 participants
@adrienthebo
Copy link
Contributor

adrienthebo commented Apr 15, 2014

This refactors GH-2452 into smaller commits and simplifies how tests are run with all digest algorithms.

This supersedes GH-2452.

@puppetcla

This comment has been minimized.

Copy link

puppetcla commented Apr 16, 2014

CLA signed by all contributors.

@jaredjennings

This comment has been minimized.

Copy link
Contributor

jaredjennings commented Apr 16, 2014

These changes look complete to me. This pull request is indeed a better replacement for GH-2452.

Do you think the setting should be called file_checksum_algorithm rather than digest_algorithm, to avoid terminology conflicts with the SSL parts of Puppet?

@adrienthebo

This comment has been minimized.

Copy link
Contributor

adrienthebo commented Apr 16, 2014

I think that right now the only major use of the digest algorithm is in file checksumming, but I don't think it's limited to that. Given https://github.com/puppetlabs/puppet/pull/2453/files it's reasonable to see other components will need to use a digest algorithm so I think it's good to keep it generic.

@jaredjennings

This comment has been minimized.

Copy link
Contributor

jaredjennings commented Apr 16, 2014

Good point; I agree. Now I think the only thing in the way is the migration concerns (http://projects.puppetlabs.com/issues/8120#note-10).

@@ -688,6 +688,11 @@ def self.default_diffargs
:type => :duration,
:desc => "The window of time leading up to a certificate's expiration that a notification
will be logged. This applies to CA, master, and agent certificates. #{AS_DURATION}"
},
:digest_algorithm => {
:default => 'md5',

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

The type of this setting should be an enum that only allows the valid values.

@@ -23,6 +25,9 @@ def initialize(hash = {})
@local_path = nil
@rest_path = "https://#{server}:#{port}/#{environment}/file_bucket_file/"
end
@checksum_type = Puppet[:digest_algorithm] || Puppet::Util::Checksums::DEFAULT_DIGEST_ALGORITHM

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

Based on the definition of digest_algorithm I don't see how it would ever be nil or false, so trying to default it here isn't needed.

This comment has been minimized.

@adrienthebo

adrienthebo Apr 22, 2014

Contributor

I was running into issues where applying the settings catalog could call into this code, and the default value had not necessarily been applied. Does this sound possible or was I mistaking the source of the issue?

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

I just tried out simplifying all of these calls and haven't encountered a problem yet. I haven't put it through many different scenarios yet, however.

@@ -23,6 +25,9 @@ def initialize(hash = {})
@local_path = nil
@rest_path = "https://#{server}:#{port}/#{environment}/file_bucket_file/"
end
@checksum_type = Puppet[:digest_algorithm] || Puppet::Util::Checksums::DEFAULT_DIGEST_ALGORITHM
@checksum_type = @checksum_type.intern unless @checksum_type.is_a? Symbol

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

Once the above line is fixed to just use the setting value, there isn't any need for the Symbol check.

This comment has been minimized.

@adrienthebo

adrienthebo Apr 22, 2014

Contributor

If a user has digest_algorithm = sha256 set in puppet.conf, will the value of Puppet[:digest_algorithm] also be a string? Are enum values automatically converted from Strings to Symbols?

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

It will be a string, but it will always be a string so you can just blindly call intern or to_sym.

def initialize(contents, options = {})
raise ArgumentError.new("contents must be a String, got a #{contents.class}") unless contents.is_a?(String)
@contents = contents

@bucket_path = options.delete(:bucket_path)
Puppet.settings.use('main')
@checksum_type = Puppet[:digest_algorithm] || Puppet::Util::Checksums::DEFAULT_DIGEST_ALGORITHM
@checksum_type = @checksum_type.intern unless @checksum_type.is_a? Symbol

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

Same comments as in the dipper file.

def initialize(contents, options = {})
raise ArgumentError.new("contents must be a String, got a #{contents.class}") unless contents.is_a?(String)
@contents = contents

@bucket_path = options.delete(:bucket_path)
Puppet.settings.use('main')

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

Calling Puppet.settings.use here isn't a good idea. It forces puppet to apply a settings catalog, which it has probably already done. I think this line should just go away.

This comment has been minimized.

@adrienthebo

adrienthebo Apr 22, 2014

Contributor

When the settings catalog is compiled it's possible for file resources to back up files to the filebucket, so it'll call into this code. This all really hinges on when Puppet settings receive their default values; do you think it's possible to reliably have those set before

To make things more complex, if files are backed up during the settings catalog I think it may be possible for md5 to be used for that catalog, and then after the settings are parsed from conf files then it may switch to a different algorithm. Should we worry about that right now?

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

It seems like it using md5 at any point along the way presents a problem because if it is on a FIPS system md5 isn't allowed. I thought one of the major reasons for this change was so that puppet worked better on FIPS compliant systems.

This comment has been minimized.

@adrienthebo

adrienthebo Apr 22, 2014

Contributor

I don't believe that the settings catalog was taken into consideration, and it complicates things. Off the top of your head do you know if puppet.conf is read and evaluated before the settings catalog is applied?

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

Puppet.conf should be entirely read before the settings catalog is considered.

This comment has been minimized.

@jaredjennings

jaredjennings Apr 22, 2014

Contributor

I don't know what the settings catalog is. I think I added this line because when I ran the code under test from the spec examples without this, it didn't seem to be paying attention to the assignments I had made to Puppet[:digest_algorithm]. I believe I saw Puppet.settings.use used somewhere else in the code and assumed that it meant, "pay attention to this hunk of the settings from puppet.conf as defined in puppet/defaults.rb." Sounds like I was wrong.

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

It really does look like that is what it would mean :)

The "Settings Catalog" is kinda, halfway, sorta what you said. It is the way that puppet manages the ownership, mode, and creation of some settings that refer to files or directories. The sections in lib/puppet/defaults.rb are referenced by use and only are useful for controlling which subset of the settings to try to control on the system.

@@ -11,6 +12,7 @@ class Puppet::FileBucket::Dipper

# Create our bucket client
def initialize(hash = {})
Puppet.settings.use :main

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

Calling Puppet.settings.use here isn't a good idea. It forces puppet to apply a settings catalog, which it has probably already done. I think this line should just go away.

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

Technically it doesn't force it since it won't do anything if :main has already been used, but I still don't think it should be calling use here.

@checksum_type ||= "md5"
Puppet.settings.use :main
@checksum_type ||= Puppet[:digest_algorithm]
@checksum_type ||= Puppet::Util::Checksums::DEFAULT_DIGEST_ALGORITHM

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

Same comments as in the dipper and file files.

@@ -9,6 +9,10 @@ class File < Puppet::Indirector::Code

desc "Store files in a directory set based on their checksums."

def initialize
Puppet.settings.use(:main)

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

Same again as dipper and file.

defaultto :md5
defaultto do
algo = Puppet[:digest_algorithm] || Puppet::Util::Checksums::DEFAULT_DIGEST_ALGORITHM
algo = algo.intern unless algo.is_a? Symbol

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

This also shouldn't need to defaulting behavior. And then it shouldn't be doing the Symbol check.

# Return the appropriate digest algorithm with fallbacks in case puppet defaults have not
# been initialized.
def digest_algorithm
value || Puppet[:digest_algorithm] || Puppet::Util::Checksums::DEFAULT_DIGEST_ALGORITHM

This comment has been minimized.

@zaphod42

zaphod42 Apr 22, 2014

Contributor

And once again shouldn't need the defaulting behavior.

@puppetcla

This comment has been minimized.

Copy link

puppetcla commented Apr 23, 2014

CLA signed by all contributors.

@adrienthebo

This comment has been minimized.

Copy link
Contributor

adrienthebo commented Apr 28, 2014

The changes made in this pull request are uncovering some very bad stubbing in spec/unit/type/file/content_spec.rb; I'm going to work on correcting those specs. Please hold off on merging this until I've wrapped things up.

jaredjennings and others added some commits Mar 18, 2014

(PUP-1840) Add a digest_algorithm Puppet setting, defaulting to md5
Without this patch applied, Puppet only performs checksums of files
using the MD5 algorithm. MD5 is not available on hosts configured for
FIPS 140-2 compliance, so Puppet fails. This patch adds a setting,
settable in puppet.conf, whose name is digest_algorithm and whose
value can be any checksum type known to Puppet::Util::Checksums. The
setting indicates which digest algorithm should be used for performing
checksums. (This patch does not contain the code that pays attention
to the setting.) The value defaults to md5, so that if you do not set
this setting yourself, Puppet will act like it always has before.
(PUP-1840) Ease testing under different digest_algorithm settings
Without this patch, spec tests for pieces of the code having to do
with checksums would have to be duplicated twice and contain repeated
code.

The patch adds the `using_checksums_describe` method, which can be used
instead of the `describe` method to enclose spec tests where checksums
are involved. This method only defined inside describe blocks where the
`:uses_checksums => true` user metadata is given, to mitigate the risk
of unintended changes to the behavior of spec tests not having to do
with checksums.

`using_checksums_describe` defines a group of examples, each having the
given block as body. Each example happens with a different value set
for `digest_algorithm`. Many `let`s are defined for use inside the
spec test; foremost are `plaintext`, `checksum`, and
`algo`. `plaintext` is some text; `checksum` is the checksum for this
text under the present setting of `digest_algorithm`. `algo` contains
the name of the digest algorithm in use.
unset, `algo` is 'md5'.

Code restructured and simplified by Adrien Thebo <adrien@puppetlabs.com>
(maint) checksum methods should be class methods
Before this change, using a checksum algorithm required creating a class
and including the Checksums module. Since there's no state required for
the checksum methods, this was a silly thing to need to do.

This commit makes the module extend itself, so that the methods can be
invoked by doing `Puppet::Util::Checksums#method`.

In the long run, this module should use the `module_function`
declaration so that the checksum methods are private when included.
(PUP-1840) Respect digest_algorithm in file metadata
Original code implemented by Jared Jennings <jared.jennings.ctr@us.af.mil>
(PUP-1840) Respect digest algorithm in filebucket file
Original code implemented by Jared Jennings <jared.jennings.ctr@us.af.mil>
(PUP-1840) Respect digest algorithm in filebucket dipper
Original code implemented by Jared Jennings <jared.jennings.ctr@us.af.mil>
(PUP-1840) Use configured digest_algorithm as default checksum_type f…
…or file resources

Without this patch, file resources will always try to checksum
themselves using MD5. On FIPS 140-2 compliant hosts, this will
fail. This patch adds sha256 as a permissible value for the File
resource's checksum parameter, and makes the checksum parameter
default to using the digest_algorithm, as set in the puppet.conf.
(PUP-1840) Remove use of "use"
Puppet.settings.use isn't needed in these areas of code and we can rely
on the setting having a value. Also by putting the validation on the
setting itself no other areas need to validate it.
(PUP-1840) Remove unused constant
There isn't any need for the DEFAULT_DIGEST_ALGORITHM anymore since
everything can use the setting.
(PUP-1840) Remove magic variables from shared context
Putting these values in shared context just makes it harder on any
readers to track them down later. They are only used in 2 places and so
can be put there.
(maint) Remove redundant and useless tests
Some of these tests were preserved simply because they are there. On
closer inspection many were not needed because either they tested
functionality that simply doesn't exist (and it isn't very useful to
state that the functionality doesn't exist), stated the condition
incorrectly and misleadingly, or are handled in other places.

zaphod42 and others added some commits Apr 24, 2014

(PUP-1840) Use module methods of Puppet::Util::Checksum
Rather than mixing in methods that are then looked up we can make the
code a little more readable by clarifying that it is getting methods
from Puppet::Util::Checksum.
@adrienthebo

This comment has been minimized.

Copy link
Contributor

adrienthebo commented Apr 28, 2014

Okay, I think I'm happy with this pull request again.

zaphod42 added a commit that referenced this pull request Apr 30, 2014

Merge pull request #2537 from adrienthebo/feature/master/pup-1840-tun…
…able-digest-algorithm

Feature/master/pup 1840 tunable digest algorithm

@zaphod42 zaphod42 merged commit 282837f into puppetlabs:master Apr 30, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@adrienthebo adrienthebo deleted the adrienthebo:feature/master/pup-1840-tunable-digest-algorithm branch Apr 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment