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

Add ability to disable mTLS #181

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

virtualzone
Copy link

Description

This PR adds support for disabling mTLS/TLS. This allows for running fleet telemetry behind a trusted proxy in a secure network which takes care of mTLS handling. mTLS can be disabled in the config by setting disable_tls to true. By default, this value is not set (= false), resulting in the same behaviour as before and ensuring a secure configuration.

Fixes #171

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.

Copy link
Collaborator

@patrickdemers6 patrickdemers6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review.

Has this been tested? I don't see how it would work as is. The vehicle connection handler uses the TLS cert to identify a device. With this change, wouldn't that be unavailable?

Reviewing this from a code perspective... will need opinion for @sethterashima for security.

@@ -71,5 +71,8 @@ func startServer(config *config.Config, airbrakeNotifier *gobrake.Notifier, logg
if server.TLSConfig, err = config.ExtractServiceTLSConfig(logger); err != nil {
return err
}
if config.DisableTLS {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this up, no point in extracting TLSConfig if not going to use it.

@@ -272,3 +272,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 (and only if!) your're running your Fleet Telemetry instance behind a trusted proxy which takes care of mTLS handling, set ```"disable_tls"``` to ```true``` in the config.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which takes care of mTLS handling => which handles mTLS

return nil, errors.New("tls config is empty - telemetry server is mTLS only, make sure to provide certificates in the config")
}

if c.DisableTLS {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't enter this code path at all if TLS disabled?

@@ -0,0 +1,30 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imo no need for this example file since disable_tls is the only option that has changed from the base example.

@@ -111,9 +111,13 @@ func serveHTTPWithLogs(h http.Handler, logger *logrus.Logger) http.Handler {
}

// Status API shows server with mtls config is up
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment incorrect now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running fleet telemetry behind a trusted proxy
2 participants