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

Generate OSCORE object and use it for endpoint creation #736

Closed

Conversation

rikard-sics
Copy link
Contributor

@rikard-sics rikard-sics commented Sep 12, 2019

Code to implement the points listed here:
#718 (comment)

  • create the OSCORE object
  • use it in leshan client demo (keeping hardcoded value but create oscore object)
  • modify ServersInfoExtractor and ServerInfo to support oscore.
  • use ServerInfo in CaliforniumEndpointsManager to create endpoint with oscore support.

The OSCORE object is generated and filled in the Leshan client demo. It will then be used to create a ServerInfo object using the ServersInfoExtractor. Next the ServerInfo is used in the CaliforniumEndpointsManager to generate endpoints towards servers that OSCORE should be used with.

See also issue regarding OSCORE Leshan client integration:
#726

Code to implement the points listed here:
eclipse-leshan#718 (comment)

The OSCORE object is generated and filled in the Leshan client demo.
It will then be used to create a ServerInfo object using the
ServersInfoExtractor. Next the ServerInfo is used in the
CaliforniumEndpointsManager to generate endpoints towards servers
that OSCORE should be used with.

See also issue regarding OSCORE client integration:
eclipse-leshan#726

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
@rikard-sics
Copy link
Contributor Author

One issue I had was how exactly to link the Security and Oscore objects together. I now see your suggestion here:
#726 (comment)

So having an instance ID in the Oscore object and then also in the Security object could bind them together?

@rikard-sics
Copy link
Contributor Author

Some outstanding issues:
What is an appropriate identity when using OSCORE. Currently I left the following code.
serverIdentity = Identity.unsecure(serverInfo.getAddress()); //TODO: FIX?

Currently the HashMapCtxDB is used as a singleton:
HashMapCtxDB db = HashMapCtxDB.getInstance(); //TODO: Do not use singleton here but give it to endpoint builder (for Cf-M16)
When we update to Californium 2.0.0-M16 or higher this can be changed. Then the db is instead tied to a specific endpoint.

Lastly I see I forgot to apply the formatter. I can do this before final merge.

<groupId>org.eclipse.californium</groupId>
<artifactId>cf-oscore</artifactId>
<version>${californium.version}</version>
</dependency>
Copy link
Contributor

