TLS support in KUKSA.val Client #580

erikbosch · 2023-06-16T14:21:57Z

This intends to provide a working solution for TLS usage with KUKSA.val Client.

Functional changes:

Adding arguments so you can specify what certificates to use
Trying to solve name verification (that host name and name in certificate must match). Sometimes is a problem when using numeric IP, by that reason there is a work-around parameter to specify name to expect

Tests have so far been performed towards both Server and Databroker, and also with DBC Feeder.

There should hopefully be no changes introducing incompatibility, but there are things we need to agree on later:

Shall Databroker start to use TLS by default?
In which applications shall we use "example certs" by default? Could be OK for clients, but for server/databroker I would prefer that you need to make an active decision (command line argument, change config, env-var) to use the default certs. I.e. default behavior for Broker/Server shall be that you get an error "No CA file provided"

lukasmittag

What I did:

ran against databroker (cargo run .. and docker confirmed)
running against kuksa-val-server (docker run ..) throws error provided in comments

lukasmittag · 2023-06-19T12:15:21Z

doc/tls.md

+To use a secure connection specify `--tls-cert`and `--tls-private-key`
+
+```
+~/kuksa.val/kuksa_databroker$ cargo run --bin databroker -- --metadata ../data/vss-core/vss_release_4.0.json --tls-cert ../kuksa_certificates/Server.pem --tls-private-key ../kuksa_certificates/Server.key


I think we should use here the container solution instead as it is the preferred one.

If so I think it makes sense to describe both alternatives, that is quite handy for testing.
And is the container solution the preferred one? If so why?

I'd say it is the preferred one because it's easier for testing. And we mention the docker things in our quickstart. Butmentioning both wouldn't hurt.

It depends on what type of testing you intend to do. If you intend to test with a released version it is true. If you intend to test with your own changes or with your own certificates it is somewhat more complicated. But it would indeed be good to show how to do it.

I sort of agree that "cargo run" is more a development kind of view (also important), but there should be an answer for people "just downloaded/pulled a release", which also would act as guidance how e.g. projects like Velocitas would do it, when they provision everthing as containers

I think this is part of a bigger discussion on what different deployment alternatives we support out of the box. In the future we might for instance release Databroker as a Debian/Apertis package as well and then we might need to have yet another alternative documented. I added a Docker example below.

lukasmittag · 2023-06-19T12:15:32Z

doc/tls.md

+to have "Server" as subjectAltName.
+
+```
+~/kuksa.val/kuksa_databroker$ cargo run --bin databroker-cli --  --ca-cert ../kuksa_certificates/CA.pem


lukasmittag · 2023-06-19T12:15:44Z

doc/tls.md

+Uses TLS by default, but doe not support mutual TLS. By default it uses KUKSA.val example certificates/keys `Server.key`, `Server.pem` and `CA.pem`.
+
+```
+~/kuksa.val/kuksa-val-server/build/src$ ./kuksa-val-server  --vss ./vss_release_4.0.json


lukasmittag · 2023-06-19T12:15:53Z

doc/tls.md

+It is posible to specify a different certificate path, but the file names must be the same as listed above.
+
+```
+~/kuksa.val/kuksa-val-server/build/src$ ./kuksa-val-server  --vss ./vss_release_4.0.json -cert-path ../../../kuksa_certificates


and here again

Yes, I get the point

lukasmittag · 2023-06-19T12:17:29Z

kuksa-client/README.md

+~/kuksa.val/kuksa-client$ kuksa-client --ip localhost --port 55555 --protocol grpc --certificate ../kuksa_certificates/Client.pem --keyfile ../kuksa_certificates/Client.key --cacertificate ./kuksa_certificates/CA.pem
+```
+
+As do this, as mutual authentication is not supported in KUKSA.val Databroker:


this sentence is not clear to me. Maybe rephrase it.

lukasmittag · 2023-06-19T12:51:52Z

kuksa-client/README.md

+
+```
+kuksa-client --ip 127.0.0.1 --port 8090 --protocol ws --cacertificate ./kuksa_certificates/CA.pem
+```


This returns: Disconnected!! [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: IP address mismatch, certificate is not valid for '127.0.0.1'. (_ssl.c:1129)
ran kuksa-val-server with docker run -it --rm -v $HOME/kuksaval.config:/config -p 127.0.0.1:8090:8090 -e LOG_LEVEL=ALL ghcr.io/eclipse/kuksa.val/kuksa-val:master

What is in your config at $HOME/kuksaval.config? Do you use the new Server.pem certificate?

If not the error is expected as the old certificate only use "Server" as name. Try to use the --tls-server-name Server option and see if it helps. That is a "work-around" if your Server.pem does not include the name you are connecting to (in this case 127.0.0.1) or the TLS client by other reasons cannot handle name validation of IP-addresses

I tested by building the kuksa-val-server container locally and running it as

docker run -it --rm -p 127.0.0.1:8090:8090 -e LOG_LEVEL=ALL kuksa-val:latest

and connecting to it with both 127.0.0.1 and localhost with kuksa-client and both connections worked.

(kuksa-client) erik@debian3:~/kuksa.val/kuksa-client$ kuksa-client --ip localhost --port 8090 --protocol ws --cacertificate ./kuksa_certificates/CA.pem Welcome to Kuksa Client version 0.4.0a2.post36+git.556a8bbc.dirty

(kuksa-client) erik@debian3:~/kuksa.val/kuksa-client$ kuksa-client --ip 127.0.0.1 --port 8090 --protocol ws --cacertificate ./kuksa_certificates/CA.pem Welcome to Kuksa Client version 0.4.0a2.post36+git.556a8bbc.dirty

Tested it too with --tls-server-name Server it works fine.

argerus · 2023-06-19T21:19:46Z

kuksa_databroker/databroker-cli/src/main.rs

+            // With new tonic we do not need the hack of setting Server as domain name,
+            // it can resolve both localhost and 127.0.0.1 as subjectAltName
+            //.domain_name("Server");


While informative for this PR, the comment should probably not be kept in the code going forward (i.e. should not be here).

Agree, as soon as you tonic fix has been merged I will remove it. For now your tonic-fix is cherry-picked into this branch.

I can just close that PR as it would be just as good to include that it in this one (as it's TLS related anyway).

Closed the PR. Also, when you remove this comment, run cargo fmt as the build is failing because of faulty formatting (at these lines).

SebastianSchildt · 2023-06-21T15:50:17Z

Shall Databroker start to use TLS by default?
My feeling is yes, but maybe "not yet", but also depends a little bit: At this point, how much more complicated would quickstart get?

In which applications shall we use "example certs" by default? Could be OK for clients, but for server/databroker I would prefer that you need to make an active decision (command line argument, change config, env-var) to use the default certs. I.e. default behavior for Broker/Server shall be that you get an error "No CA file provided"

That sounds like a good idea, so you would not just "enable TLS" and run into danger of (unintendedly) running with "factory certs". In the long run/making TLS mandatory question would be, how easy it can be get up and running (thinking quickstart again)

erikbosch · 2023-06-22T06:32:58Z

Shall Databroker start to use TLS by default?
My feeling is yes, but maybe "not yet", but also depends a little bit: At this point, how much more complicated would quickstart get?

In which applications shall we use "example certs" by default? Could be OK for clients, but for server/databroker I would prefer that you need to make an active decision (command line argument, change config, env-var) to use the default certs. I.e. default behavior for Broker/Server shall be that you get an error "No CA file provided"

That sounds like a good idea, so you would not just "enable TLS" and run into danger of (unintendedly) running with "factory certs". In the long run/making TLS mandatory question would be, how easy it can be get up and running (thinking quickstart again)

Taking Databroker as an example, in the future I would like to support only two alternatives to start it:

databroker --tls-cert /my/path/Server.pem --tls-private-key /my/path/Server.key
databroker --insecure

I.e. not having TLS mandatory, but if you want to run without TLS you must state it explicitly. The quickstart does not need to be that much more complicated, just add --insecure, which I better should do as part of this PR anyway.

erikbosch · 2023-06-26T10:17:23Z

@lukasmittag - please review when you are back from vacation

lukasmittag

What I've done again:

Started kuksa-client and Databroker with tls set some datapoints and it worked
Read through the documentation

Comments can be ignored just some typos

lukasmittag · 2023-06-27T07:46:30Z

doc/quickstart.md

@@ -2,11 +2,13 @@

 The quickest possible way to get KUKSA.val up and running

+*Note: The examples in this document does not use TLS or access control.*


typo the examples and does

lukasmittag · 2023-06-27T08:30:55Z

doc/tls.md

+## KUKSA.val Client (library)
+
+Clients like [KUKSA.val CAN Feeder](https://github.com/eclipse/kuksa.val.feeders/tree/main/dbc2val)
+tht use KUKSA.val Client library must typically set the path to the root CA certificate.


lukasmittag · 2023-06-27T08:36:40Z

kuksa-client/README.md

 ```

+## Using TLS


structure wise comment: Using TLS and then Connecting to KUKSA.val Server as sub headline without having a Connecting to KUKSA.val Databroker is not the best way. Only my opinion and nothing 'big'

Good point, will fix it as soon as I see that all builds pass

Update tonic Make --insecure (or tls config) mandatory for databroker Remove server hack

erikbosch mentioned this pull request Jun 16, 2023

DBC, GPS, DDS, CSV Feeder TLS support eclipse-kuksa/kuksa.val.feeders#111

Merged

erikbosch requested a review from argerus June 16, 2023 14:27

erikbosch force-pushed the erik_tls branch from 1128b57 to 4001c2a Compare June 19, 2023 07:06

lukasmittag suggested changes Jun 19, 2023

View reviewed changes

argerus reviewed Jun 19, 2023

View reviewed changes

argerus mentioned this pull request Jun 20, 2023

Update tonic #579

Closed

erikbosch force-pushed the erik_tls branch 2 times, most recently from 49c1889 to 7acf03f Compare June 21, 2023 06:53

erikbosch requested a review from lukasmittag June 21, 2023 07:01

erikbosch force-pushed the erik_tls branch 2 times, most recently from 1ffc530 to e8cedce Compare June 21, 2023 14:45

erikbosch force-pushed the erik_tls branch from e8cedce to 38c460f Compare June 22, 2023 07:03

erikbosch changed the title ~~TLS support in KUKA.val Client~~ TLS support in KUKSA.val Client Jun 22, 2023

erikbosch force-pushed the erik_tls branch from 38c460f to bff4511 Compare June 26, 2023 10:16

erikbosch force-pushed the erik_tls branch from 20539d9 to fcafba5 Compare June 27, 2023 07:47

lukasmittag approved these changes Jun 27, 2023

View reviewed changes

erikbosch force-pushed the erik_tls branch from fcafba5 to 03ba28d Compare June 27, 2023 12:03

TLS support in KUKSA.val Client

ec54788

Update tonic Make --insecure (or tls config) mandatory for databroker Remove server hack

erikbosch force-pushed the erik_tls branch from 03ba28d to ec54788 Compare June 27, 2023 12:08

lukasmittag merged commit 9ab495b into eclipse:master Jun 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS support in KUKSA.val Client #580

TLS support in KUKSA.val Client #580

erikbosch commented Jun 16, 2023 •

edited

Loading

lukasmittag left a comment

lukasmittag Jun 19, 2023

erikbosch Jun 20, 2023 •

edited

Loading

lukasmittag Jun 20, 2023

erikbosch Jun 20, 2023

SebastianSchildt Jun 21, 2023

erikbosch Jun 22, 2023

lukasmittag Jun 19, 2023

lukasmittag Jun 19, 2023

lukasmittag Jun 19, 2023

erikbosch Jun 20, 2023

lukasmittag Jun 19, 2023

lukasmittag Jun 19, 2023

erikbosch Jun 20, 2023

erikbosch Jun 20, 2023 •

edited

Loading

lukasmittag Jun 20, 2023

argerus Jun 19, 2023

erikbosch Jun 20, 2023

argerus Jun 20, 2023

erikbosch Jun 20, 2023

argerus Jun 20, 2023

SebastianSchildt commented Jun 21, 2023

erikbosch commented Jun 22, 2023

erikbosch commented Jun 26, 2023

lukasmittag left a comment

lukasmittag Jun 27, 2023

lukasmittag Jun 27, 2023

lukasmittag Jun 27, 2023

erikbosch Jun 27, 2023

		@@ -2,11 +2,13 @@

		The quickest possible way to get KUKSA.val up and running

		Note: The examples in this document does not use TLS or access control.

TLS support in KUKSA.val Client #580

TLS support in KUKSA.val Client #580

Conversation

erikbosch commented Jun 16, 2023 • edited Loading

lukasmittag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikbosch Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikbosch Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianSchildt commented Jun 21, 2023

erikbosch commented Jun 22, 2023

erikbosch commented Jun 26, 2023

lukasmittag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikbosch commented Jun 16, 2023 •

edited

Loading

erikbosch Jun 20, 2023 •

edited

Loading

erikbosch Jun 20, 2023 •

edited

Loading