Skip to content

Add custom ssl_context to aiohttp helper#84573

Closed
starkillerOG wants to merge 4 commits into
home-assistant:devfrom
starkillerOG:async_create_clientsession
Closed

Add custom ssl_context to aiohttp helper#84573
starkillerOG wants to merge 4 commits into
home-assistant:devfrom
starkillerOG:async_create_clientsession

Conversation

@starkillerOG
Copy link
Copy Markdown
Contributor

Breaking change

For custom components only:
The verify_ssl optional argument of the async_create_clientsession has been renamed to ssl_context and now allows to pass in a ssl_context besides a boolean value.

Proposed change

Allow to specify custom ssl_context in the async_create_clientsession function of the aiohttp_client helper.
This can be needed to prevent the [[SSL: SSLV3_ALERT_HANDSHAKE_FAILURE] sslv3 alert handshake failure (_ssl.c:992)] error.
This error can be solved by passing in a custom ssl_context:

SSL_CONTEXT=ssl.create_default_context()
SSL_CONTEXT.set_ciphers("DEFAULT")
SSL_CONTEXT.check_hostname = False
SSL_CONTEXT.verify_mode = ssl.CERT_NONE

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@home-assistant
Copy link
Copy Markdown
Contributor

Hey there @Kane610, mind taking a look at this pull request as it has been labeled with an integration (unifi) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of unifi can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign unifi Removes the current integration label and assignees on the issue, add the integration domain after the command.

@home-assistant
Copy link
Copy Markdown
Contributor

Hey there @bdraco, @shbatm, mind taking a look at this pull request as it has been labeled with an integration (isy994) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of isy994 can trigger bot actions by commenting:

  • @home-assistant close Closes the issue.
  • @home-assistant rename Awesome new title Change the title of the issue.
  • @home-assistant reopen Reopen the issue.
  • @home-assistant unassign isy994 Removes the current integration label and assignees on the issue, add the integration domain after the command.

@MartinHjelmare MartinHjelmare changed the title aiohttp helper allow to specify custom ssl_context Add custom ssl_context to aiohttp helper Dec 26, 2022
Comment thread homeassistant/helpers/aiohttp_client.py Outdated
def async_create_clientsession(
hass: HomeAssistant,
verify_ssl: bool = True,
ssl_context: bool | SSLContext = True
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a breaking change. Maybe keep the old parameter while adding the new parameter next to the old?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is a good idea, schould I log a warning when the old parameter is used that it will be deprecated?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we want to deprecate the old parameter, yes. I'm not sure if we want that. Let's hear what others think first.

Copy link
Copy Markdown
Member

@bdraco bdraco Dec 26, 2022

Choose a reason for hiding this comment

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

I agree with Martin. We should keep both parameters here. See my comment above as well.

@starkillerOG
Copy link
Copy Markdown
Contributor Author

A alternative to this PR would be to replace the ssl=False option
by the following ssl_context that also does not verify ssl (I think):

SSL_CONTEXT=ssl.create_default_context()
SSL_CONTEXT.set_ciphers("DEFAULT")
SSL_CONTEXT.check_hostname = False
SSL_CONTEXT.verify_mode = ssl.CERT_NONE

But I do not know enough about aiohttp and ssl, to comfortably make that change.

port = host.port or 80
session = aiohttp_client.async_create_clientsession(
hass, verify_ssl=False, cookie_jar=CookieJar(unsafe=True)
hass, ssl_context=False, cookie_jar=CookieJar(unsafe=True)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The behavior here is a bit unexpected. If ssl_context is false it seems to imply a non secure connection

Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

We shouldn't merge this. The one custom component that needs this should just create their own ClientSession instead of trying to adjust a generic helper.

We haven't needed it until now, and a single instance is not good enough to drive this change.

@balloob balloob closed this Dec 27, 2022
@starkillerOG
Copy link
Copy Markdown
Contributor Author

Alright, I will just use a separate clientsession for the new reolink integration.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants