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.

Sign up for GitHub

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

Support verify_hostname #25

Merged
merged 3 commits into from
Sep 16, 2024
Merged

Support verify_hostname #25

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
17e8020
Support verify_hostname
stefanmb Sep 9, 2024
01f9c79
Linter fix
stefanmb Sep 10, 2024
96882fa
Review comments
stefanmb Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion faraday-net_http_persistent.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,5 @@ Gem::Specification.new do |spec|
spec.require_paths = ["lib"]

spec.add_dependency "faraday", "~> 2.5"
spec.add_dependency "net-http-persistent", "~> 4.0"
spec.add_dependency "net-http-persistent", "~> 4.0.4"
Copy link
Member

Choose a reason for hiding this comment

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

This change will also lock the version to be <= 4.1, which is probably too restrictive.
If we need 4.0.4 as a minimum version, but still want to allow any version until 5.0, we could do either:

  • "~> 4.0", ">= 4.0.4"; or
  • ">= 4.0.4", "< 5"

Copy link
Member

Choose a reason for hiding this comment

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

The alternative is to leave ~> 4.0 as the min version and change the code to support both version (maybe with a respond_to? check. If we do this, you can disregard my other comment on specs.

Personally tough, I don't think it's gonna be a big ask to ask users to upgrade form 4.0+ to 4.0.4+, so happy to keep the higher version constraint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

happy to keep the higher version constraint

Done, I've opted for ">= 4.0.4", "< 5" since it's very literal.

end
5 changes: 3 additions & 2 deletions lib/faraday/adapter/net_http_persistent.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ def request_with_wrapped_block(http, env)
ca_file: :ca_file,
ssl_version: :version,
min_version: :min_version,
max_version: :max_version
max_version: :max_version,
verify_hostname: :verify_hostname
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Faraday defaults this option to true, so we don't need to hardcode any default in the adapter.

}.freeze

def configure_ssl(http, ssl)
Expand All @@ -193,7 +194,7 @@ def configure_ssl(http, ssl)
http_set(http, :cert_store, ssl_cert_store(ssl))

SSL_CONFIGURATIONS
.select { |_, key| ssl[key] }
.select { |_, key| !ssl[key].nil? }
.each { |target, key| http_set(http, target, ssl[key]) }
end

Expand Down
14 changes: 13 additions & 1 deletion spec/faraday/adapter/net_http_persistent_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,18 @@
expect(http.pool.size).to eq(5) if http.respond_to?(:pool)
end

it "allows to set verify_hostname in SSL settings to false" do
url = URI("https://example.com")

adapter = described_class.new(nil)

http = adapter.send(:connection, url: url, request: {})
adapter.send(:configure_ssl, http, verify_hostname: false)

# `verify_hostname` is only present in net_http_persistent >= 4.0.4
expect(http.verify_hostname).to eq(false) if http.respond_to?(:verify_hostname)
Copy link
Member

Choose a reason for hiding this comment

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

If we're depending on v4.0.4+, we don't need to check for the presence of verify_hostname

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, removed all the respond_to? checks for old net_http_persistent versions and their respective comments.

end

context "min_version" do
it "allows to set min_version in SSL settings" do
url = URI("https://example.com")
Expand All @@ -50,7 +62,7 @@
http = adapter.send(:connection, url: url, request: {})
adapter.send(:configure_ssl, http, min_version: :TLS1_2)

# `min_version` is only present in net_http_persistent >= 3.1 (UNRELEASED)
# `min_version` is only present in net_http_persistent >= 3.1
Copy link
Member

Choose a reason for hiding this comment

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

Since we now depend on Net::HTTP::Persistent v4+, we don't need this respond_to?(:min_version) check anymore 🙌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, removed all the respond_to? checks for old net_http_persistent versions and their respective comments.

expect(http.min_version).to eq(:TLS1_2) if http.respond_to?(:min_version)
end
end
Expand Down