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

Pass configured host string instead of always forcing an ip-address #23164

Merged
merged 2 commits into from Apr 17, 2019

Conversation

Projects
None yet
5 participants
@itn3rd77
Copy link
Contributor

commented Apr 17, 2019

Pass the configured host (https://www.home-assistant.io/components/homematic/#host) instead of always forcing an ip-address. This is required to get SSL certificate validation working.

Breaking Change:

Not that I'm aware of

Description:

The current implementation in the component at lines homeassistant/components/homematic/init.py#L265 and homeassistant/components/homematic/init.py#L281 always forces an ip-address passed to pyhomematic. This made it practically impossible to get SSL certificate validation working. With the proposed changes the configured value for host is passed to pyhomematic. If a user wants to pass an ip-address he is free to do.

Talking to @danielperna84 the maintainer of pyhomematic he wasn't even aware of this. I would have renamed the key 'id' to 'host' but @danielperna84 said it's not worth the effort as pyhomematic has to be adpted than as well.

This is my first pull request for Home Assistant so please bear with me if I have forgotten something.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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 (example).
  • New dependencies have been added to requirements in the manifest (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Pass host string instead of forcing an ip-address
Pass the configured host (https://www.home-assistant.io/components/homematic/#host) instead of always forcing an ip-address. This is required to get SSL certificate validation working.
@github-actions

This comment has been minimized.

Copy link

commented Apr 17, 2019

Hey there @pvizeli, @danielperna84, mind taking a look at this pull request as its been labeled with a integration (homematic) you are listed as a codeowner for? Thanks!

@@ -262,7 +262,7 @@ def setup(hass, config):
# Create hosts-dictionary for pyhomematic
for rname, rconfig in conf[CONF_INTERFACES].items():
remotes[rname] = {
'ip': socket.gethostbyname(rconfig.get(CONF_HOST)),
'ip': rconfig.get(CONF_HOST),

This comment has been minimized.

Copy link
@pvizeli

pvizeli Apr 17, 2019

Member

That is only for xmlrpc server they we run. You are sure that you not only mean the settings from hosts part?

This comment has been minimized.

Copy link
@itn3rd77

itn3rd77 Apr 17, 2019

Author Contributor

Sorry I don't get what you mean. I'm using Home Assistant for three days now and not very familiar with the whole stuff. Without the changes I got the following exception:

2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic.connection] HMConnection: Creating server object
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] ServerThread.__init__
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] __init__: Creating proxies
2019-04-16 07:58:42 INFO (SyncWorker_19) [pyhomematic._hm] Creating proxy rf. Connecting to 192.168.100.74:42001
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] LockingServerProxy.__init__: Getting local ip
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] LockingServerProxy.__init__: Got local ip 192.168.100.2
2019-04-16 07:58:42 INFO (SyncWorker_19) [pyhomematic._hm] Creating proxy ip. Connecting to 192.168.100.74:42010
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] LockingServerProxy.__init__: Getting local ip
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] LockingServerProxy.__init__: Got local ip 192.168.100.2
2019-04-16 07:58:42 INFO (SyncWorker_19) [pyhomematic._hm] Creating proxy raspberrymatic. Connecting to 192.168.100.74:2001
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] LockingServerProxy.__init__: Getting local ip
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] LockingServerProxy.__init__: Got local ip 192.168.100.2
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] RPCFunctions.__init__
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] RPCFunctions.__init__: iterating proxy = homeassistant-rf
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] RPCFunctions.__init__: iterating proxy = homeassistant-ip
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] RPCFunctions.__init__: iterating proxy = homeassistant-raspberrymatic
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] ServerThread.__init__: Setting up server
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] ServerThread.__init__: Registering RPC functions
2019-04-16 07:58:42 INFO (Thread-3) [pyhomematic._hm] Starting server at http://0.0.0.0:47051
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] ServerThread.proxyInit: init('http://192.168.100.2:47051', 'homeassistant-rf')
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] proxyInit: Exception: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: IP address mismatch, certificate is not valid for '192.168.100.74'. (_ssl.c:1056)
2019-04-16 07:58:42 WARNING (SyncWorker_19) [pyhomematic._hm] Failed to initialize proxy
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] ServerThread.proxyInit: init('http://192.168.100.2:47051', 'homeassistant-ip')
2019-04-16 07:58:42 DEBUG (SyncWorker_19) [pyhomematic._hm] proxyInit: Exception: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: IP address mismatch, certificate is not valid for '192.168.100.74'. (_ssl.c:1056)
2019-04-16 07:58:42 WARNING (SyncWorker_19) [pyhomematic._hm] Failed to initialize proxy

All those created proxies are connecting to an IP address (as previously forced). And as far as I understand the settings used beneath the line pyhomematic/_hm.py#L521 come from remotes settings.

But maybe I am completely lost at the moment :-/

This comment has been minimized.

Copy link
@danielperna84

danielperna84 Apr 17, 2019

Contributor

@pvizeli
socket.gethostbyname turns myhost.local into 192.168.1.123. So even with the hostname correctly set in configuration.yaml, the component initializes pyhomematic with the IP addresses. pyhomematic has gained SSL support since the release of the CCU3 (and recent RaspberryMatic releases). Verification of course fails because the IP address never matches the hostname of the certificate.

@codecov

This comment has been minimized.

Copy link

commented Apr 17, 2019

Codecov Report

Merging #23164 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##             dev   #23164   +/-   ##
======================================
  Coverage   94.2%    94.2%           
======================================
  Files        453      453           
  Lines      36913    36913           
======================================
  Hits       34773    34773           
  Misses      2140     2140
Impacted Files Coverage Δ
...ssistant/components/islamic_prayer_times/sensor.py 94.68% <0%> (-1.07%) ⬇️
homeassistant/components/uk_transport/sensor.py 94.16% <0%> (+0.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37ca9ca...605b797. Read the comment docs.

@pvizeli
Copy link
Member

left a comment

For me is fine. @danielperna84 ?

@danielperna84

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@pvizeli
Just commented on your comment. ^^
Yes, I'm fine with it. 👍

@pvizeli pvizeli merged commit 0afa016 into home-assistant:dev Apr 17, 2019

13 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch Coverage not affected when comparing 37ca9ca...605b797
Details
codecov/project 94.2% (target 90%)
Details

@pvizeli pvizeli removed the in progress label Apr 17, 2019

@itn3rd77 itn3rd77 deleted the itn3rd77:itn3rd77-pass-host branch Apr 18, 2019

@itn3rd77

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Could this PR be added to the 0.93 milestone?

@balloob balloob referenced this pull request May 14, 2019

Merged

0.93.0 #23864

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.