Remote server tls authentication - #1885
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1885 +/- ##
======================================
Coverage 61.0% 61.1%
======================================
Files 182 182
Lines 14906 14996 +90
======================================
+ Hits 9105 9164 +59
- Misses 5801 5832 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
|
||
| .. code-block:: bash | ||
|
|
||
| $ labgrid-client --secure [--cert PATH] places |
There was a problem hiding this comment.
Feels very much like bikeshedding to write this, but should we rename --secure to --tls instead? This makes it clear that we are using TLS for gRPC secure channels instead of a generic --secure which does not carry much meaning.
| Refer to the ``labgrid-coordinator`` man page for details. | ||
|
|
||
| When you are connecting with ``labgrid-client`` or ``labgrid-exporter`` to a | ||
| ``labgrid-coordinator``that has secure gRPC channels enabled you need to pass |
There was a problem hiding this comment.
| ``labgrid-coordinator``that has secure gRPC channels enabled you need to pass | |
| ``labgrid-coordinator`` that has secure gRPC channels enabled you need to pass |
| --secure | ||
| enable TLS gRPC channel | ||
| --cert | ||
| path to TLS certificate (in PEM format) | ||
| --key | ||
| path to TLS key (in PEM format) |
There was a problem hiding this comment.
On the coordinator side, the tls at the reverse proxy is also an option, right?
At servers, I would rather have nginx handling the certificates.
If so, could you add an alternative, such as
Or use a reverse proxy to add TLS, for example with ``nginx``:
.. code-block:: nginx
server {
listen 20407 ssl http2;
server_name labgrid.example.com;
ssl_certificate /etc/ssl/labgrid-coordinator.crt;
ssl_certificate_key /etc/ssl/labgrid-coordinator.key;
location / {
grpc_pass grpc://127.0.0.1:20408;
}
}
| the ``--secure`` (and ``--cert`` if the certificate is not trusted by the host | ||
| machine) option. | ||
| Refer to the ``labgrid-client`` and ``labgrid-exporter`` man pages for details. | ||
|
|
There was a problem hiding this comment.
explain roots.pem and GRPC_DEFAULT_SSL_ROOTS_FILE_PATH precedence here, please
|
@Emantor I have added 3 commits for the comments in the PR, if you've happy with these I'll squash all 3 into the first commit to keep the PR tidy |
gastmaier
left a comment
There was a problem hiding this comment.
Sounds good!
Users have the option to pass explicitly or read gprc doc for the env alternative GRPC_DEFAULT_SSL_ROOTS_FILE_PATH.
Looks good, please squash. |
c6d1da1 to
33ae157
Compare
|
|
||
| Instead of enabling TLS in ``labgrid-coordinator`` directly, a reverse proxy can | ||
| terminate TLS and forward cleartext gRPC to the coordinator. For example, with | ||
| ``nginx``: |
There was a problem hiding this comment.
In which cases would a reverse proxy preferred over having the coordinator do TLS itself?
There was a problem hiding this comment.
I think that depends on the deployment environment. I would expect the default option to be the direct coordinator TLS but some orgs might standardise TLS at the ingress layer, even then they might want to use the TLS enabled coordinator on top of that
| the ``--tls`` option. If ``--cert`` is not set, labgrid uses the host CA | ||
| certificates to verify the coordinator certificate. Use ``--cert`` to provide | ||
| a specific CA certificate instead. |
There was a problem hiding this comment.
For the client side, the usual approach is to use --cacert (like curl) to set the trusted CA certificates to check against. This leaves --cert free to allow future extension to client certificates.
d814bd4 to
c925307
Compare
|
updated with a new commit for supporting external clients with the client stream, will squash the latest 2 when you're both happy with the content |
Emantor
left a comment
There was a problem hiding this comment.
IMO this looks good, two things:
- The first commit should have a proper commit message instead of the commit message for the work done in the second commit.
- The second commit should have the commit message of the first.
Also please squash, the kw_only=True commit for compatibility makes sense to me.
b47b9f4 to
2d97b99
Compare
jluebbe
left a comment
There was a problem hiding this comment.
With the changes requested by @Emantor and @Bastian-Krause fid, I'd be fine with this.
Allow the coordinator to serve gRPC over TLS, and allow clients, exporters, and RemotePlace connections to authenticate the coordinator. Add command-line and configuration options along with documentation and tests for secure channels. Signed-off-by: Asher Pemberton <asher.pemberton@arm.com> Reviewed-by: Asher Pemberton <asher.pemberton@arm.com> # gatekeeper Co-authored-by: Luke Beardsmore <luke.beardsmore2@arm.com>
The Linux root certificate handling is intentionally done in labgrid instead of relying on grpcio's Python default. In gRPC's ComputePemRootCerts() precedence, GRPC_DEFAULT_SSL_ROOTS_FILE_PATH is considered first. The Python grpcio package then installs an ssl_roots_override_cb from grpc/_cython/cygrpc.pyx which loads the bundled grpc/_cython/_credentials/roots.pem. Because that callback succeeds before the later LoadSystemRootCerts() path is used, a normal pip-installed grpcio package can ignore CA certificates installed in the host trust store. Therefore labgrid's TLS certificate precedence is: 1. explicit certificate argument 2. system trust store loaded by labgrid 3. grpcio default roots, including bundled roots.pem, as fallback Signed-off-by: Asher Pemberton <asher.pemberton@arm.com> Reviewed-by: Asher Pemberton <asher.pemberton@arm.com> # gatekeeper Co-authored-by: Luke Beardsmore <luke.beardsmore2@arm.com>
2d97b99 to
9f39d0f
Compare
This MR adds support for gRPC SSL/TLS server authentication to
labgrid-coordinator,labgrid-exporterandlabgrid-client.Enabling a secure channel on all three components is done by adding the
--secureargument. Paths to the certificate and key (labgrid-coordinator-only) can be specified with the--certand--keyarguments.Testing has been added to verify that, given a secure-enabled
labgrid-coordinator, bothlabgrid-clientandlabgrid-exportercan connect successfully and perform an operation.Note on use of
labgrid-clientandlabgrid-exporterwithout an explicit--certspecifiedOn Linux, this currently reads the Debian/Ubuntu CA bundle at:
/etc/ssl/certs/ca-certificates.crtOn macOS, this reads certificates from the system Keychain using:
security find-certificate -a -pIf the coordinator certificate is not trusted by the host, or the platform's system roots cannot be loaded, pass the coordinator certificate explicitly with
--cert.Closes: #1541