pam/main-exec: Exit with failure if the the server connection is lost

[3v1n0]

In case the DBus connection that the module provides has gone away we
should also stop the client, so that there won't be a dangling process
around.

This happens for example when a login timeout occurs, as in such case the PAM module connection goes away, while it's child is not.

Add tests checking this is the case.

This is enough to solve #903, while we can also use more low-level stuff, but for sure we've to die if the connection is lost, for whatever reason.

UDENG-6770

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.53%. Comparing base (c27fba1) to head (8b07100).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pam/internal/dbusmodule/transaction.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #904      +/-   ##
==========================================
+ Coverage   85.52%   85.53%   +0.01%     
==========================================
  Files          82       82              
  Lines        5707     5712       +5     
  Branches      109      109              
==========================================
+ Hits         4881     4886       +5     
  Misses        771      771              
  Partials       55       55              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[adombeck]

4 return values is a bit excessive, and an indication that this function is doing too much. Also, returning a context is not an idiomatic pattern in Go.

Since it's the context of the connection object which we want to access in the caller, and also the cleanup function is just calling the Close() method of the connection object, IMO it would be better to let the caller create the connection via dbus.Dial and pass that to NewTransaction:

func NewTransaction(conn *dbus.Conn, o ...TransactionOptions) (tx pam.ModuleTransaction, err error) {
	opts := options{}
	for _, f := range o {
		f(&opts)
	}

	if err = conn.Auth(nil); err != nil {
		return nil, err
	}
	if opts.isSharedConnection {
		if err = conn.Hello(); err != nil {
			return nil, err
		}
	}

	obj := conn.Object(ifaceName, objectPath)
	return &Transaction{obj: obj}, nil
}

and then in main-exec.go:

	log.Debugf(context.TODO(), "Connecting to %s", serverAddress)
	conn, err := dbus.Dial(serverAddress, dbus.WithContext(ctx))
	if err != nil {
		return fmt.Errorf("could not connect to %s: %w", serverAddress, err)
	}
	defer conn.Close()

	mTx, err := dbusmodule.NewTransaction(conn)
	if err != nil {
		return fmt.Errorf("%w: can't connect to server: %w", pam.ErrSystem, err)
	}

	actionDone := make(chan struct{})
	defer close(actionDone)

	go func() {
		select {
		case <-actionDone:
		case <-conn.Context().Done():
			log.Warningf(context.Background(), "Connection closed: %v",
				conn.Context().Err())
			os.Exit(255)
		}
	}()

[3v1n0]

So initially my implementation was by just exposing the Context() as a function, but since NewTransaction returns the interface type, that was leading to type-cast... Now, that's not a big deal but I preferred to avoid it. Or even making the function Returning the actual type instead...

However, I decided to just return more things, since the good thing of having return values is that they makes it clear that they should be used, while ignoring them has to be explicit.

And since we really want that all the users of this API should use the context to check the connection status, I thought it was the best way to achieve it.

The other option was adding another parameter, but again it would have not enforced the context usage.

[3v1n0]

So if returning the context isn't the best thing, maybe we can return just a chan, but it wouldn't allow to see errors.

[adombeck]

Returning a channel is definitely better than returning a context. I would still prefer letting the caller pass a dbus.Conn and handle closing and unexpected connection closing.

However, I decided to just return more things, since the good thing of having return values is that they makes it clear that they should be used, while ignoring them has to be explicit.

And since we really want that all the users of this API should use the context to check the connection status, I thought it was the best way to achieve it.

AFAICT, we only have one caller of NewTransaction in production code, i.e. main-exec.go.

[3v1n0]

Yeah, one thing I didn't like much was the fact that then caller has much control over the dbus.Conn value, but it's likely not a big deal...

[3v1n0]

So maybe I was thinking on something more like:

