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

@@ -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
 

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())
 

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
+}

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;