From 8908dccf6d933ea793ca69e674bb685d9e72d9c9 Mon Sep 17 00:00:00 2001 From: Johan Brandhorst-Satzkorn Date: Thu, 17 Nov 2022 17:21:20 -0600 Subject: [PATCH] Forward port some release fixes (#2631) * fix(worker): Verify contents of loaded session (#2629) * fix(worker): Verify contents of loaded session Previously, it was possible for an empty private key to be used when the session had no user or project associated with it. The new checks guarantee that we will fail gracefully in these cases. * Check against len instead of nil * Add CHANGELOG * fix(session): Always invoke all parts of session cancel trigger The session cancelation trigger would not set the key_id to null appropriately if another one of the fields on the session was already null, since that case statement would be matched first. The new structure matches all statements, in case any of them have special logic (such as in the project case). --- CHANGELOG.md | 2 + internal/daemon/worker/worker.go | 10 ++ internal/daemon/worker/worker_test.go | 110 ++++++++++++++++++ ...add_data_key_foreign_key_references.up.sql | 40 ++++--- 4 files changed, 145 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9f3b42ae4..2addfaae1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,8 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. ([PR](https://github.com/hashicorp/boundary/pull/2553)) * sessions: Fixed a panic in a controller when a worker is deleted while sessions are ongoing ([PR](https://github.com/hashicorp/boundary/pull/2612)) +* sessions: Fixed a panic in a worker when a user with an active + session is deleted ([PR](https://github.com/hashicorp/boundary/pull/2629)) ### Deprecations/Changes diff --git a/internal/daemon/worker/worker.go b/internal/daemon/worker/worker.go index 1d31742491..d4229d360b 100644 --- a/internal/daemon/worker/worker.go +++ b/internal/daemon/worker/worker.go @@ -615,6 +615,16 @@ func (w *Worker) getSessionTls(sessionManager session.Manager) func(hello *tls.C return nil, fmt.Errorf("error refreshing session: %w", err) } + if sess.GetCertificate() == nil { + return nil, fmt.Errorf("requested session has no certifificate") + } + if len(sess.GetCertificate().Raw) == 0 { + return nil, fmt.Errorf("requested session has no certificate DER") + } + if len(sess.GetPrivateKey()) == 0 { + return nil, fmt.Errorf("requested session has no private key") + } + certPool := x509.NewCertPool() certPool.AddCert(sess.GetCertificate()) diff --git a/internal/daemon/worker/worker_test.go b/internal/daemon/worker/worker_test.go index 0d249fee4d..7a64ba376c 100644 --- a/internal/daemon/worker/worker_test.go +++ b/internal/daemon/worker/worker_test.go @@ -2,14 +2,19 @@ package worker import ( "context" + "crypto/ed25519" "crypto/rand" + "crypto/tls" + "crypto/x509" "os" "testing" "time" "github.com/hashicorp/boundary/internal/cmd/base" "github.com/hashicorp/boundary/internal/cmd/config" + "github.com/hashicorp/boundary/internal/daemon/worker/session" "github.com/hashicorp/boundary/internal/db" + "github.com/hashicorp/boundary/internal/gen/controller/servers/services" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-secure-stdlib/configutil/v2" "github.com/hashicorp/go-secure-stdlib/listenerutil" @@ -272,3 +277,108 @@ func TestSetupWorkerAuthStorage(t *testing.T) { }) } } + +func Test_Worker_getSessionTls(t *testing.T) { + conf := &Config{ + Server: &base.Server{ + Listeners: []*base.ServerListener{ + {Config: &listenerutil.ListenerConfig{Purpose: []string{"api"}}}, + {Config: &listenerutil.ListenerConfig{Purpose: []string{"proxy"}}}, + {Config: &listenerutil.ListenerConfig{Purpose: []string{"cluster"}}}, + }, + Logger: hclog.Default(), + }, + } + conf.RawConfig = &config.Config{SharedConfig: &configutil.SharedConfig{DisableMlock: true}} + w, err := New(conf) + require.NoError(t, err) + w.lastStatusSuccess.Store(&LastStatusInformation{StatusResponse: &services.StatusResponse{}, StatusTime: time.Now(), LastCalculatedUpstreams: nil}) + w.baseContext = context.Background() + + t.Run("success", func(t *testing.T) { + m := &fakeManager{ + session: &fakeSession{ + cert: &x509.Certificate{ + Raw: []byte("something"), + }, + privateKey: []byte("something_else"), + }, + } + hello := &tls.ClientHelloInfo{ServerName: "s_1234567890"} + tlsConf, err := w.getSessionTls(m)(hello) + require.NoError(t, err) + require.Len(t, tlsConf.Certificates, 1) + require.Len(t, tlsConf.Certificates[0].Certificate, 1) + assert.Equal(t, m.session.cert.Raw, tlsConf.Certificates[0].Certificate[0]) + assert.Equal(t, m.session.cert, tlsConf.Certificates[0].Leaf) + assert.Equal(t, ed25519.PrivateKey(m.session.privateKey), tlsConf.Certificates[0].PrivateKey) + require.Len(t, tlsConf.NextProtos, 1) + assert.Equal(t, "http/1.1", tlsConf.NextProtos[0]) + assert.Equal(t, tls.VersionTLS13, int(tlsConf.MinVersion)) + assert.Equal(t, tls.RequireAnyClientCert, tlsConf.ClientAuth) + assert.Equal(t, true, tlsConf.InsecureSkipVerify) + assert.NotNil(t, tlsConf.VerifyConnection) + }) + t.Run("errors-on-empty-cert", func(t *testing.T) { + m := &fakeManager{ + session: &fakeSession{ + cert: nil, + privateKey: []byte("something_else"), + }, + } + hello := &tls.ClientHelloInfo{ServerName: "s_1234567890"} + _, err := w.getSessionTls(m)(hello) + require.Error(t, err) + }) + t.Run("errors-on-empty-cert-der", func(t *testing.T) { + m := &fakeManager{ + session: &fakeSession{ + cert: &x509.Certificate{ + Raw: nil, + }, + privateKey: []byte("something_else"), + }, + } + hello := &tls.ClientHelloInfo{ServerName: "s_1234567890"} + _, err := w.getSessionTls(m)(hello) + require.Error(t, err) + }) + t.Run("errors-on-empty-private-key", func(t *testing.T) { + m := &fakeManager{ + session: &fakeSession{ + cert: &x509.Certificate{ + Raw: []byte("something"), + }, + privateKey: nil, + }, + } + hello := &tls.ClientHelloInfo{ServerName: "s_1234567890"} + _, err := w.getSessionTls(m)(hello) + require.Error(t, err) + }) +} + +type fakeSession struct { + cert *x509.Certificate + privateKey []byte + + session.Session +} + +func (f *fakeSession) GetCertificate() *x509.Certificate { + return f.cert +} + +func (f *fakeSession) GetPrivateKey() []byte { + return f.privateKey +} + +type fakeManager struct { + session.Manager + + session *fakeSession +} + +func (f *fakeManager) LoadLocalSession(ctx context.Context, id string, workerId string) (session.Session, error) { + return f.session, nil +} diff --git a/internal/db/schema/migrations/oss/postgres/58/02_add_data_key_foreign_key_references.up.sql b/internal/db/schema/migrations/oss/postgres/58/02_add_data_key_foreign_key_references.up.sql index c2dcd7a3be..35368fe9cc 100644 --- a/internal/db/schema/migrations/oss/postgres/58/02_add_data_key_foreign_key_references.up.sql +++ b/internal/db/schema/migrations/oss/postgres/58/02_add_data_key_foreign_key_references.up.sql @@ -39,23 +39,29 @@ begin; create or replace function cancel_session_with_null_fk() returns trigger as $$ begin - case - when new.user_id is null then - perform cancel_session(new.public_id); - when new.host_id is null then - perform cancel_session(new.public_id); - when new.target_id is null then - perform cancel_session(new.public_id); - when new.host_set_id is null then - perform cancel_session(new.public_id); - when new.auth_token_id is null then - perform cancel_session(new.public_id); - when new.project_id is null then - -- Setting the key_id to null will allow the scope - -- to cascade delete its keys. - new.key_id = null; - perform cancel_session(new.public_id); - end case; + -- Note that we need each of these to run in case + -- more than one of them is null. + if new.user_id is null then + perform cancel_session(new.public_id); + end if; + if new.host_id is null then + perform cancel_session(new.public_id); + end if; + if new.target_id is null then + perform cancel_session(new.public_id); + end if; + if new.host_set_id is null then + perform cancel_session(new.public_id); + end if; + if new.auth_token_id is null then + perform cancel_session(new.public_id); + end if; + if new.project_id is null then + -- Setting the key_id to null will allow the scope + -- to cascade delete its keys. + new.key_id = null; + perform cancel_session(new.public_id); + end if; return new; end; $$ language plpgsql;