search

eclipse-californium / californium

CoAP/DTLS Java Implementation
https://www.eclipse.org/californium/
Other
723 stars 364 forks source link

Evolving the SessionCache #1345

Closed boaks closed 3 years ago

boaks commented 4 years ago

Though some issues around the SessionCachehave been raised, it may be a good idea to start a discussion about this SessionCache and the intended usage.

Some related issues of the past as starting point:

948

1163

1343

From #1343: The SessionCache seems for me to be not completely "specified". I don't know, if it is assumed to be "consistent" or "eventually consistent". The "removes" in case of a missing session id in the cache seems to point to the first, but intended "remote implementations" would then slow down too much. So, FMPOV, there is even specification work left, which then may change the implementations. That will be "time consuming".

I currently don't use this SessionCache and currently I prefer to improve the connection id usage in a cluster. Therefore my plan to invest time is more to make the "connection id cluster" working, than a clustered session resumption. With that it depends on others (including you :-) ), how much time you want to spend into the SessionCache.

boaks commented 3 years ago

Do you have a specific one in mind ?

Yep, it doesn't require to use the SessionTicket. And the current InMemoryConnectionStore takes also care about cleanup old connections based on the session id. (I'm not sure, how that was intended by the idea of that Session??? interface.)

sbernard31 commented 3 years ago

I looked at this a little bit.

Regarding SessionTicket, I see that SessionCache API is not so consistent. 

// current
void put(DTLSSession session);
SessionTicket get(SessionId id);

I would rather go to : 

// 1. alternative
void put(SessionId id, SessionTicket ticket);
SessionTicket get(SessionId id);

OR if we want to avoid SessionTicket<-->DTLSSession conversion for in memory implementation. (like the default one or persistent one with in memory cache)

// 2. alternative
void put(SessionId id, DTLSSession session);
DTLSSession get(SessionId id);

So either your first remark about SessionTicket is not so important and we could go for 1. or we can take it into account with the cost of more documentation in SessionStore explaining this is only needed to persist SessionTicket. (or we add an AbstractPersistentSessionStore which takes in parameter a SessionTicketStore)

sbernard31 commented 3 years ago

the current InMemoryConnectionStore takes also care about cleanup old connections based on the session id.

This is the lifetime/lifecycle question about Session. My intuition is that the API should allow to implement most used way to handle this. (captain obvious ... :sweat_smile:)

Some classic way I see is :

I don't know exactly what is the current behavior, but either it should be replaced by one those above :point_up: or the new API should be allow to implement it ? :thinking:

Which kind of clean-up exactly you think it will not be able to do with the SessionStore API ?

boaks commented 3 years ago

putEstablishedSession

At least, I don't see a obvious solution without the connectionsByEstablishedSession.

sbernard31 commented 3 years ago

This is about cleaning previous connection for the same session. To be sure there is only 1 connection by session, right ?

I guess at (D)TLS side, there is nothing to prevent to have several connections for the same session ? (see conccurent connection). So unless there is this kind of restriction at CoAP side and we want to limit Scandium DTLS implement to the CoAP flavor, I would just don't remove the connection.

It should not be an issue as Connection will be removed later anyway thanks to LRU cache or I missed something ?

boaks commented 3 years ago

I guess at (D)TLS side, there is nothing to prevent to have several connections for the same session ?

No, it's not forbidden. It makes just not that much sense. Similiar to renegotiation. Both seems in my opinion to be more caused by TCP than by the encryption. My point is more, that everyone must care about that and check, if it doesn't harm. It changes the behavior of the last 2-3 years. Therefore I would prefer, that those who want to use the "distributed session store", are those who then report their experience with it.

sbernard31 commented 3 years ago

As far as I understand this will not change the "behavior", just the memory management. (not so true this change the behavior a little as it will allow several connection for one session but it should not harm) But If you are not so confident and prefer to let user go back to the previous behavior, I would keep the connectionsByEstablishedSession attribute and provide the InMemorySessionStore as default behavior.

So if someone complain we can propose to go back to previous memory management and if after a while no complain we could maybe consider to remove it ?

boaks commented 3 years ago

keep the connectionsByEstablishedSession attribute and provide the InMemorySessionStore as default behavior.

Hm, that would require some additional changes, because:

public DTLSConnector(final DtlsConnectorConfig configuration, final SessionCache sessionCache)

Using null will create the default, using non-null will use a special implementation. So, I'm not sure, how it will be possible to go back to the connectionsByEstablishedSession.

sbernard31 commented 3 years ago

I guess with a new setter which activates the old behavior ? setDoNotUseSessionStore(true) (or something with a better name :sweat_smile:)?

sbernard31 commented 3 years ago

Uups I just get that you are talking about DTLSConnector. My previous idea was that SessionStore was set via DtlsConnectorConfig and currently this is not possible right ? But maybe we should (being able to set SessionStore via DtlsConnectorConfig.) ? :thinking:

boaks commented 3 years ago

But maybe we should (being able to set SessionStore via DtlsConnectorConfig.) ?

That may be a solution.

boaks commented 3 years ago

One resulting change in the "SessionStore" LRU behavior will be the "U". Currently it's the time of the last exchanged message, with the "SessionStore" it will be the time of the last resumption.

sbernard31 commented 3 years ago

Good point !

I don't know if this is an issue. To be honest, I don't even know what should be the right default behavior. I mean when the store is full which one should be removed first ? :thinking: Maybe the session which was create in first ? so you avoid to keep it for a too long period in the store. I'm not sure. (This is again the question : "is there security issue to keep Session for too long")

Anyway IF we want to be able to keep the session regarding the last exchanged message THEN we could maybe add a new method to the store, like :

boaks commented 3 years ago

I started yesterday evening with some code changes, hope I can finish that today.

May first results: without the "connectionsByEstablishedSession" some unit test are failing. Some because the tests checks the "remaining in the connection store". Though that change is expected, the test must be adapted. One other failure makes me more wondering. It's

DTLSConnectorAdvancedTest.testResumeWithVerify

With that, it requires the reanalyze, when a hello verify request should be used and when not. At least, before removing that connectionsByEstablishedSession.

boaks commented 3 years ago

The 

DTLSConnectorAdvancedTest.testResumeWithVerify

was failing, because the pendingHandshakesWithoutVerifiedPeer was only used for "local connections". NO it's fixed and the other tests, which have checked the connections in the connection store, now check the sessions in the session store.

boaks commented 3 years ago

See PR #1550 for first results.

boaks commented 3 years ago

About the connectionsByEstablishedSession:

The difference in keep it is:

FMPOV, though the rest of the functionality not longer depends now on that connectionsByEstablishedSession, I prefer to not use a SessionStoreby default. That limits this effect only to those, who want to use such a SessionStore.

sbernard31 commented 3 years ago

"remove connection on establish session" is about https://github.com/eclipse/californium/issues/1345#issuecomment-783541169 ?

"remove connection on failing find", I'm not sure to get this one. :thinking: Is it this part of the code ? https://github.com/eclipse/californium/blob/e7bd4e4bfe3076eb4f0271e788f68f836264b14e/scandium-core/src/main/java/org/eclipse/californium/scandium/dtls/InMemoryConnectionStore.java#L404-L415

boaks commented 3 years ago

Yes. Without the connectionsByEstablishedSession, it's not efficient to get the connection for a session.

Both points are not "too" hard nor "show stopper". It's more my experience of the past months/years, that "forcing" the usage of the "SessionStore" will cause a couple of weeks testing on my side, which I'm not really want for now. The related PR cleans up a lot for resumption handshake (I'm not completely ready). So the left difference with using a "SessionStore" by default or not, seems for me to ensure, that the "SessionStore" is really working, which requires that time. So, FMPOV, those who want the "cluster resumption" should have a very solid base to verify, that it works or what needs to be changed.

sbernard31 commented 3 years ago

To those 2 points :

We should also add "the meaning of 'used' for LRU cache of SessionStore" (see https://github.com/eclipse/californium/issues/1345#issuecomment-784950197)

About keeping connectionsByEstablishedSession as default, I think it could make sense at very short term but at short or mid term we should go to the InMemorySessionStore as default to get feedback.

I looked at the PR quickly (but not too deeply because there is several changes not really related to the SessionStore behavior and so not so easy to me to split it in my mind :sweat_smile:) Anyway, I didn't see changes on SessionStore API (like https://github.com/eclipse/californium/issues/1345#issuecomment-783501810 or https://github.com/eclipse/californium/issues/1345#issuecomment-784950197), this is something you consider to add for the first version or you prefer to not change it in a first time or you didn't thing about that or maybe you consider this is no so good idea ?

boaks commented 3 years ago

Anyway, I didn't see changes on SessionStore API

Basically I spent my time in understand all the obscure "if/else/maybe" parts in order to see, what is really mandatory and not. That results in a medium cleanup of the "new client hello". One "major" consequence is the ResumptionSupportingConnectionStore.find(SessionId), which now returns a DTLSSession instead of the Connector.

I'm still on verifying the changes so far. I will also try to test, if switching from SessionTicket to DTLSSession in the SessionStore. really makes sense. Maybe it just doesn't payoff ;-).

boaks commented 3 years ago

(I still feel, that a real, well-tested solution of this topic requires much more time, I'm able and willing to spend for now.)

Trying to follow all proposals and questions here, make me feel, that even that requires too much time. Anyway let me try to give some answers.

To be honest, I don't even know what should be the right default behavior. I mean when the store is full which one should be removed first ? thinking Maybe the session which was create in first ? so you avoid to keep it for a too long period in the store. I'm not > sure. (This is again the question : "is there security issue to keep Session for too long")

I have also no preference here. For the current simple demo implementation InMemorySessionStoreI choose the last put. For me, this is more similiiar to the used behavior then the age. Anyway, I guess it will require real experience to get a better approach.

Anyway IF we want to be able to keep the session regarding the last exchanged message THEN we could maybe add a new method to the store, like :

I don't think, that "fine grained updates" should be used. But also here, I guess it will require real experience to get a better approach.

boaks commented 3 years ago

Working on PR #1555 makes me finally believe, that just removing the SessionTicket because it gets obsolete, is the best choice. The SessionTicket is mainly a copy of the DTLSSession (with final fields). And so, without using it really as SessionTicket to keep the data on the client side, that class doesn't make sense to be maintained. If such a SessionTicket gets used for that, it will be much easier to split the DTLSSession.write/read functions than to maintain it as separate class.

sbernard31 commented 3 years ago

Does it impact the encoding decoding way ?

boaks commented 3 years ago

Sure, the point is, that any new field will change the encoding/decoding. The SessionTicket 2.6 was therefore hard to maintain. The new idea about serialization is to start with a "version", which helps to overcome a situation, if a change is hard to support it in a backwards compatible way. With that it is possible to keep "old readers" but use "new writers".

boaks commented 3 years ago

If your question targets how to migrate "existing binary sessionticket v2.6", I can try to add that encoding to the DTLSSession.readFrom(). That would also require to adapt the setter for the session id, but that should be not too hard.

sbernard31 commented 3 years ago

if your question targets how to migrate "existing binary sessionticket v2.6"

This was the idea behind question. But more to be aware of the impact than to discourage you to not make that change. (I mean I'm generally not against refactoring to make the code better)

I can try to add that encoding to the DTLSSession.readFrom(). That would also require to adapt the setter for the session id, but that should be not too hard.

If it's possible, I guess having backward compatibility solution could be good.

boaks commented 3 years ago

I tried to read the old encoded session tickets. But that requires also the old PrincipalSerializer and ServerNames, so I'm not sure if to add that pays off.

boaks commented 3 years ago

@sbernard31

I added a second commit with the "backwards compatible session ticket" read. Please have a rough check on that second commit, and signal, if you think, it paysoff. Otherwise I would remove that second commit.

sbernard31 commented 3 years ago

I quickly look at it. To be honest I didn't think too much about how we will handle the migration but I guess we will need something like this. It's hard to me to guess if we should keep this it californium or not. If you decide to not keep, could you create a branch containing this commit ? I guess that could be useful later at least as a code snippet.

boaks commented 3 years ago

If you decide to not keep, could you create a branch containing this commit ? I guess that could be useful later at least as a code snippet.

FMPOV, that code is mainly executed "once". I'm not sure, what the best approach will be. At least a separate branch should help those, who want to migrate serialized 2.6 SessionTickets.

read_sessionticket

PR #1558

boaks commented 3 years ago

FMPOV, that's done with the work in master/3.0.

Other opinions?

boaks commented 3 years ago

Update:

Using session resumption for the hono coap-adapter required to add also a none-blocking API for session resumption. See PR #1618 . With that, it should be possible to implement also other strategies to resume a session within a cluster (e.g. "lazy loading/loading on request").