-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Jdm/fips #3945
Conversation
No longer very unit testy, however we will not have an md5 implementation when in that mode.
💩💩💩💩😿 |
@@ -690,6 +690,11 @@ def self.windows_home_path | |||
default :watchdog_timeout, 2 * (60 * 60) # 2 hours | |||
end | |||
|
|||
# Only use fips compliant algorithms | |||
default( :fips_mode ) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is a constant, it's probably best to use default :fips_mode, OpenSSL::OPENSSL_FIPS
(do
blocks are for computed defaults).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be default( :openssl_fips_mode), OpenSSL::OPENSSL_FIPS
We aren't actually building "fips_mode" for Chef, this is just a "can we use MD5 or not?" switch for the client. What the client would need to do for "fips_mode" would be massively more extensive, and we don't want to even think about starting to build that yet without having an audit to tell us what work would need to be done. And calling what we're building now "fips_mode" would be false advertising to people who read it in the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And mostly where I see this being used is in remote_file fallback code. If we find an MD5 checksum in the cache control data, but we have openssl_fips_mode set, then you're getting a new download. It won't actually be used there to switch people to using SHA256, since everyone will be upgraded to SHA256, but it will indicate the capabilities of the openssl library.
Possibly this shouldn't even be a configurable...
What will happen if a chef-client not in FIPS mode is told to run a cookbook with SHA256 sums in it? Can we just check the cookbook to see if it was FIPS'd? |
Chef::Digester.generate_md5_checksum_for_file(filepath) | ||
if Chef::Config.fips_mode | ||
# This will require a chef server that can handle | ||
# sha256 checksums |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a large undertaking, but would be a win for everyone. If we have enough customer demand that we're gonna do this, perhaps we could use Chef Server API versioning to make the transition easier. Also, does FIPS mode completely disable MD5? For a smooth upgrade, you'd want to probably include both SHA2-256 and MD5, so that a newer version of knife could upload a cookbook and Chef Server would have all the info required to serve it to older chef-client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danielsdeleo if you ask openssl to take the md5 of something, and fips mode is enabled, it will raise an error. If you ask ruby through digest/md5
, it will abort the process. This is why we need to use OpenSSL::Digest
instead of Digest
.
Right now, the plan is that we will have different omnibus packages with fips mode enabled. So while a regular build of chef would be able to accept both, the same would not be true for the build with openssl running in fips mode
Just to echo some internal discussion here. This will also require Chef Server changes for cookbook uploads to work correctly. |
@@ -153,7 +153,15 @@ def sanitized_cache_file_basename | |||
# human-readable but within the bounds of local file system | |||
# path length limits | |||
scrubbed_uri = uri.gsub(/\W/, '_')[0..63] | |||
uri_md5 = Chef::Digester.instance.generate_md5_checksum(StringIO.new(uri)) | |||
uri_md5 = if Chef::Config.fips_mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to make this a separate patch and make an upgrade path for everyone to use SHA2 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, do it for everyone and transparently upgrade from MD5 to SHA2 on reads without re-download.
Modulo the cache_control_data question, I'm 👍 on this as an incremental step. There is a ton of work to get chef server working with this, and it'll definitely be broken out of the box, but it's guarded by a feature flag so it's safe and we can run tests, which is good. |
This is the minimal set of changes needed for Chef to run when fips mode is enabled without crashing the process. There will be more once changes are made to the chef server.
There is also a good amount of work to do in getting all the tests to pass when that mode is enabled. We're working out on building a pipeline where that will be tested