Skip to content

Conversation

@GabrielNagy
Copy link
Contributor

Puppet's digest_algorithm setting defaults to md5, which is outdated. This commit switches it to to sha256.

Update spec tests accordingly.

@GabrielNagy GabrielNagy requested review from a team September 3, 2020 13:18
@GabrielNagy
Copy link
Contributor Author

@joshcooper I skipped the following test

xit "fails if the contents collide with existing contents" do
since there are no known SHA256 collisions. Should we change the default digest for this test to md5 and still run it?

@GabrielNagy GabrielNagy requested review from a team and joshcooper September 3, 2020 13:22
@joshcooper
Copy link
Contributor

The puppet agent multiple agents running exits if an agent is already running transient seems to be failing often on Windows Ruby 2.7. Not sure what's up with that, maybe ruby is raising a different exception when the error happens?

@puppetcla
Copy link

CLA signed by all contributors.

Puppet's `digest_algorithm` setting defaults to `md5`, which is outdated.
This commit switches it to to `sha256`.

Update spec tests accordingly.
@GabrielNagy GabrielNagy force-pushed the PUP-10583/change-default-digest branch from 537b084 to f9ce353 Compare September 4, 2020 07:49
@joshcooper
Copy link
Contributor

Looks good! I forgot about a couple of other areas where md5 is still referenced/used:

  • lib/puppet/application/filebucket.rb: the help still references md5. Maybe better to say it defaults to Puppet[:default_digest]?
  • lib/puppet/defaults.rb: We should move md5 to the last position in the valid_digest_algorithms array (when fips is disabled) as it is also used to define the agent's Puppet[:supported_checksum_types], which is the agent's list of preferred algorithms when it requests a catalog.
  • lib/puppet/file_serving/http_metadata.rb: the collect method should prefer sha256 over md5.
  • lib/puppet/http/service/compiler.rb: the post_catalog method references the default checksum types. Maybe delete the "currently defaults to part"?
  • lib/puppet/http/service/fileserver.rb: checksum_type parameter in the get_file_metadata comments need updating
  • lib/puppet/type/file/checksum.rb: description needs updating

@GabrielNagy GabrielNagy force-pushed the PUP-10583/change-default-digest branch from 6a0004a to 5ceb33c Compare September 7, 2020 13:34
@GabrielNagy
Copy link
Contributor Author

@joshcooper I pushed a separate commit with the latest changes so it's easier to review. Let me know if I missed anything.

There was some non-ASCII whitespace at 5ceb33c#diff-908b50f77db1b5645f5c125514deccd3R798 which I removed.

@Dorin-Pleava
Copy link
Contributor

LGTM, also locally tested filebucket, and it works as expected.

Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

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

LGTM. One nit, could you update the commit message for 5ceb33c to mention that it changes the checksum precedence.

@joshcooper
Copy link
Contributor

jenkins please test this on redhat7-64a,ubuntu1804-64a,windows2019-64a with servertests

Prefer `sha256` and other SHA algorithms over `md5` and `md5lite`.

Update docs to refer to SHA256 as the default digest algorithm.

Change md5-related variable names in the filebucket application to more
general names.
Update filebucket specs to not make assumptions about the type of digest
used.
@GabrielNagy GabrielNagy force-pushed the PUP-10583/change-default-digest branch from 983310f to 367db85 Compare September 10, 2020 07:24
Now that sha256 is the default digest, md5 filebucketing fails.

Previously a sha256 manifest was only applied on FIPS agents. Change the
test to always use sha256.
@GabrielNagy GabrielNagy force-pushed the PUP-10583/change-default-digest branch from 367db85 to efa72c2 Compare September 10, 2020 07:45
@GabrielNagy
Copy link
Contributor Author

jenkins please test this on redhat7-64a,ubuntu1804-64a,windows2019-64a with servertests

@joshcooper joshcooper merged commit 1b28327 into puppetlabs:main Sep 11, 2020
@joshcooper
Copy link
Contributor

joshcooper commented Sep 11, 2020

Dang, I forgot there were some CI failures when running medium tests manually. Could you update these tests?

bx rake ci:test:aio TESTS=tests/parser_functions/calling_all_functions.rb,tests/puppet_apply_a_file_should_create_a_file_and_report_the_md5.rb,tests/resource/file/content_attribute.rb,tests/resource/file/source_attribute.rb,tests/ticket_6541_invalid_filebucket_files.rb SHA=e6c176914d8a1effe6920843fd05b755def07ed0 SERVER_VERSION=6.13.1.SNAPSHOT.2020.09.04T1735 TEST_TARGET=redhat7-64a OPTIONS='--preserve-hosts=always'
...
Failed Tests Cases:
  Test Case tests/parser_functions/calling_all_functions.rb reported: #<Minitest::Assertion: digest output didn't match expected value...
  Test Case tests/puppet_apply_a_file_should_create_a_file_and_report_the_md5.rb reported ...
  Test Case tests/ticket_6541_invalid_filebucket_files.rb reported:...

GabrielNagy added a commit to GabrielNagy/puppetlabs-yumrepo_core that referenced this pull request Sep 15, 2020
The default digest algorithm was changed in puppet 7 to sha256.[1]

Update the acceptance test to work regardless of puppet version.

[1] puppetlabs/puppet#8315
GabrielNagy added a commit to GabrielNagy/puppetlabs-yumrepo_core that referenced this pull request Sep 15, 2020
The default digest algorithm was changed in puppet 7 to sha256.[1]

Update the acceptance test to work regardless of puppet version.

[1] puppetlabs/puppet#8315
gimmyxd added a commit to gimmyxd/puppetserver that referenced this pull request Oct 6, 2020
Puppet default digest algorithm was changed to sha256
puppetlabs/puppet#8315

This commit updates the compatibilty test to
specify the checksum explicitly as `sha256`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants