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

Merge TLS verification patch from Faraday #340

Merged
merged 1 commit into from Jun 1, 2020
Merged

Conversation

igrigorik
Copy link
Owner

@igrigorik igrigorik commented May 30, 2020

Closes #339, CVE-2020-13482

Credit to Mislav Marohnić for original implementation, merged from Faraday.

Closes #339, CVE-2020-13482

Credit to Mislav Marohnić for original implementation, merged from
Faraday.
@agustingianni
Copy link

agustingianni commented Jun 1, 2020

LGTM

@igrigorik igrigorik merged commit e5fa144 into master Jun 1, 2020
2 checks passed

def ssl_handshake_completed
unless verify_peer?
warn "[WARNING; em-http-request] TLS hostname validation is disabled (use 'tls: {verify_peer: true}'), see" +
Copy link
Contributor

@aharbick aharbick Jun 14, 2020

Choose a reason for hiding this comment

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

Could this be conditional? If a caller explicitly doesn't want to verify_peer and they do

EventMachine::HttpRequest.new(url, opts.merge({tls: {verify_peer: false}})

It seems unnecessary kick this warning out...

Copy link
Owner Author

@igrigorik igrigorik Jun 14, 2020

Choose a reason for hiding this comment

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

Yeah, that's reasonable. Willing to put together a quick patch? :)

Copy link
Contributor

@aharbick aharbick Jun 15, 2020

Choose a reason for hiding this comment

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

Sure. How about #341

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