Remove MD5 digest from everywhere and default to SHA256 #31162

isbm · 2016-02-12T16:07:27Z

This removes obsolete MD5 digest that was by default everywhere. The new MD5 is SHA256. This is why.

thatch45 · 2016-02-12T16:53:52Z

salt/modules/tomcat.py

@@ -631,7 +631,7 @@ def deploy_war(war,

 def passwd(passwd,
           user='',
-           alg='md5',
+           alg='sha1',


wait, not sha256?

I was not sure if this is OK for Tomcat, since this module replaces that there. SHA1 OK but others I didn't looked up if this is compatible.

cachedout · 2016-02-12T16:56:14Z

@isbm We would like this to go into the develop branch instead of 2015.8.

@jfindlay We need to test to see what happens when the hashing algorithm is changed during an upgrade.

thatch45 · 2016-02-12T16:57:23Z

While I completely agree with this it should not go in a point release, this needs to be rebased on develop. Also, the primary use of md5 in salt is for file integrity and caches, and not for cryptographic means. The key fingerprints are just that, fingerprints.

This also begs the question, should we split out the hash usage for file integrity tasks to use crc and just bump the cryptographic hashes up to sha256?

Also, should we go ahead and build in support for alternative hashes like blake?

thatch45 · 2016-02-12T16:58:34Z

The main upgrade worry here is that all of the cache data for files, git backend etc will be invalidated, and we need to see if salt will just handle it, or if it will require a flush of the cache, and if it requires a cache flush we need to figure out how to make that automatic.

thatch45 · 2016-02-12T17:03:19Z

Keep in mind that it would need to be a controlled wipe

cachedout · 2016-02-12T17:07:48Z

Personally, I would prefer not to switch this out from underneath people. There are people who use Salt's cache from external systems and forcing a full cache wipe on an upgrade (or even on a config change) strikes me as too aggressive.

My preference here would be to warn people that they should change the hash type in their config for at least a few releases before we even consider a hard-switch like this.

isbm · 2016-02-15T10:05:18Z

The main problem here is that the fingerprints for the key exchange is using MD5 and in our eyes this is a "no go" and we can't ship that. Hence I've changed this to be at all configurable, since the fingerprint function although had default to MD5, nothing was passing anything else from the hash_type options. Others, who stil wants MD5 can still configure it that way.

Also since we operate at 2015.8 at the moment, we would like to have this there (too), while @cachedout told me earlier that you, guys, doing "upward" rebase on your own, so it is enough to just commit to the lowest supported?

@cachedout maybe it would help to leave MD5 by default in the configs and hash_type, but write a big banner in the config that this is not a good idea any more? Or keep default SHA256 and write in the docs "From now on we use SHA256, so change it yourself back". I would suggest second, so the new installations won't use MD5 by an accident. But you to decide...

Does it makes sense?

thatch45 · 2016-02-16T16:24:38Z

@isbm, I completely agree and understand. and @cachedout and myself have discussed how to best handle this.
What we want to do is accept what you have done here apart from changing the default and then add a deprecation warning to the develop branch that we will be migrating to sha256 in the future and what the ramifications are, that way we can migrate cleanly and we can have plenty of time for upgrade testing etc.
This also means that you would be able to set the default config in your packages to sha256 without issue

cachedout · 2016-02-18T18:47:49Z

@isbm Bump. How does what @thatch45 has outlined sound to you?

cachedout · 2016-02-22T23:59:22Z

Hi @isbm

One more bump. Could you please take a look at the above comments and respond to the latest from @thatch45 ? We'd still like to see this problem solved, so hopefully we can all get on the same page. :]

Thanks.

isbm · 2016-02-23T10:27:14Z

@thatch45 @cachedout sorry being late here (we were busy on other PRs here). So what I understand is:

Make MD5 available by default as is.
Add deprecation warning
Rebase (or cherry-pick) that for develop branch.

Question: Where/how to add that deprecation warning other than just write some commented out block in the config files of /etc/salt/m*?

thatch45 · 2016-02-23T17:09:26Z

We want the deprecation warning to show up in the logs each time the master/minion starts. So we would add a check to see if the config value is set to anything lower than sha256 at the end of the config loading process in salt/config/init.py. Then use Salt's deprecation system to flag it.

cachedout · 2016-02-25T17:59:14Z

Hi @isbm Just checking in to see how we're coming along with this one. Let me know if there's anything we can do. Thanks!

isbm · 2016-02-25T18:58:25Z

@thatch45 Yes, I will likely tackle it either tomorrow (Friday, CET) or Monday. One step at a time. 😉

cachedout · 2016-02-25T18:59:59Z

No hurry. Just checking in. Thanks, @isbm

isbm · 2016-02-26T17:36:57Z

@cachedout @thatch45 Here is an attempt to speak the same things across the daemons for hashes, hence the mixin for all of them.

The develop branch is "a bit" diverged, so I will up-port this thing there in the separate PR.

isbm · 2016-02-26T18:22:55Z

Added a Unit test, that verifies if a nag-message is shown in the logs in case hash_type is MD5 or SHA1.

thatch45 · 2016-02-26T22:15:14Z

This looks great @isbm ! Thanks for the work, and for begin understanding as we try to ease into things and be careful on all fronts!

rallytime · 2016-02-26T22:26:55Z

@isbm There's a small lint error. Would you mind cleaning that up? Thanks for your work here!

(The other test failure is not related.)

isbm · 2016-02-27T00:43:31Z

@rallytime Oops, overlooked. Done!

isbm · 2016-02-27T00:44:25Z

@thatch45 Thanks! I will "up-port" that to the develop branch and will test that prior.

cachedout · 2016-03-01T04:26:12Z

@isbm Is this now ready for review from your perspective?

isbm · 2016-03-01T08:57:11Z

@cachedout sure. I am now working on develop part, which seems a bit different. Please review it ASAP so I can fix what is still missing, if anything, and take care of it for the development and 2016.4

cachedout · 2016-03-03T18:22:26Z

@isbm This has merge conflicts. Could you please rebase it and then I'll take a look and see if we can get this in? Thanks.

…rent algorithms

…emons)

isbm · 2016-03-03T20:44:59Z

@cachedout Done.

Remove MD5 digest from everywhere and default to SHA256

Updates hash_type default to sha256 instead of md5 for new instances installed via salt-cloud. Fixes saltstack#32246 Refs saltstack#31162

isbm changed the title ~~Remove MD5 digest from everywhere and default to SHA1~~ Remove MD5 digest from everywhere and default to SHA256 Feb 12, 2016

thatch45 reviewed Feb 12, 2016
View reviewed changes

isbm force-pushed the isbm-md5-to-sha1 branch from 5a4f5fa to 0c121b1 Compare February 26, 2016 17:33

isbm mentioned this pull request Mar 2, 2016

Use SHA256 instead of MD5 (develop branch) #31626

Merged

2 tasks

Bo Maryniuk added 3 commits March 3, 2016 21:39

Set default checksum for key fingerprint to SHA1

413eca1

Set config hash_type to SHA1

8f9543c

Set defalt hash as SHA1 in config and explain why.

95cb59d

Bo Maryniuk added 18 commits March 3, 2016 21:41

Use hash_type configuration for the Cloud

f0d931f

Use configurable hash_type for general Key fingerprinting

7857177

Use SHA1 hash by default

0d4e4e3

Use SHA1 hash by default in Tomcat module, refactor for support diffe…

73f2df7

…rent algorithms

Use SHA1 by default instead of MD5

6198976

Remove SHA1 to SHA265 by default

efb78f1

Add note to the Tomcat module for SHA256

73b8d35

Remove sha1 to sha265

d5cb4dd

Remove SHA1 for SHA256

373493c

Remove SHA1 in favor of SHA256

5843257

Use MD5 hash algorithm by default (until deprecated)

36da8f5

Create a mixin class that will be reused in the similar instances (da…

82dd383

…emons)

Use mixin for the daemon classes

63eedef

Report environment failure, if any

95ec634

Verify if hash_type is using vulnerable algorithms

51f5562

Standardize logging

f3aecc0

Add daemons unit test to verify hash_type settings

327ea11

Fix PyLint

9d64abe

isbm force-pushed the isbm-md5-to-sha1 branch from 6466725 to 9d64abe Compare March 3, 2016 20:44

cachedout pushed a commit that referenced this pull request Mar 7, 2016

Merge pull request #31162 from isbm/isbm-md5-to-sha1

826fea6

Remove MD5 digest from everywhere and default to SHA256

cachedout merged commit 826fea6 into saltstack:2015.8 Mar 7, 2016

isbm deleted the isbm-md5-to-sha1 branch March 10, 2016 11:48

rallytime mentioned this pull request Mar 31, 2016

New salt-cloud instances should not use old hash_type default. #32289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove MD5 digest from everywhere and default to SHA256 #31162

Remove MD5 digest from everywhere and default to SHA256 #31162

isbm commented Feb 12, 2016

thatch45 Feb 12, 2016

isbm Feb 15, 2016

cachedout commented Feb 12, 2016

thatch45 commented Feb 12, 2016

thatch45 commented Feb 12, 2016

thatch45 commented Feb 12, 2016

cachedout commented Feb 12, 2016

isbm commented Feb 15, 2016

thatch45 commented Feb 16, 2016

cachedout commented Feb 18, 2016

cachedout commented Feb 22, 2016

isbm commented Feb 23, 2016

thatch45 commented Feb 23, 2016

cachedout commented Feb 25, 2016

isbm commented Feb 25, 2016

cachedout commented Feb 25, 2016

isbm commented Feb 26, 2016

isbm commented Feb 26, 2016

thatch45 commented Feb 26, 2016

rallytime commented Feb 26, 2016

isbm commented Feb 27, 2016

isbm commented Feb 27, 2016

cachedout commented Mar 1, 2016

isbm commented Mar 1, 2016

cachedout commented Mar 3, 2016

isbm commented Mar 3, 2016

Remove MD5 digest from everywhere and default to SHA256 #31162

Remove MD5 digest from everywhere and default to SHA256 #31162

Conversation

isbm commented Feb 12, 2016

thatch45 Feb 12, 2016

Choose a reason for hiding this comment

isbm Feb 15, 2016

Choose a reason for hiding this comment

cachedout commented Feb 12, 2016

thatch45 commented Feb 12, 2016

thatch45 commented Feb 12, 2016

thatch45 commented Feb 12, 2016

cachedout commented Feb 12, 2016

isbm commented Feb 15, 2016

thatch45 commented Feb 16, 2016

cachedout commented Feb 18, 2016

cachedout commented Feb 22, 2016

isbm commented Feb 23, 2016

thatch45 commented Feb 23, 2016

cachedout commented Feb 25, 2016

isbm commented Feb 25, 2016

cachedout commented Feb 25, 2016

isbm commented Feb 26, 2016

isbm commented Feb 26, 2016

thatch45 commented Feb 26, 2016

rallytime commented Feb 26, 2016

isbm commented Feb 27, 2016

isbm commented Feb 27, 2016

cachedout commented Mar 1, 2016

isbm commented Mar 1, 2016

cachedout commented Mar 3, 2016

isbm commented Mar 3, 2016