openthread / ot-br-posix

OpenThread Border Router, a Thread border router for POSIX-based platforms.
https://openthread.io/
BSD 3-Clause "New" or "Revised" License
415 stars 231 forks source link

There are two entry for one SRP service. #1425

Closed jinpeng1989 closed 2 years ago

jinpeng1989 commented 2 years ago

There are two entry for one SRP service, is it a issue? Please see the below picture and wireshark attachment.

Thread network key: e03c8d1772e7151e3694808fa822a632 Git commit id: 5be35949c6f1fd4cc3a8e63169cec6a2211135a4

image

image srp_register.zip

wgtdkp commented 2 years ago

There is one additional/unexpected PTR record in the DNS update messages:

FD10F18847._knx._udp.default.service.arpa: type PTR, class IN, siemens-FD10F18847._knx._udp.default.service.arpa

But the only expected PTR record is:

_knx._udp.default.service.arpa: type PTR, class IN, siemens-FD10F18847._knx._udp.default.service.arpa

Could you share the SRP client commit ID and steps (SRP client CLI commands) to construct the SRP message? Please also share the output of srp client service on the client side. Thanks!

wgtdkp commented 2 years ago

@jinpeng1989 Are you still seeing the same issue?

abtink commented 2 years ago

I investigated this a bit

W see in the pcap records in SRP update message that there are really two services (two PTR records): 1) A PTR mapping a service named _knx._udp.default.service.arpa to sevice instance siemens-FD10F18847._knx._udp.default.service.arpa 2) Another PTR mapping a service named FD10F18847._knx._udp.default.service.arpa to the same service instance siemens-FD10F18847._knx._udp.default.service.arpa

So the two entries we see in the SRP server CLI command output are actually two different but similarly named services:

Note that the SRP server CLI command will output the full instance name (which contains/ends with service name) and does not output the service name separately.


Some other points:

I don't think the SRP update message here was generated by OpenThread SRP client implementation.

jinpeng1989 commented 2 years ago

The SRP update message here was not generated by OpenThread SRP client implementation. The source code of the SRP client: https://github.com/Abhayakara/dnssd-registration

I aim to use FD10F18847 as sub service type. Could you show me the correct records format?

jinpeng1989 commented 2 years ago

There is no SRP Client in the earlier versions of the OpenThread. So we integrated the SRP Client ourselves.

wgtdkp commented 2 years ago

Some other points:

  1. The second PTR in the message does not actually follow the proper naming model. The instance name should be "{instance label}.{service-name}" which is not the case for this.
  2. I think It makes sense to reject this as invalid SRP update message due to this.
  3. The OT SRP server does check the instance name to end with service name (which happens to be the case with names used here) but does not enforce that it happens at label boundary. So the SRP update message is not rejected in this case.

Agree, the SRP server should reject this PTR record.

wgtdkp commented 2 years ago

@jinpeng1989 You can find the subtype format definition in https://www.rfc-editor.org/rfc/rfc6763.html#section-7.1, for your case, the PTR record name should be FD10F18847._sub._knx._udp.default.service.arpa.

We encourage you to try out the OT SRP client which supports all features in the SRP spec and adopted by Matter. To enable OT SRP client, simply build with OT_SRP_CLIENT=ON and OT_ECDSA=ON. Concrete SRP client example can be found in the BR codelab: https://openthread.io/codelabs/openthread-border-router

jinpeng1989 commented 2 years ago

We use STM32WB55 SDK version 1.12 which uses OpenThread stack SHA-1: 3dbd91aa2b70c7d5cc71b2c465ce3583a13dea79 (thread-reference-20191113). This product doesn't have time to upgrade to a higher version.

abtink commented 2 years ago

I actually guessed that second PTR may be intended as sub-type. 😃 @wgtdkp already mentioned the proper name for sub-type, you need the ._sub. as label in the name after the subtype label.

Later (when I get a chance), will update the SRP server code to enforce the name check at boundary of label (so to reject such invalid formats in a message).