Add endpoint identifier. #322

sbernard31 · 2017-06-15T13:03:02Z

Here is a try to define an identifier for foreign endpoint.
The PR was largely inspired by this long discussion.

This is a work in progress and should not be merged now.
This first step introduces the concept of endpoint identifier and uses it to identify observation.

Next step : Adding Principal to CorrelationContext to be able to define PrincipalEndpointIdentifier to be able to test that in Leshan.

While this first step, I had the feeling that maybe CorrelationContext could be not necessary at CoAP side anymore (only at Connector side). This is a track I would explore. This could simplify the code and also solve some questions like #311.

Another point will be to change TokenProvider (replace reserve/release by random generation) to solve issue raised by Achim in #173.

Even, if this is a work in progress, I strongly advice to try to review this PR each time I add new commits. This is the only way to make this review digestible.

boaks · 2017-06-15T15:31:24Z

To be frank:
Though the discussion about RFC7252, section 9.1.2 has started again, I would not change the API too much until that discussion seems to get a direction "with future".

So in my view, providing the "intended credentials or parts of the credential as identity" ahead when sending a Message, should overcome the callbacks with that information at all. The calback are only required, if you bind the matching to the "created session".
Therefore I would not add the callbacks for the "destination endpoint". To use these credentials as identity, these credential could not be reused (e.g. groups of devices with same credentials), but until someone came up with that, I would not care too much about that.

For notifies received from changed addresses, the credential are also available on receive and could be used to find the proper exchange (and adjust the ip-address for future communication to that device). Again, this requires unique credentials for all devices.

boaks · 2017-06-15T15:47:09Z

Not to change the API to much, my intention was to put that "identity" into the correlation context and add a optional correlation context also for sending a request.

sbernard31 · 2017-06-15T16:58:40Z

I would not change the API too much until that discussion seems to get a direction "with future".

I'm not so confident. I don't think a final solution will be proposed soon :/

The calback are only required, if you bind the matching to the "created session".

Callback is necessary for use case where peer don't use authentication, or where the identifier chosen could not be know by the user before (e.g. session ID)

Therefore I would not add the callbacks for the "destination endpoint"

At short term, I need it for backward compatibility. For reason exposed above, I'm not sure we will be able to remove it. But If we can I agree to remove it. (Less code we have better, it is)

To use these credentials as identity, these credential could not be reused (e.g. groups of devices with same credentials),

That's why having a EndpoinIdentifierFactory is a good way to let users choose what they want to use as "identifier"

Not to change the API to much, my intention was to put that "identity" into the correlation context and add a optional correlation context also for sending a request.

I rather prefer to remove the CorrelationContext to CoapStack and just let CorrelationContext at element-connector level.

Unless you have strong opinion against my first step, I will continue in that way more in a POC spirit. We have this "identifier" issue for a long time ago and I would like to move forward. I'm sure I will face issue I don't think about for now.

boaks · 2017-06-15T17:59:21Z

I'm not so confident. I don't think a final solution will be proposed soon :/

I agree, that it will not be proposed soon.

Callback is necessary for use case where peer don't use authentication, or where the identifier chosen could not be know by the user before (e.g. session ID)

That's exactly the NONE use case for me. But if you want to solve that, try it.
I just don't understand, why additional values in the correlation context will not do it.

sbernard31 · 2017-06-16T13:29:46Z

I just don't understand, why additional values in the correlation context will not do it.

You mean instead of using endpoint identifier ?

boaks · 2017-06-16T13:32:29Z

Yes.
Add the "stringified credential identity" into it and add a optional correlation context to the send(request).

sbernard31 · 2017-06-16T13:48:22Z

And we could use a partial/incomplete correlation context on send() ?
Is this context will be used as identifier for observe ? (correlationContext+token)

boaks · 2017-06-16T13:55:48Z

And we could use a partial/incomplete correlation context on send() ?

Therefore the exchangeable correlation context matcher was introduced. My idea was to make a matcher, which uses the identity.

Is this context will be used as identifier for observe ? (correlationContext+token)

My idea was to use the "identity" to adjust the "address".

sbernard31 · 2017-06-16T14:06:52Z

My idea was to use the "identity" to adjust the "address".

That's mean you identify observation by (address+token) ?

boaks · 2017-06-16T14:12:24Z

The UdpMatcher.receiveResponse() currently uses address + token to lookup for the exchange.

So my idea (not detailed right now) was, that a correlation context may also be used for adjust addresses before this lookup.

sbernard31 · 2017-06-16T15:48:55Z

So you should have a kind of index {identity => keys (address+token) }?
Then update keys with the new address. Hopping that news address was not already used.
Does it simpler to just use identity+token as key instead ?

boaks · 2017-06-19T07:41:07Z

