Repository: XKNX/xknx
Pull request #942 (2022-05-03 17:36:24)

Fix keyring parsing and show proper errors in logs
<!--
  You are awesome! Thanks for contributing to our project!
  Please, DO NOT DELETE ANY TEXT from this template!.
-->
## Description
<!--
Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
List any dependencies that are required for this change.
-->
Fixes the keyring parsing and make some attributes optional. Also show proper errors if there are any


Fixes # (issue)

## Type of change
<!--
Please tick the applicable options.
NOTE: Ticking multiple options most likely indicates
that your change is to big and it is suggested to split it into several smaller PRs.
-->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update

## Checklist:

- [x] I have performed a self-review of my own code
- [x] The documentation has been adjusted accordingly
- [x] The changes generate no new warnings
- [x] Tests have been added that prove the fix is effective or that the feature works
- [x] The changes are documented in the changelog
- [x] The Homeassistant plugin has been adjusted in case of new config options


codecov[bot]: # [Codecov](https://codecov.io/gh/XKNX/xknx/pull/942?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) Report
> Merging [#942](https://codecov.io/gh/XKNX/xknx/pull/942?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (ac996da) into [main](https://codecov.io/gh/XKNX/xknx/commit/43e958ef8d8123838a98b34271dfeb9b8054ef1c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) (43e958e) will **decrease** coverage by `0.03%`.
> The diff coverage is `86.95%`.

[![Impacted file tree graph](https://codecov.io/gh/XKNX/xknx/pull/942/graphs/tree.svg?width=650&height=150&src=pr&token=irWbIygS84&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)](https://codecov.io/gh/XKNX/xknx/pull/942?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX)

```diff
@@            Coverage Diff             @@
##             main     #942      +/-   ##
==========================================
- Coverage   95.96%   95.93%   -0.04%     
==========================================
  Files         130      130              
  Lines        7777     7789      +12     
==========================================
+ Hits         7463     7472       +9     
- Misses        314      317       +3     
```


| [Impacted Files](https://codecov.io/gh/XKNX/xknx/pull/942?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX) | Coverage Δ | |
|---|---|---|
| [xknx/secure/keyring.py](https://codecov.io/gh/XKNX/xknx/pull/942/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=XKNX#diff-eGtueC9zZWN1cmUva2V5cmluZy5weQ==) | `97.81% <86.95%> (-1.27%)` | :arrow_down: |


Patch:
diff --git a/changelog.md b/changelog.md
index df08a4ce..9f6966b1 100644
--- a/changelog.md
+++ b/changelog.md
@@ -2,6 +2,10 @@
 
 ## Unreleased changes
 
+### Bugfixes
+
+- IP Secure: Fix MAC calculation for 22-byte payloads
+
 ### Internals
 
 - Rename TaskRegistry.register and Task `task` attribute to `async_func` to avoid confusion; return Task from `start()`
diff --git a/xknx/secure/keyring.py b/xknx/secure/keyring.py
index e9464069..e23bc7e7 100644
--- a/xknx/secure/keyring.py
+++ b/xknx/secure/keyring.py
@@ -6,6 +6,7 @@ from abc import ABC
 import base64
 import enum
 from itertools import chain
+import logging
 from typing import Any
 from xml.dom.minidom import Attr, Document, parse
 from xml.etree.ElementTree import Element, ElementTree
@@ -17,11 +18,13 @@ from cryptography.hazmat.primitives import hashes
 from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes
 from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC
 
-from xknx.exceptions.exception import InvalidSignature
+from xknx.exceptions.exception import InvalidSecureConfiguration, InvalidSignature
 from xknx.telegram import GroupAddress, IndividualAddress
 
 from .util import sha256_hash
 
+logger = logging.getLogger("xknx.core")
+
 
 class InterfaceType(enum.Enum):
     """Interface type enum."""
@@ -127,7 +130,6 @@ class XMLBackbone(AttributeReader):
     """Backbone in a knxkeys file."""
 
     multicast_address: str
-    latency: int
     key: str
     decrypted_key: bytes
 
@@ -137,16 +139,16 @@ class XMLBackbone(AttributeReader):
         self.multicast_address = self.get_attribute_value(
             attributes.get("MulticastAddress")
         )
-        self.latency = int(self.get_attribute_value(attributes["Latency"]))
         self.key = self.get_attribute_value(attributes.get("Key"))
 
     def decrypt_attributes(
         self, password_hash: bytes, initialization_vector: bytes
     ) -> None:
         """Decrypt attribute data."""
-        self.decrypted_key = decrypt_aes128cbc(
-            base64.b64decode(self.key), password_hash, initialization_vector
-        )
+        if self.key:
+            self.decrypted_key = decrypt_aes128cbc(
+                base64.b64decode(self.key), password_hash, initialization_vector
+            )
 
 
 class XMLGroupAddress(AttributeReader):
@@ -193,9 +195,10 @@ class XMLDevice(AttributeReader):
         self, password_hash: bytes, initialization_vector: bytes
     ) -> None:
         """Decrypt attributes."""
-        self.decrypted_tool_key = decrypt_aes128cbc(
-            base64.b64decode(self.tool_key), password_hash, initialization_vector
-        )
+        if self.tool_key is not None:
+            self.decrypted_tool_key = decrypt_aes128cbc(
+                base64.b64decode(self.tool_key), password_hash, initialization_vector
+            )
 
         if self.authentication is not None:
             self.decrypted_authentication = extract_password(
@@ -264,7 +267,8 @@ class Keyring(AttributeReader):
             if sub_node.nodeName == "Interface":
                 interface: XMLInterface = XMLInterface()
                 interface.parse_xml(sub_node)
-                self.interfaces.append(interface)
+                if interface.password is not None:
+                    self.interfaces.append(interface)
             if sub_node.nodeName == "Backbone":
                 backbone: XMLBackbone = XMLBackbone()
                 backbone.parse_xml(sub_node)
@@ -304,12 +308,17 @@ def load_key_ring(path: str, password: str, validate_signature: bool = True) ->
             raise InvalidSignature()
 
     keyring: Keyring = Keyring()
-    with open(path, encoding="utf-8") as file:
-        dom: Document = parse(file)
-        keyring.parse_xml(dom.getElementsByTagName("Keyring")[0])
+    try:
+        with open(path, encoding="utf-8") as file:
+            dom: Document = parse(file)
+            keyring.parse_xml(dom.getElementsByTagName("Keyring")[0])
+
+        keyring.decrypt(password)
 
-    keyring.decrypt(password)
-    return keyring
+        return keyring
+    except Exception as exception:
+        logger.exception("There was an error during loading the knxkeys file.")
+        raise InvalidSecureConfiguration() from exception
 
 
 class KeyringSAXContentHandler(ContentHandler):
diff --git a/xknx/secure/util.py b/xknx/secure/util.py
index 38eb34a1..20fc9076 100644
--- a/xknx/secure/util.py
+++ b/xknx/secure/util.py
@@ -14,13 +14,10 @@ def bytes_xor(a: bytes, b: bytes) -> bytes:  # pylint: disable=invalid-name
 
 
 def byte_pad(data: bytes, block_size: int) -> bytes:
-    """
-    Pad data with 0x00 until its length is a multiple of block_size.
-
-    Add a whole block of 0 if data lenght is already a multiple of block_size.
-    """
-    padding = bytes(block_size - (len(data) % block_size))
-    return data + padding
+    """Pad data with 0x00 until its length is a multiple of block_size."""
+    if remainder := len(data) % block_size:
+        return data + bytes(block_size - remainder)
+    return data
 
 
 def sha256_hash(data: bytes) -> bytes:


Test patch:
diff --git a/test/io_tests/knxip_interface_test.py b/test/io_tests/knxip_interface_test.py
index 0af39050..58d00aab 100644
--- a/test/io_tests/knxip_interface_test.py
+++ b/test/io_tests/knxip_interface_test.py
@@ -219,7 +219,7 @@ class TestKNXIPInterface:
             connection_type=ConnectionType.TUNNELING_TCP_SECURE,
             gateway_ip=gateway_ip,
             secure_config=SecureConfig(
-                user_id=4, knxkeys_file_path=knxkeys_file, knxkeys_password="password"
+                user_id=3, knxkeys_file_path=knxkeys_file, knxkeys_password="password"
             ),
         )
         with patch(
@@ -230,8 +230,8 @@ class TestKNXIPInterface:
             start_secure_tunnel.assert_called_once_with(
                 gateway_ip="192.168.1.1",
                 gateway_port=3671,
-                user_id=4,
-                user_password="user2",
+                user_id=3,
+                user_password="user1",
                 device_authentication_password="authenticationcode",
             )
         with patch("xknx.io.tunnel.SecureTunnel.connect") as connect_secure:
@@ -242,8 +242,8 @@ class TestKNXIPInterface:
             assert interface._interface.gateway_port == 3671
             assert interface._interface.auto_reconnect is True
             assert interface._interface.auto_reconnect_wait == 3
-            assert interface._interface._user_id == 4
-            assert interface._interface._user_password == "user2"
+            assert interface._interface._user_id == 3
+            assert interface._interface._user_password == "user1"
             assert (
                 interface._interface._device_authentication_password
                 == "authenticationcode"
diff --git a/test/secure_tests/keyring_test.py b/test/secure_tests/keyring_test.py
index f8cd8450..e2d15a4c 100644
--- a/test/secure_tests/keyring_test.py
+++ b/test/secure_tests/keyring_test.py
@@ -3,7 +3,7 @@ import os
 
 import pytest
 
-from xknx.exceptions.exception import InvalidSignature
+from xknx.exceptions.exception import InvalidSecureConfiguration, InvalidSignature
 from xknx.secure import Keyring, load_key_ring
 from xknx.secure.keyring import XMLDevice, XMLInterface, verify_keyring_signature
 
@@ -22,18 +22,18 @@ class TestKeyRing:
     def test_load_keyring(self):
         """Test load keyring from knxkeys file."""
         keyring: Keyring = load_key_ring(self.keyring_test_file, "pwd")
-        TestKeyRing.assert_interface(keyring, "user2", "5")
-        TestKeyRing.assert_interface(keyring, "user1", "1")
-        TestKeyRing.assert_interface(keyring, "user4", "2")
-        TestKeyRing.assert_interface(keyring, "@zvI1G&_", "3")
+        TestKeyRing.assert_interface(keyring, "user4", 2)
+        TestKeyRing.assert_interface(keyring, "@zvI1G&_", 3)
+        TestKeyRing.assert_interface(keyring, "ZvDY-:g#", 4)
+        TestKeyRing.assert_interface(keyring, "user2", 5)
 
     def test_load_keyring_real(self):
         """Test load keyring from knxkeys file."""
         keyring: Keyring = load_key_ring(self.testcase_file, "password")
-        TestKeyRing.assert_interface(keyring, "user1", "3")
-        TestKeyRing.assert_interface(keyring, "user2", "4")
-        TestKeyRing.assert_interface(keyring, "user3", "5")
-        TestKeyRing.assert_interface(keyring, "user4", "6")
+        TestKeyRing.assert_interface(keyring, "user1", 3)
+        TestKeyRing.assert_interface(keyring, "user2", 4)
+        TestKeyRing.assert_interface(keyring, "user3", 5)
+        TestKeyRing.assert_interface(keyring, "user4", 6)
         assert keyring.devices[0].decrypted_management_password == "commissioning"
 
         interface: XMLInterface = keyring.interfaces[0]
@@ -51,9 +51,19 @@ class TestKeyRing:
         with pytest.raises(InvalidSignature):
             load_key_ring(self.testcase_file, "wrong_password")
 
+    def test_raises_error(self):
+        """Test raises error if password is wrong."""
+        with pytest.raises(InvalidSecureConfiguration):
+            load_key_ring(
+                self.testcase_file, "wrong_password", validate_signature=False
+            )
+
     @staticmethod
-    def assert_interface(keyring: Keyring, password: str, user: str) -> None:
+    def assert_interface(keyring: Keyring, password: str, user: int) -> None:
         """Verify password for given user."""
-        for interface in keyring.interfaces:
-            if interface.user_id == user:
-                assert interface.decrypted_password == password
+        matched = False
+        if interface := keyring.get_interface_by_user_id(user):
+            matched = True
+            assert interface.decrypted_password == password
+
+        assert matched
diff --git a/test/secure_tests/util_test.py b/test/secure_tests/util_test.py
index fec23d94..ebf66f54 100644
--- a/test/secure_tests/util_test.py
+++ b/test/secure_tests/util_test.py
@@ -52,13 +52,17 @@ def test_byte_xor_error():
         (
             4,
             bytes([1, 23, 0, 0]),
-            bytes([1, 23, 0, 0, 0, 0, 0, 0]),
+            bytes([1, 23, 0, 0]),
         ),
         (
             16,
             bytes([123]),
             bytes([123, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]),
         ),
+        (16, bytes(16), bytes(16)),
+        (16, bytes(17), bytes(32)),
+        (16, bytes(32), bytes(32)),
+        (16, bytes(47), bytes(48)),
     ],
 )
 def test_byte_pad(block_size, data, result):