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

Support of SNI #1447

Open
sbernard31 opened this issue May 12, 2023 · 24 comments
Open

Support of SNI #1447

sbernard31 opened this issue May 12, 2023 · 24 comments
Labels
core Impact core of Leshan new feature New feature from LWM2M specification

Comments

@sbernard31
Copy link
Contributor

SNI support is currently not planed at short / mid term but this issue aims to discuss about it.

@sbernard31 sbernard31 added new feature New feature from LWM2M specification core Impact core of Leshan labels May 12, 2023
@sbernard31
Copy link
Contributor Author

sbernard31 commented May 12, 2023

@jakubsobolewskisag, I see you request about it at #1386 (comment)

Do you have a clear idea about how it should looks like ?
Do not hesitate to share any input about it here.

(I will be not too much available in May, so don't worry if I didn't answer this month)

@jakubsobolewskisag
Copy link
Contributor

Hi @sbernard31 !
I tried to enable SNI support on Leshan server using DTLS settings like below:
coapConfig.set(DtlsConfig.DTLS_USE_SERVER_NAME_INDICATION, true);

And it seemed to work fine when device connected in regular (no bootstrap) session. Connection worked, SNI name was correctly provided in "serverNames" variable in CertificateProvider. It looked really, really good!

However, when I tried to bootstrap with SNI, at some point the SNI information was lost. It started OK, "serverNames" variable was OK, cipher suite negotiation went fine, and at the point where bootstrap server was making "WriteBootstrapRequest", there was an exception because of not matching endpoints/contexts.

2023-05-10 17:31:12.547  INFO 1665197 --- [/0.0.0.0:5684#1] o.e.c.scandium.DTLSConnector.drops       : DTLSConnector (DTLS-0.0.0.0:5684) drops 539 bytes outgoing, IP([0:0:0:0:0:0:0:1]:42063) != DTLS([0:0:0:0:0:0:0:1]:42063,ID:F6C770916D)
2023-05-10 17:31:12.549  WARN 1665197 --- [/0.0.0.0:5684#1] c.l.l.s.c.b.CumulocityBootstrapHandler   : Error during bootstrap write of security instance LwM2mObjectInstance [id=1, resources={0=LwM2mSingleResource [id=0, value=coaps://192.168.1.69:5784, type=STRING], 1=LwM2mSingleResource [id=1, value=false, type=BOOLEAN], 2=LwM2mSingleResource [id=2, value=2, type=INTEGER], 3=LwM2mSingleResource [id=3, value=453Bytes, type=OPAQUE], 4=LwM2mSingleResource [id=4, value=453Bytes, type=OPAQUE], 5=LwM2mSingleResource [id=5, value=150Bytes, type=OPAQUE], 6=LwM2mSingleResource [id=6, value=3, type=INTEGER], 10=LwM2mSingleResource [id=10, value=1, type=INTEGER], 11=LwM2mSingleResource [id=11, value=1, type=INTEGER], 12=LwM2mSingleResource [id=12, value=0, type=INTEGER]}] on urn:imei:0001
org.eclipse.leshan.core.request.exception.SendFailedException: Unable to send request coap://[0:0:0:0:0:0:0:1]:42063/0/1
	at org.eclipse.leshan.core.californium.CoapAsyncRequestObserver.onSendError(CoapAsyncRequestObserver.java:167)
	at org.eclipse.californium.core.coap.Message.setSendError(Message.java:1029)
	at org.eclipse.californium.core.coap.Request.setSendError(Request.java:1032)
	at org.eclipse.californium.core.network.CoapEndpoint$SendingCallback.onError(CoapEndpoint.java:1299)
	at org.eclipse.californium.elements.RawData.onError(RawData.java:314)
	at org.eclipse.californium.scandium.DTLSConnector.checkOutboundEndpointContext(DTLSConnector.java:3288)
	at org.eclipse.californium.scandium.DTLSConnector.sendMessage(DTLSConnector.java:3244)
	at org.eclipse.californium.scandium.DTLSConnector.sendMessageWithSession(DTLSConnector.java:3234)
	at org.eclipse.californium.scandium.DTLSConnector.sendMessage(DTLSConnector.java:3075)
	at org.eclipse.californium.scandium.DTLSConnector.access$1600(DTLSConnector.java:257)
	at org.eclipse.californium.scandium.DTLSConnector$18.run(DTLSConnector.java:2971)
	at org.eclipse.californium.elements.util.SerialExecutor$1.run(SerialExecutor.java:293)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
	at java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:264)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java)
	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:304)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: org.eclipse.californium.elements.exception.EndpointMismatchException: DTLS sending
	... 14 common frames omitted 

After some debugging I think the problem is here: CoapRequestBuilder.java line 353

  protected void setSecurityContext(Request coapRequest) {
        EndpointContext context = EndpointContextUtil.extractContext(destination, allowConnectionInitiation); // problem here
        coapRequest.setDestinationContext(context);

        if (destination.isOSCORE()) {
            coapRequest.getOptions().setOscore(Bytes.EMPTY);
        }
    }

