oscore branch cleaning

[sbernard31]

This PR aims to do a check about all TODO OSCORE + re-review the code to polish the current oscore branch.

(This is part of the plan : #920 (comment))

This is work in progress.

[sbernard31]

@rikard-sics, I will try to push small commit regulary on this PR. This way you can follow/review my work day to day.

[rikard-sics]

@rikard-sics, I will try to push small commit regulary on this PR. This way you can follow/review my work day to day.

Okay, sounds great. Then I will keep track of commits being added here. Let us discuss if there is anything unclear, or about additional things to add in Californium if any more come up.

[sbernard31]

Which aead and hkdf algorithm are supported by current implementation ?
Which ones do we want for the minimal viable feature ?
Only the default one ?

[rikard-sics]

Which aead and hkdf algorithm are supported by current implementation ?
Which ones do we want for the minimal viable feature ?
Only the default one ?

Currently the following AEAD algorithms are supported:

• AES_CCM_16_64_128
• AES_CCM_16_128_128
• AES_CCM_64_64_128
• AES_CCM_64_128_128

And for HKDF:

• HKDF_HMAC_SHA_256
• HKDF_HMAC_SHA_512

I think for a minimal viable feature only supporting the defaults are sufficient. Otherwise going with supporting all that OSCORE supports is also an option, except for consistency checking all the HKDF/AEAD logic should be happening in the OSCORE code.

[sbernard31]

I think for a minimal viable feature only supporting the defaults are sufficient.

I added some temporary check about this.

[sbernard31]

I did most of what I wanted about cleaning.

The last thing I plan to add is to deactivate OSCORE by default and add an option to activate it (as this is still an experimental feature)

Then, I will do the list of possible modification of californium API.

[sbernard31]

I fall on this OSCORE code :
https://github.com/eclipse/leshan/blob/7fff92d8b8a23e9cd088f37bce310468c79711a1/leshan-server-cf/src/main/java/org/eclipse/leshan/server/californium/request/LwM2mResponseBuilder.java#L255-L267

@rikard-sics, I feel this should not be in Leshan code. Could you help me to better understand this ?

[rikard-sics]

@rikard-sics, I feel this should not be in Leshan code. Could you help me to better understand this ?

I need to dig a bit deeper into this to recall the exact problem.

From my memory right now the issue was that the coapRequest object here sometimes lacked the Token (even though it was sent over the wire with a Token), this in turn caused issues with later OSCORE processing. It is not clear to me why this was happening.

One simple thing to test would be to simply remove these lines and see if observations with OSCORE still works. I will try to have a look at this today or tomorrow to understand how things can be fixed (if still an issue) in the OSCORE code in Californium.

[rikard-sics]

I did most of what I wanted about cleaning.

The last thing I plan to add is to deactivate OSCORE by default and add an option to activate it (as this is still an experimental feature)

Then, I will do the list of possible modification of californium API.

Sounds great. Yeah, having it enabled with a flag for now can make sense.

[sbernard31]

I currently testing to update my previous OSCORE demo PRs with our last modification.

And I just saw that current californium API does not like if masterSalt is an empty byte array.
I guess this comes from that codes from : org.eclipse.californium.oscore.OSCoreCtx.OSCoreCtx` line 261

if (master_salt == null) {
	// Default value. Automatically initialized with 0-es.
	this.common_master_salt = new byte[this.kdf.getKeySize() / Byte.SIZE];
} else {
	this.common_master_salt = master_salt.clone();
}

But reading the OSCORE RFC8613 :

Master Salt
* Default is the empty byte string

Is it a kind of bug or is it the expected behavoir ?

[rikard-sics]

Looking into things a bit this is my understanding. The default Master Salt in OSCORE should indeed be the empty byte string. However, later when that is fed into the HKDF, if the salt is zero length, it should be padded:
https://datatracker.ietf.org/doc/html/rfc5869#section-2.2

Inputs:
      salt     optional salt value (a non-secret random value);
               if not provided, it is set to a string of HashLen zeros.

So that is why if the master salt is null that padding occurs. But yeah what seems to be missing is considering if the developer sets the empty byte string as salt in the context constructor. So it would be better if it was:

if (master_salt == null || master_salt.length == 0) {
	// Default value. Automatically initialized with 0-es.
	this.common_master_salt = new byte[this.kdf.getKeySize() / Byte.SIZE];
} else {
	this.common_master_salt = master_salt.clone();
}

(Or waiting to do the padding in the deriveKey method may be even clearer). I will add this to the list of updates to do.

[sbernard31]

About #1226 (comment),

One simple thing to test would be to simply remove these lines and see if observations with OSCORE still works. I will try to have a look at this today or tomorrow to understand how things can be fixed (if still an issue) in the OSCORE code in Californium.

I tested it on my side and it seems observe works with or without this code.
Testing this I saw that passive cancel observation seems to not work with or without this code. (so probably not related)

[rikard-sics]

I tested it on my side and it seems observe works with or without this code.
Testing this I saw that passive cancel observation seems to not work with or without this code. (so probably not related)

Very good. That code may very well have been a workaround for something which is fixed now in later releases.

I will make a note to look into the passive observation cancellations.

[sbernard31]

I think we can integrate this in oscore branch ? Tell me if you think this is not ready.

[rikard-sics]

I think we can integrate this in oscore branch ? Tell me if you think this is not ready.

Yes I believe this can be integrated now.

[sbernard31]

I rebased oscore branch on master then integrated this PR oscore branch in the squashed commit 11d4053