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

Modify remote_file cache_control_data to use sha256 for its name #3991

Merged
merged 3 commits into from
Nov 12, 2015

Conversation

jaym
Copy link
Contributor

@jaym jaym commented Sep 29, 2015

We want to support a fips mode, and doing MD5 with fips mode enabled
is wrong/hard. In this case, the type of checksum does not
matter, so let's just use sha256 since fips mode will be happy
with that.

For cases where the cache control data exists, we update it
to provide a seamless upgrade.

We want to support a fips mode, and doing MD5 with fips mode enabled
is wrong/hard. In this case, the type of checksum does not
matter, so let's just use sha256 since fips mode will be happy
with that.

For cases where the cache control data exists, we update it
to provide a seamless upgrade.
@@ -145,18 +145,51 @@ def load_data
end

def load_json_data
Chef::FileCache.load("remote_file/#{sanitized_cache_file_basename}")
path = sanitized_cache_file_path(sanitized_cache_file_basename)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of doing this as a one off, we should put some kind of version number in the metadata this time so we can do this more cleanly next time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general, that sounds like a good idea. I don't think it would have helped this case as the only the file name is incorrect, the data inside is exactly the same. Will look into adding this, may make the solution cleaner

@@ -38,7 +38,11 @@ def checksum_for_file(file)
end

def generate_checksum(file)
checksum_file(file, OpenSSL::Digest::SHA256.new)
if file.is_a?(StringIO)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to check for respond_to?(:read) ? I don't know why you'd need it, but it would allow you to File.open(thing, opts) { |f| generate_checksum(f) } (maybe that would be helpful if dealing with a gzipped file?).

@danielsdeleo
Copy link
Contributor

👍 there are some improvements that can be made, but I won't block merge on those if this work is time sensitive.

@thommay
Copy link
Contributor

thommay commented Oct 28, 2015

@jaym is this waiting on anything?

@lamont-granquist
Copy link
Contributor

👍

if you want to address Dan's respond_to?(:read) comment that'd be cool as well, that's an idiom that I've seen before.

thommay added a commit that referenced this pull request Nov 12, 2015
Modify remote_file cache_control_data to use sha256 for its name
@thommay thommay merged commit 880f744 into master Nov 12, 2015
@jaym jaym deleted the jdm/remote-file-sha2 branch January 27, 2016 04:47
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants