calimero-core icon indicating copy to clipboard operation
calimero-core copied to clipboard

GO Diagnostics: Improve selection of surrogate

Open holgerfriedrich opened this issue 2 years ago • 8 comments

Current implementation of SecureApplicationLayer uses the following approach to select the surrogate which actually issues the read request of a GA: final var surrogate = security.groupSenders().getOrDefault(group, Set.of()).stream().findAny().orElseThrow(() -> new KnxSecureException(group + " does not have a surrogate specified"));

This gives a rather arbitrary choice of the surrogate basically determined by the order of elements in the keyring export.

It seems to have the following shortcomings:

  • sometimes a surrogate does not have a device tool key which causes a later exception (this seems to be the case when the user connects GAs to a tunnel of the Sec IP interface in ETS tool; when the PA of the tunnel is picked, there is not devtoolkey, as only the main PA of the interface has a key defined)
  • sometimes the connection to a device is not possible due to settings of the line couplers (block physical access), but other surrogates might still be reachable

Does AN170 give some recommendations how to select a surrogate? The current implementation bases on the Senders field of the GA in the keyring, shouldn't it be based on the receivers (not contained in the keyring)? Sorry for asking this questions, I do not have AN170 at hand.

holgerfriedrich avatar Jun 09 '22 20:06 holgerfriedrich

Adding .filter(pa -> security.deviceToolKeys().containsKey(pa)). before findAny() could at least work around the first issue. AFAIR I tried to place the main PA of the interface instead - but this did not work (at least in my setup).

holgerfriedrich avatar Jun 09 '22 20:06 holgerfriedrich

I don't understand your first bullet point, when a user connects GAs to a tunnel and the tunnel's PA has no device toolkey? Please try again :)

For the second bullet point, if a coupler blocks physically addressed telegrams, you need to remove all sender addresses of that segment/line from the set of group senders. In that case, GOD simply doesn't work.

bmalinowsky avatar Jun 12 '22 19:06 bmalinowsky

Hi, let me try again.

I have seen PAs in Group Senders which do not have a matching device with a device tool key. These PAs correspond to the different tunnels of my IP interface.

It took me a moment to reproduce, because creating my minimal example showed perfectly reasonable behavior of ETS6. I connected the GAs "0/0/1" and "0/0/2" to the first tunnel. Group Senders did show the PA of the secure device (for the GA "0/0/2" configured as "R", not for "0/0/1" only configured as "W"), and the filter table of the coupler was adapted to allow both GAs.

grafik

...but once I connect a GA to more than one tunnel like this....

grafik

The keyring export did show the following, see the entries 1.0.2 and 1.0.3 below:

<?xml version="1.0" encoding="utf-8"?>
<Keyring Project="knxsectest1" CreatedBy="6.0.4" Created="2022-06-14T18:49:39" Signature="/f6Xg1HiDlZa9p3R8G5Xng==" xmlns="http://knx.org/xml/keyring/1">
  <Backbone MulticastAddress="224.0.23.12" />
  <Interface IndividualAddress="1.0.2" Type="Tunneling" Host="1.0.1" UserID="2" Password="PUqNi7UIIenx9Ey4dua7ESFbAOuqng5Zr/iuNui3mvI=" Authentication="sWeeqJ225Idb6MY9PK5nrdmZc9Vo+W5snBMIKwWnNYA=">
    <Group Address="1" Senders="1.0.3" />
    <Group Address="2" Senders="1.0.3 1.1.7" />
  </Interface>
  <Interface IndividualAddress="1.0.3" Type="Tunneling" Host="1.0.1" UserID="3" Password="tRKT1OKwKsmQR/YzWI/YdYPOp0dfCSujoGTOBnxS5rQ=" Authentication="84eTbVPVk68Xuk73Jy+FCLkAis862F3a6MAjrT6h7E0=">
    <Group Address="1" Senders="1.0.2" />
    <Group Address="2" Senders="1.0.2 1.1.7" />
  </Interface>
  <Interface IndividualAddress="1.0.4" Type="Tunneling" Host="1.0.1" UserID="4" Password="Rv0fGSf6vdYEjwxpgx2W+LkyfNK/nrDGmqCU+WwM/B8=" Authentication="NtQGSXuVBvZjJKXM7xcGqI7xOYYB4xtoI0WI8NpihGI=" />
  <Interface IndividualAddress="1.0.5" Type="Tunneling" Host="1.0.1" UserID="5" Password="WipSRd6bNBU66AjdyMhnEXckrrxgq/5EWxs/wr4n5xk=" Authentication="W4ml2gGY1+wy8UQus+6ZDUgk5f6pgU67bP3TH49tYWE=" />
  <Interface IndividualAddress="1.0.6" Type="Tunneling" Host="1.0.1" UserID="6" Password="HM0257keyfP1+MAx/EbkIagmwjiLpcpwRok3x5QmucU=" Authentication="wOG2TrGRPXCmGK8+ntL/5QfhnOUfDcMJklNRwhhDXkc=" />
  <GroupAddresses>
    <Group Address="1" Key="QWCu/Rsu+JQmPTxVnD7xBg==" />
    <Group Address="2" Key="mAF8QABR7zWVG/25w30iLQ==" />
  </GroupAddresses>
  <Devices>
    <Device IndividualAddress="1.0.1" ToolKey="1UddAAoGX1EXVUX/Vh1F6Q==" ManagementPassword="OUWnbBQsQZhkiXpwtoH18L1k8/8kIrmL7OgFIEvLF6E=" Authentication="DjXIzPVdjj6ZwjjysqUL4XprWKSRhRgxHbV1ZT8lGjQ=" />
    <Device IndividualAddress="1.1.7" ToolKey="zgPSFXSeLIW88kpizpM8vg==" />
  </Devices>
</Keyring>

I don't know the specification, thus I'm not sure if adding the additional PAs to Group Senders is to be considered as bug in ETS6 or intended behavior.

The IP interface has only a device tool key for the main PA, not for the additional PAs of the tunnels. This leads to an exception in Calimero if the tunnel address is selected as surrogate. This is why I suggested code change above to filter out all PAs which do not have a device key.

holgerfriedrich avatar Jun 14 '22 20:06 holgerfriedrich

Having the addresses of other secure interfaces in the sender list is fine, as they can actually be senders, too. For group diagnostics it would make sense to ignore those interface addresses. You're right, there are no extra toolkeys foreseen for tunneled endpoints.

bmalinowsky avatar Jun 20 '22 13:06 bmalinowsky

Maybe to add: although I currently don't check the sender address in the client implementation against the sender list for standard data secure (i.e., not GO diag), it is better to leave the interface addresses themselves in. Because if I add that check, it would reject group telegrams sent from other secure interfaces.

bmalinowsky avatar Jun 20 '22 13:06 bmalinowsky

Maybe to add: although I currently don't check the sender address in the client implementation against the sender list for standard data secure (i.e., not GO diag), it is better to leave the interface addresses themselves in. Because if I add that check, it would reject group telegrams sent from other secure interfaces.

Thanks for making this point. So the goal should be to keep the groupSenders unchanged.

To address the first issue caused by connecting more than one tunnel, surrogate() could filter for those PAs which have a dev key, as proposed in https://github.com/calimero-project/calimero-core/issues/113#issuecomment-1151571522. I can create a PR if you like.

For filtering out the devices on KNX lines behind a coupler filtering PA communication, I am lacking an interface (as groupSenders should not be touched). Would it be possible to introduce a denylist, e.g. filtering all PAs like "1.2.*"? WDYT?

holgerfriedrich avatar Jun 22 '22 16:06 holgerfriedrich

I looked at the implementation for checking group senders, and that will use a different map in Security. The current groupSenders is really there for having a basic set of senders for GOdiag, derived from a keyring if there is any. It can be arbitrarily changed, so what I wrote above is not true.

Because an interface address assigned in ETS cannot (or shouldn't) be reused anyway for anything else, I think that I can remove those when importing the keyring. When someone manually populates the group senders, it also doesn't make sense to add an interface address.

So, if you want to communicate with devices behind a blocking coupler, you have to have other devices in front of it that you can address for GOdiag, or you have all the group senders of that blocked line programmed for that interface for data secure.

bmalinowsky avatar Jun 23 '22 18:06 bmalinowsky

Interfaces are now filtered out during keyring import, so the problem with the device toolkey should be gone.

bmalinowsky avatar Jul 02 '22 16:07 bmalinowsky

@holgerfriedrich What's the progress on this? Any issues with the security implementation in 2.5 can still go into 2.5.1.

bmalinowsky avatar Oct 21 '22 14:10 bmalinowsky

@bmalinowsky thanks for asking. Unfortunately, not much progress due to time constraints. I would be happy if you release, because I can only use released builds for the openHAB main branch. Currently we use 2.5 for IP secure support. Data secure is not yet there.

holgerfriedrich avatar Oct 26 '22 21:10 holgerfriedrich