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

load_verify_locations blocking I/O check needs an exception implemented for cadata via check_allowed #125993

Closed
jamesonuk opened this issue Sep 15, 2024 · 8 comments · Fixed by #133299

Comments

@jamesonuk
Copy link

The problem

Use of load_verify_locations from ssl is getting flagged as blocking IO even if the CA is actually being passed as a variable not read from disk.

eg. if you have the equivalent of

myca = """
-----BEGIN CERTIFICATE-----
# Cert....
-----END CERTIFICATE-----
"""
ctx =- ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
ctx.load_verify_locations(cadata=myca)

this is flagged as blocking IO but should not be? Other uses of load_verifiy_locations will read from disk (cafile or capath arguments)

In practice this is normally done by a custom CA being uploaded in a config flow and stored in config rather than hardcoded as above but that is just showing the issue.

The docs (https://developers.home-assistant.io/docs/asyncio_blocking_operations/#load_verify_locations) say to use homeassistant.util.ssl for generic ssl (as that caches single context instances) however that only provides contexts to disable verification or use the default certificates. You can not use this for a custom CA.

What version of Home Assistant Core has the issue?

core-2024.9.1

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant Container

Integration causing the issue

No response

Link to integration documentation on our website

https://developers.home-assistant.io/docs/asyncio_blocking_operations/#load_verify_locations

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

No response

Additional information

No response

@mib1185
Copy link
Contributor

mib1185 commented Sep 15, 2024

You can wrap the call to SSLContext.load_verify_locations into an executer job as described in Running blocking calls in the executor. I've not checked the code of SSLContext.load_verify_locations (most properly because it is part of openssl code), but it is possible, that there is still some IO within the openssl code.

@jamesonuk
Copy link
Author

It has been a very long since I was forced to do C at university but
https://github.com/python/cpython/blob/401fff7423ca3c8bf1d02e594edfd1412616a559/Modules/_ssl.c#L4113
does suggest there is no IO at al. The whole point of the SSLContext is to hold the config to be used when doing the actual IO and the load_verify_locations call is purely to add the certificate to this config (passing cafile or capath would cause it read from disk but cadata will just read from memory)

I can add the code in an executer job but that seems like a workaround to avoid the erroneous error flagging.

@mib1185
Copy link
Contributor

mib1185 commented Sep 15, 2024

I can add the code in an executer job but that seems like a workaround to avoid the erroneous error flagging.

it is not flagged as doing blocking IO, but it is detected that there is IO done (so it is not an assumption, it is a detected behavior).

It has been a very long since I was forced to do C at university but
https://github.com/python/cpython/blob/401fff7423ca3c8bf1d02e594edfd1412616a559/Modules/_ssl.c#L4113
does suggest there is no IO at al.

The code you're referring to, does also make use of a function called _add_ca_certs which itself relies on functions from openssl code.

Anyway, figuring out, what is doing IO at the end in some _ssl or openssl library code, should not be part of an issue report in the HA core repository 😉

@jamesonuk
Copy link
Author

jamesonuk commented Sep 15, 2024

it is not flagged as doing blocking IO, but it is detected that there is IO done (so it is not an assumption, it is a detected behavior).

My understanding is that the blocking IO checks are based on their being a call to SSLContext.load_verify_locations not that there is any actual blocking IO taking place.

BlockingCall(
original_func=SSLContext.load_verify_locations,
object=SSLContext,
function="load_verify_locations",
check_allowed=None,
strict=False,
strict_core=False,
skip_for_tests=True,
),

Or have I missed something in how the blocking IO checks are working?

Hence the issue with Core, I am 99.99% certain that in this scenario the warning is wrong.

@mib1185
Copy link
Contributor

mib1185 commented Sep 15, 2024

Until now I thought it was based on real detected IO, was not aware, that there is a list of functions known to do IO 🙈

we could add a check_allowed function to this that filters out function calls which only has cadata defined 🤔

@issue-triage-workflows
Copy link

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates.
Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍
This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

@jamesonuk
Copy link
Author

Bumping this. It is still there in 2024.12.
Still think this should not be flagged as blocking IO

@github-actions github-actions bot removed the stale label Dec 14, 2024
@bdraco bdraco removed the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Dec 14, 2024
@bdraco
Copy link
Member

bdraco commented Dec 14, 2024

It looks like its true that cadata doesn't do blocking I/O, and check_allowed should be implemented for this function

https://github.com/python/cpython/blob/9c6bd272b40bbd8264a00413e94e8246f550275a/Modules/_ssl.c#L4278

@bdraco bdraco changed the title Incorrectly flagged blocking IO load_verify_locations blocking I/O check needs an exception implemented for cadata via check_allowed Dec 14, 2024
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 a pull request may close this issue.

3 participants