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

[xserver] TLS support added to xserver, aggregator server, and aggregator client #4266

Open
wants to merge 51 commits into
base: master
Choose a base branch
from

Conversation

roman-mazhut
Copy link
Contributor

@roman-mazhut roman-mazhut commented Apr 18, 2024

What this PR does / why we need it:

TLS support was added to xserver and aggregator client.
The server supports 3 modes: disabled(allows plaintext connections only), permissive(allows both plaintext and TLS connections), and enforced(TLS connections only). Also, mutual TLS can be enabled in the server config.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

To enable TLS support for the xserver a new section `tls` should be added to the server config. For instance:
----
rawtcp:
  listenAddress: 0.0.0.0:6403
  tls:
    mode: permissive
    mTLSEnabled: true
    certFile: /tmp/server.crt
    keyFile: /tmp/server.key
    clientCAFile: /tmp/rootCA.crt  # required for mTLS
    certificatesTTL: 1h
----

To enable TLS support for the aggregator client a new section `tls` should be added to the client config.
----
connection:
  tls:
    enabled: true
    insecureSkipVerify: false
    serverName: myserver
    caFile: /tmp/rootCA.crt
    certFile: /tmp/client.crt  # required for mTLS
    keyFile: /tmp/client.key  # required for mTLS
----

Benchmarks:
---
go test -bench=. -benchtime=40s -shuffle on
goos: linux
goarch: amd64
pkg: github.com/m3db/m3/src/x/server
cpu: AMD EPYC 7B13

# Create a connection for every data write
BenchmarkPlainTCPServer-96                           641020          202226 ns/op
BenchmarkTLSServer-96                                   24619             1936240 ns/op
BenchmarkMTLSServer-96                                15334            3193834 ns/op

# Use one connection for all data writes
BenchmarkKeepAlivePlainTCPServer-96          10322742      4630 ns/op
BenchmarkKeepAliveMTLSServer-96               12344016      4522 ns/op
BenchmarkKeepAliveTLSServer-96                   10149930      4924 ns/op
---

Does this PR require updating code package or user-facing documentation?:

NONE

@CLAassistant
Copy link

CLAassistant commented Apr 18, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

@SokolAndrey SokolAndrey left a comment

Choose a reason for hiding this comment

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

lgtm but pls get another stamp from @andrewmains12 or @justinjc

src/aggregator/client/config.go Outdated Show resolved Hide resolved
src/aggregator/client/conn.go Show resolved Hide resolved
src/x/server/buffered_conn.go Outdated Show resolved Hide resolved
// TLSMode represents the TLS mode
type TLSMode uint16

func (t *TLSMode) UnmarshalText(mode []byte) error {

Choose a reason for hiding this comment

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

nit: in statsdex we use enumer to deal with this boilerplate, check any generate.go file, i think this will also allow you to use it directly in config structures instead of a string with regexes

src/x/server/tls_options.go Outdated Show resolved Hide resolved
@SokolAndrey SokolAndrey enabled auto-merge (squash) May 7, 2024 12:48
auto-merge was automatically disabled May 17, 2024 21:39

Pull request was closed

@SokolAndrey SokolAndrey reopened this May 17, 2024
func (c *connection) upgradeToTLS(conn net.Conn) (net.Conn, error) {
certPool := x509.NewCertPool()
if c.tls.CAFile() != "" {
certs, err := os.ReadFile(c.tls.CAFile())
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be caching the cert file? We recreate connections with some amount of frequency.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe more appropriately, it just goes in the pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a TTL config cache on the connection level. It will be helpful for reconnecting but not for establishing new connections. The client has fewer connections than the server, so I believe it will be a neglectable overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, fair enough. Just for my understanding though--why not use the same cache for connecting as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because the write manager is responsible for creating connections(it can be considered as a connection pool). Theoretically, it can contain TLS options and use its TLS config cache to establish a connection. However, it knows nothing about connection parameters, and I don't want it to have that information because it doesn't seem to be something that should be responsible for secure connections. Also, even if it would manage the TLS configuration, a connection should do certificate rotation on its own in case of reconnection.

Copy link
Contributor

@andrewmains12 andrewmains12 left a comment

Choose a reason for hiding this comment

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

Do we have an end to end integration test of this? I'd like to see aggregator client with TLS enabled talking to aggregator server with TLS enabled.

src/x/server/buffered_conn.go Outdated Show resolved Hide resolved
src/x/server/server.go Outdated Show resolved Hide resolved
src/x/server/buffered_conn.go Outdated Show resolved Hide resolved
@roman-mazhut
Copy link
Contributor Author

Do we have an end to end integration test of this? I'd like to see aggregator client with TLS enabled talking to aggregator server with TLS enabled.
The test is added

src/x/server/secured_conn.go Outdated Show resolved Hide resolved
src/x/server/secured_conn.go Outdated Show resolved Hide resolved
src/x/server/secured_conn.go Outdated Show resolved Hide resolved
// Before reading we need to ensure if we know the type of the connection.
// After reading data it will be impossible to determine
// if the connection has the TLS layer or not.
if s.isTLS == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Fyi: if this were multithreaded, this would be a racy read (not protected by your lock).

src/x/server/secured_conn.go Outdated Show resolved Hide resolved
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.

8 participants