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

Handle verify hostname ssl option #1428

Merged
merged 3 commits into from
Jun 30, 2022

Conversation

kazarin
Copy link
Contributor

@kazarin kazarin commented Jun 29, 2022

Description

Hi,
This is rebased version of #1172. Next part will be in net_http adapter

Todos

List any remaining work that needs to be done, i.e:

  • Tests
  • Documentation

Additional Notes

Optional section

cosmo0920 and others added 2 commits June 29, 2022 18:23
This had been introduced in CRuby's openssl v2.0.0.
And it has been bundled since CRuby 2.4.

ref: https://docs.ruby-lang.org/en/2.4.0/NEWS.html

Signed-off-by: Hiroshi Hatake <cosmo0920.oucc@gmail.com>

Refer verify_hostname on net/http adapter

But this option is just handled in Ruby master for now:
ruby/ruby@be6931f

Signed-off-by: Hiroshi Hatake <cosmo0920.oucc@gmail.com>

Prevent false-positive assertion

Signed-off-by: Hiroshi Hatake <cosmo0920.oucc@gmail.com>

Handle verify_hostname in excon adapter on SSL context

Signed-off-by: Hiroshi Hatake <cosmo0920.oucc@gmail.com>
@iMacTia
Copy link
Member

iMacTia commented Jun 30, 2022

@kazarin I took a quick look and the code changes seem good.
Apologies about the Rubocop issue, it seems like the latest minor version has removed a cop we've been explicitly enabling.
Feel free to remove it from .rubcop.yml, that should hopefully fix tests 👍

@kazarin
Copy link
Contributor Author

kazarin commented Jun 30, 2022

Done. As suggested in rubocop warning I am using now Gemspec/DeprecatedAttributeAssignment cop

@iMacTia
Copy link
Member

iMacTia commented Jun 30, 2022

That's even better! I didn't think it would have passed 😄 I'll have a final look

Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

Looks good 👍!
It should be relatively safe to merge considering this is basically a no-op and we'll need individual adapters to deal with this option internally.

Thanks for tackling this 🙌!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants