Only set encryption option to net-ldap when needed. #16954
Conversation
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1542222 The net-ldap gem, which implements authentication mode ldap(s), requires encryption options when doing secure ldapS but can not handle empty encryption options when doing unsecure ldap. An empty encryption option can not be provided.
|
@jvlcek unrecognized command 'add', ignoring... Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone |
1 similar comment
|
@jvlcek unrecognized command 'add', ignoring... Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone |
|
@miq-bot assign @gtanzillo |
|
@jvlcek unrecognized command 'add', ignoring... Accepted commands are: add_label, assign, close_issue, move_issue, remove_label, rm_label, set_milestone |
|
@bdunne Please review, especially the spec file change. |
|
@miq-bot add_label gaprindashvili/yes |
|
@miq-bot add_label bug |
|
@miq-bot add_label authentication |
spec/lib/miq_ldap_spec.rb
Outdated
| end | ||
|
|
||
| it 'returns a hostname when a hostname is availble and does not set encryption options' do | ||
| ldap = MiqLdap.new(:mode => "ldap", :host => ["testhostname", "localhost", "dummy", @host]) |
There was a problem hiding this comment.
You could set this up with a let since it is the same in this context
There was a problem hiding this comment.
Sorry, I completely missed the ldap vs ldaps difference.
spec/lib/miq_ldap_spec.rb
Outdated
| it 'returns a hostname when a hostname is availble and does not set encryption options' do | ||
| ldap = MiqLdap.new(:mode => "ldap", :host => ["testhostname", "localhost", "dummy", @host]) | ||
| expect(ldap.ldap.host).to eq("testhostname") | ||
| expect(ldap.ldap.instance_variable_get(:@encryption)).to be_falsey |
There was a problem hiding this comment.
Can this line be added to the previous it since they're the same otherwise?
There was a problem hiding this comment.
Wait... will it be false or unset or nil?
|
Thank you @bdunne! I reworked the test subject lines to try to make I also test for nil instead of falsey when encryption options should not be provided. Thank you! JoeV |
|
Checked commits jvlcek/manageiq@e76128c~...d1dbc9a with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
| options.store_path(:encryption, :tls_options, :verify_mode, OpenSSL::SSL::VERIFY_NONE) if options[:host].ipaddress? | ||
|
|
||
| if mode == "ldaps" | ||
| options[:encryption] = {:method => :simple_tls} |
There was a problem hiding this comment.
Does options[:encryption] need to be a hash even when not using ldaps? I ask because it used to be an empty hash.
There was a problem hiding this comment.
No, options[:encryption] does not need to be a hash even when not using ldaps.
In fact this PR is fixing exactly that. options[:encryption] can not be an empty hash if it is not needed because net-ldap chokes on and empty hash for options[:encryption]
Making it an empty hash was the bug I introduced with #16850
that this PR is fixing.
Initially options[:encryption] was only set if mode == "ldaps" as follows:
options[:encryption] = {:method => :simple_tls} if mode == "ldaps"
Only set encryption option to net-ldap when needed. (cherry picked from commit 92faed2) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1543635
|
Gaprindashvili backport details: |
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1542222
The net-ldap gem, which implements authentication mode ldaps and ldap, requires
encryption options when doing secure ldapS but can not handle empty
encryption options when doing unsecure ldap.
This PR ensure no encryption options, empty or otherwise, are passed to net-ldap
when the encryption options are unneeded.
Links
issue this PR addresses.
Steps for Testing/QA
Configure an appliance for Authentication Mode: LDAP
Attempt to log in with a valid user should succeed.