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

trustedCertificates from LeshanServerBuilder doesn't allow subtypes of Certificate #418

Closed
AlexITC opened this issue Nov 2, 2017 · 11 comments
Labels
new feature New feature from LWM2M specification server Impact LWM2M server

Comments

@AlexITC
Copy link
Contributor

AlexITC commented Nov 2, 2017

Problem:

The field LeshanServerBuilder#trustedCertificates doesn't allow to specify subtypes of Certificate, I need to cast my types just to comply with value type.

Solution

Modify the field to type to allow subtypes of Certificate, one way could be to use List<? extends Certificate> trustedCertificates.

We could keep backwards compatibility, in case this is viable, I can upload a pull request with the changes.

@sbernard31
Copy link
Contributor

it sounds a good idea.

I suppose if we change this for trustedCertificates, we also need to change it for certificateChain

I look at the underlying DTLS library (scandium) and it seems that only X509Certificate (and subclasses) was supported.

Maybe we should change List<? extends X509Certificate> trustedCertificates. to be consistent ?

@AlexITC
Copy link
Contributor Author

AlexITC commented Nov 3, 2017

Sounds perfect, I'll upload a pull request later.

@AlexITC
Copy link
Contributor Author

AlexITC commented Nov 4, 2017

I didn't change the type of trustedCertificates to X509Certificate to not introduce breaking API changes.

@sbernard31
Copy link
Contributor

sbernard31 commented Nov 6, 2017

API break is not really an issue, if we really think the new one is better.

You previously talk about using List instead of Array. Is there any reason to finally use Array ?

I don't know what could be the best choice,

  • java keystore returns Arrays,
  • but often you want to manipulate it, like in demo and so you go with List.

@AlexITC
Copy link
Contributor Author

AlexITC commented Nov 6, 2017

The reason was to not break API compatibility, do you think it would be better to use the list?

I would be happy to switch it.

@sophokles73
Copy link
Contributor

Is there a reason why you would want to change the trust anchor (the argument for using a List instead of an array) once it has been established?
If at all, we should change it to a Set, I guess, since we do not need duplicates in it ...

@sbernard31
Copy link
Contributor

what do you mean by "once it has been established" ?

@sophokles73
Copy link
Contributor

Well, the trust anchor is explicitly configured by a user of the library, i.e. the user establishes the trust anchor by setting the trustedCertificates property. After that, why should it be changed? That would actually be a problem from a security standpoint, wouldn't it?

@sbernard31
Copy link
Contributor

Ok I see.
Don't worry, if we change array to collection, it will just be about builder API not about internal structure. By the way the internal DtlsConnectorConfig.builder make a copy of this so now way to modify it afterwards. (it internally creates an arrayList from the array then recreate an array :))

Often in leshan we use 2 kinds of signature (collection+vargs) for convenience :
(e.g. in LwM2mObject)

 public LwM2mObject(int id, Collection<LwM2mObjectInstance> instances) {
 }
 public LwM2mObject(int id, LwM2mObjectInstance... instances) {
 }

I don't know if is it worth it ?

@sbernard31
Copy link
Contributor

It seems there is no strong reason/opinion to go to collection for now.
So let's keep on array, if this little modification helps scala users. (even if this is a bit strange for java users as the 2 signatures seems to do the same thing in java)

This is not set in stone and could be changed later if needed.
So I will merge #419 soon if there is no objection.

@sbernard31 sbernard31 added new feature New feature from LWM2M specification server Impact LWM2M server labels Nov 8, 2017
sbernard31 pushed a commit that referenced this issue Nov 9, 2017
…anServerBuilder

Signed-off-by: Alexis Hernandez <alexis22229@gmail.com>
@sbernard31
Copy link
Contributor

Fixed by #419

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature from LWM2M specification server Impact LWM2M server
Projects
None yet
Development

No branches or pull requests

3 participants