Skip to content

Commit

Permalink
TLS configuration for Admin Server
Browse files Browse the repository at this point in the history 
Signed-off-by: Matthieu MOREL <[email protected]>
  • Loading branch information
@mmorel-35
mmorel-35 committed May 17, 2022
1 parent d30b292 commit d324649
Show file tree
Hide file tree
Showing 3 changed files with 253 additions and 11 deletions.
51 changes: 41 additions & 10 deletions cmd/flags/admin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ package flags

import (
"context"
"crypto/tls"
"flag"
"fmt"
"io"
"net"
"net/http"
"net/http/pprof"
Expand All @@ -26,6 +28,7 @@ import (
"go.uber.org/zap"
"go.uber.org/zap/zapcore"

"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
"github.com/jaegertracing/jaeger/pkg/healthcheck"
"github.com/jaegertracing/jaeger/pkg/recoveryhandler"
"github.com/jaegertracing/jaeger/pkg/version"
Expand All @@ -35,15 +38,19 @@ const (
adminHTTPHostPort = "admin.http.host-port"
)

var tlsAdminHTTPFlagsConfig = tlscfg.ServerFlagsConfig{
Prefix: "admin.http",
}

// AdminServer runs an HTTP server with admin endpoints, such as healthcheck at /, /metrics, etc.
type AdminServer struct {
logger *zap.Logger
adminHostPort string

hc *healthcheck.HealthCheck

mux *http.ServeMux
server *http.Server
logger *zap.Logger
adminHostPort string
hc *healthcheck.HealthCheck
mux *http.ServeMux
server *http.Server
tlsCfg *tls.Config
tlsCertWatcherCloser io.Closer
}

// NewAdminServer creates a new admin server.
Expand All @@ -70,13 +77,28 @@ func (s *AdminServer) setLogger(logger *zap.Logger) {
// AddFlags registers CLI flags.
func (s *AdminServer) AddFlags(flagSet *flag.FlagSet) {
flagSet.String(adminHTTPHostPort, s.adminHostPort, fmt.Sprintf("The host:port (e.g. 127.0.0.1%s or %s) for the admin server, including health check, /metrics, etc.", s.adminHostPort, s.adminHostPort))
tlsAdminHTTPFlagsConfig.AddFlags(flagSet)
}

// InitFromViper initializes the server with properties retrieved from Viper.
func (s *AdminServer) initFromViper(v *viper.Viper, logger *zap.Logger) {
func (s *AdminServer) initFromViper(v *viper.Viper, logger *zap.Logger) error {
s.setLogger(logger)

s.adminHostPort = v.GetString(adminHTTPHostPort)
var tlsAdminHTTP tlscfg.Options
s.tlsCertWatcherCloser = &tlsAdminHTTP
tlsAdminHTTP, err := tlsAdminHTTPFlagsConfig.InitFromViper(v)
if err != nil {
return fmt.Errorf("failed to parse admin server TLS options: %w", err)
}
if tlsAdminHTTP.Enabled {
tlsCfg, err := tlsAdminHTTP.Config(s.logger) // This checks if the certificates are correctly provided
if err != nil {
return err
}
s.tlsCfg = tlsCfg
}
return nil
}

// Handle adds a new handler to the admin server.
Expand Down Expand Up @@ -111,10 +133,18 @@ func (s *AdminServer) serveWithListener(l net.Listener) {
Handler: recoveryHandler(s.mux),
ErrorLog: errorLog,
}

if s.tlsCfg != nil {
s.server.TLSConfig = s.tlsCfg
}
s.logger.Info("Starting admin HTTP server", zap.String("http-addr", s.adminHostPort))
go func() {
switch err := s.server.Serve(l); err {
var err error
if s.tlsCfg != nil {
err = s.server.ServeTLS(l, "", "")
} else {
err = s.server.Serve(l)
}
switch err {
case nil, http.ErrServerClosed:
// normal exit, nothing to do
default:
Expand All @@ -138,5 +168,6 @@ func (s *AdminServer) registerPprofHandlers() {

// Close stops the HTTP server
func (s *AdminServer) Close() error {
_ = s.tlsCertWatcherCloser.Close()
return s.server.Shutdown(context.Background())
}
209 changes: 209 additions & 0 deletions cmd/flags/admin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,27 @@
package flags

import (
"crypto/tls"
"fmt"
"net"
"net/http"
"strconv"
"strings"
"testing"
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zaptest/observer"

"github.com/jaegertracing/jaeger/pkg/config"
"github.com/jaegertracing/jaeger/pkg/config/tlscfg"
"github.com/jaegertracing/jaeger/ports"
)

var testCertKeyLocation = "../../pkg/config/tlscfg/testdata"

func TestAdminServerHandlesPortZero(t *testing.T) {
adminServer := NewAdminServer(":0")

Expand All @@ -47,3 +57,202 @@ func TestAdminServerHandlesPortZero(t *testing.T) {
port, _ := strconv.Atoi(strings.Split(hostPort, ":")[3])
assert.Greater(t, port, 0)
}

func TestCollectorAdminWithFailedFlags(t *testing.T) {
adminServer := NewAdminServer(fmt.Sprintf(":%d", ports.CollectorAdminHTTP))
zapCore, _ := observer.New(zap.InfoLevel)
logger := zap.New(zapCore)
v, command := config.Viperize(adminServer.AddFlags)
err := command.ParseFlags([]string{
"--admin.http.tls.enabled=false",
"--admin.http.tls.cert=blah", // invalid unless tls.enabled
})
require.NoError(t, err)
err = adminServer.initFromViper(v, logger)
require.Error(t, err)
assert.Contains(t, err.Error(), "failed to parse admin server TLS options")
}

func TestAdminServerTLS(t *testing.T) {
testCases := []struct {
name string
serverTLSFlags []string
clientTLS tlscfg.Options
expectTLSClientErr bool
expectAdminClientErr bool
expectServerFail bool
}{
{
name: "should fail with TLS client to untrusted TLS server",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
},
clientTLS: tlscfg.Options{
Enabled: true,
ServerName: "example.com",
},
expectTLSClientErr: true,
expectAdminClientErr: true,
expectServerFail: false,
},
{
name: "should fail with TLS client to trusted TLS server with incorrect hostname",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "nonEmpty",
},
expectTLSClientErr: true,
expectAdminClientErr: true,
expectServerFail: false,
},
{
name: "should pass with TLS client to trusted TLS server with correct hostname",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
},
expectTLSClientErr: false,
expectAdminClientErr: false,
expectServerFail: false,
},
{
name: "should fail with TLS client without cert to trusted TLS server requiring cert",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
"--admin.http.tls.client-ca=" + testCertKeyLocation + "/example-CA-cert.pem",
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
},
expectTLSClientErr: false,
expectServerFail: false,
expectAdminClientErr: true,
},
{
name: "should pass with TLS client with cert to trusted TLS server requiring cert",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
"--admin.http.tls.client-ca=" + testCertKeyLocation + "/example-CA-cert.pem",
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
CertPath: testCertKeyLocation + "/example-client-cert.pem",
KeyPath: testCertKeyLocation + "/example-client-key.pem",
},
expectTLSClientErr: false,
expectServerFail: false,
expectAdminClientErr: false,
},
{
name: "should fail with TLS client without cert to trusted TLS server requiring cert from a different CA",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
"--admin.http.tls.client-ca=" + testCertKeyLocation + "/wrong-CA-cert.pem", // NB: wrong CA
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
CertPath: testCertKeyLocation + "/example-client-cert.pem",
KeyPath: testCertKeyLocation + "/example-client-key.pem",
},
expectTLSClientErr: false,
expectServerFail: false,
expectAdminClientErr: true,
},
{
name: "should fail with TLS client with cert to trusted TLS server with incorrect TLS min",
serverTLSFlags: []string{
"--admin.http.tls.enabled=true",
"--admin.http.tls.cert=" + testCertKeyLocation + "/example-server-cert.pem",
"--admin.http.tls.key=" + testCertKeyLocation + "/example-server-key.pem",
"--admin.http.tls.client-ca=" + testCertKeyLocation + "/example-CA-cert.pem",
"--admin.http.tls.min-version=1.5",
},
clientTLS: tlscfg.Options{
Enabled: true,
CAPath: testCertKeyLocation + "/example-CA-cert.pem",
ServerName: "example.com",
CertPath: testCertKeyLocation + "/example-client-cert.pem",
KeyPath: testCertKeyLocation + "/example-client-key.pem",
},
expectTLSClientErr: true,
expectServerFail: true,
expectAdminClientErr: false,
},
}

for _, test := range testCases {
t.Run(test.name, func(t *testing.T) {
adminServer := NewAdminServer(fmt.Sprintf(":%d", ports.CollectorAdminHTTP))

v, command := config.Viperize(adminServer.AddFlags)
err := command.ParseFlags(test.serverTLSFlags)
require.NoError(t, err)
zapCore, _ := observer.New(zap.InfoLevel)
logger := zap.New(zapCore)

err = adminServer.initFromViper(v, logger)

if test.expectServerFail {
require.Error(t, err)
return
}
require.NoError(t, err)

adminServer.Serve()
defer adminServer.Close()

clientTLSCfg, err0 := test.clientTLS.Config(zap.NewNop())
require.NoError(t, err0)
dialer := &net.Dialer{Timeout: 2 * time.Second}
conn, clientError := tls.DialWithDialer(dialer, "tcp", fmt.Sprintf("localhost:%d", ports.CollectorAdminHTTP), clientTLSCfg)

if test.expectTLSClientErr {
require.Error(t, clientError)
} else {
require.NoError(t, clientError)
require.Nil(t, conn.Close())
}

client := &http.Client{
Transport: &http.Transport{
TLSClientConfig: clientTLSCfg,
},
}

response, requestError := client.Get(fmt.Sprintf("https://localhost:%d", ports.CollectorAdminHTTP))

if test.expectAdminClientErr {
require.Error(t, requestError)
} else {
require.NoError(t, requestError)
require.NotNil(t, response)
}
})
}
}
4 changes: 3 additions & 1 deletion cmd/flags/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,9 @@ func (s *Service) Start(v *viper.Viper) error {
}
s.MetricsFactory = metricsFactory

s.Admin.initFromViper(v, s.Logger)
if err = s.Admin.initFromViper(v, s.Logger); err != nil {
s.Logger.Fatal("Failed to initialize admin server", zap.Error(err))
}
if h := metricsBuilder.Handler(); h != nil {
route := metricsBuilder.HTTPRoute
s.Logger.Info("Mounting metrics handler on admin server", zap.String("route", route))
Expand Down

0 comments on commit d324649

Please sign in to comment.