When destination context is built, the SNI infornation is not there and Californium/Scandium throws exception when we try to send the message because contexts do not match.

To support SNI on the server side, we must pass it to the code above so it can properly prepare endpoint context. I think this should solve the issue completely because all the other necessary blocks seem to be in place (i.e. it's possible to set SNI on BootstrapConfig.ServerSecurity).

The Leshan Client on the other hand has zero SNI support (from what I've seen in the code) so it would have to be designed. Basically the idea is to:

  1. be able to start the client with SNI parameter so it can use it when establishing connection to server/bootstrap server
  2. save SNI name that's comming from bootstrap server and use it when connecting to server
  3. use SNI when validating X509 certificate - it's expected CN in server's certificate

And that's all I can think of :)

Best regards!

@sbernard31
Copy link
Contributor Author

@jakubsobolewskisag thx for this valuable feedback.

I will let you know if/when we move forward on this.

If you want to help on this, let us know. You could even precise which kind of help you can provide :

  • code review ?
  • test code ?
  • providing code ?

@JaroslawLegierski @cyril2maq is it SNI support something that you will need at some point ?

@jakubsobolewskisag
Copy link
Contributor

@sbernard31 We'd love to help. My team agreed that we should contribute as much as possible, so we'd like to work on the code (and test code) starting with server changes. The only problem is that we can't start right away because of other obligations, but I think we could start at the end of June. If it's OK with you of course :)

@sbernard31
Copy link
Contributor Author

@jakubsobolewskisag that sounds good.

I think we can first focus on bootstrap server issue you report above.

@jakubsobolewskisag
Copy link
Contributor

@sbernard31 I agree - that's a good place to start.

@sbernard31
Copy link
Contributor Author

I was taking a look at LWM2M-v1.1.1@transport about SNI and I understand there are 2 SNI usage described :

  • when multiple virtual servers are using a single underlying network address.
  • when you connect to server using IP literal (e.g. Deployments without DNS )

I'm curious which one of them are you using ? 🙂

@jakubsobolewskisag
Copy link
Contributor

@sbernard31 actually both :)

@sbernard31
Copy link
Contributor Author

Thx.

Just to let you l know, I started to write some code but for now this is just to experiment a bit.

I think we can first focus on bootstrap server issue you report above.

That seemed like a good idea but I haven't any client to be able to test that, so I started to work on client side 😅

Anyway about LWM2M-v1.1.1@core§Table: E.1-2 LwM2M Object: LWM2M Security Resource definitions :

ID Name Operations Instances Mandatory Type Range or Enumeration Units Description
14 SNI   Single Optional String     This resource holds the value of the Server Name Indication (SNI) value to be used during the TLS handshake. When this resource is present then the LwM2M Server URI acts as the address of the service while the SNI value is used for matching a presented certificate, or PSK identity.

I'm not sure to understand the last part about PSK identity ? do you have any idea ?

@jakubsobolewskisag
Copy link
Contributor

Oh wow... I missed that! Until now I was sure that SNI is only used with X.509 authentication. I have to dig deeper to understand what it means for PSK... Thank you!

@ghost
Copy link

ghost commented Jul 5, 2023

Until the OMA says something, I'd assume that the SNI resource doesn't have a meaning for NO_SEC and PSK.

@sbernard31
Copy link
Contributor Author

sbernard31 commented Jul 5, 2023

I guess this could be eventually used as a hint for a server to know which PSK key to use for a given PSK Identity ?
Maybe to remove constraint about having uniqueness of PSK identity, I mean this could allow to have 2 client with same PSK identity as long as they use different virtual host ?

But I find it hard to understand why the specification would refer to server behavior in a client object resource 🤔. So this is probably not what the authors intended to express.

But I agree waiting answer, let's focus on X509 (and maybe RPK too)

@sbernard31
Copy link
Contributor Author

I looked at SNI support a bit deeper and I try to summarize my current understanding :

At LWM2M client side

We need to support SNI resource (id 14) of Security Object (0).
Once a user add a value to this resource (at client start OR via Bootstrap session), it will be used :

  • to send serverName at (D)TLS level in CLIENT_HELLO. This can be used for x509 and rpk for sure. (but also maybe with PSK see comment just above ☝️ )
  • as presented identifier when x509 is used to validate subjectName as described here.

At LWM2M Server and Bootstrap server side

For "usage about client connecting server using IP literal" (e.g. Deployments without DNS ), the server just need to add ServerName extension in SERVER_HELLO

For "multiple virtual servers using a single underlying network address", It's not so clear what is expected at Leshan Server.
I guess if Leshan Server is behind a loadbalancer, this loadbalancer could be able to redirect traffic to right Leshan Server with the given "virtualhost" name. In that case this is like use case just above. Server just need to add ServerName extension in SERVER_HELLO
But I could imagine that virtualhost name could also be used to select different credentials on same instance of Leshan Server. If we want to support this that means that Leshan server can have more than 1 certificate or more than 1 public key. (and maybe a psk store indexed by virtual host name)