My basic idea is to add a "identifier" (say the psk identity) to the correlation context and pass it in with the request. The connector the decided, if the "identifier" matches using the correlation context matcher. On receiving a message, the connector adds the identity to the correlation context.
The Exchange-Matcher tries to match the incoming message as it does now (so normal request/response mapping will not work with address changes). If no exchange is found, but the message is identified as notify, the ObservationStore is considered. To possibly used the identity there (and to solve issue #173), the ObservationStore API must be extended. Though its no detailed design, sure there may occur some follow-up issues. But for now this seems to me to be the smallest change with the required effect. It depends then on the ObservationStore implementation, how to match, if no identity is used. If someone would like to us the token only approach, it would be his decision.

boaks · 2017-07-06T13:13:31Z

californium-core/src/main/java/org/eclipse/californium/core/network/BaseMatcher.java

@@ -158,7 +163,8 @@ protected final Exchange matchNotifyResponse(final Response response, final Corr
 		final Exchange.KeyToken idByToken = Exchange.KeyToken.fromInboundMessage(response);
 		Exchange exchange = null;

-		final Observation obs = observationStore.get(response.getToken());
+		final EndpointIdentifier sourceEndpoint = response.getSourceEndpoint();


But, where is that "reponse source endpoint" set during receiving a response?
That's the core of my question, on what "protocol artifact" your are going to build the identity.

This is set in coapEndpoint.

And the IdentifierFactory is responsible to define what should be used as Identifier.

(For Leshan I want to use Principal)

The IdentifierFactory is responsible to translate Identifier in context and context in Identifier.
(translation is centralized in this class)

I have in mind that context could be a connector concept and Identifier a CoAP stack one.

OK.

So the main difference seems to be, that I would just pass the CorrelationContext into the ObserveStore, and you would prefer to have a "IdentifierFactory" to convert that CorrelationContext ahead into a EndpointIdentifier.

boaks · 2017-07-07T12:42:57Z

californium-core/src/main/java/org/eclipse/californium/core/network/BaseMatcher.java

+
+				public void onDestinationEndpointDefined(EndpointIdentifier destination) {
+					LOG.log(Level.FINER, "registering observe request {0}", request);
+					observationStore.add(destination, new Observation(request, exchange.getCorrelationContext()));


If we delay the add until onDestinationEndpointDefined, I'm not sure if we require the remove in failed()

I'm not sure. The destination endpoint is defined just after an handshake success, but the APPLICATION _DATA sending can fail. In that case we want to remove observation from store.

sbernard31 · 2017-11-13T17:21:43Z

This will never be merged.
I hope the idea to have a better identifier for observations will be not dropped too :)

sbernard31 added 2 commits June 15, 2017 14:28

#173: add endpoint identifier concept

d0d1458

Add test to check observation is not just identified by token anymore.

ccf1a0e

sbernard31 mentioned this pull request Jul 5, 2017

CorrelationContext #344

Closed

boaks reviewed Jul 6, 2017

View reviewed changes

boaks reviewed Jul 7, 2017

View reviewed changes

boaks mentioned this pull request Jul 25, 2017

Reg. Coap Server using Scandium #362

Closed

sbernard31 mentioned this pull request Sep 6, 2017

Make request/response correlation configurable #174

Closed

sbernard31 closed this Nov 13, 2017

sbernard31 deleted the endpoint_identifier branch March 8, 2018 13:25

sbernard31 mentioned this pull request Sep 5, 2019

Extend TokenGenerator - reintroduce address scoped tokens #1064

Closed

sbernard31 mentioned this pull request Feb 11, 2020

Observationstore improvement #87

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add endpoint identifier. #322

Add endpoint identifier. #322

sbernard31 commented Jun 15, 2017

boaks commented Jun 15, 2017

boaks commented Jun 15, 2017

sbernard31 commented Jun 15, 2017

boaks commented Jun 15, 2017

sbernard31 commented Jun 16, 2017

boaks commented Jun 16, 2017

sbernard31 commented Jun 16, 2017

boaks commented Jun 16, 2017

sbernard31 commented Jun 16, 2017

boaks commented Jun 16, 2017

sbernard31 commented Jun 16, 2017 •

edited

Loading

boaks commented Jun 19, 2017

boaks Jul 6, 2017

sbernard31 Jul 6, 2017 •

edited

Loading

sbernard31 Jul 6, 2017 •

edited

Loading

boaks Jul 7, 2017

sbernard31 Jul 7, 2017

boaks Jul 7, 2017

sbernard31 Jul 7, 2017 •

edited

Loading

sbernard31 commented Nov 13, 2017

Add endpoint identifier. #322

Add endpoint identifier. #322

Conversation

sbernard31 commented Jun 15, 2017

boaks commented Jun 15, 2017

boaks commented Jun 15, 2017

sbernard31 commented Jun 15, 2017

boaks commented Jun 15, 2017

sbernard31 commented Jun 16, 2017

boaks commented Jun 16, 2017

sbernard31 commented Jun 16, 2017

boaks commented Jun 16, 2017

sbernard31 commented Jun 16, 2017

boaks commented Jun 16, 2017

sbernard31 commented Jun 16, 2017 • edited Loading

boaks commented Jun 19, 2017

boaks Jul 6, 2017

Choose a reason for hiding this comment

sbernard31 Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

sbernard31 Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

boaks Jul 7, 2017

Choose a reason for hiding this comment

sbernard31 Jul 7, 2017

Choose a reason for hiding this comment

boaks Jul 7, 2017

Choose a reason for hiding this comment

sbernard31 Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

sbernard31 commented Nov 13, 2017

sbernard31 commented Jun 16, 2017 •

edited

Loading

sbernard31 Jul 6, 2017 •

edited

Loading

sbernard31 Jul 6, 2017 •

edited

Loading

sbernard31 Jul 7, 2017 •

edited

Loading