allow SSL certs to be verified #179

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@thoughtless

Currently if the option :verify_peer is set to true, all requests will fail. This is because ssl_verify_peer is not implemented. According to the EM docs, "It is up to user defined code to perform a check on the certificates."

Since the method isn't defined by em-http-request, it returns nil, and all https requests fail.

I have attach a first stab at implementing this method. It is lacking tests, and this is almost certainly not a completely correct way of verifying the SSL certs; however, it is far better than not verifying SSL certs at all.

Are you interested in including this feature in em-http-request? Can anyone make some suggestion for improving the cert checking?

allow SSL certs to be verified
* This is almost certainly not a completely correct way of verifying the
  SSL certs; however, it is far better than not verifying SSL certs at
  all.
@igrigorik

This comment has been minimized.

Show comment Hide comment
@igrigorik

igrigorik Mar 16, 2012

Owner

Are you interested in including this feature in em-http-request? - Yes.

Having said that, I don't have any good experience with this stuff either.. and it's a tricky subject. @nahi, perhaps you have some tips or suggestions?

Owner

igrigorik commented Mar 16, 2012

Are you interested in including this feature in em-http-request? - Yes.

Having said that, I don't have any good experience with this stuff either.. and it's a tricky subject. @nahi, perhaps you have some tips or suggestions?

@stve

This comment has been minimized.

Show comment Hide comment
@stve

stve Mar 16, 2012

I recently added ssl cert verification to an EM client and used @alloy's ssalleyware as a model for my code.

https://github.com/alloy/ssalleyware/blob/master/lib/ssalleyware.rb

Fairly similar to what @thoughtless has added. Might also be worth a look.

stve commented Mar 16, 2012

I recently added ssl cert verification to an EM client and used @alloy's ssalleyware as a model for my code.

https://github.com/alloy/ssalleyware/blob/master/lib/ssalleyware.rb

Fairly similar to what @thoughtless has added. Might also be worth a look.

@nahi

This comment has been minimized.

Show comment Hide comment
@nahi

nahi Mar 16, 2012

Yes, it's not em-http-request's issue. EM must implement SSL certificate verification with help of OpenSSL API... Because it's extra hard to implement it properly. Ad-hoc patching would cause severe security flaw... (e.g. JRuby-OpenSSL skipped all verification to get test green when I came to it.)

EM should allow SSLContext to be configured and ssl_verify_peer of SslBox should handle preverify_ok properly. Hope someone can enough time to try it. Rewriting SslBox with ext/openssl would be far better, though.

nahi commented Mar 16, 2012

Yes, it's not em-http-request's issue. EM must implement SSL certificate verification with help of OpenSSL API... Because it's extra hard to implement it properly. Ad-hoc patching would cause severe security flaw... (e.g. JRuby-OpenSSL skipped all verification to get test green when I came to it.)

EM should allow SSLContext to be configured and ssl_verify_peer of SslBox should handle preverify_ok properly. Hope someone can enough time to try it. Rewriting SslBox with ext/openssl would be far better, though.

@thoughtless

This comment has been minimized.

Show comment Hide comment
@thoughtless

thoughtless Mar 16, 2012

it's not em-http-request's issue

The current EM docs say that the user (em-http-request in this case) should implement ssl_verify_peer. Are you saying that that is not a good idea?

That certainly makes sense. I would expect that the EM code is in a much better position to work with OpenSSL as it controls the connection itself. I couldn't figure out how to use SSLContext within an ssl_verify_peer method.

it's not em-http-request's issue

The current EM docs say that the user (em-http-request in this case) should implement ssl_verify_peer. Are you saying that that is not a good idea?

That certainly makes sense. I would expect that the EM code is in a much better position to work with OpenSSL as it controls the connection itself. I couldn't figure out how to use SSLContext within an ssl_verify_peer method.

@igrigorik

This comment has been minimized.

Show comment Hide comment
@igrigorik

igrigorik Mar 17, 2012

Owner

@tmm1 any experience with SSL + EM?

Owner

igrigorik commented Mar 17, 2012

@tmm1 any experience with SSL + EM?

@igrigorik

This comment has been minimized.

Show comment Hide comment
@igrigorik

