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;