Add ability to disable mTLS #279

klurpicolo · 2024-12-24T10:40:45Z

Description

Add option to disable mTLS

Fixes (issue)

Type of change

Please select all options that apply to this change:

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Bug fix (non-breaking change which fixes an issue)
Documentation update

Checklist:

Confirm you have completed the following steps:

My code follows the style of this project.
I have performed a self-review of my code.
I have made corresponding updates to the documentation.
I have added/updated unit tests to cover my changes.
I have added/updated integration tests to cover my changes.

cmd/main.go

klurpicolo · 2024-12-27T13:15:37Z

@patrickdemers6 Hi could you help review again?

patrickdemers6

This is looking better, thanks!

I will try using this once back from holiday travels. Have you been able to test this with a real vehicle?

An integration test for this would be nice to have but at first glance, it doesn't look like the easiest thing to add.

patrickdemers6 · 2024-12-27T16:40:29Z

config/config.go

@@ -47,6 +47,11 @@ type Config struct {
 	// TLS contains certificates & CA info for the webserver
 	TLS *TLS `json:"tls,omitempty"`

+	// DisableTLS indicates whether to disable mutual TLS (mTLS) for incoming connections.
+	// This should only be set to true if there is a reverse proxy that is already handling
+	// mTLS on behalf of this service. TLS will be ignored.


Include a comment about how client cert needs to be included in headers.

patrickdemers6 · 2024-12-27T16:40:52Z

README.md

@@ -284,3 +284,4 @@ Moreover, the following application-specific considerations apply:
  the frequency they need.
 * Providers agree to take full responsibility for privacy risks, as soon as data
  leave the devices (for more info read our privacy policies).
+* If you're running your Fleet Telemetry instance behind a trusted proxy which handles mTLS, set ```"disable_tls"``` to ```true``` in the config.


Include a comment about how client cert needs to be included in headers.

patrickdemers6 · 2024-12-27T16:56:43Z

server/streaming/server.go

+	if len(allCerts) == 0 {
+		return nil, errors.New("missing_certificate_error")
+	}
+	return allCerts[0], nil


Is index 0 correct? The TLS method grabs the last cert, not the first. If first is correct, let's exit early in for loop.

This confuse me a bit. As I understand implementation of this get identity thing, it should get from the most leaf certificate. However, from method extractCertFromTLS it gets from the last which by doc, it means root/ca cert.

So, should I get from leaf or root cert.

patrickdemers6 · 2024-12-27T16:56:59Z

server/streaming/server_test.go

+	"crypto/x509/pkix"
+	"encoding/base64"
+	"encoding/pem"
+	"github.com/teslamotors/fleet-telemetry/metrics/adapter/noop"


nit: grouping of imports

klurpicolo · 2024-12-28T10:53:05Z

I will try using this once back from holiday travels. Have you been able to test this with a real vehicle?

I haven't test with real vehicle setup.

An integration test for this would be nice to have but at first glance, it doesn't look like the easiest thing to add.

I'll try to add another integration test.

patrickdemers6 · 2025-01-02T19:27:38Z

I'll try to add another integration test.

There's only a unit test currently. Integration tests are defined in test/integration.

klurpicolo · 2025-01-06T17:08:39Z

There's only a unit test currently. Integration tests are defined in test/integration.

I try to add integration test, but still struggle with it. My idea is to add nginx that expose another port that terminate mTLS and forward in header on docker compose. Add a similar test as existing one, but call on that another port.

Adminius · 2025-01-09T08:25:56Z

Hey folks! Thanks for PR! I'm planing to use telemetry behind cloudflare tunnel and it's now not possible because of mTLS.

lotharbach · 2025-01-09T11:05:00Z

telemetry behind cloudflare tunnel

I'm not so sure this is possible. I would be happy if you prove me wrong.

mTLS is not something you should/can just ignore, this PR just tries to handle it differently. Instead of terminating the mTLS connection directly in the telemetry-server, where the full client cert is validated, it expects some proxy to terminate it and forward the full client cert via the "Client-Cert-Chain" header, so it again can be validated.

cloudflare is technically such a proxy. But as far as I am aware, it can not insert the full client cert into a header. This is the list of fields you could add into headers: https://developers.cloudflare.com/ruleset-engine/rules-language/fields/dynamic-fields/#cftls_client_authcert_revoked

If you just use the issuer and client subject DN without any verification, this information could easily be spoofed.

There is a "cf.tls_client_auth.cert_verified" field that would work for Cloudflare-issued client certificates, but in order to validate the certificate the vehicle sends against the Tesla Vehicle CA, you need to provide Teslas CA cert to cloudflare, which is only possible for enterprise acccounts: https://developers.cloudflare.com/ssl/client-certificates/byo-ca/#availability

quentinplessis · 2025-01-09T13:23:46Z

server/streaming/server.go

@@ -235,6 +244,35 @@ func extractIdentityFromConnection(r *http.Request) (*telemetry.RequestIdentity,
 }

 func extractCertFromHeaders(r *http.Request) (*x509.Certificate, error) {
+	raw := r.Header.Get("Client-Cert-Chain") // Client-Cert and Client-Cert-Chain HTTP Header Field  https://datatracker.ietf.org/doc/rfc9440/


Thank you for your contribution, I am interested as well so please let me know if I can help in any way (like checking the behavior in my environment etc.)!

Just a thought: it could be useful to be able to specify which header contains the client cert chain, for example to use it behind an AWS ALB in mutual TLS passthrough mode (in which case the header is X-Amzn-Mtls-Clientcert).

Implementation image:

# config/config.go type Config struct { ... ClientCertChainHeader string `json:"client_cert_chain_header,omitempty"` ... } # server/streaming/server.go func extractCertFromHeaders(r *http.Request, config *config.Config) (*x509.Certificate, error) { header := "Client-Cert-Chain" // Client-Cert and Client-Cert-Chain HTTP Header Field if config.ClientCertChainHeader != "" { header = config.ClientCertChainHeader } raw := r.Header.Get(header) ... }

Reference: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/mutual-authentication.html#mtls-http-headers

Nice idea. @patrickdemers6 as maintainer, what do you think?
Should we adhere to rfc9440 or allow custom header to pass certificate?

I'm a fan of ensuring we support the most popular proxies, load balancers, etc. If rfc9440 isn't widely adopted, we should try to create a flexible configuration that can evolve to differing header names and formats.

@quentinplessis I read https://docs.aws.amazon.com/elasticloadbalancing/latest/application/mutual-authentication.html#mtls-http-headers document. Simply adding customer header client_cert_chain_header won't work because it doesn't is base64 format same as RFC9440 does. I checked another LB from GCP it follows https://cloud.google.com/load-balancing/docs/https/custom-headers-global#variables RFC9440.

I've got an idea. Instead of a custom ClientCertChainHeader, make a enum config

# config/config.go type Config struct { ... TLSHeaderConfig HeaderExtractConfig `json:"client_cert_header_config,omitempty"` // naming this is hard. Open for suggestion ... } type HeaderExtractConfig string const ( RFC9440 HeaderExtractConfig = "rfc9440" AWSApplicationLoadBalancer HeaderExtractConfig = "aws_application_load_balancer" )

The valid value are rfc9440, aws_application_load_balancer. If another wants to add custom implementation then they can made PR.

patrickdemers6 · 2025-01-10T04:54:18Z

Just pushed an image to docker hub and will try testing shortly.

patrickdemers6/fleet-telemetry:279-disable-mtls-1d4ff5f

add DisableTLS

1d4ff5f

klurpicolo changed the title ~~add DisableTLS~~ add ability to disable mTLS Dec 24, 2024

klurpicolo changed the title ~~add ability to disable mTLS~~ Add ability to disable mTLS Dec 24, 2024

klurpicolo marked this pull request as ready for review December 24, 2024 16:50

klurpicolo mentioned this pull request Dec 24, 2024

Add ability to disable mTLS #181

Closed

9 tasks

patrickdemers6 reviewed Dec 24, 2024

View reviewed changes

cmd/main.go Show resolved Hide resolved

patrickdemers6 linked an issue Dec 24, 2024 that may be closed by this pull request

Running fleet telemetry behind a trusted proxy #171

Open

waritboonmasiri added 3 commits December 25, 2024 17:58

extract certificate from header

a61dac1

fix test

2affa62

add test and fix

a819484

patrickdemers6 reviewed Dec 27, 2024

View reviewed changes

patrickdemers6 requested a review from sethterashima December 27, 2024 17:02

add doc and group import

4bcead6

quentinplessis reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to disable mTLS #279

Add ability to disable mTLS #279

klurpicolo commented Dec 24, 2024 •

edited

Loading

klurpicolo commented Dec 27, 2024

patrickdemers6 left a comment

patrickdemers6 Dec 27, 2024

patrickdemers6 Dec 27, 2024

patrickdemers6 Dec 27, 2024

klurpicolo Jan 6, 2025

patrickdemers6 Dec 27, 2024

klurpicolo commented Dec 28, 2024

patrickdemers6 commented Jan 2, 2025

klurpicolo commented Jan 6, 2025

Adminius commented Jan 9, 2025

lotharbach commented Jan 9, 2025 •

edited

Loading

quentinplessis Jan 9, 2025 •

edited

Loading

klurpicolo Jan 10, 2025

patrickdemers6 Jan 10, 2025

klurpicolo Jan 12, 2025

klurpicolo Jan 12, 2025 •

edited

Loading

patrickdemers6 commented Jan 10, 2025

Add ability to disable mTLS #279

Are you sure you want to change the base?

Add ability to disable mTLS #279

Conversation

klurpicolo commented Dec 24, 2024 • edited Loading

Description

Type of change

Checklist:

klurpicolo commented Dec 27, 2024

patrickdemers6 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

Choose a reason for hiding this comment

klurpicolo commented Dec 28, 2024

patrickdemers6 commented Jan 2, 2025

klurpicolo commented Jan 6, 2025

Adminius commented Jan 9, 2025

lotharbach commented Jan 9, 2025 • edited Loading

quentinplessis Jan 9, 2025 • 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

klurpicolo Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

patrickdemers6 commented Jan 10, 2025

klurpicolo commented Dec 24, 2024 •

edited

Loading

lotharbach commented Jan 9, 2025 •

edited

Loading

quentinplessis Jan 9, 2025 •

edited

Loading

klurpicolo Jan 12, 2025 •

edited

Loading