igrigorik Mar 31, 2012

Owner

A relevant discussion from EM side: eventmachine/eventmachine#84

  • From discussion above, EM itself needs some good work to get proper SSL verification support
  • The logic for verification should live in EM, not em-http

Given the two points above, I'm not sure there is much we can do from here.. Unless someone is willing to invest the time to make SSL work in EM properly first.

Owner

igrigorik commented Mar 31, 2012

A relevant discussion from EM side: eventmachine/eventmachine#84

  • From discussion above, EM itself needs some good work to get proper SSL verification support
  • The logic for verification should live in EM, not em-http

Given the two points above, I'm not sure there is much we can do from here.. Unless someone is willing to invest the time to make SSL work in EM properly first.

@thoughtless

This comment has been minimized.

Show comment Hide comment
@thoughtless

thoughtless Mar 31, 2012

OK. It sounds like this ticket should be closed and future discussion should happen in EM tickets.

Thanks for your time looking into this.

OK. It sounds like this ticket should be closed and future discussion should happen in EM tickets.

Thanks for your time looking into this.

@burke

This comment has been minimized.

Show comment Hide comment
@burke

burke May 24, 2012

I've opened a new pull request to address this (I think) more completely than EM's pull request 84:

eventmachine/eventmachine#334

I've also made a branch that should take advantage of those improvements to EM, which I'll pullrequest here once I can get my work merged into EM:

https://github.com/burke/em-http-request/tree/em_ssl_improvements

Hopefully I can get both merged and working fairly shortly.

burke commented May 24, 2012

I've opened a new pull request to address this (I think) more completely than EM's pull request 84:

eventmachine/eventmachine#334

I've also made a branch that should take advantage of those improvements to EM, which I'll pullrequest here once I can get my work merged into EM:

https://github.com/burke/em-http-request/tree/em_ssl_improvements

Hopefully I can get both merged and working fairly shortly.

@igrigorik

This comment has been minimized.

Show comment Hide comment
@igrigorik

igrigorik May 24, 2012

Owner

@burke that's awesome, thanks for your work on this!

Owner

igrigorik commented May 24, 2012

@burke that's awesome, thanks for your work on this!

@rubiii

This comment has been minimized.

Show comment Hide comment
@rubiii

rubiii Oct 20, 2012

could anyone please post an update on the status of ssl authentication?
i'm currently adding some integration specs to httpi and can't get em-http to verify peer.

rubiii commented Oct 20, 2012

could anyone please post an update on the status of ssl authentication?
i'm currently adding some integration specs to httpi and can't get em-http to verify peer.

@igrigorik

This comment has been minimized.

Show comment Hide comment
@igrigorik

igrigorik Oct 20, 2012

Owner

Not much. As you can see from the links above, the pull is still open on the EM repo. :(

Owner

igrigorik commented Oct 20, 2012

Not much. As you can see from the links above, the pull is still open on the EM repo. :(

@rubiii rubiii referenced this pull request in savonrb/httpi Oct 20, 2012

Closed

Make SSL work with httpclient adapter #65

@rubiii

This comment has been minimized.

Show comment Hide comment
@rubiii

rubiii Oct 20, 2012

thanks @igrigorik. so i guess i'll better raise an error if someone tries to use ssl authentication
with httpi's em_http adapter then.

rubiii commented Oct 20, 2012

thanks @igrigorik. so i guess i'll better raise an error if someone tries to use ssl authentication
with httpi's em_http adapter then.

@mislav

This comment has been minimized.

Show comment Hide comment
@mislav

mislav Aug 11, 2013

I've added a patch to Faraday that adds SSL peer verification support (enabled by default) with em-http adapter.

Not battle-tested, but passes our test suite. Patch is not Faraday-dependend and you can drop it in your projects.

mislav commented Aug 11, 2013

I've added a patch to Faraday that adds SSL peer verification support (enabled by default) with em-http adapter.

Not battle-tested, but passes our test suite. Patch is not Faraday-dependend and you can drop it in your projects.

@mrichar1 mrichar1 referenced this pull request in seegno/sensu-influxdb-extension Mar 20, 2015

Merged

Add support for https/ssl #18

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment