eclipse-leshan / leshan

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

OSCORE Californium API/behavior changes. #1231

Open sbernard31 opened 2 years ago

sbernard31 commented 2 years ago

I will list here all the Californium changes we talked about with @rikard-sics.

  1. probably move classes about OscoreStore in org.eclipse.leshan.core.californium.oscore.cf(or something like this) from Leshan to Californium.
  2. Add a way to specify in EndpointContext the recipient ID targeted. (https://github.com/eclipse/leshan/pull/1212#discussion_r830937966)
  3. Being able to create an CoapEnpoint which allow OSCORE communication only.
  4. Do not send Request if there is an OSCORE option but no context (see https://github.com/eclipse/leshan/pull/1212#issuecomment-1072266510)
  5. (maybe some context lifetime feature ? (https://github.com/eclipse/leshan/issues/1230)
  6. (maybe a fix about :https://github.com/eclipse/leshan/pull/1226#issuecomment-1086024736)

@rikard-sics tell me if I missed something ?

rikard-sics commented 2 years ago

Yes I had many of the same points listed down.

The only possible additions are:

As a note on point 2: It would be good if both the RID and ID Context could be specified (if desired). Even though the ID Context is not used by Leshan right now, it adds flexibility for the future and for other users.

sbernard31 commented 2 years ago

I also forgot to mention this point :

 AlgorithmID.FromCBOR(
     CBORObject.FromObject(
          securityInfo.getOscoreSetting().getAeadAlgorithm().getValue())), 

It could be better to have an API which doesn't need a CBOR object but just a int ?

rikard-sics commented 2 years ago

It could be better to have an API which doesn't need a CBOR object but just a int ?

In the OSCoreCtx constructor? Yeah I think that would be convenient and make sense.

sbernard31 commented 2 years ago

In the OSCoreCtx constructor? Yeah I think that would be convenient and make sense.

Yes or/and adding AlgorithmID.fromValue(int value) ?

rikard-sics commented 2 years ago

Yes or/and adding AlgorithmID.fromValue(int value) ?

Yep, that one also makes sense to add.

sbernard31 commented 2 years ago

(Not really an API changes but maybe a tiny possible improvement) In org.eclipse.californium.oscore.OSCoreCtx.OSCoreCtx() line 319

try {
    this.sender_key = deriveKey(this.common_master_secret, this.common_master_salt, this.key_length, digest,
            info.EncodeToBytes());
} catch (CoseException e) {
-   LOGGER.error(e.getMessage());
-       throw new OSException(e.getMessage());
+   LOGGER.error(e.getMessage(), e); 
+      throw new OSException(e.getMessage(), e);
}

First line, it could make sense to display exception stack trace in the log. Second line, CoseException should be added as cause of OSException

(except all of this is hiding for Security reason ?)

rikard-sics commented 2 years ago

(except all of this is hiding for Security reason ?)

Yes, I did not write that particular code (it was a previous colleague), but I could imagine it was for security reasons.

rikard-sics commented 2 years ago

I think we have a good list of changes to be done in Californium then. I have one PR pending related to the plugtest server/client, after that has been merged I can go through and perform the listed changes.

sbernard31 commented 2 years ago

I add the point about Ctx.setURI raised at : https://github.com/eclipse/leshan/pull/1232#discussion_r841851488

sbernard31 commented 2 years ago

(@rikard-sics just to let you know : I will be unavailable from the end of day to Monday 18th april, back on Tuesday)

rikard-sics commented 2 years ago

(@rikard-sics just to let you know : I will be unavailable from the end of day to Monday 18th april, back on Tuesday)

Okay, thanks for letting me know.

Meanwhile I can spend time on testing the current code (as you mention in #920), and work on implementing the additions to Californium.

sbernard31 commented 2 years ago

@rikard-sics any news about this :slightly_smiling_face: ?

rikard-sics commented 2 years ago

@rikard-sics any news about this slightly_smiling_face ?

I have been focusing a bit on the addition of OSCORE support to the sandbox client/server in Californium, and related discussions on interoperability with libcoap that was triggered by this which took some time to investigate.

However, I have begun working on some of the points listed and am planning to create a PR covering those. Currently I am on a business trip and will be back on Saturday so probably I can get this first PR done by the middle of next week.

When I create the PR you could also review it to check that it suits the intended functionality.

sbernard31 commented 2 years ago

OK I will be often unavailable during April and May. (e.g. I'm back on Tuesday)

Do you think Leshan opened PR about OSCORE should be merge in oscore branch or prefer to wait you test it more ?

I also wonder when we can integrate it in master ? :thinking:

sbernard31 commented 2 years ago

@rikard-sics, I integrated all opened OSCORE PR in oscore branch.

For the integration in master I don't know yet, maybe too soon. :thinking:

Just to let you know about my availability on May :

I will be unavailable :

then I will be avaialble again as usual :slightly_smiling_face:.

sbernard31 commented 2 years ago

@rikard-sics, I try to move forward on Bootstrap config support at server side : #1254

(to do that I rebase oscore branch again on master)

rikard-sics commented 2 years ago

@rikard-sics, I try to move forward on Bootstrap config support at server side : #1254

(to do that I rebase oscore branch again on master)

Sounds good. Sorry for the long silence, unfortunately I have been quite busy with other tasks. I want to get back to working on the modifications to Californium we discussed and finalize a PR for them.

sbernard31 commented 2 years ago

@rikard-sics, let me know if I can help in any way ?

sbernard31 commented 2 years ago

Working on https://github.com/eclipse/leshan/pull/1220#issuecomment-1199063016, I found a blocker issue :x:. For transport layer abstraction, we need to know the endpoint which receive a request. We currently retrieve it using Message.getLocalAddress() :

/**
 * Gets the local address of the receiving endpoint.
 * 
 * @return local address of the receiving endpoint. {@code null} for
 *         outgoing messages.
 * @since 3.0
 */
public InetSocketAddress getLocalAddress() {
    return localAddress;
}

But with OSCORE this returns null, maybe because a request is created under the hood and this field in not set. (maybe here ? : https://github.com/eclipse/californium/blob/main/cf-oscore/src/main/java/org/eclipse/californium/oscore/OptionJuggle.java#L280-L291)