[knx] Improve handling of unknown encrypted frames

* Show encrypted frames without a matching key using console command "openhab:knx list-unknown-ga"; sort output numerically by GA. * Add trace logging to show decryption error due to missing key. Previously, those frames were silently dropped unless logging for Calimero was explicitly enabled as well. Signed-off-by: Holger Friedrich <[email protected]>
openhab · Nov 8, 2024 · 37bc7ba · 37bc7ba
1 parent f0d4a0a
commit 37bc7ba
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 8 deletions.
diff --git a/....binding.knx/src/main/java/org/openhab/binding/knx/internal/client/AbstractKNXClient.java b/....binding.knx/src/main/java/org/openhab/binding/knx/internal/client/AbstractKNXClient.java
@@ -39,12 +39,16 @@
 import org.slf4j.LoggerFactory;
 
 import tuwien.auto.calimero.CloseEvent;
+import tuwien.auto.calimero.DataUnitBuilder;
 import tuwien.auto.calimero.DetachEvent;
 import tuwien.auto.calimero.FrameEvent;
 import tuwien.auto.calimero.GroupAddress;
 import tuwien.auto.calimero.IndividualAddress;
+import tuwien.auto.calimero.KNXAddress;
 import tuwien.auto.calimero.KNXException;
 import tuwien.auto.calimero.KNXIllegalArgumentException;
+import tuwien.auto.calimero.cemi.CEMILData;
+import tuwien.auto.calimero.cemi.CemiTData;
 import tuwien.auto.calimero.datapoint.CommandDP;
 import tuwien.auto.calimero.datapoint.Datapoint;
 import tuwien.auto.calimero.device.ProcessCommunicationResponder;
@@ -60,6 +64,7 @@
 import tuwien.auto.calimero.process.ProcessEvent;
 import tuwien.auto.calimero.process.ProcessListener;
 import tuwien.auto.calimero.secure.KnxSecureException;
+import tuwien.auto.calimero.secure.SecureApplicationLayer;
 import tuwien.auto.calimero.secure.Security;
 
 /**
@@ -348,12 +353,13 @@ private void processEvent(String task, ProcessEvent event, ListenerNotification
         if (!isHandled) {
             logger.trace("Address '{}' is not configured in openHAB", destination);
             final String type = switch (event.getServiceCode()) {
-                case 0x80 -> " GROUP_WRITE(";
-                case 0x40 -> " GROUP_RESPONSE(";
-                case 0x00 -> " GROUP_READ(";
-                default -> " ?(";
+                case 0x80 -> "GROUP_WRITE";
+                case 0x40 -> "GROUP_RESPONSE";
+                case 0x00 -> "GROUP_READ";
+                default -> "?";
             };
-            final String key = destination.toString() + type + event.getASDU().length + ")";
+            final String key = String.format("%2d/%1d/%3d  %s(%02d)", destination.getMainGroup(),
+                    destination.getMiddleGroup(), destination.getSubGroup8(), type, event.getASDU().length);
             commandExtensionData.unknownGA().compute(key, (k, v) -> v == null ? 1 : v + 1);
         }
     }
@@ -429,7 +435,36 @@ public void linkClosed(@Nullable CloseEvent closeEvent) {
 
     @Override
     public void indication(@Nullable FrameEvent e) {
-        // no-op
+        // NetworkLinkListener indication. This implementation is triggered whenever a frame is received.
+        // It is not necessary for OH, as we process incoming group writes via different triggers.
+        // However, this indication also covers encrypted data secure frames, which would typically
+        // be dropped silently by the Calimero library (a log message is only visible when log level for Calimero
+        // is set manually).
+
+        // Implementation searches for incoming data secure frames which cannot be decoded due to missing key
+        final var cemi = e.getFrame();
+        if (!(cemi instanceof CemiTData)) {
+            final CEMILData f = (CEMILData) cemi;
+            final int ctrl = f.getPayload()[0] & 0xfc;
+            if (ctrl == 0) {
+                final KNXAddress dst = f.getDestination();
+                if (dst instanceof GroupAddress ga) {
+                    if (dst.getRawAddress() != 0) {
+                        final byte[] payload = f.getPayload();
+                        final int service = DataUnitBuilder.getAPDUService(payload);
+                        if (service == SecureApplicationLayer.SecureService) {
+                            if (!openhabSecurity.groupKeys().containsKey(dst)) {
+                                logger.trace("Address '{}' cannot be decrypted, group key missing", dst);
+                                final String key = String.format(
+                                        "%2d/%1d/%3d  secure: missing group key, cannot decrypt", ga.getMainGroup(),
+                                        ga.getMiddleGroup(), ga.getSubGroup8());
+                                commandExtensionData.unknownGA().compute(key, (k, v) -> v == null ? 1 : v + 1);
+                            }
+                        }
+                    }
+                }
+            }
+        }
     }
 
     @Override

diff --git a/...nding.knx/src/main/java/org/openhab/binding/knx/internal/console/KNXCommandExtension.java b/...nding.knx/src/main/java/org/openhab/binding/knx/internal/console/KNXCommandExtension.java
@@ -56,7 +56,7 @@ public void execute(String[] args, Console console) {
                 console.println("KNX bridge \"" + bridgeHandler.getThing().getLabel()
                         + "\": group address, type, number of bytes, and number of occurrence since last reload of binding:");
                 for (Entry<String, Long> entry : bridgeHandler.getCommandExtensionData().unknownGA().entrySet()) {
-                    console.println(entry.getKey() + " " + entry.getValue());
+                    console.println(entry.getKey() + "  " + entry.getValue());
                 }
             }
             return;

diff --git a/...knx/src/main/java/org/openhab/binding/knx/internal/handler/KNXBridgeBaseThingHandler.java b/...knx/src/main/java/org/openhab/binding/knx/internal/handler/KNXBridgeBaseThingHandler.java
@@ -21,6 +21,7 @@
 import java.util.NoSuchElementException;
 import java.util.Optional;
 import java.util.Set;
+import java.util.SortedMap;
 import java.util.TreeMap;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
@@ -85,7 +86,7 @@ public SecureRoutingConfig() {
      * Helper class to carry information which can be used by the
      * command line extension (openHAB console).
      */
-    public record CommandExtensionData(Map<String, Long> unknownGA) {
+    public record CommandExtensionData(SortedMap<String, Long> unknownGA) {
     }
 
     private final ScheduledExecutorService knxScheduler = ThreadPoolManager.getScheduledPool("knx");