-
Notifications
You must be signed in to change notification settings - Fork 3.7k
feat: support separate client TLS certificates #27543
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
Open
gwossum
wants to merge
18
commits into
master-1.x
Choose a base branch
from
gw/clientCert
base: master-1.x
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 17 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
401eec3
feat: support separate client TLS certificates
gwossum 0a1fd24
chore: code cleanups and comment fixes
gwossum 02cab0b
chore: add tests and improve existing tests
gwossum f587913
fix: subscriber service properly reloads TLS configuration
gwossum cc846f5
chore: code cleanup
gwossum 24e28e5
feat: httpd service supports full TLS config reloading
gwossum 6b1012a
feat: add server certificate sanity checking.
gwossum f77fdde
feat: add client certificate sanity checks
gwossum a88759a
fix: disabled TLSConfigManager can dial and listen
gwossum ebbd11a
chore: fix issues with comments
gwossum 8c05930
chore: fix compilation issue, slight test improvement
gwossum 1784768
fix: mutex protected fields accessed without lock
gwossum 27a416c
chore: rename `ignore-sanity-checks` to `ignore-cert-sanity-checks`
gwossum 9bec655
fix: logger safety in TLSCertMonitor
gwossum 3de4c7f
feat: add mTLS support to `influx` CLI command
gwossum 5ab7d5c
chore: update man page for `influx` CLI command
gwossum 69c311a
chore: test improvements and fix spelling mistakes
gwossum 630d224
chore: test improvements
gwossum File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| package cli_test | ||
|
|
||
| import ( | ||
| "crypto/tls" | ||
| "crypto/x509" | ||
| "net" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "net/url" | ||
| "os" | ||
| "strconv" | ||
| "testing" | ||
|
|
||
| "github.com/influxdata/influxdb/cmd/influx/cli" | ||
| "github.com/influxdata/influxdb/pkg/testing/selfsigned" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // mtlsTestServer starts an HTTPS test server that requires and verifies a client | ||
| // certificate signed by cert's CA. It returns the host and port the CLI should | ||
| // dial. | ||
| func mtlsTestServer(t *testing.T, cert *selfsigned.Cert) (host string, port int) { | ||
| t.Helper() | ||
|
|
||
| serverPair, err := tls.LoadX509KeyPair(cert.CertPath, cert.KeyPath) | ||
| require.NoError(t, err) | ||
|
|
||
| caPEM, err := os.ReadFile(cert.CACertPath) | ||
| require.NoError(t, err) | ||
| pool := x509.NewCertPool() | ||
| require.True(t, pool.AppendCertsFromPEM(caPEM), "failed to append CA certificate") | ||
|
|
||
| ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("X-Influxdb-Version", SERVER_VERSION) | ||
| })) | ||
| ts.TLS = &tls.Config{ | ||
| Certificates: []tls.Certificate{serverPair}, | ||
| ClientAuth: tls.RequireAndVerifyClientCert, | ||
| ClientCAs: pool, | ||
| } | ||
| ts.StartTLS() | ||
| t.Cleanup(ts.Close) | ||
|
|
||
| u, err := url.Parse(ts.URL) | ||
| require.NoError(t, err) | ||
| h, p, err := net.SplitHostPort(u.Host) | ||
| require.NoError(t, err) | ||
| port, err = strconv.Atoi(p) | ||
| require.NoError(t, err) | ||
| return h, port | ||
| } | ||
|
|
||
| // TestRunCLI_MutualTLS verifies the CLI presents its client certificate and | ||
| // verifies the server against the configured root CA. | ||
| func TestRunCLI_MutualTLS(t *testing.T) { | ||
| cert := selfsigned.NewSelfSignedCert(t) | ||
| host, port := mtlsTestServer(t, cert) | ||
|
|
||
| c := cli.New(CLIENT_VERSION) | ||
| c.Host = host | ||
| c.Port = port | ||
| c.Ssl = true | ||
| c.ClientCert = cert.CertPath | ||
| c.ClientKey = cert.KeyPath | ||
| c.RootCA = cert.CACertPath | ||
| c.Format = "column" | ||
| c.ClientConfig.Precision = "ns" | ||
| c.Execute = "SHOW DATABASES" | ||
| c.IgnoreSignals = true | ||
| c.ForceTTY = true | ||
|
|
||
| require.NoError(t, c.Run()) | ||
| require.Equal(t, SERVER_VERSION, c.ServerVersion) | ||
| } | ||
|
|
||
| // TestRunCLI_MutualTLS_MissingClientCert verifies the connection is refused when | ||
| // the CLI trusts the server but presents no client certificate. | ||
| func TestRunCLI_MutualTLS_MissingClientCert(t *testing.T) { | ||
| cert := selfsigned.NewSelfSignedCert(t) | ||
| host, port := mtlsTestServer(t, cert) | ||
|
|
||
| c := cli.New(CLIENT_VERSION) | ||
| c.Host = host | ||
| c.Port = port | ||
| c.Ssl = true | ||
| c.RootCA = cert.CACertPath // trust the server, but present no client cert | ||
| c.Execute = "SHOW DATABASES" | ||
| c.IgnoreSignals = true | ||
| c.ForceTTY = true | ||
|
|
||
| require.Error(t, c.Run()) | ||
| } | ||
|
|
||
| // TestRunCLI_MutualTLS_UntrustedServer verifies the server certificate is | ||
| // rejected when no root CA that signed it is configured. | ||
| func TestRunCLI_MutualTLS_UntrustedServer(t *testing.T) { | ||
| cert := selfsigned.NewSelfSignedCert(t) | ||
| host, port := mtlsTestServer(t, cert) | ||
|
|
||
| c := cli.New(CLIENT_VERSION) | ||
| c.Host = host | ||
| c.Port = port | ||
| c.Ssl = true | ||
| c.ClientCert = cert.CertPath | ||
| c.ClientKey = cert.KeyPath | ||
| // No RootCA: the self-signed server is not in the system pool. | ||
| c.Execute = "SHOW DATABASES" | ||
| c.IgnoreSignals = true | ||
| c.ForceTTY = true | ||
|
|
||
| require.Error(t, c.Run()) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know anything about the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in latest push