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

Make the shard plugin work under FIPS by using SHA2 instead of MD5 #1175

Merged
merged 8 commits into from
May 3, 2018

Conversation

coderanger
Copy link
Contributor

This makes life easier for most users since there is no reason to disable this plugin by default other than FIPS, and having to set optional_plugins is more of a barrier than I think we planned (I couldn't find an implementation of the cookbook-metadata-driven path we had talked about).

…y default if needed.

This makes life easier for most users since there is no reason to disable this plugin by default other than FIPS.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
@coderanger coderanger requested review from thommay, jaymzh and a team April 15, 2018 21:58
@coderanger
Copy link
Contributor Author

Fixes #1174

Copy link
Contributor

@thommay thommay left a comment

Choose a reason for hiding this comment

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

I think shard is just as niche, and that we should behave consistently across the board. It's much better for everyone to know that shard is an optional plugin and they need to enable it.

@coderanger
Copy link
Contributor Author

I disagree that the level of niche-ness is anywhere close, FIPS-mode is used by only places that actually require it, while shard_seed has been an advertised feature for a long time now. And the improved UX for almost everyone seems like it's worth the minor confusion for the tiny subset of FIPS users.

@coderanger
Copy link
Contributor Author

Summary of a chat on Slack, in an effort to reduce special-case-iness, I'm going to rework this so the plugin isn't disabled for any users, but will switch to SHA2 (or Adler?) if FIPS-mode is active. This means that enabling or disabling FIPS-mode will change the shard seed on all nodes, but since that is expected to happen approximately never, it's the least edge case approach.

@coderanger
Copy link
Contributor Author

I made this configurable for the potential edge case of actually needing to turn FIPS on and off, if you reeeeeally want to, you can force it to SHA2 even without FIPS. Also leaves room to add new stuff if needed.

@coderanger coderanger changed the title Use the new OpenSSL.fips_mode flag to only disable the shard plugin by default if needed Make the shard plugin work under FIPS by using SHA2 instead of MD5 Apr 16, 2018
@tas50
Copy link
Contributor

tas50 commented Apr 17, 2018

@coderanger You're missing a DCO signoff on a commit here

@@ -34,6 +33,27 @@ def default_sources
[:machinename, :serial, :uuid]
end

def default_digest_algorithm
if defined?(OpenSSL.fips_mode) && OpenSSL.fips_mode
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you're not just using the data we already gather from the FIPS plugin? We gather data in very specific ways within that plugin per platform.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No specific reason, this seemed more accurate. It was only added in Ruby 2.5 which is why I'm checking defined? and probably why we didn't use it before :)

…-mode.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
@coderanger
Copy link
Contributor Author

coderanger commented Apr 19, 2018

Fixed DCO, my script apparently forgot my name and email ¯\_(ツ)_/¯

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
This makes things work basically the same on all platforms, as much as possible.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
@coderanger
Copy link
Contributor Author

By the power of scope creep, this now adds support for Windows to the Shard plugin, as well as best-effort support for all other OSes.

… number on Windows.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>

it "should provide a shard with a default-safe set of sources" do
# Note: this is different than the other defaults.
expect(subject).to eq(253499154)
Copy link
Collaborator

Choose a reason for hiding this comment

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

above you raise an error in the default case

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, nevermind, only if it's not one of the default ones.

Signed-off-by: Noah Kantrowitz <noah@coderanger.net>
@jaymzh
Copy link
Collaborator

jaymzh commented May 2, 2018

Thanks for keeping the defaults compatible @coderanger !

@coderanger
Copy link
Contributor Author

@jaymzh With the current defaults baked down to actual ints in the tests now, we should very very notice if we ever break compat :)

@thommay thommay merged commit 2b7d307 into chef:master May 3, 2018
@lock
Copy link

lock bot commented Jan 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2019
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.

None yet

4 participants