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

Invalid datatype for DTLS/TLS Ciphersuite resource (/0/?/16) in BootstrapConfig. #1402

Closed
Tracked by #1401
Warmek opened this issue Feb 23, 2023 · 16 comments
Closed
Tracked by #1401
Labels
bsserver Impact LWM2M bootstrap server bug Dysfunctionnal behavior

Comments

@Warmek
Copy link
Contributor

Warmek commented Feb 23, 2023

Question

In BootstrapConfig.java line 335:

        /**
         * When this resource is present it instructs the TLS/DTLS client to propose the indicated ciphersuite(s) in the
         * ClientHello of the handshake. A ciphersuite is indicated as a 32-bit integer value. The IANA TLS ciphersuite
         * registry is maintained at https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml. As an
         * example, the TLS_PSK_WITH_AES_128_CCM_8 ciphersuite is represented with the following string "0xC0,0xA8". To
         * form an integer value the two values are concatenated. In this example, the value is 0xc0a8 or 49320.
         * <p>
         * Since Security v1.1
         */
        public ULong cipherSuite = null;

In OMA documentation cipherSuite is defined as "Multiple" but from what I can read above it seems that the resource can be "concatenated" to integer value. My question is: Is ULong a correct type for this resource or should it be another type that could contain 2 Strings?

@Warmek Warmek added the question Any question about leshan label Feb 23, 2023
@sbernard31
Copy link
Contributor

I just looked at this and I think you find a bug in BootstrapConfig. 👍

You right that ciphersuite resource (Id:16) from Security Object (id:0) is defined as Multiple and so we should be able to define more than 1 ciphersuite in BootstrapConfig.

<Item ID="16">
    <Name>DTLS/TLS Ciphersuite</Name>
    <Operations></Operations>
    <MultipleInstances>Multiple</MultipleInstances>
    <Mandatory>Optional</Mandatory>
   <Type>Unsigned Integer</Type>
   <RangeEnumeration></RangeEnumeration>
   <Units></Units>
   <Description><![CDATA[When this resource is present it instructs the TLS/DTLS client to propose
the indicated ciphersuite(s) in the ClientHello of the handshake. A ciphersuite is indicated as a 32-bit integer value. 
The IANA TLS ciphersuite registry is maintained at https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml. 
As an example, the TLS_PSK_WITH_AES_128_CCM_8 ciphersuite is represented with the following string "0xC0,0xA8".
To form an integer value the two values are concatenated. In this example, the value is 0xc0a8 or 49320.]]></Description>
</Item>

(Source : https://github.com/OpenMobileAlliance/lwm2m-registry/blob/prod/version_history/0-1_1.xml#L260-L269)

This resource was added in LWM2M v1.1 (so doesn't not affect Leshan v1.x).

My question is: Is ULong a correct type for this resource or should it be another type that could contain 2 Strings?

So first we need to fix the single => multiple mistake and so replace ULong type by something like :

  • ULong[]
  • OR List<ULong>
  • OR something else but this should be ordered.

There is no other Multiple resource in BootstrapConfig except ACL resource (id:2) from Access Control Object (id:2).
For this one, a Map<Integer,Long> was chosen. where key is the resource instance Id and value the resource instance value.
(Let me know if I'm not clear enough)

For ciphersuite resource, I'm not sure we need to let user being able to chose resource instance id.... (that's why I propose ordered collection without key)

ULong, OR String OR Dedicated CipherSuite type ? is another question.

The benefits to use ULong, is that Leshan doesn't need to know all ciphersuite (no need to have a kind of registry class for it)
The drawback is that this is clearly less userfriendly...

Current ULong way, is very very not userfriendly as user need to create the ULong from CipherSuite ID.
I mean it must not provide something like "0xC0,0xA8" but 49320 ...

So I don't know what we should do.

  1. I guess more userfriendly way is to provide something like List<CipherSuite>
    But that means having an extendable registry of cipher suite.

  2. another solution is to keep ULong but provide a kind of utility class with which help to create the Ulong from Ciphersuite Id (e.g. :"0xC0,0xA8").

  3. another solution is to create a type CipherSuiteId you can create from 2 byte.

Or any other idea ?

@sbernard31 sbernard31 added bug Dysfunctionnal behavior bsserver Impact LWM2M bootstrap server and removed question Any question about leshan labels Feb 23, 2023
@Warmek
Copy link
Contributor Author

Warmek commented Feb 23, 2023

I think the second solution is the best one as it would be easy to implement and wouldn't require dedicated classes which seems like an overkill to me

@sbernard31
Copy link
Contributor

sbernard31 commented Feb 23, 2023

Thinking a bit more about this.

Probably 1.) is overkill. (But we can keep it in mind in case one day we found another use case where a CipherSuite class could be needed)

Between 2.) and 3.) the difference is mainly between creating an utility class or create a new "bean" class.
I feel both will lead to add approximately same among of code but I feel this will be maybe easier for user to understand how to create "a CipherSuiteId" than "an ULong from an utility class".

