rbw / aiosnow

Asynchronous ServiceNow Library
MIT License
74 stars 12 forks source link

Need option to disable SSL certificate verification #31

Closed sulbig closed 4 years ago

sulbig commented 4 years ago

Many enterprise environments use SSL MITM proxy services, and thus get the following exception.

aiohttp.client_exceptions.ClientConnectorCertificateError: Cannot connect to host servicenow.example.com:443 ssl:True [SSLCertVerificationError: (1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain (_ssl.c:1108)')]

We need the ability to disable SSL certificate verification through a configuration option when creating an Application object. I believe that configuration should end up looking like this in the snow library.

aiohttp.ClientSession(connector=aiohttp.TCPConnector(ssl=False))

sulbig commented 4 years ago

I was able to add connector=aiohttp.TCPConnector(ssl=False) to resolve the issue above. Would be nice if this could be exposed as a configurable option.

def get_session(self):
    """New client session

    Returns:
        aiohttp.ClientSession:  HTTP client session

    Raises:
        NoAuthenticationMethod
    """

    if self.config.basic_auth:
        return aiohttp.ClientSession(connector=aiohttp.TCPConnector(ssl=False), auth=aiohttp.BasicAuth(*self.config.basic_auth))
    else:
        raise NoAuthenticationMethod("No known authentication methods was provided")
rbw commented 4 years ago

Thanks for pointing this out.

I've added support for verify_ssl in https://github.com/rbw/snow/pull/36. Can you test it out?

sulbig commented 4 years ago

I have pulled the flexible-tcp-connector branch and tested the 'verify_ssl' configuration setting set as both True and False. I am able to fail and succeed with my proxy as expected, so I would say this tests good in my scenario.

If i do not include the 'verify_ssl' item in the configuration dict, I get the following exception.

AttributeError: 'InternalConfig' object has no attribute 'verify_ssl'

I am thinking it would be a sane default if set as True and more user friendly if it were an optional configuration item -- not required.

rbw commented 4 years ago

I have pulled the flexible-tcp-connector branch and tested the 'verify_ssl' configuration setting set as both True and False. I am able to fail and succeed with my proxy as expected, so I would say this tests good in my scenario.

Great

I am thinking it would be a sane default if set as True and more user friendly if it were an optional configuration item -- not required.

Argh.. yeah, that was the idea. I incorrectly passed default instead of missing to the Field constructor.

It should default to True now, can you test again?

Thanks.

sulbig commented 4 years ago

I pulled the updated branch and it behaves the same, only the configuration value is now optional. No exception was raised without it, and the default value is True when not configured. Looks good to me!

rbw commented 4 years ago

:+1:

Merged