-
Notifications
You must be signed in to change notification settings - Fork 157
Automatically use system openssl configuration on rpm builds #13625
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
Closed
+32
−2
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
25 changes: 25 additions & 0 deletions
25
packages/foreman/foreman-proxy/foreman-proxy-use-crypto-policies.patch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| --- a/lib/launcher.rb | ||
| +++ b/lib/launcher.rb | ||
| @@ -5,7 +5,6 @@ | ||
| require 'proxy/log_buffer/trace_decorator' | ||
| require 'sd_notify' | ||
|
|
||
| -CRYPTO_POLICIES_CONFIG = '/etc/crypto-policies/back-ends/opensslcnf.config'.freeze | ||
| TLS_MIN_VERSION_MAP = { | ||
| '1.0' => OpenSSL::SSL::TLS1_VERSION, | ||
| '1.1' => OpenSSL::SSL::TLS1_1_VERSION, | ||
| @@ -117,13 +116,7 @@ | ||
| return nil if configured&.empty? | ||
| return configured unless configured.nil? | ||
|
|
||
| - if File.exist?(CRYPTO_POLICIES_CONFIG) | ||
| - logger.info "Crypto-policies detected, using PROFILE=SYSTEM for TLS ciphers." | ||
| - 'PROFILE=SYSTEM' | ||
| - else | ||
| - logger.debug "No crypto-policies detected, using HIGH cipher string as default." | ||
| - 'HIGH' | ||
| - end | ||
| + 'PROFILE=SYSTEM' | ||
| end | ||
|
|
||
| def validate_tls_ciphers!(ciphers) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
On upstream, this either logs crypto-policies are used, or not.
On Satellite, this deletes any logging - there will be no information about crypto-policies.
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.
Would it make sense to log something that has no alternatives?
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.
I think it won't leave the user wondering. It's probably nice to see "yes, if I set crypto policies, they will be taken into account".
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.
Based on what @evgeni said in the comments I am reevaluating whether we should have this patch at all. If a RHEL system has crypto-policies enabled but no config file, that's a broken system and probably not something we should deal with. Opinions?
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.
@ekohl also said that the patch might not be necessary in here theforeman/smart-proxy#947 (comment)
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.
I thought the reason for this patch was to avoid autodetection during startup every time. If it isn't a problem (and I don't think it is), I also think this patch doesn't need to exist at all. A system either has crypto-policies or it doesn't, and we don't really care whether it's rpm based or something else based.
If a system is broken in a way that it does have crypto policies and doesn't have the file, it's nice to have this exception caught and communicated somehow, but for this case you don't need to explain things, just say that the file doesn't exist.
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.
Thank you!