`Identity` Refactoring #1445

sbernard31 · 2023-05-05T09:44:56Z

This aims to implement #1436, based on @JaroslawLegierski work (#1436 (comment))

sbernard31 · 2023-05-12T13:34:39Z

@JaroslawLegierski, I moved forward on this.

There is more changes than what I expected. I guess someone could say : "as usual" 😬
I try to put changes in different commit to make it more readable.

Maybe you want to review/take a look at this ?
And/or test it ?

About Redis implementation (See some TODO in code) :

this is not backward compatible, I guess we need to talk about How to handle data format Compatibility break in Redis Store ? #1417 before.
we probably need to add a way to provide custom LwM2mIdentitySerDes and LwM2mPeerSerDes.
I also don't know if we need to add a type field in LwM2mIdentitySerDes and LwM2mPeerSerDes.

I don't know if naming is so good but didn't find anything better for now.

Hope this goes in the right direction.

sbernard31 · 2023-05-12T13:53:55Z

Maybe we also need some tests about : #1421.
To simulate NAT maybe we can use : NioNatUtil or maybe you have a more simple idea ?

JaroslawLegierski · 2023-05-12T14:10:26Z

@JaroslawLegierski, I moved forward on this.

There is more changes than what I expected. I guess someone could say : "as usual" 😬 I try to put changes in different commit to make it more readable.

Maybe you want to review/take a look at this ? And/or test it ?