@jakubsobolewskisag @sag-eweingaertner, let me know if you understand SNI support in a same way ? and if there is some point described above that you don't really need ? Maybe you could describe more precisely "your multiple virtual servers" use case ? (this will maybe help me to better understand how Leshan should be modified)

@jakubsobolewskisag
Copy link
Contributor

@sbernard31 our main goal is to make X.509 work with SNI. To my understanding, SNI is provided by the client when it establishes connection with the server. It's a hint for the server that client expects specific server name (CN) in X.509 certificate.
Leshan Server already supports having more than 1 certificate because it's supported by

org.eclipse.californium.scandium.dtls.x509.CertificateProvider

method: CertificateIdentityResult requestCertificateIdentity(ConnectionId cid, boolean client, List issuers,
ServerNames serverNames, List certificateKeyAlgorithms,
List signatureAndHashAlgorithms, List curves);

SNI is provided in "serverNames" variable and this part is working fine right now. We can write our own CertificateProvider and based on "serverNames" we can select one of configured certificates.

When I tested this with a client that supports SNI, regular server connection worked like a charm. The only problem I had was when I tried to bootstrap with SNI (see my first comment).

Most probably what we need to do is to fix "endpoint context" and like you said - add ServerName extension in SERVER_HELLO.

@sbernard31
Copy link
Contributor Author

our main goal is to make X.509 work with SNI.

Noted

Leshan Server already supports having more than 1 certificate because it's supported by
org.eclipse.californium.scandium.dtls.x509.CertificateProvider
SNI is provided in "serverNames" variable and this part is working fine right now. We can write our own CertificateProvider and based on "serverNames" we can select one of configured certificates.

Not really a Leshan API, but maybe we don't need Leshan abstraction for that in a first time. 🤔

When I tested this with a client that supports SNI, regular server connection worked like a charm. The only problem I had was when I tried to bootstrap with SNI (see my first comment).

That sounds strange to me 🤔
I modified the client in a crappy way which should be enough to test SNI, so I will try to understand this now.

@sbernard31
Copy link
Contributor Author

Just to let you know that I didn't succeed to reproduce the EndpointMismatchException exception with my client and a bootstrap server using current master (commit 6a05894). I see the ServerName extension at client et bootstrap server side, then this ends with a bootstrap success. I use default CertificateProvider but this should not change anything.

Do you succeed to bootstrap using same Certificate without SNI ?
Let me know if you have more precise information about the error.

On my side, I will try to implement a cleaner minimal version for the client, then write some unit tests, then share this code.

@jakubsobolewskisag
Copy link
Contributor

@sbernard31 actually I was testing with M7 Server version. I will update and test again with the latest main branch. Maybe the problem is somewhere on our side. I will let you know the results.

@sbernard31
Copy link
Contributor Author

I created a new PR about SNI support. See : #1466
If you want you can review / test it. It could be simpler to review it commit by commit .
Note 3 first commits are not really related to SNI, this was more cleaning before starting real work. (So eventually you can skip it)

I was testing with M7 Server version. I will update and test again with the latest main branch. Maybe the problem is somewhere on our side. I will let you know the results.

On my side, I still can't reproduce the problem

@sbernard31
Copy link
Contributor Author

actually I was testing with M7 Server version.

Just in case, you are using M7 in production environment, I let you know that it is affected by some californium security issue. See https://github.com/eclipse-leshan/leshan/security/policy#versions-security-state

So it would be better to upgrade to a safe version, e.g 2.0.0-M10 or 2.0.0-M11 after reading all information about it.

@sbernard31
Copy link
Contributor Author

@jakubsobolewskisag do you find time to look at #1466 ?

@jakubsobolewskisag
Copy link
Contributor

@sbernard31 I looked at the code changes you've made and it looks cool! I just need some time to test them on our environment (unfortunately it's not that easy since we have some custom stuff on top of older Leshan version so upgrading will take some time). I'll let you know as soon as I'll do it. Thank you!

@sbernard31
Copy link
Contributor Author

@jakubsobolewskisag, I integrated 3 first commits of PR #1466 which was not related to SNI in master.

As soon as I get your green light, I will integrate the rest of the PR in master.
(Unless you think the PR is good enough to be integrated now ? 🤔 )

@sbernard31 sbernard31 mentioned this issue Nov 2, 2023
2 tasks
@sbernard31
Copy link
Contributor Author

Hi @jakubsobolewskisag,

just need some time to test them on our environment (unfortunately it's not that easy since we have some custom stuff on top of older Leshan version so upgrading will take some time).

I would like to integrate #1466 in few days/weeks.

If you think you will be able to test it before I can wait a little bit, so let me know.
If you can't, no problem you will still be able to test it after the integration and you could still give us some feedback 🙂

@sbernard31
Copy link
Contributor Author

The PR #1466 is now integrated in master and should be available in 2.0.0-M15.
Like I said even if #1466 is integrated, feedback are still welcome 🙂

About that issue, I don't know if we should keep it open because it still misses some point OR if we can close it ?

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

No branches or pull requests

2 participants