Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSCORE Californium API/behavior changes. #1231

Open
sbernard31 opened this issue Apr 1, 2022 · 19 comments
Open

OSCORE Californium API/behavior changes. #1231

sbernard31 opened this issue Apr 1, 2022 · 19 comments
Labels
discussion Discussion about anything

Comments

@sbernard31
Copy link
Contributor

sbernard31 commented Apr 1, 2022

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. (Some ideas about OSCORE Store #1212 (comment))
  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 Some ideas about OSCORE Store #1212 (comment))
  5. (maybe some context lifetime feature ? (OSCORE Context lifetime. #1230)
  6. (maybe a fix about :oscore branch cleaning  #1226 (comment))

@rikard-sics tell me if I missed something ?

@sbernard31 sbernard31 added the discussion Discussion about anything label Apr 1, 2022
@sbernard31 sbernard31 changed the title OSCORE Californium API changes. OSCORE Californium API/behavior changes. Apr 1, 2022
@rikard-sics
Copy link
Contributor

rikard-sics commented Apr 4, 2022

Yes I had many of the same points listed down.

The only possible additions are:

  • Make sure the removeContext method removes the context from the URI map
  • Simplify OSCORE context constructor to remove the client boolean (not strictly needed)
  • Add method to remove contexts by RID (related to OSCORE Context lifetime. #1230)
    • This can already be done by first retrieving and then deleting the context (but a direct method may be better)

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
Copy link
Contributor Author

sbernard31 commented Apr 4, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

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

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

@rikard-sics
Copy link
Contributor

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

Yep, that one also makes sense to add.

@sbernard31
Copy link
Contributor Author

sbernard31 commented Apr 5, 2022

(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
Copy link
Contributor

(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
Copy link
Contributor

rikard-sics commented Apr 7, 2022

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
Copy link
Contributor Author

sbernard31 commented Apr 8, 2022

I add the point about Ctx.setURI raised at : #1232 (comment)

@sbernard31
Copy link
Contributor Author

(@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
Copy link
Contributor

(@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
Copy link
Contributor Author

@rikard-sics any news about this 🙂 ?

@rikard-sics
Copy link
Contributor

@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
Copy link
Contributor Author

sbernard31 commented Apr 21, 2022

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 ? 🤔

@sbernard31
Copy link
Contributor Author

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

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

Just to let you know about my availability on May :

I will be unavailable :

  • from April 29th to May 8th (included)
  • from May 13th to 22th
  • and 26/27th

then I will be avaialble again as usual 🙂.

@sbernard31
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor Author

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

@sbernard31
Copy link
Contributor Author

sbernard31 commented Jul 29, 2022

Working on #1220 (comment), I found a blocker issue ❌.
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion about anything
Projects
None yet
Development

No branches or pull requests

2 participants