About Redis implementation (See some TODO in code) :
* this is not backward compatible, I guess we need to talk about [How to handle data format Compatibility break in Redis Store ? #1417](https://github.com/eclipse/leshan/issues/1417) before.

* we probably need to add a way to provide custom `LwM2mIdentitySerDes` and  `LwM2mPeerSerDes`.

* I also don't know if we need to add a `type` field in `LwM2mIdentitySerDes`  and   `LwM2mPeerSerDes`.
I don't know if naming is so good but didn't find anything better for now.

Hope this goes in the right direction.

Thank You very much - I will continue tests/review in next week

JaroslawLegierski · 2023-05-12T14:14:02Z

Maybe we also need some tests about : #1421. To simulate NAT maybe we can use : NioNatUtil or maybe you have a more simple idea ?

OK - this is very good idea - I will also check Leshan server behavior when the client's IP address changes

JaroslawLegierski · 2023-05-22T07:25:49Z

This is not is related to "new Identity concept " however during project building I noticed the following new WARNING

[INFO] Browserslist: caniuse-lite is outdated. Please run:
[INFO]   npx update-browserslist-db@latest
[INFO]   Why you should do it regularly: https://github.com/browserslist/update-db#readme

sbernard31 · 2023-05-24T12:23:48Z

I noticed the following new WARNING

~~This should be done regularly, I will do that at same time I update demo dependencies. 🙂~~

Done with commit :

I also update webapp README about common maintenance tasks : 8af670c

JaroslawLegierski · 2023-05-25T09:38:34Z

I finished first part of tests "new identity concept" using InMemoryRegistrationStore

Security method	Send after IP change	Notification (observation) after IP change
Unsecure	NOT OK	OK
PSK	OK	OK
RPK	OK	OK
X.509	OK	OK
Oscore	OK	OK

only device with unsecure connection is not able to send data to the server after IP address change (which is obvious and expected 😉 )

JaroslawLegierski · 2023-05-26T07:45:12Z

Currently when unsecure device changes IP address the response on Send is 500 Internal Server Error.
Maybe something like "no registration found" will be better response in such case ?

JaroslawLegierski · 2023-05-29T08:45:57Z

Please find bellow results of tests using RedisRegistrationStore:

Security method	Send after IP change	Notification (observation) after IP change
Unsecure	NOT OK	OK
PSK	OK	OK
RPK	OK	OK
X.509	OK	OK
Oscore	not implemented	not implemented

JaroslawLegierski · 2023-05-31T12:44:16Z

About Redis implementation (See some TODO in code)
this is not backward compatible, I guess we need to talk about #1417 before.
we probably need to add a way to provide custom LwM2mIdentitySerDes and LwM2mPeerSerDes.
I also don't know if we need to add a type field in LwM2mIdentitySerDes and LwM2mPeerSerDes.

Regarding LwM2mIdentitySerDes and LwM2mPeerSerDes are You thinking about something like this solution ?

sbernard31 · 2023-06-01T15:56:58Z

Regarding LwM2mIdentitySerDes and LwM2mPeerSerDes are You thinking about something like this solution ?

Yep exactly. Could you provide a PR targeting identity_refactoring branch.

Thx for testing this 🙏, do you also review the code ? if no, do you plan to do it (it's just to know for organization 🙂 ) ?

So what is missing now ?

Find better name or keep the current one if this is good enough
Decide if we add a type field in json generated by LwM2mIdentitySerDes and LwM2mPeerSerDes.
Discuss about backward compatibility, (How to handle data format Compatibility break in Redis Store ? #1417)

JaroslawLegierski · 2023-06-02T12:19:44Z

Yep exactly. Could you provide a PR targeting identity_refactoring branch.

OK done in PR

Thx for testing this 🙏, do you also review the code ? if no, do you plan to do it (it's just to know for organization 🙂 ) ?

I did a quick check only. Only one remark is described here (already included in PR)

So what is missing now ?

Find better name or keep the current one if this is good enough
Decide if we add a type field in json generated by LwM2mIdentitySerDes and LwM2mPeerSerDes.
Discuss about backward compatibility, (#1417)

Today I must focus on another task but I will come back to these questions on Monday

JaroslawLegierski · 2023-06-05T14:04:16Z

Find better name or keep the current one if this is good enough

I think that current names are OK

Decide if we add a type field in json generated by LwM2mIdentitySerDes and LwM2mPeerSerDes.

Do you mean by type fields something like this:

{
	"regDate": 1685972628230,
	"transportdata": 
		"type": "ippeer",
		"address": "127.0.0.1",
		"port": 53486,
		"identity": {
			"type": "unsecure",
			"address": "127.0.0.1",
			"port": 53486
		}
	},
...

for usecure ?

or

{
	"regDate ": 1685965383275,
	"transportdata ": {
		"type": "ippeer",
		"address ": "127.0.0.1 ",
		"port ": 57674,
		"identity ": {
			"type": "pskid",
			"pskid ": "leshan_1 "
		}
....

for psk: ?

Discuss about backward compatibility, (How to handle data format Compatibility break in Redis Store ? #1417)

I commented this here (I also confirmed this with my French colleagues).

sbernard31 · 2023-06-05T15:37:24Z

About type, yes something like this.

JaroslawLegierski · 2023-06-06T14:00:36Z

About type, yes something like this.

Regarding 'type' I prepared simple implementation in this branch. Please let me know if this is what you have in mind.

sbernard31 · 2023-06-07T09:05:25Z

Regarding 'type' I prepared simple implementation in this branch. Please let me know if this is what you have in mind.

Yep I had something like this in mind.
About naming, I feel that maybe : type is enough (instead of identitytype and peertype) unless I missed a good reason to use a more explicit name ?

So about :

Decide if we add a type field in json generated by LwM2mIdentitySerDes and LwM2mPeerSerDes.

Do you thing this is better with or without it ?

JaroslawLegierski · 2023-06-07T10:44:26Z

Yep I had something like this in mind.
About naming, I feel that maybe : type is enough (instead of identitytype and peertype) unless I missed a good reason to use a more explicit name ?

OK - already corrected in this commit

Do you thing this is better with or without it ?

Apart from development cases (data validation), I think that this field can be usefull in some maintenance cases (such as collection information from Redis about existing session grouped by identity type).

sbernard31 · 2023-06-14T15:58:25Z

@JaroslawLegierski, I rebased this branch (resolving conflict) on master so if you have work based on it, you will probably need to cherrypick your commits (or any other way) on it.

JaroslawLegierski · 2023-06-16T08:07:43Z

@JaroslawLegierski, I rebased this branch (resolving conflict) on master so if you have work based on it, you will probably need to cherrypick your commits (or any other way) on it.

OK thx for info.

sbernard31 · 2023-06-19T15:08:35Z

I re-read this discussion and missing point are :

type field in Redis serialization : @JaroslawLegierski do you plan to create a PR for that ?
Adding integration/automatic tests to check behavior when IP changes (Identity Refactoring #1445 (comment))

JaroslawLegierski · 2023-06-20T08:59:08Z

I re-read this discussion and missing point are :

1. `type` field in Redis serialization : @JaroslawLegierski do you plan to create a PR for  that  ?

2. Adding integration/automatic tests to check behavior when IP changes ([`Identity` Refactoring #1445 (comment)](https://github.com/eclipse/leshan/pull/1445#issuecomment-1545786926))

Oops I completely forgot about keys. PR already created

I am (mostly) out of the office this week, but @Warmek will help us with this topic.

JaroslawLegierski · 2023-06-20T11:10:44Z

Sorry for messy commits (work during breaks in training is not good idea) 😅

sbernard31 · 2023-06-23T13:11:02Z

@Warmek you asked at (#1421 (comment)):

How can I change LeshanTestClient ip?

I'm answering here because this is the most appropriate place.
Currently there is no way to do that. If you read comment above, you will see that I refer to NatUtil from californium maybe we can use it but I hope this is not too overkill ...

sbernard31 · 2023-06-26T15:32:53Z

@Warmek, @JaroslawLegierski Do you start to work on "adding automatic test about address changing" OR plan to work on it ?

If not I will begin to work on this tomorrow. I would like to not wait too much before to merge this because this is a very impacting PR which could probably generate conflict with parallel work. 🙂

Warmek · 2023-06-26T15:40:18Z

I started to implement those tests, but I have an issue running them. I used NioNatUtil as you suggested above, but when i import it into tests and run the test i get following exception:
java.lang.SecurityException: class "ch.qos.logback.classic.spi.TurboFilterList"'s signer information does not match signer information of other classes in the same package

Do you have an idea what might cause it and how to fix it?

sbernard31 · 2023-06-26T16:08:55Z

So the alternative are :

using NatUtil if my exclude trick works AND if you are comfortable with the idea to depends on it.
implementing our own simple utility (a bit like NatUtil but with less feature)
find something else (I tried to search quickly and for now I don't find anything else)

Warmek · 2023-06-27T12:15:39Z

The trick didn't work, but I think it still would be easier to modify NatUtli to fix this mismatch than to write the same tool myself. Tell me what you think about it

sbernard31 · 2023-06-27T16:24:28Z

The trick didn't work

OK I will try to do it myself tomorrow, just to be sure.

I think it still would be easier to modify NatUtli to fix this mismatch than to write the same tool myself. Tell me what you think about it.

I'm not sure what means fixing the mismatch, I guess we don't want to go back to slf4j 1.7.x and not sure that californium want to move to slf4j 2.x. (Note that we are not sure this is really the problem)

We don't need all feature of NioNatUtil, so I tried to implement a simple alternative this is less than 200 line of code. (bee11a6)
Probably still a bit buggy, code need to be polish and I need to find a better way to integrate it in our integration test.

sbernard31 · 2023-06-28T09:03:02Z

About NatNioUtil :

java.lang.SecurityException: class "ch.qos.logback.classic.spi.TurboFilterList"'s signer information does not match signer information of other classes in the same package

I was not able to reproduce it using (58384ed) . So I can not test my trick either 😬
Eventually, If you have code to share I can try to reproduce.

Anyway on my side, I will explore the SimpleNat solution just above. ☝️

sbernard31 · 2023-06-29T15:40:50Z

I moved forward about implementing our own Nat Utility.
I decided to rename it from SimpleNat to ReserveProxy because finally I think this is more a reverseProxy than a NAT.
(But Maybe I'm wrong, so feel free to ask to rename it again for the right name if needed)

I face some difficulty to integrate it correctly in integration tests API but finally I think it is not so bad.
The code is available at : f8181a8
And example on how to use it : 21f80ec (The test failed but looking at wireshark all seems to work as expected)

Co-authored-by: Simon Bernard <sbernard@sierrawireless.com>

sbernard31 · 2023-07-03T13:32:12Z

I fixed some issue in ReserveProxy and add automatic tests about IP changing use cases. All is available in this PR.

For now I didn't add tests for OSCORE.

There still an issue about leshanServerBuilder.setUpdateRegistrationOnNotification(true) which does not update client socket address correctly but it should be addressed in another PR.

Concerning this PR, I think it is in a reviewable / testable state.

Only last little point which I would like to understand before to integrate it in master (just in case there is an hidden issue)

My tests results show :

Security method	Send after IP change	Notification (observation) after IP change
Unsecure	NOT OK	NOT OK
PSK	OK	OK
RPK	OK	OK
X.509	OK	OK
Oscore	OK	OK

which is a bit different than @JaroslawLegierski's result at #1445 (comment). It would be good to clarify this before to integrate the PR 🤔

@JaroslawLegierski @Warmek, I don't know if you want to review and/or test it ? (let me know if you plan to do it)

JaroslawLegierski · 2023-07-03T19:32:34Z

Only last little point which I would like to understand before to integrate it in master (just in case there is an hidden issue)

My tests results show :
Security method Send after IP change Notification (observation) after IP change
Unsecure NOT OK NOT OK
PSK OK OK
RPK OK OK
X.509 OK OK
Oscore OK OK

which is a bit different than @JaroslawLegierski's result at #1445 (comment). It would be good to clarify this before to integrate the PR 🤔

@JaroslawLegierski @Warmek, I don't know if you want to review and/or test it ? (let me know if you plan to do it)

I retested today Notification (observation) after IP change - and I want to confirm that for unsecure client final result is NOT OK (I got RST from the server)

sbernard31 · 2023-07-04T08:56:03Z

I want to confirm that for unsecure client final result is NOT OK (I got RST from the server)

Great, so on my side, I have nothing more to add to this PR.

@JaroslawLegierski, @Warmek let me know :

if you want to add something in this PR,
if you want to test / review some part of it.
OR if we are good to integrated in master ?

Warmek · 2023-07-04T14:36:59Z

We are good to integrate into master

Warmek · 2023-07-05T09:03:27Z

@sbernard31 When are you planing to integrate this PR into master?

sbernard31 · 2023-07-05T09:11:16Z

I don't know, do we need @JaroslawLegierski green-light ?
If, yes then I wait for him. (I can even wait for a week if needed, not a big deal to me)
If, no then this can be done very quickly.

Warmek · 2023-07-05T09:13:19Z

I talked with him and we have his greenlight

sbernard31 · 2023-07-05T09:39:28Z

So I integrated it in master 🙂

sbernard31 mentioned this pull request May 5, 2023

Redesign peer's Identity class. #1436

Closed

sbernard31 mentioned this pull request Jun 5, 2023

How to handle data format Compatibility break in Redis Store ? #1417

Open

sbernard31 force-pushed the identity_refactoring branch 3 times, most recently from e3c50ad to 4dcedbb Compare June 14, 2023 15:56

sbernard31 force-pushed the identity_refactoring branch from 004d3aa to 2c146b2 Compare July 3, 2023 09:21

sbernard31 and others added 7 commits July 3, 2023 14:31

Remove useless class which should not have been committed in 6091fc3

a3d0931

GH-1436: Refactoring about Identity class

f994bc1

Co-authored-by: Simon Bernard <sbernard@sierrawireless.com>

Rename ServerIdentity in LwM2mServer at client side.

1186c01

ClientProfileProvider returns Profile only when registration is found

ec90257

Make LwM2mIdentitySerDes / LwM2mPeerSerDes customizable in Redis store

2066961

Add ReserverProxy to be able to test client IP address changes.

c8606bc

Expose CoapEndpoint instead of Endpoint at client side

330617f

sbernard31 force-pushed the identity_refactoring branch from 2c146b2 to dba8cb1 Compare July 3, 2023 13:26

Add tests about dynamic IP for observer and send operation

6a05894

sbernard31 force-pushed the identity_refactoring branch from dba8cb1 to 6a05894 Compare July 3, 2023 13:28

sbernard31 marked this pull request as ready for review July 3, 2023 13:32

sbernard31 mentioned this pull request Jul 3, 2023

Unable to handle notification after device ip change in M11 #1421

Closed

sbernard31 merged commit 6a05894 into master Jul 5, 2023

sbernard31 deleted the identity_refactoring branch July 10, 2023 15:51

This was referenced Jul 11, 2023

Plan for 2.0.0-M12 🎯 #1468

Closed

Some polish relative to Peer Identity refactoring #1470

Merged

Identity Refactoring #1445

Identity Refactoring #1445

Conversation

sbernard31 commented May 5, 2023 • edited Loading

sbernard31 commented May 12, 2023

sbernard31 commented May 12, 2023

JaroslawLegierski commented May 12, 2023

JaroslawLegierski commented May 12, 2023

JaroslawLegierski commented May 22, 2023

sbernard31 commented May 24, 2023 • edited Loading

JaroslawLegierski commented May 25, 2023

JaroslawLegierski commented May 26, 2023

JaroslawLegierski commented May 29, 2023

JaroslawLegierski commented May 31, 2023

sbernard31 commented Jun 1, 2023

JaroslawLegierski commented Jun 2, 2023

JaroslawLegierski commented Jun 5, 2023

sbernard31 commented Jun 5, 2023

JaroslawLegierski commented Jun 6, 2023

sbernard31 commented Jun 7, 2023

JaroslawLegierski commented Jun 7, 2023 • edited Loading

sbernard31 commented Jun 14, 2023

JaroslawLegierski commented Jun 16, 2023

sbernard31 commented Jun 19, 2023 • edited Loading

JaroslawLegierski commented Jun 20, 2023

JaroslawLegierski commented Jun 20, 2023

sbernard31 commented Jun 23, 2023 • edited Loading

sbernard31 commented Jun 26, 2023

Warmek commented Jun 26, 2023

sbernard31 commented Jun 26, 2023

Warmek commented Jun 27, 2023

sbernard31 commented Jun 27, 2023 • edited Loading

sbernard31 commented Jun 28, 2023

sbernard31 commented Jun 29, 2023

sbernard31 commented Jul 3, 2023

JaroslawLegierski commented Jul 3, 2023

sbernard31 commented Jul 4, 2023

Warmek commented Jul 4, 2023

Warmek commented Jul 5, 2023

sbernard31 commented Jul 5, 2023

Warmek commented Jul 5, 2023

sbernard31 commented Jul 5, 2023

`Identity` Refactoring #1445

`Identity` Refactoring #1445

sbernard31 commented May 5, 2023 •

edited

Loading

sbernard31 commented May 24, 2023 •

edited

Loading

JaroslawLegierski commented Jun 7, 2023 •

edited

Loading

sbernard31 commented Jun 19, 2023 •

edited

Loading

sbernard31 commented Jun 23, 2023 •

edited

Loading

sbernard31 commented Jun 27, 2023 •

edited

Loading