diff --git a/pam/internal/dbusmodule/transaction.go b/pam/internal/dbusmodule/transaction.go
index 3a40d6c77..95b7d9856 100644
--- a/pam/internal/dbusmodule/transaction.go
+++ b/pam/internal/dbusmodule/transaction.go
@@ -15,7 +15,8 @@ import (
 
 // Transaction is a [pam.Transaction] with dbus support.
 type Transaction struct {
-	obj dbus.BusObject
+	conn *dbus.Conn
+	obj  dbus.BusObject
 }
 
 type options struct {
@@ -38,11 +39,13 @@ const objectPath = "/com/ubuntu/authd/pam"
 // FIXME: dbus.Variant does not support maybe types, so we're using a variant string instead.
 const variantNothing = "<@mv nothing>"
 
+// Statically Ensure that [Transaction] implements [pam.ModuleTransaction].
+var _ pam.ModuleTransaction = &Transaction{}
+
 // NewTransaction creates a new [dbusmodule.Transaction] with the provided connection.
-// A [pam.ModuleTransaction] implementation is returned together with the connection context
-// that will be cancelled when the connection to the server has been closed and with a cleanup
-// function that should be called to release the connection.
-func NewTransaction(ctx context.Context, address string, o ...TransactionOptions) (tx pam.ModuleTransaction, connCtx context.Context, cleanup func(), err error) {
+// A [pam.ModuleTransaction] implementation is returned together with a cleanup function that
+// should be called to release the connection.
+func NewTransaction(ctx context.Context, address string, o ...TransactionOptions) (tx *Transaction, cleanup func(), err error) {
 	opts := options{}
 	for _, f := range o {
 		f(&opts)
@@ -51,22 +53,22 @@ func NewTransaction(ctx context.Context, address string, o ...TransactionOptions
 	log.Debugf(context.TODO(), "Connecting to %s", address)
 	conn, err := dbus.Dial(address, dbus.WithContext(ctx))
 	if err != nil {
-		return nil, nil, nil, err
+		return nil, nil, err
 	}
 	cleanup = func() { conn.Close() }
 	if err = conn.Auth(nil); err != nil {
 		cleanup()
-		return nil, nil, nil, err
+		return nil, nil, err
 	}
 	if opts.isSharedConnection {
 		if err = conn.Hello(); err != nil {
 			cleanup()
-			return nil, nil, nil, err
+			return nil, nil, err
 		}
 	}
-
 	obj := conn.Object(ifaceName, objectPath)
-	return &Transaction{obj: obj}, conn.Context(), cleanup, nil
+	return &Transaction{conn: conn, obj: obj}, cleanup, nil
 }
 
 // BusObject gets the DBus object.
@@ -74,6 +77,12 @@ func (tx *Transaction) BusObject() dbus.BusObject {
 	return tx.obj
 }
 
+// Context returns the context associated with the connection.  The
+// context will be cancelled when the connection is closed.
+func (tx *Transaction) Context() context.Context {
+	return tx.conn.Context()
+}
+
 // SetData allows to save any value in the module data that is preserved
 // during the whole time the module is loaded.
 func (tx *Transaction) SetData(key string, data any) error {
diff --git a/pam/internal/dbusmodule/transaction_test.go b/pam/internal/dbusmodule/transaction_test.go
index 6868a6a9d..48b32fc97 100644
--- a/pam/internal/dbusmodule/transaction_test.go
+++ b/pam/internal/dbusmodule/transaction_test.go
@@ -21,10 +21,9 @@ const objectPath = "/com/ubuntu/authd/pam"
 func TestTransactionConnectionError(t *testing.T) {
 	t.Parallel()
 
-	tx, dbusCtx, cleanup, err := dbusmodule.NewTransaction(context.TODO(), "invalid-address")
+	tx, cleanup, err := dbusmodule.NewTransaction(context.TODO(), "invalid-address")
 	require.Nil(t, tx, "Transaction must be unset")
 	require.Nil(t, cleanup, "Cleanup func must be unset")
-	require.Nil(t, dbusCtx, "DBus context must be unset")
 	require.NotNil(t, err, "Error must be set")
 }
 
@@ -607,15 +606,15 @@ func TestStartBinaryConv(t *testing.T) {
 
 func TestDisconnectionHandler(t *testing.T) {
 	address, _, cleanup := prepareTestServerWithCleanup(t, nil)
-	_, dbusCtx, txCleanup, err := dbusmodule.NewTransaction(context.TODO(), address,
+	tx, txCleanup, err := dbusmodule.NewTransaction(context.TODO(), address,
 		dbusmodule.WithSharedConnection(true))
 	require.NoError(t, err, "Setup: Can't connect to %s", address)
 	t.Cleanup(txCleanup)
 
-	require.NoError(t, dbusCtx.Err(), "Context must not be cancelled")
+	require.NoError(t, tx.Context().Err(), "Context must not be cancelled")
 	cleanup()
-	<-dbusCtx.Done()
-	require.ErrorIs(t, dbusCtx.Err(), context.Canceled, "Context must be cancelled")
+	<-tx.Context().Done()
+	require.ErrorIs(t, tx.Context().Err(), context.Canceled, "Context must be cancelled")
 }
 
 type methodCallExpectations struct {
@@ -647,10 +646,10 @@ func prepareTransaction(t *testing.T, expectedReturns []methodReturn) (pam.Modul
 	t.Helper()
 
 	address, obj := prepareTestServer(t, expectedReturns)
-	tx, dbusCtx, cleanup, err := dbusmodule.NewTransaction(context.TODO(), address,
+	tx, cleanup, err := dbusmodule.NewTransaction(context.TODO(), address,
 		dbusmodule.WithSharedConnection(true))
 	require.NoError(t, err, "Setup: Can't connect to %s", address)
-	t.Cleanup(func() { <-dbusCtx.Done() })
+	t.Cleanup(func() { <-tx.Context().Done() })
 	t.Cleanup(cleanup)
 
 	t.Logf("Using bus at address %s", address)
diff --git a/pam/main-exec.go b/pam/main-exec.go
index efb18a401..5720c1586 100644
--- a/pam/main-exec.go
+++ b/pam/main-exec.go
@@ -48,7 +48,7 @@ func mainFunc() error {
 
 	ctx, cancel := context.WithTimeout(context.TODO(), time.Duration(*timeout)*time.Second)
 	defer cancel()
-	mTx, _, closeFunc, err := dbusmodule.NewTransaction(ctx, serverAddress)
+	mTx, closeFunc, err := dbusmodule.NewTransaction(ctx, *serverAddress)
 	if err != nil {
 		return fmt.Errorf("%w: can't connect to server: %w", pam.ErrSystem, err)
 	}

As we avoid external stuff to control the connection (that is quite low level things)

[adombeck]

@3v1n0 what was the status here? I seem to remember that this branch had some issue and we decided that we want to go ahead with #903 instead, but I'm not sure.

[3v1n0]

Maybe only with the part of #903 doing the killing, although I'm still curious to understand why this fail, that could be an issue we could just hide otherwise 🤔