Add HTTPS/TLS support to the auth proxy #2430

jarhodes314 · 2023-11-07T17:09:47Z

Proposed changes

As #2397 is large and needs some significant changes from the agent's perspective, this PR separates out the changes to the auth proxy, which add the HTTPS support, but not (public-facing) support for client certificates, in an attempt to make some progress in merging the changes. Internally, all the support for certificate authentication exists, and the unit tests use this, but the built mapper will be unchanged apart from using HTTPS to serve the API.

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#2363

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

reubenmiller · 2023-11-07T17:46:19Z

@jarhodes314 is the file transfer http service? Or are going to address that part in a different PR?

codecov · 2023-11-07T17:52:01Z

Codecov Report

Merging #2430 (ee20800) into main (7f6e084) will decrease coverage by 0.1%.
Report is 8 commits behind head on main.
The diff coverage is 75.1%.

Additional details and impacted files

Files	Coverage Δ
crates/common/axum_tls/src/lib.rs	`100.0% <100.0%> (ø)`
crates/common/tedge_config/src/lib.rs	`0.0% <ø> (ø)`
...tes/core/tedge_agent/src/software_manager/error.rs	`0.0% <ø> (ø)`
crates/core/tedge_api/src/error.rs	`0.0% <ø> (ø)`
crates/extensions/c8y_http_proxy/src/tests.rs	`91.8% <100.0%> (+<0.1%)`	⬆️
crates/extensions/c8y_mapper_ext/src/actor.rs	`77.8% <100.0%> (+0.2%)`	⬆️
crates/extensions/c8y_mapper_ext/src/converter.rs	`81.1% <100.0%> (+<0.1%)`	⬆️
crates/extensions/c8y_mapper_ext/src/tests.rs	`91.6% <100.0%> (+<0.1%)`	⬆️
...rates/extensions/tedge_downloader_ext/src/tests.rs	`96.3% <100.0%> (ø)`
crates/core/tedge_actors/src/servers/builders.rs	`71.7% <0.0%> (ø)`
... and 19 more

... and 11 files with indirect coverage changes

jarhodes314 · 2023-11-07T18:23:17Z

@jarhodes314 is the file transfer http service? Or are going to address that part in a different PR?

Not currently, although I should probably move some of the logic out of the auth proxy crate so it can be re-used there. Aside from some minor tweaks, this doesn’t contain anything new compared to #2397, it’s just a subset of that that should make merging the work less painful.

github-actions · 2023-11-07T19:41:45Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
359	0	3	359	100	55m46.117s

didier-wenzek

The mapper fails to run with no privileged and no specific settings. Running as root it works using the main device certificate which is not the best settings.
curl --insecure https://127.0.0.1:8001/c8y/inventory/managedObjects is working as expected. The --insecure making sense as my device use a self signed certificate.
curl http://127.0.0.1:8001/c8y/inventory/managedObjects leads to a weird error curl: (1) Received HTTP/0.9 when not allowed. Minor, but it would be good to serve a 404 not found on all HTTP requests.

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

crates/extensions/c8y_auth_proxy/src/actor.rs

jarhodes314 · 2023-11-08T10:51:51Z

* `curl http://127.0.0.1:8001/c8y/inventory/managedObjects` leads to a weird error `curl: (1) Received HTTP/0.9 when not allowed`. Minor, but it would be good to serve a 404 not found on all HTTP requests.

Should this be a 404, or should it be a 302 (temporary redirect) with the https URL in the location header? If we do want a redirect, I think 302 makes most sense, as there's no guarantee the HTTPS won't later be disabled (and if an application caches a 301, this can be really quite annoying to reverse).

didier-wenzek · 2023-11-08T12:39:36Z

* `curl http://127.0.0.1:8001/c8y/inventory/managedObjects` leads to a weird error `curl: (1) Received HTTP/0.9 when not allowed`. Minor, but it would be good to serve a 404 not found on all HTTP requests.
Should this be a 404, or should it be a 302 (temporary redirect) with the https URL in the location header? If we do want a redirect, I think 302 makes most sense, as there's no guarantee the HTTPS won't later be disabled (and if an application caches a 301, this can be really quite annoying to reverse).

Indeed, a 302 redirect from HTTP to HTTPS might be more appropriate.

didier-wenzek

Happy to approve.

Remaining details:

add the missing tedge config documentation for the two new settings.
clean c8y_auth_proxy dependencies

crates/extensions/c8y_auth_proxy/Cargo.toml

crates/extensions/c8y_mapper_ext/src/config.rs

crates/common/axum_tls/src/acceptor.rs

didier-wenzek · 2023-11-09T17:27:48Z

crates/common/axum_tls/src/maybe_tls.rs

+/// An optional [TlsStream], i.e. a stream of either TLS or non-TLS data
+///
+/// This is useful for redirecting HTTP requests to HTTPS.
+pub enum MaybeTlsStream<I> {


Kudos!

When I asked to redirect HTTP requests to HTTPS, I didn't imagine a sec such a dance with pin and streams.

Yeah, the trait requirements for hyper and axum machinery are low level. But pin_project makes it almost trivial

didier-wenzek · 2023-11-10T11:54:23Z

In order to avoid all these annoying typos warning in the cert and private key files, you can add them to https://github.com/thin-edge/thin-edge.io/blob/main/.typos.toml#L9

didier-wenzek

Approved. Well done!

Signed-off-by: James Rhodes <[email protected]>

jarhodes314 · 2023-11-15T16:22:43Z

@didier-wenzek I have added certificate authentication to this PR, the most recent two commits are new to you, the rest have only trivial changes from rebasing since you approved.

didier-wenzek

I will be happy to commit. I still need some time to play with client authentication, though.

In order to avoid all these annoying typos warning in the cert and private key files, add an exception in https://github.com/thin-edge/thin-edge.io/blob/main/.typos.toml#L9

didier-wenzek · 2023-11-15T18:29:57Z

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

@@ -807,6 +828,33 @@ pub struct MqttAuthClientConfig {
    pub key_file: Utf8PathBuf,
 }

+impl TEdgeConfigReaderHttpClientAuth {


I still have to understand how tedge config works under the hood!

How is this impl TEdgeConfigReaderHttpClientAuth connected to tedge_config.http.client.auth?

The top level tedge config that we interact with is TEdgeConfigReader. This struct contains a struct for each config field, with the struct names generated by appending the field name to TEdgeConfigReader (so tedge_config.http has type TEdgeConfigReaderHttp), and this continues recursively until a field has a value.

Because the data is stored within http.client.auth, I have implemented this method on TEdgeConfigReaderHttpClientAuth so it can be used as tedge_config.http.client.auth.identity().

Thanks. It's clearer.

didier-wenzek · 2023-11-15T18:35:37Z

docs/src/references/tedge-cumulocity-proxy.md

+Once HTTPS is enabled for the mapper, certificate-based authentication can also be enabled.
+The directory containing the certificates that the mapper will trust can be configured using `c8y.proxy.ca_path`,
+and the agent can be configured to use a trusted certificate using the `http.client.auth.cert_file` and `http.client.auth.key_file`
+setings.


Suggested change

setings.

settings

didier-wenzek · 2023-11-16T09:27:08Z

I observed an issue. Am I missing something?

The mapper being configured to enforce certificate-based client authentication and the agent being given a local certificate, I cannot install a software which has to be downloaded form c8y:

2023-11-16T09:08:21.343290588Z ERROR plugin_sm::plugin: Download error: DownloadError {
    reason: "Could not make a successful request to the remote server",
    url: "https://127.0.0.1:8001/c8y/inventory/binaries/367251",
    source_err: "error sending request for url (https://127.0.0.1:8001/c8y/inventory/binaries/367251): error trying to connect: invalid peer certificate: Other(CaUsedAsEndEntity)",

I set the following tedge config settings:

$ tedge config get c8y.proxy.cert_path                       
/etc/tedge/device-local-certs/demo-device-777.pem
$ tedge config get c8y.proxy.key_path 
/etc/tedge/device-local-certs/demo-device-777.key
$ tedge config get c8y.proxy.ca_path                                                          
/etc/tedge/device-local-certs/roots
$ tedge config get http.client.auth.cert_file 
/etc/tedge/device-local-certs/demo-device-007.pem
$ tedge config get http.client.auth.key_file                           
/etc/tedge/device-local-certs/demo-device-007.key

Note that curl works fine when given the same certificate to authenticate the client. i.e. the certificate is trusted by c8y_auth_proxy.

didier-wenzek · 2023-11-16T14:38:02Z

I confirm that this is nicely working. The settings is simply not obvious.

Things, I missed during my first testing session:

The certificate that authenticates the auth proxy (c8y.proxy.cert_path) must not be a signed certificate and must have a CN that is the hostname used to access the auth proxy. subjectAltName must include this hostname.
c8y.proxy.client.host must be set to the auth proxy hostname aka its CN
The signing certificate of the auth proxy must be added to /etc/ssl/certs of the clients
The signing certificate of the clients must be added to /etc/ssl/certs of the auth proxy.
Proper keyUsage and extendedKeyUsage must be attached to the key (at least keyAgreement for the proxy, and clientAuth for the clients).

I will create a ticket to document these steps.

jarhodes314 requested a review from didier-wenzek November 7, 2023 17:09

jarhodes314 had a problem deploying to Test Pull Request November 7, 2023 17:22 — with GitHub Actions Failure

jarhodes314 force-pushed the feat/auth-proxy-https branch 2 times, most recently from 531bfe8 to a2d8e6f Compare November 7, 2023 17:30

jarhodes314 had a problem deploying to Test Pull Request November 7, 2023 17:43 — with GitHub Actions Failure

didier-wenzek reviewed Nov 8, 2023

View reviewed changes

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs Outdated Show resolved Hide resolved

crates/extensions/c8y_auth_proxy/src/actor.rs Outdated Show resolved Hide resolved

jarhodes314 force-pushed the feat/auth-proxy-https branch from 66b8277 to e4c1f22 Compare November 8, 2023 10:36

jarhodes314 temporarily deployed to Test Pull Request November 8, 2023 10:43 — with GitHub Actions Inactive

jarhodes314 had a problem deploying to Test Pull Request November 8, 2023 14:08 — with GitHub Actions Failure

jarhodes314 temporarily deployed to Test Pull Request November 9, 2023 12:16 — with GitHub Actions Inactive

jarhodes314 temporarily deployed to Test Pull Request November 9, 2023 14:17 — with GitHub Actions Inactive

jarhodes314 temporarily deployed to Test Pull Request November 9, 2023 15:44 — with GitHub Actions Inactive

didier-wenzek reviewed Nov 9, 2023

View reviewed changes

jarhodes314 temporarily deployed to Test Pull Request November 9, 2023 17:40 — with GitHub Actions Inactive

jarhodes314 had a problem deploying to Test Pull Request November 10, 2023 10:33 — with GitHub Actions Failure

jarhodes314 temporarily deployed to Test Pull Request November 10, 2023 11:48 — with GitHub Actions Inactive

didier-wenzek approved these changes Nov 10, 2023

View reviewed changes

jarhodes314 temporarily deployed to Test Pull Request November 10, 2023 14:24 — with GitHub Actions Inactive

jarhodes314 force-pushed the feat/auth-proxy-https branch from cf90767 to cb7907a Compare November 14, 2023 14:40

jarhodes314 temporarily deployed to Test Pull Request November 14, 2023 14:47 — with GitHub Actions Inactive

jarhodes314 temporarily deployed to Test Pull Request November 14, 2023 16:21 — with GitHub Actions Inactive

jarhodes314 added 2 commits November 15, 2023 15:16

Add TLS support to the auth proxy

66a8d3e

Signed-off-by: James Rhodes <[email protected]>

Disable HTTPS when certificate is not provided

e9993a1

Signed-off-by: James Rhodes <[email protected]>

Redirect HTTP requests to HTTPS if HTTPS is enabled

4198446

Signed-off-by: James Rhodes <[email protected]>

jarhodes314 had a problem deploying to Test Pull Request November 15, 2023 15:22 — with GitHub Actions Failure

jarhodes314 added 8 commits November 15, 2023 15:23

Move TLS module to separate crate

67cee26

Signed-off-by: James Rhodes <[email protected]>

Add unit tests to new crate

71b7ca3

Signed-off-by: James Rhodes <[email protected]>

Abstract axum tls components into single function

6e1af18

Signed-off-by: James Rhodes <[email protected]>

Add unit tests to key loading logic

b380293

Signed-off-by: James Rhodes <[email protected]>

Add more unit tests and clean auth proxy Cargo.toml

4bbd527

Signed-off-by: James Rhodes <[email protected]>

Add documentation to new configurations

51d3b77

Signed-off-by: James Rhodes <[email protected]>

Run cargo sort

e2e1bb1

Regenerate certificates with sensible lifetimes and add script

0153108

jarhodes314 force-pushed the feat/auth-proxy-https branch from 3ad6fa3 to 0153108 Compare November 15, 2023 15:24

Update docs to mention HTTPS

570a5b6

jarhodes314 temporarily deployed to Test Pull Request November 15, 2023 15:30 — with GitHub Actions Inactive

Add certificate-authentication support

ee20800

jarhodes314 mentioned this pull request Nov 15, 2023

Add certificate based authentication to c8y-auth-proxy #2397

Closed

11 tasks

jarhodes314 had a problem deploying to Test Pull Request November 15, 2023 16:21 — with GitHub Actions Failure

jarhodes314 requested a review from didier-wenzek November 15, 2023 16:21

jarhodes314 temporarily deployed to Test Pull Request November 15, 2023 17:37 — with GitHub Actions Inactive

didier-wenzek reviewed Nov 15, 2023

View reviewed changes

jarhodes314 mentioned this pull request Nov 16, 2023

Replace routerify in file transfer server with axum #2461

Merged

11 tasks

jarhodes314 merged commit e0fce62 into thin-edge:main Nov 16, 2023
18 checks passed

gligorisaev self-assigned this Dec 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HTTPS/TLS support to the auth proxy #2430

Add HTTPS/TLS support to the auth proxy #2430

jarhodes314 commented Nov 7, 2023 •

edited

Loading

reubenmiller commented Nov 7, 2023

codecov bot commented Nov 7, 2023 •

edited

Loading

jarhodes314 commented Nov 7, 2023

github-actions bot commented Nov 7, 2023 •

edited

Loading

didier-wenzek left a comment

jarhodes314 commented Nov 8, 2023

didier-wenzek commented Nov 8, 2023

didier-wenzek left a comment

didier-wenzek Nov 9, 2023

jarhodes314 Nov 10, 2023

didier-wenzek commented Nov 10, 2023

didier-wenzek left a comment

jarhodes314 commented Nov 15, 2023

didier-wenzek left a comment

didier-wenzek Nov 15, 2023

jarhodes314 Nov 15, 2023

didier-wenzek Nov 15, 2023

didier-wenzek Nov 15, 2023

didier-wenzek commented Nov 16, 2023 •

edited

Loading

didier-wenzek commented Nov 16, 2023

Add HTTPS/TLS support to the auth proxy #2430

Add HTTPS/TLS support to the auth proxy #2430

Conversation

jarhodes314 commented Nov 7, 2023 • edited Loading

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

reubenmiller commented Nov 7, 2023

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

jarhodes314 commented Nov 7, 2023

github-actions bot commented Nov 7, 2023 • edited Loading

Robot Results

didier-wenzek left a comment

Choose a reason for hiding this comment

jarhodes314 commented Nov 8, 2023

didier-wenzek commented Nov 8, 2023

didier-wenzek left a comment

Choose a reason for hiding this comment

didier-wenzek Nov 9, 2023

Choose a reason for hiding this comment

jarhodes314 Nov 10, 2023

Choose a reason for hiding this comment

didier-wenzek commented Nov 10, 2023

didier-wenzek left a comment

Choose a reason for hiding this comment

jarhodes314 commented Nov 15, 2023

didier-wenzek left a comment

Choose a reason for hiding this comment

didier-wenzek Nov 15, 2023

Choose a reason for hiding this comment

jarhodes314 Nov 15, 2023

Choose a reason for hiding this comment

didier-wenzek Nov 15, 2023

Choose a reason for hiding this comment

didier-wenzek Nov 15, 2023

Choose a reason for hiding this comment

didier-wenzek commented Nov 16, 2023 • edited Loading

didier-wenzek commented Nov 16, 2023

jarhodes314 commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

github-actions bot commented Nov 7, 2023 •

edited

Loading

didier-wenzek commented Nov 16, 2023 •

edited

Loading