What do you think ? @JaroslawLegierski any opinion about this ?

@sbernard31
Copy link
Contributor

(let us know at #1401 if we need this one for M11)

@JaroslawLegierski
Copy link
Contributor

What do you think ? @JaroslawLegierski any opinion about this ?

It seems to me that solution 2 may be a bit simpler to implement, but solution 3 will be easier for future usage

@sbernard31
Copy link
Contributor

3. should not be so hard, just adding a new class like this :

public class CipherSuiteId {

   private byte firstByte;
   private byte secondByte;
   // OR 
   // private byte[] id, // should have a size of 2

  /**
  * The IANA TLS ciphersuite registry is maintained at https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml. 
  * As an example, the TLS_PSK_WITH_AES_128_CCM_8 ciphersuite is represented with the following string "0xC0,0xA8"
  */
  public CipherSuiteId(byte firstByte, byte secondByte) {
    // set attibutes
  }

  public CipherSuiteId(Ulong valueFromSecurityObject) {
    // extract bytes from ULong : same code as if we used an utility method like 2.
    // set attibutes
  }

  /**
  *  As an example, the TLS_PSK_WITH_AES_128_CCM_8 ciphersuite is represented with the following string "0xC0,0xA8".
  To form an integer value the two values are concatenated. In this example, the value is 0xc0a8 or 49320.
  */
  public ULong getValueForSecurityObject() {
      // create ULong from attributes : same case as if we used an utility method like 2. 
  }
}

Javadoc could probably be better.

@Warmek
Copy link
Contributor Author

Warmek commented Feb 24, 2023

I've created a branch and implemented this change on opl/CipherSuiteId

@sbernard31
Copy link
Contributor

I looked at the branch quickly, there are some minor issues but it will be easier to comment in a PR.

The main problem I see is that you don't use a type that allows multiple ciphersuite 🤔 ... I thought that was the main problem you reported?

@Warmek
Copy link
Contributor Author

Warmek commented Feb 24, 2023

My bad. I've pushed a correction

@sbernard31
Copy link
Contributor

I don't get your last commit 🤔

Why not just using : CipherSuiteId[] or List<CipherSuiteId> ?

And here :

resources.add(LwM2mSingleResource.newULongArray(16, securityConfig.cipherSuite.getULongArray()));

You should not create a single resource of type OPAQUE but several resource instance of type UNSIGNED_INTEGER.
See model at #1402 (comment).

@Warmek
Copy link
Contributor Author

Warmek commented Feb 24, 2023

I wanted to have an method in BootstrapConfig.java to generate ULong[] without any additional imports in BootstrapUtil.java and this was soulution that i thought of. I don't remember why.
Anyways, I've pushed version with List

@sbernard31
Copy link
Contributor

I guess you don't get what I meant by :

You should not create a single resource of type OPAQUE but several resource instance of type UNSIGNED_INTEGER.

The model is Multiple so you must create LwM2mMultipleResource
And the model says it is UNSIGNED_INTEGER, so you must not create OPAQUE resource.

HTH

(any reason to not create a PR ?)

@Warmek
Copy link
Contributor Author

Warmek commented Feb 27, 2023

I've created a PR

@sbernard31
Copy link
Contributor

👍 I will look at it.

@sbernard31
Copy link
Contributor

I found something which sounds strange to me in specification about that. I asked for clarification : OpenMobileAlliance/OMA_LwM2M_for_Developers#560

@sbernard31 sbernard31 changed the title Potential mismatch with OMA specification Invalid datatype for DTLS/TLS Ciphersuite resource (/0/?/16) in BootstrapConfig. Mar 2, 2023
sbernard31 added a commit that referenced this issue Mar 3, 2023
Co-authored-by: Simon Bernard <sbernard@sierrawireless.com>
@sbernard31
Copy link
Contributor

This is fixed by #1408 which is now integrated in master.

@Warmek thx for reporting and working on this 🙏

Warmek added a commit to JaroslawLegierski/leshan that referenced this issue Mar 28, 2023
… BootstrapConfig

Co-authored-by: Simon Bernard <sbernard@sierrawireless.com>
Warmek added a commit to JaroslawLegierski/leshan that referenced this issue Mar 28, 2023
… BootstrapConfig

Co-authored-by: Simon Bernard <sbernard@sierrawireless.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bsserver Impact LWM2M bootstrap server bug Dysfunctionnal behavior
Projects
None yet
3 participants