From 7f87ede1e27fe6bb4131518bd6be66ec5f80bdf1 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Mon, 11 Dec 2023 14:20:13 -0500 Subject: [PATCH] auth: respect default `tls.verify_server_hostname=false` (#19425) In Nomad 1.7.0, we refactored much of the authentication code to eliminate nil ACLs and create "virtual" ACL objects that can be used to reduce the risk of fail-open security bugs. In doing so, we accidentally dropped support for the default `tls.verify_server_hostname=false` option. Fix the bug by including the field in the set of conditions we check for the TLS argument we pass into the constructor (this keeps "policy" separate from "mechanism" in the auth code and reduces the number of dimensions we need to test). Change the field name in the Authenticator to better match the intent. --- .changelog/19425.txt | 3 +++ nomad/auth/auth.go | 12 ++++++------ nomad/auth/auth_test.go | 32 ++++++++++++++++++++++++-------- nomad/server.go | 2 +- 4 files changed, 34 insertions(+), 15 deletions(-) create mode 100644 .changelog/19425.txt diff --git a/.changelog/19425.txt b/.changelog/19425.txt new file mode 100644 index 000000000..5cfc85173 --- /dev/null +++ b/.changelog/19425.txt @@ -0,0 +1,3 @@ +```release-note:bug +auth: Fixed a bug where `tls.verify_server_hostname=false` was not respected, leading to authentication failures between Nomad agents +``` diff --git a/nomad/auth/auth.go b/nomad/auth/auth.go index 3d05553bc..fa79385f7 100644 --- a/nomad/auth/auth.go +++ b/nomad/auth/auth.go @@ -41,7 +41,7 @@ type Encrypter interface { type Authenticator struct { aclsEnabled bool - tlsEnabled bool + verifyTLS bool logger hclog.Logger getState StateGetter getLeaderACL LeaderACLGetter @@ -63,7 +63,7 @@ type AuthenticatorConfig struct { Logger hclog.Logger GetLeaderACLFn LeaderACLGetter AclsEnabled bool - TLSEnabled bool + VerifyTLS bool Region string Encrypter Encrypter } @@ -71,7 +71,7 @@ type AuthenticatorConfig struct { func NewAuthenticator(cfg *AuthenticatorConfig) *Authenticator { return &Authenticator{ aclsEnabled: cfg.AclsEnabled, - tlsEnabled: cfg.TLSEnabled, + verifyTLS: cfg.VerifyTLS, logger: cfg.Logger.With("auth"), getState: cfg.StateFn, getLeaderACL: cfg.GetLeaderACLFn, @@ -251,7 +251,7 @@ func (s *Authenticator) AuthenticateServerOnly(ctx RPCContext, args structs.Requ identity := &structs.AuthenticatedIdentity{RemoteIP: remoteIP} defer args.SetIdentity(identity) // always set the identity, even on errors - if s.tlsEnabled && !ctx.IsStatic() { + if s.verifyTLS && !ctx.IsStatic() { tlsCert := ctx.Certificate() if tlsCert == nil { return nil, errors.New("missing certificate information") @@ -294,7 +294,7 @@ func (s *Authenticator) AuthenticateClientOnly(ctx RPCContext, args structs.Requ identity := &structs.AuthenticatedIdentity{RemoteIP: remoteIP} defer args.SetIdentity(identity) // always set the identity, even on errors - if s.tlsEnabled && !ctx.IsStatic() { + if s.verifyTLS && !ctx.IsStatic() { tlsCert := ctx.Certificate() if tlsCert == nil { return nil, errors.New("missing certificate information") @@ -342,7 +342,7 @@ func (s *Authenticator) AuthenticateClientOnlyLegacy(ctx RPCContext, args struct identity := &structs.AuthenticatedIdentity{RemoteIP: remoteIP} defer args.SetIdentity(identity) // always set the identity, even on errors - if s.tlsEnabled && !ctx.IsStatic() { + if s.verifyTLS && !ctx.IsStatic() { tlsCert := ctx.Certificate() if tlsCert == nil { return nil, errors.New("missing certificate information") diff --git a/nomad/auth/auth_test.go b/nomad/auth/auth_test.go index 22ab2c727..c43c8152b 100644 --- a/nomad/auth/auth_test.go +++ b/nomad/auth/auth_test.go @@ -33,14 +33,14 @@ func TestAuthenticateDefault(t *testing.T) { ci.Parallel(t) testAuthenticator := func(t *testing.T, store *state.StateStore, - hasACLs, hasTLS bool) *Authenticator { + hasACLs, verifyTLS bool) *Authenticator { leaderACL := uuid.Generate() return NewAuthenticator(&AuthenticatorConfig{ StateFn: func() *state.StateStore { return store }, Logger: testlog.HCLogger(t), GetLeaderACLFn: func() string { return leaderACL }, AclsEnabled: hasACLs, - TLSEnabled: hasTLS, + VerifyTLS: verifyTLS, Region: "global", Encrypter: newTestEncrypter(), }) @@ -367,14 +367,14 @@ func TestAuthenticateServerOnly(t *testing.T) { ci.Parallel(t) testAuthenticator := func(t *testing.T, store *state.StateStore, - hasACLs, hasTLS bool) *Authenticator { + hasACLs, verifyTLS bool) *Authenticator { leaderACL := uuid.Generate() return NewAuthenticator(&AuthenticatorConfig{ StateFn: func() *state.StateStore { return store }, Logger: testlog.HCLogger(t), GetLeaderACLFn: func() string { return leaderACL }, AclsEnabled: hasACLs, - TLSEnabled: hasTLS, + VerifyTLS: verifyTLS, Region: "global", Encrypter: nil, }) @@ -400,6 +400,22 @@ func TestAuthenticateServerOnly(t *testing.T) { must.True(t, aclObj.AllowServerOp()) }, }, + { + name: "no mTLS but client cert", + testFn: func(t *testing.T) { + ctx := newTestContext(t, "client.global.nomad", "192.168.1.1") + args := &structs.GenericRequest{} + + store := testStateStore(t) + auth := testAuthenticator(t, store, true, false) + + aclObj, err := auth.AuthenticateServerOnly(ctx, args) + must.NoError(t, err) + must.NotNil(t, aclObj) + must.Eq(t, ":192.168.1.1", args.GetIdentity().String()) + must.True(t, aclObj.AllowServerOp()) + }, + }, { name: "with mTLS but client cert", testFn: func(t *testing.T) { @@ -446,7 +462,7 @@ func TestAuthenticateClientOnly(t *testing.T) { ci.Parallel(t) testAuthenticator := func(t *testing.T, store *state.StateStore, - hasACLs, hasTLS bool) *Authenticator { + hasACLs, verifyTLS bool) *Authenticator { leaderACL := uuid.Generate() return NewAuthenticator(&AuthenticatorConfig{ @@ -454,7 +470,7 @@ func TestAuthenticateClientOnly(t *testing.T) { Logger: testlog.HCLogger(t), GetLeaderACLFn: func() string { return leaderACL }, AclsEnabled: hasACLs, - TLSEnabled: hasTLS, + VerifyTLS: verifyTLS, Region: "global", Encrypter: nil, }) @@ -894,7 +910,7 @@ func TestIdentityToACLClaim(t *testing.T) { Logger: testlog.HCLogger(t), GetLeaderACLFn: func() string { return leaderACL }, AclsEnabled: true, - TLSEnabled: true, + VerifyTLS: true, Region: "global", Encrypter: newTestEncrypter(), }) @@ -1156,7 +1172,7 @@ func testDefaultAuthenticator(t *testing.T) *Authenticator { Logger: testlog.HCLogger(t), GetLeaderACLFn: func() string { return leaderACL }, AclsEnabled: true, - TLSEnabled: true, + VerifyTLS: true, Region: "global", Encrypter: nil, }) diff --git a/nomad/server.go b/nomad/server.go index c748fdb2a..c7e86fd3d 100644 --- a/nomad/server.go +++ b/nomad/server.go @@ -446,7 +446,7 @@ func NewServer(config *Config, consulCatalog consul.CatalogAPI, consulConfigFunc Logger: s.logger, GetLeaderACLFn: s.getLeaderAcl, AclsEnabled: s.config.ACLEnabled, - TLSEnabled: s.config.TLSConfig != nil && s.config.TLSConfig.EnableRPC, + VerifyTLS: s.config.TLSConfig != nil && s.config.TLSConfig.EnableRPC && s.config.TLSConfig.VerifyServerHostname, Region: s.Region(), Encrypter: s.encrypter, })