@sbernard31 sbernard31 Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leshan-client-code must not depend to californium. (see https://github.com/eclipse/leshan/wiki/Modules#leshan-client-core)

That means that you can not really used AlgorithmID in this module... :/
In a first time manipulating just the value should be OK and you will see if we find a better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for the link. I will amend the code so leshan-client-core does not have any dependencies from Californium. I can probably keep it as an int instead of AlgorithmID.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should now be taken care of in commit 8fc7bfc
I needed the hex2ByteArray method from StringUtil in Californium. For now I extracted and took the code from there. If that is not fine I can re-implement it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you could find what you need in org.eclipse.leshan.util.Hex ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you could find what you need in org.eclipse.leshan.util.Hex ?

Yes, I will check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update to use a method from the Leshan Hex class rather than the imported method from Californium in commit ebc728a

Copy link
Contributor

@sbernard31 sbernard31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So having an instance ID in the Oscore object and then also in the Security object could bind them together?

Yep, see my comments below.

What is an appropriate identity when using OSCORE.

Good question !

  • in PSK this is the pskIdentity (login)
  • in RPK this is the public key
  • in X509 we use the CN of the certificate (which is debatable)

I suppose we should add a new attribute for OSCORE which could be used in addition of existing DTLS identity (in case of OSCORE over coaps) or without it (in case of OSCORE over coap).

In oscore what could be the value which could be used as an identifier ? I need to read the rfc again (intuitively I suppose that Sender ID or/and Recipient ID could be candidate?)

then the db is instead tied to a specific endpoint.

👍

I see I forgot to apply the formatter. I can do this before final merge.

ok

} else if (useOSCore) {
initializer.setInstancesForObject(SECURITY, secOSCore(serverURI, 123));
initializer.setInstancesForObject(OSCORE, Oscore.set("11223344", "AA", "BB")); //Hardcoded values
initializer.setInstancesForObject(SERVER, new Server(123, 30, BindingMode.U, false));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding your remarks here :

One issue I had was how exactly to link the Security and Oscore objects together. I now see your suggestion here: #726 (comment)

To be able to do that, you will probably need to add a new XML version of security object.

You should get it here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The security object xml has been updated and a link to the OSCORE object introduced in commit ecebdfa

*******************************************************************************/
package org.eclipse.leshan;

/**
* The different DTLS security modes
*/
public enum SecurityMode {
PSK(0), RPK(1), X509(2), NO_SEC(3);
PSK(0), RPK(1), X509(2), NO_SEC(3), OSCORE(4);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the specification, 4 is for "Certificate mode with EST" and there is nothing for OSCORE 🤔 .

(It is not excluded there is issue in the specification but for now, I don't understand all of this enough to say if this is an issue 😁)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now removed the SecurityMode for OSCORE. Instead the OSCORE Security Mode link in the Security object will be used. Done in commit 3a4e777

@@ -112,6 +122,16 @@ public static ServersInfo getInfo(Map<Integer, LwM2mObjectEnabler> objectEnabler
info.clientCertificate = getClientCertificate(security);
info.serverCertificate = getServerCertificate(security);
info.privateKey = getPrivateKey(security);
} else if (info.secureMode == SecurityMode.OSCORE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as there is no OSCORE securityMode maybe we can just look if the resource OSCORE Security Mode(17) is set.

If this is set you get the right index for the FIXME below :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am no longer checking the security mode here to decide what to do but rather if the link from the Security object has been set. I am also using the link from the Security object to find the OSCORE object here. Done in commit 3e7db26

int defaultHmacAlgorithm = -10; //HKDF_HMAC_SHA_256

return new Oscore(masterSecret, senderId, recipientId, defaultAeadAlgorithm, defaultHmacAlgorithm, "");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a benefits of both static "constructor" above ?

Copy link
Contributor Author

@rikard-sics rikard-sics Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that the Security object has many static constructors for different combinations of security settings. I also wanted a constructor with good default values for the two algorithm parameters.

Do you mean that I should reduce it to only 1 static constructor or not have any at all, just the normal constructor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create several constructors with different number of parameter.

(static constructor is a good way to name a constructor but the con is that you can not reused when you extend the class)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create several constructors with different number of parameter.

(static constructor is a good way to name a constructor but the con is that you can not reused when you extend the class)

Okay, so it would be better to have 1 or several normal constructors, but no static constructors for the Oscore class?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, as it seems you don't really need to "name" your constructor, I think it's better to have several "normal" constructors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the static "constructors" in commit dc37bd9

/**
* Returns a new security instance (OSCORE) for a device management server.
*/
public static Security secOSCore(String serverUri, int shortServerId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here this would be more something like :

  • oscore (or oscoreOnly)

and one day we will add something like this ?

  • pskWithOscore
  • rpkWithOscore
  • x509WithOscore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense. I can rename the current method to oscore/oscoreOnly. And then we proceed with adding methods later that combine DTLS+OSCORE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed the current method to oscoreOnly in commit dd58e43 . In the future further methods can be added.

@@ -28,6 +30,8 @@
public interface EndpointFactory {

CoapEndpoint createUnsecuredEndpoint(InetSocketAddress address, NetworkConfig coapConfig, ObservationStore store);

CoapEndpoint createOSCoreEndpoint(InetSocketAddress address, NetworkConfig coapConfig, ObservationStore store, HashMapCtxDB db);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for now my feeling is that there no need to add createOSCoreEndpoint maybe just add the db parameter to both other methods (but as the californium API is not yet finished, I maybe missed something)

Copy link
Contributor Author

@rikard-sics rikard-sics Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I removed this new method and added the db parameter to the others instead. Done in commit 495d501 (After updating to Californium 2.0.0-M16 this parameter can be used.)

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
@rikard-sics
Copy link
Contributor Author

I suppose we should add a new attribute for OSCORE which could be used in addition of existing DTLS identity (in case of OSCORE over coaps) or without it (in case of OSCORE over coap).

Yes, that makes sense.

In oscore what could be the value which could be used as an identifier ? I need to read the rfc again (intuitively I suppose that Sender ID or/and Recipient ID could be candidate?)

A combination of the sender ID, recipient ID and ID context should identify a pair of endpoints communicating using OSCORE.

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
initializer.setInstancesForObject(SECURITY, oscoreOnly(serverURI, 123));
initializer.setInstancesForObject(OSCORE, new Oscore("11223344", "AA", "BB")); //Hardcoded values
Oscore oscoreObject = new Oscore("11223344", "AA", "BB"); //Hardcoded values
oscoreObject.setId(12345);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want you can create an Oscore constructor which take an instanceID in parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will do that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now done in commit ae73ab2

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
@sbernard31
Copy link
Contributor

You should have a look at §5.5.5 - Endpoint Client Name about you "identity" concern.

About that is sender ID / receiver ID available in californium endpoint context ?

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
public Oscore(String masterSecret, String senderId, String recipientId, int aeadAlgorithm, int hkdfAlgorithm, String masterSalt) {
this.masterSecret = masterSecret;
public Oscore(int instanceId, String masterSecret, String senderId, String recipientId, int aeadAlgorithm, int hkdfAlgorithm, String masterSalt) {
this.setId(instanceId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clearly not a big deal but you can use super(id)

Copy link
Contributor Author

@rikard-sics rikard-sics Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I will change that as part of the next commit. Done in commit 6617c21

boolean useOscore = false;
if(oscoreObjectId == OSCORE) {
useOscore = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boolean useOscore = oscoreObjectId == OSCORE ?

Copy link
Contributor Author

@rikard-sics rikard-sics Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks more compact and nicer. I will change it. Done in commit 6617c21

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
if(useOscore) {
// search corresponding oscore object
LwM2mObjectInstance oscoreInstance = null;
for (LwM2mObjectInstance oscore : oscores.getInstances().values()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not need to iterate, there is a getter by Id => oscores.getInstance(id)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is much nicer. Fixed in commit a255bd6

@rikard-sics
Copy link
Contributor Author

About that is sender ID / receiver ID available in californium endpoint context ?

Yes it is available but only from Californium 2.0.0-M17. Since the context DB is a singleton in the Californium we are using now the values could also be gotten from there (for now).

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
@rikard-sics
Copy link
Contributor Author

rikard-sics commented Sep 18, 2019

You should have a look at §5.5.5 - Endpoint Client Name about you "identity" concern.

I now updated the code so that the Sender ID of the client is equal to its name (endpoint). So if I start a client with name "c1" its sender ID will be 0x6331, and on the server side in the web interface the sender ID must be entered as "6331" (hexadecimal). Done in commit 401db2b

Of course this restricts the configuration options for the administrator somewhat since the sender ID and client name cannot be chosen independently.

@rikard-sics
Copy link
Contributor Author

For further work on the identity I would perhaps wait until after updating to Californium 2.0.0-M17. There the sender ID, recipient ID and ID context are conveniently available in the endpoint context.

System.out.println("Adding OSCORE CTX " + serverInfo.getFullUri().toASCIIString());
// oscore only mode
if(serverInfo.useOscore) {
System.out.println("Adding OSCORE CTX " + serverInfo.getFullUri().toASCIIString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use LOG instead of sysout ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was an old debug statement I should have fixed. Now done in commit a83918e

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
@rikard-sics
Copy link
Contributor Author

rikard-sics commented Sep 18, 2019

I had to do one more commit regarding the registration, Identity and SecurityChecker. Previously I could not register an OSCORE client using the name I configured it with in the web interface of the server. Since it counted as not secure but yet had a non-null securityInfo it wanted me to use DTLS. I fixed this in commit 40213f6

The SecurityChecker should be expanded with checks to the identity when using OSCORE. Also the Identity class expanded to support OSCORE-related identities. SecurityInfo already has a useOSCore flag.

@@ -65,7 +66,7 @@ public URI getFullUri() {
}

public boolean isSecure() {
return secureMode != SecurityMode.NO_SEC;
return secureMode != SecurityMode.NO_SEC || useOscore == true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a bit strange but here I will not really change this.

Historically this was used to know if we should use unsecured(coap) endpoint or secured(coaps) endpoint.

So maybe the right name is "isCoaps" ?
But anyway I read the code again and maybe this does not make so much sense now, so it would be more wise to not touch it for now.

Copy link
Contributor Author

@rikard-sics rikard-sics Sep 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fair enough, I do not need to change the functionality of isSecure. I see your point that it is more about coap vs. coaps (and OSCORE could apply to both cases). I undid this in commit 17098a7

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
@sbernard31
Copy link
Contributor

About CaliforniumEndpointsManager, maybe I missed something but the idea is to begin by supporting Oscore without DTLS right ?

So I think the oscore stuff is not at "The Good Place".
this is the bad place!

For further work on the identity I would perhaps wait until after updating to Californium 2.0.0-M17. There the sender ID, recipient ID and ID context are conveniently available in the endpoint context.

makes sense !

@rikard-sics
Copy link
Contributor Author

About CaliforniumEndpointsManager, maybe I missed something but the idea is to begin by supporting Oscore without DTLS right ?

Yes, my intention was to start by supporting OSCORE without DTLS. So the good place would be outside of the if(serverInfo.isSecure()), like down by row 220?

@sbernard31
Copy link
Contributor

So the good place would be outside of the if(serverInfo.isSecure()), like down by row 220?

yep :)

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
@rikard-sics
Copy link
Contributor Author

yep :)

Okay, sounds good :)

I have now done this in commit 7116077

Perhaps at some later stage it could be extracted to a separate method since we may want to combine the OSCORE configuration of an endpoint with different DTLS modes in a convenient way and minimize code reuse.

@sbernard31
Copy link
Contributor

Perhaps at some later stage it could be extracted to a separate method since we may want to combine the OSCORE configuration of an endpoint with different DTLS modes in a convenient way and minimize code reuse.

Correct, we will see when we will add this.

I think we are good, do you plan to add more stuff in this PR ? (maybe just fix formatting issue?)

@rikard-sics
Copy link
Contributor Author

I think we are good, do you plan to add more stuff in this PR ? (maybe just fix formatting issue?)

Yes, fixing the formatting was the last thing I had planned. I will proceed with that.

Signed-off-by: Rikard Höglund <rikard.hoglund@ri.se>
@rikard-sics
Copy link
Contributor Author

I think we are good, do you plan to add more stuff in this PR ? (maybe just fix formatting issue?)

I have now applied the Leshan Eclipse formatter to the parts of the code I have changed. Done in commit 0138768

@sbernard31
Copy link
Contributor

I squashed all your commits in one (e310127) which is now integrated in oscore branch.

I tried a "mvn clean install" and that failed so I fixed issues, see commit cc6136a
Do not hesitate to launch this often, tests is here to help you ;)

About formatting I find several more issues, see commit e2ae4bc.

Are you using eclipse ? Did you follow this guide. In theory "format on save" is activated per project, so it should be very difficult to write bad formatted code 🤔.

What do you plan as next steps ? :)

@sbernard31 sbernard31 closed this Sep 19, 2019
@rikard-sics
Copy link
Contributor Author

rikard-sics commented Sep 19, 2019

I squashed all your commits in one (e310127) which is now integrated in oscore branch.

Thanks!

I tried a "mvn clean install" and that failed so I fixed issues, see commit cc6136a
Do not hesitate to launch this often, tests is here to help you ;)

Yes, I was using Eclipse and not running the maven tests from the command line like that. I should have ran tests more frequently and will try to do so in the future.

About formatting I find several more issues, see commit e2ae4bc.

Are you using eclipse ? Did you follow this guide. In theory "format on save" is activated per project, so it should be very difficult to write bad formatted code .

Yes, I am using Eclipse. Unfortunately up until now I have not been using the "format on save" feature. But rather formatting manually with CTRL+SHIFT+F. I will start using "format on save" as you suggest.

What do you plan as next steps ? :)

Some thoughts for next steps:

  • Update code to use Californium 2.0.0-M17
  • Implement an Identity and related things in SecurityChecker for OSCORE
  • Work on the server side issues according to discussion in OSCORE over coap at server side #727
  • Add possibility of entering OSCORE context information as command line parameters for client

What do you think makes most sense to prioritize?

@sbernard31
Copy link
Contributor

This order makes sense to me :)

@rikard-sics
Copy link
Contributor Author

This order makes sense to me :)

Okay, sounds good. I will proceed accordingly. :)

@sbernard31 sbernard31 added this to the 2.0.0 milestone Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants