eclipse-leshan / leshan

Java Library for LWM2M
https://www.eclipse.org/leshan/
BSD 3-Clause "New" or "Revised" License
652 stars 408 forks source link

Redesign peer's `Identity` class. #1436

Closed sbernard31 closed 1 year ago

sbernard31 commented 1 year ago

At #1421, we discover that we have maybe an issue with current Identity class/concept in Leshan.

For CoAP, Identity contains only "socket address" (I think that makes sense, because we have nothing more to use) For CoAPs, Identity contains "TLS Identifier" + "socket address" and maybe this is not so good, "socket peer address" should not be used to identify the peer. If that was the case when we search by identity for CoAPs even if socket address changed we will be able to find the profile.

(from : https://github.com/eclipse/leshan/issues/1421#issuecomment-1473511816)

This issue aims to talk about a new design.

This will probably lead to some Redis Serialization break again :grimacing: (See https://github.com/eclipse/leshan/issues/1417)

sbernard31 commented 1 year ago

Some thoughts about this :

Current Design:

Currently we have 1 class called Identity with these attributes :

public class Identity {
  InetSocketAddress peerAddress;
  String pskIdentity;
  PublicKey rawPublicKey;
  String x509CommonName;
  OscoreIdentity oscoreIdentity;
  ...
}

This idea of that class was to identified a Foreign Peer (Client or Server)

Current Drawbacks :

But there is maybe several issues with this design : 1) we see just above that's not alway a good idea to consider peerAddress as part of identity. 2) Adding new kind of identity means create new fields in the only 1 existing class ...

Some Concepts :

Here some similar concept in Java :

I'm not sure we should reuse those classes but we should maybe create a new class hierarchy to separate foreign peer and identity concepts ? About naming I don't know ?

Proposed Design :

I guess classes could looks like

public interface Peer {
  Identity getIdentity()
}

public class IpPeer implements Peer {  // This class is to anticipate Non-IP protocol
  InetSocketAddress getSocketAddress()
  Identity getIdentity()
}

public interface Identity {
  String getKeyIdentifier() // TODO I don't know if we really need this. 
  String toString();
  boolean equals(Object obj)
  int hashCode();
}

public class SocketIdentity implements Identity {
  InetSocketAddress getSocketAddress();
}

public class PskIdentity implements Identity {
  String getIdentity();
}
public class RpkIdentity implements Identity {
  PublicKey getPublicKey();
}
public class X509Identity implements Identity {
  String getX509CommonName(); // when we will go futher we will probably see that current design is too limited. 
}

public class OscoreIdentity implements Identity {
   byte[] getRecipientId();
}

OSCORE + TLS case :

Now there is one problem with this design ... OSCORE can be used togeter with (D)TLS protocol. So to support that either :

public class OscoreOverTlsIdentity implements Identity {
   OscoreIdentity getOscoreIdentity();
   Identity getTlsIdentity(); // we can create a TlsIdentity interface. 
}

@JaroslawLegierski, @jvermillard, @rikard-sics if you have any opinions / ideas about this, do not hesitate to share.

sbernard31 commented 1 year ago

Just see, we maybe also need to think about Identity of Gateway (#1237) https://github.com/eclipse/leshan/blob/a9f5ab469d22a44af68713217ba8a292bb66706f/leshan-server-core/src/main/java/org/eclipse/leshan/server/registration/RegistrationHandler.java#L139-L142

JaroslawLegierski commented 1 year ago

Maybe the name LwM2mIdentity would be better to differentiate new identity design from the current solution ?

for non IP devices we can extend IpPeer class by adding in the future:

e.g. for LoRaWAN DevEUI getDevEUI() or devId getdevId() for NIDD (where devId is: IMEI, IMSI, MSISDN, ICCID, ... etc)

for OSCORE with (D)TLS proposal of usage OscoreOverTlsIdentity class is in my opinion very good idea.

sbernard31 commented 1 year ago

@JaroslawLegierski thx for feedback.

Maybe the name LwM2mIdentity would be better to differentiate new identity design from the current solution ?

You think this will help for migration ?

JaroslawLegierski commented 1 year ago

@JaroslawLegierski thx for feedback.

Maybe the name LwM2mIdentity would be better to differentiate new identity design from the current solution ?

You think this will help for migration ?

Yes mainly for migration

For better understanding your concept I started preparing PoC

From what I understood Identity will be replaced by IpPeer from API user point of view e.g: here ?

I'm not sure what the getKeyIdentifier() method should return something like this "EP#IDENTITY#" (for Redis)?

sbernard31 commented 1 year ago

For better understanding your concept I started preparing PoC

:+1:

From what I understood Identity will be replaced by IpPeer from API user point of view e.g: here ?

Yep.

I'm not sure what the getKeyIdentifier() method should return something like this "EP#IDENTITY#" (for Redis)?

Nope.

I was thinking that maybe this getter could help store implementation as this string could be used by store as index to search by Identity. E.G. a PskIdentity will maybe returns something like psk#my_device_identity, RpkIdentity maybe rpk#my_public_key_as_hex_or_base64

But I'm not sure this is a good idea to do that in that class. Maybe better to let store to that ? We need to decide which class is responsible to do that.

JaroslawLegierski commented 1 year ago

I created first working version IpPeer implementation:

sbernard31 commented 1 year ago

:+1: I will start to look at this on 3rd May.

sbernard31 commented 1 year ago

I created PR #1445 with your work (2 commit squashed in one) rebased on master + some additions.

About,

(old) Identity was eliminated and replaced by IpPeer

I think this not just about replacing Identity by IpPeer, some Identity must be replaced by Peer, some other by IpPeer and other one by LwM2mIdentity. I will try to move forward on this this afternoon. :slightly_smiling_face:

sbernard31 commented 1 year ago

Since #1436 is integrated in master, we can close this one.