SSL certificate --insecure option does not work

[milanvo]

Hi,
This is great tool for CLI access to HA ❤️

HA version: 0.87.1
CLI version: current dev (c4889fa)

Issue
I have issue connecting to Home Assistant with HTTPS and self-signed certificate. It looks like disabled verification at session level is ignored.

I make it work by workaround - change this for HTTP GET https://github.com/home-assistant/home-assistant-cli/blob/c4889faa7d0a9a5c1a9f06e52d9e8587c44e1d93/homeassistant_cli/remote.py#L77
to this:

return requests.get(url, params=data_str, headers=headers, verify=False)

or with path to server cert:

return requests.get(url, params=data_str, headers=headers, 
                    verify='/home/hass/.homeassistant/certificate.pem')

For WebSockets, it is probably not implemented yet, but this also works - changed this https://github.com/home-assistant/home-assistant-cli/blob/c4889faa7d0a9a5c1a9f06e52d9e8587c44e1d93/homeassistant_cli/remote.py#L107

to:

resolve_server(ctx) + "/api/websocket", verify_ssl=False

I can make PR if needed or test code change etc. Thanks

[maxandersen]

Yes please PRs and tests very Welcome over manual fixed :) Thank you!

[milanvo]

I searched remote.py history, with PR #28 --insecure flag was introduced and requests.get (etc.) called with verify argument.

Later it was changed by d2e6d71 to session.verify property. Do you remember, if it worked that time ?

It is question, If it is not upstream issue ? (Requests ? Urllib3 ?)

[maxandersen]

@milanvo i unfortunately don't recall the exact sequence of events. I believe I at least did test it at the initial implementation. i might not have tested insecure certificates after the last change where I moved to use session to make sharing of settings easier.

if we could somehow find a way to test this in isolation when running unittests it would be awesome so we don't miss it again ;)

[milanvo]

Found this upstream issue https://github.com/kennethreitz/requests/issues/4938
Unfortunately closed as inactive.

I see these options:

• try to fix upstream (will try to look deeper)
• pass verify to requests.get etc. as a workaround (return to old behaviour)
• there are more and more API calls via WebSockets, so move remaining API call to WS, where verify_ssl option works

What do you prefer @maxandersen ? 😉

Unit tests to reliable test SSL non-validation - it is question. It can be tested against
https://badssl.com/, but it makes IO / network requests. Badssl.com can be Dockerized locally
https://github.com/chromium/badssl.com, it is question if not overkill for routine CI tests 🤕

[maxandersen]

Nice digging. Yeah seems that upstream bug was closed immaturely. Probably by a script not a human :/

I'm fine going back to passing the parts that doesn't work in directly especially since we need to do that for websocket APIs anyway.

Moving everything to websocket APIs might be where we end up eventually but that would be a much bigger change and for some operations the rest API is a bit nicer to work with so I would say fix the cert issues first - especially since it seems like a simple fix.

And oh boy thanks for letting me know about badssl - that's awesome and much simpler than my own past attempts on making a test xobtsiner!

Go ahead and make tests that uses the site, if too slow we'll just tag them and it Will be skipped in regular test runs and only fully run on ci.

Not at laptop so can't find the exact tag but don't worry about that - that's an easy fix when you have the pr in.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[maxandersen]

@milanvo still interested contributing this?

[milanvo]

Hi,

I am sorry, we have another project now - baby on the way.

[maxandersen]

Congratulations :)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[AndTheDaysGoBy]

Got the upstream bug re-opened. The corresponding PR was/is psf/requests#4935
Really, the commits could all be squashed. The only actual difference is the introduction of this line:
psf/requests@dc35933
(of course, in the following commit I correct my spelling of kwargs). So, presuming there is no regression, the upstream fix is simple.

Edit: Note the latest comment on the PR. I.e., an actual fix while avoiding duplicate and potentially confusing code would require more work. I.e. some reconciliation must come to def request's approach of merging and the def send's defaulting.

Edit: New PR and new approach:
psf/requests#5172
If there was a better approach, I feel we'd wind up having to move the stuff in the def requests(...) to def send(...). That would be a separate PR though. The test ensures that the cert passed in is actually propagated through to the adapter's send function so, presumably, that should fix your issue.

[maxandersen]

Thanks for digging. No wonder I couldnt get it to work then :)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[s00500]

This issue is quite annoying... is there anything that can be done to fix this without depending on the upstream issue ?

[s00500]

Just came back to this again, and found this comment: #66 (comment)
This way I do not have to use insecure but can make python actually use the correct cert, that's even better :-) Just wanted to link this into here so other people might also find it

The gist is: use the REQUESTS_CA_BUNDLE env var to point to your ca.pem file (I guess you have to setup tls with a CA proeprly, that has many benefits so I would recommend it)