Merge pull request #15 from 3v1n0/safer-transaction

Safer transaction: add End() method and don't use as error

.github/workflows/lint.yaml

@@ -0,0 +1,22 @@
+on: [push, pull_request]
+name: Lint
+
+permissions:
+  contents: read
+
+jobs:
+  golangci:
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v3
+      - uses: actions/setup-go@v4
+        with:
+          go-version: '1.21'
+          cache: false
+      - name: Install PAM
+        run: sudo apt install -y libpam-dev
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v3
+        with:
+          version: v1.54

.golangci.yaml

@@ -0,0 +1,61 @@
+# This is for linting. To run it, please use:
+# golangci-lint run ${MODULE}/... [--fix]
+
+linters:
+  # linters to run in addition to default ones
+  enable:
+    - dupl
+    - durationcheck
+    - errname
+    - errorlint
+    - exportloopref
+    - forbidigo
+    - forcetypeassert
+    - gci
+    - godot
+    - gofmt
+    - gosec
+    - misspell
+    - nakedret
+    - nolintlint
+    - revive
+    - thelper
+    - tparallel
+    - unconvert
+    - unparam
+    - whitespace
+
+run:
+  timeout: 5m
+
+# Get all linter issues, even if duplicated
+issues:
+  exclude-use-default: false
+  max-issues-per-linter: 0
+  max-same-issues: 0
+  fix: false # we don’t want this in CI
+  exclude:
+    # EXC0001 errcheck: most errors are in defer calls, which are safe to ignore and idiomatic Go (would be good to only ignore defer ones though)
+    - 'Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv|w\.Stop). is not checked'
+    # EXC0008 gosec: duplicated of errcheck
+    - (G104|G307)
+    # EXC0010 gosec: False positive is triggered by 'src, err := ioutil.ReadFile(filename)'
+    - Potential file inclusion via variable
+    # We want named parameters even if unused, as they help better document the function
+    - unused-parameter
+    # Sometimes it is more readable it do a `if err:=a(); err != nil` tha simpy `return a()`
+    - if-return
+
+nolintlint:
+  require-explanation: true
+  require-specific: true
+
+linters-settings:
+  # Forbid the usage of deprecated ioutil and debug prints
+  forbidigo:
+    forbid:
+      - ioutil\.
+      - ^print.*$
+  # Never have naked return ever
+  nakedret:
+    max-func-lines: 1

errors.go

@@ -0,0 +1,94 @@
+package pam
+
+/*
+#include <security/pam_appl.h>
+*/
+import "C"
+
+// Error is the Type for PAM Return types
+type Error int
+
+// Pam Return types
+const (
+	// OpenErr indicates a dlopen() failure when dynamically loading a
+	// service module.
+	ErrOpen Error = C.PAM_OPEN_ERR
+	// ErrSymbol indicates a symbol not found.
+	ErrSymbol Error = C.PAM_SYMBOL_ERR
+	// ErrService indicates a error in service module.
+	ErrService Error = C.PAM_SERVICE_ERR
+	// ErrSystem indicates a system error.
+	ErrSystem Error = C.PAM_SYSTEM_ERR
+	// ErrBuf indicates a memory buffer error.
+	ErrBuf Error = C.PAM_BUF_ERR
+	// ErrPermDenied indicates a permission denied.
+	ErrPermDenied Error = C.PAM_PERM_DENIED
+	// ErrAuth indicates a authentication failure.
+	ErrAuth Error = C.PAM_AUTH_ERR
+	// ErrCredInsufficient indicates a can not access authentication data due to
+	// insufficient credentials.
+	ErrCredInsufficient Error = C.PAM_CRED_INSUFFICIENT
+	// ErrAuthinfoUnavail indicates that the underlying authentication service
+	// can not retrieve authentication information.
+	ErrAuthinfoUnavail Error = C.PAM_AUTHINFO_UNAVAIL
+	// ErrUserUnknown indicates a user not known to the underlying authentication
+	// module.
+	ErrUserUnknown Error = C.PAM_USER_UNKNOWN
+	// ErrMaxtries indicates that an authentication service has maintained a retry
+	// count which has been reached. No further retries should be attempted.
+	ErrMaxtries Error = C.PAM_MAXTRIES
+	// ErrNewAuthtokReqd indicates a new authentication token required. This is
+	// normally returned if the machine security policies require that the
+	// password should be changed because the password is nil or it has aged.
+	ErrNewAuthtokReqd Error = C.PAM_NEW_AUTHTOK_REQD
+	// ErrAcctExpired indicates that an user account has expired.
+	ErrAcctExpired Error = C.PAM_ACCT_EXPIRED
+	// ErrSession indicates a can not make/remove an entry for the
+	// specified session.
+	ErrSession Error = C.PAM_SESSION_ERR
+	// ErrCredUnavail indicates that an underlying authentication service can not
+	// retrieve user credentials.
+	ErrCredUnavail Error = C.PAM_CRED_UNAVAIL
+	// ErrCredExpired indicates that an user credentials expired.
+	ErrCredExpired Error = C.PAM_CRED_EXPIRED
+	// ErrCred indicates a failure setting user credentials.
+	ErrCred Error = C.PAM_CRED_ERR
+	// ErrNoModuleData indicates a no module specific data is present.
+	ErrNoModuleData Error = C.PAM_NO_MODULE_DATA
+	// ErrConv indicates a conversation error.
+	ErrConv Error = C.PAM_CONV_ERR
+	// ErrAuthtokErr indicates an authentication token manipulation error.
+	ErrAuthtok Error = C.PAM_AUTHTOK_ERR
+	// ErrAuthtokRecoveryErr indicates an authentication information cannot
+	// be recovered.
+	ErrAuthtokRecovery Error = C.PAM_AUTHTOK_RECOVERY_ERR
+	// ErrAuthtokLockBusy indicates am authentication token lock busy.
+	ErrAuthtokLockBusy Error = C.PAM_AUTHTOK_LOCK_BUSY
+	// ErrAuthtokDisableAging indicates an authentication token aging disabled.
+	ErrAuthtokDisableAging Error = C.PAM_AUTHTOK_DISABLE_AGING
+	// ErrTryAgain indicates a preliminary check by password service.
+	ErrTryAgain Error = C.PAM_TRY_AGAIN
+	// ErrIgnore indicates to ignore underlying account module regardless of
+	// whether the control flag is required, optional, or sufficient.
+	ErrIgnore Error = C.PAM_IGNORE
+	// ErrAbort indicates a critical error (module fail now request).
+	ErrAbort Error = C.PAM_ABORT
+	// ErrAuthtokExpired indicates an user's authentication token has expired.
+	ErrAuthtokExpired Error = C.PAM_AUTHTOK_EXPIRED
+	// ErrModuleUnknown indicates a module is not known.
+	ErrModuleUnknown Error = C.PAM_MODULE_UNKNOWN
+	// ErrBadItem indicates a bad item passed to pam_*_item().
+	ErrBadItem Error = C.PAM_BAD_ITEM
+	// ErrConvAgain indicates a conversation function is event driven and data
+	// is not available yet.
+	ErrConvAgain Error = C.PAM_CONV_AGAIN
+	// ErrIncomplete indicates to please call this function again to complete
+	// authentication stack. Before calling again, verify that conversation
+	// is completed.
+	ErrIncomplete Error = C.PAM_INCOMPLETE
+)
+
+// Error returns the error message for the given status.
+func (status Error) Error() string {
+	return C.GoString(C.pam_strerror(nil, C.int(status)))
+}

transaction.go

@@ -22,13 +22,16 @@ package pam
 import "C"
 
 import (
-	"errors"
-	"runtime"
+	"fmt"
 	"runtime/cgo"
 	"strings"
+	"sync/atomic"
 	"unsafe"
 )
 
+// success indicates a successful function return.
+const success = C.PAM_SUCCESS
+
 // Style is the type of message that the conversation handler should display.
 type Style int
 
@@ -39,16 +42,16 @@ const (
 	PromptEchoOff Style = C.PAM_PROMPT_ECHO_OFF
 	// PromptEchoOn indicates the conversation handler should obtain a
 	// string while echoing text.
-	PromptEchoOn = C.PAM_PROMPT_ECHO_ON
+	PromptEchoOn Style = C.PAM_PROMPT_ECHO_ON
 	// ErrorMsg indicates the conversation handler should display an
 	// error message.
-	ErrorMsg = C.PAM_ERROR_MSG
+	ErrorMsg Style = C.PAM_ERROR_MSG
 	// TextInfo indicates the conversation handler should display some
 	// text.
-	TextInfo = C.PAM_TEXT_INFO
+	TextInfo Style = C.PAM_TEXT_INFO
 	// BinaryPrompt indicates the conversation handler that should implement
 	// the private binary protocol
-	BinaryPrompt = C.PAM_BINARY_PROMPT
+	BinaryPrompt Style = C.PAM_BINARY_PROMPT
 )
 
 // ConversationHandler is an interface for objects that can be used as
@@ -94,42 +97,61 @@ func cbPAMConv(s C.int, msg *C.char, c C.uintptr_t) (*C.char, C.int) {
 	var err error
 	v := cgo.Handle(c).Value()
 	style := Style(s)
+	var handler ConversationHandler
 	switch cb := v.(type) {
 	case BinaryConversationHandler:
 		if style == BinaryPrompt {
 			bytes, err := cb.RespondPAMBinary(BinaryPointer(msg))
 			if err != nil {
-				return nil, C.PAM_CONV_ERR
+				return nil, C.int(ErrConv)
 			}
-			return (*C.char)(C.CBytes(bytes)), C.PAM_SUCCESS
-		} else {
-			r, err = cb.RespondPAM(style, C.GoString(msg))
+			return (*C.char)(C.CBytes(bytes)), success
 		}
+		handler = cb
 	case ConversationHandler:
 		if style == BinaryPrompt {
-			return nil, C.PAM_AUTHINFO_UNAVAIL
+			return nil, C.int(ErrConv)
 		}
-		r, err = cb.RespondPAM(style, C.GoString(msg))
+		handler = cb
 	}
+	if handler == nil {
+		return nil, C.int(ErrConv)
+	}
+	r, err = handler.RespondPAM(style, C.GoString(msg))
 	if err != nil {
-		return nil, C.PAM_CONV_ERR
+		return nil, C.int(ErrConv)
 	}
-	return C.CString(r), C.PAM_SUCCESS
+	return C.CString(r), success
 }
 
 // Transaction is the application's handle for a PAM transaction.
 type Transaction struct {
 	handle     *C.pam_handle_t
 	conv       *C.struct_pam_conv
-	status C.int
+	lastStatus atomic.Int32
 	c          cgo.Handle
 }
 
-// transactionFinalizer cleans up the PAM handle and deletes the callback
-// function.
-func transactionFinalizer(t *Transaction) {
-	C.pam_end(t.handle, t.status)
-	t.c.Delete()
+// End cleans up the PAM handle and deletes the callback function.
+// It must be called when done with the transaction.
+func (t *Transaction) End() error {
+	handle := atomic.SwapPointer((*unsafe.Pointer)(unsafe.Pointer(&t.handle)), nil)
+	if handle == nil {
+		return nil
+	}
+
+	defer t.c.Delete()
+	return t.handlePamStatus(C.pam_end((*C.pam_handle_t)(handle),
+		C.int(t.lastStatus.Load())))
+}
+
+// Allows to call pam functions managing return status
+func (t *Transaction) handlePamStatus(cStatus C.int) error {
+	t.lastStatus.Store(int32(cStatus))
+	if status := Error(cStatus); status != success {
+		return status
+	}
+	return nil
 }
 
 // Start initiates a new PAM transaction. Service is treated identically to
@@ -137,13 +159,21 @@ func transactionFinalizer(t *Transaction) {
 //
 // All application calls to PAM begin with Start*. The returned
 // transaction provides an interface to the remainder of the API.
+//
+// It's responsibility of the Transaction owner to release all the resources
+// allocated underneath by PAM by calling End() once done.
+//
+// It's not advised to End the transaction using a runtime.SetFinalizer unless
+// you're absolutely sure that your stack is multi-thread friendly (normally it
+// is not!) and using a LockOSThread/UnlockOSThread pair.
 func Start(service, user string, handler ConversationHandler) (*Transaction, error) {
 	return start(service, user, handler, "")
 }
 
-// StartFunc registers the handler func as a conversation handler.
+// StartFunc registers the handler func as a conversation handler and starts
+// the transaction (see Start() documentation).
 func StartFunc(service, user string, handler func(Style, string) (string, error)) (*Transaction, error) {
-	return Start(service, user, ConversationFunc(handler))
+	return start(service, user, ConversationFunc(handler), "")
 }
 
 // StartConfDir initiates a new PAM transaction. Service is treated identically to
@@ -153,9 +183,18 @@ func StartFunc(service, user string, handler func(Style, string) (string, error)
 //
 // All application calls to PAM begin with Start*. The returned
 // transaction provides an interface to the remainder of the API.
+//
+// It's responsibility of the Transaction owner to release all the resources
+// allocated underneath by PAM by calling End() once done.
+//
+// It's not advised to End the transaction using a runtime.SetFinalizer unless
+// you're absolutely sure that your stack is multi-thread friendly (normally it
+// is not!) and using a LockOSThread/UnlockOSThread pair.
 func StartConfDir(service, user string, handler ConversationHandler, confDir string) (*Transaction, error) {
 	if !CheckPamHasStartConfdir() {
-		return nil, errors.New("StartConfDir() was used, but the pam version on the system is not recent enough")
+		return nil, fmt.Errorf(
+			"%w: StartConfDir was used, but the pam version on the system is not recent enough",
+			ErrSystem)
 	}
 
 	return start(service, user, handler, confDir)
@@ -165,15 +204,16 @@ func start(service, user string, handler ConversationHandler, confDir string) (*
 	switch handler.(type) {
 	case BinaryConversationHandler:
 		if !CheckPamHasBinaryProtocol() {
-			return nil, errors.New("BinaryConversationHandler() was used, but it is not supported by this platform")
+			return nil, fmt.Errorf("%w: BinaryConversationHandler was used, but it is not supported by this platform",
+				ErrSystem)
 		}
 	}
 	t := &Transaction{
 		conv: &C.struct_pam_conv{},
 		c:    cgo.NewHandle(handler),
 	}
+
 	C.init_pam_conv(t.conv, C.uintptr_t(t.c))
-	runtime.SetFinalizer(t, transactionFinalizer)
 	s := C.CString(service)
 	defer C.free(unsafe.Pointer(s))
 	var u *C.char
@@ -181,23 +221,21 @@ func start(service, user string, handler ConversationHandler, confDir string) (*
 		u = C.CString(user)
 		defer C.free(unsafe.Pointer(u))
 	}
+	var err error
 	if confDir == "" {
-		t.status = C.pam_start(s, u, t.conv, &t.handle)
+		err = t.handlePamStatus(C.pam_start(s, u, t.conv, &t.handle))
 	} else {
 		c := C.CString(confDir)
 		defer C.free(unsafe.Pointer(c))
-		t.status = C.pam_start_confdir(s, u, t.conv, c, &t.handle)
+		err = t.handlePamStatus(C.pam_start_confdir(s, u, t.conv, c, &t.handle))
 	}
-	if t.status != C.PAM_SUCCESS {
-		return nil, t
+	if err != nil {
+		var _ = t.End()
+		return nil, err
 	}
 	return t, nil
 }
 
-func (t *Transaction) Error() string {
-	return C.GoString(C.pam_strerror(t.handle, C.int(t.status)))
-}
-
 // Item is a an PAM information type.
 type Item int
 
@@ -206,38 +244,42 @@ const (
 	// Service is the name which identifies the PAM stack.
 	Service Item = C.PAM_SERVICE
 	// User identifies the username identity used by a service.
-	User = C.PAM_USER
+	User Item = C.PAM_USER
 	// Tty is the terminal name.
-	Tty = C.PAM_TTY
+	Tty Item = C.PAM_TTY
 	// Rhost is the requesting host name.
-	Rhost = C.PAM_RHOST
+	Rhost Item = C.PAM_RHOST
 	// Authtok is the currently active authentication token.
-	Authtok = C.PAM_AUTHTOK
+	Authtok Item = C.PAM_AUTHTOK
 	// Oldauthtok is the old authentication token.
-	Oldauthtok = C.PAM_OLDAUTHTOK
+	Oldauthtok Item = C.PAM_OLDAUTHTOK
 	// Ruser is the requesting user name.
-	Ruser = C.PAM_RUSER
+	Ruser Item = C.PAM_RUSER
 	// UserPrompt is the string use to prompt for a username.
-	UserPrompt = C.PAM_USER_PROMPT
+	UserPrompt Item = C.PAM_USER_PROMPT
+	// FailDelay is the app supplied function to override failure delays.
+	FailDelay Item = C.PAM_FAIL_DELAY
+	// Xdisplay is the X display name
+	Xdisplay Item = C.PAM_XDISPLAY
+	// Xauthdata is the X server authentication data.
+	Xauthdata Item = C.PAM_XAUTHDATA
+	// AuthtokType is the type for pam_get_authtok
+	AuthtokType Item = C.PAM_AUTHTOK_TYPE
 )
 
 // SetItem sets a PAM information item.
 func (t *Transaction) SetItem(i Item, item string) error {
 	cs := unsafe.Pointer(C.CString(item))
 	defer C.free(cs)
-	t.status = C.pam_set_item(t.handle, C.int(i), cs)
-	if t.status != C.PAM_SUCCESS {
-		return t
-	}
-	return nil
+	return t.handlePamStatus(C.pam_set_item(t.handle, C.int(i), cs))
 }
 
 // GetItem retrieves a PAM information item.
 func (t *Transaction) GetItem(i Item) (string, error) {
 	var s unsafe.Pointer
-	t.status = C.pam_get_item(t.handle, C.int(i), &s)
-	if t.status != C.PAM_SUCCESS {
-		return "", t
+	err := t.handlePamStatus(C.pam_get_item(t.handle, C.int(i), &s))
+	if err != nil {
+		return "", err
 	}
 	return C.GoString((*C.char)(s)), nil
 }
@@ -253,32 +295,28 @@ const (
 	Silent Flags = C.PAM_SILENT
 	// DisallowNullAuthtok indicates that authorization should fail
 	// if the user does not have a registered authentication token.
-	DisallowNullAuthtok = C.PAM_DISALLOW_NULL_AUTHTOK
+	DisallowNullAuthtok Flags = C.PAM_DISALLOW_NULL_AUTHTOK
 	// EstablishCred indicates that credentials should be established
 	// for the user.
-	EstablishCred = C.PAM_ESTABLISH_CRED
-	// DeleteCred inidicates that credentials should be deleted.
-	DeleteCred = C.PAM_DELETE_CRED
+	EstablishCred Flags = C.PAM_ESTABLISH_CRED
+	// DeleteCred indicates that credentials should be deleted.
+	DeleteCred Flags = C.PAM_DELETE_CRED
 	// ReinitializeCred indicates that credentials should be fully
 	// reinitialized.
-	ReinitializeCred = C.PAM_REINITIALIZE_CRED
+	ReinitializeCred Flags = C.PAM_REINITIALIZE_CRED
 	// RefreshCred indicates that the lifetime of existing credentials
 	// should be extended.
-	RefreshCred = C.PAM_REFRESH_CRED
+	RefreshCred Flags = C.PAM_REFRESH_CRED
 	// ChangeExpiredAuthtok indicates that the authentication token
 	// should be changed if it has expired.
-	ChangeExpiredAuthtok = C.PAM_CHANGE_EXPIRED_AUTHTOK
+	ChangeExpiredAuthtok Flags = C.PAM_CHANGE_EXPIRED_AUTHTOK
 )
 
 // Authenticate is used to authenticate the user.
 //
 // Valid flags: Silent, DisallowNullAuthtok
 func (t *Transaction) Authenticate(f Flags) error {
-	t.status = C.pam_authenticate(t.handle, C.int(f))
-	if t.status != C.PAM_SUCCESS {
-		return t
-	}
-	return nil
+	return t.handlePamStatus(C.pam_authenticate(t.handle, C.int(f)))
 }
 
 // SetCred is used to establish, maintain and delete the credentials of a
@@ -286,55 +324,35 @@ func (t *Transaction) Authenticate(f Flags) error {
 //
 // Valid flags: EstablishCred, DeleteCred, ReinitializeCred, RefreshCred
 func (t *Transaction) SetCred(f Flags) error {
-	t.status = C.pam_setcred(t.handle, C.int(f))
-	if t.status != C.PAM_SUCCESS {
-		return t
-	}
-	return nil
+	return t.handlePamStatus(C.pam_setcred(t.handle, C.int(f)))
 }
 
 // AcctMgmt is used to determine if the user's account is valid.
 //
 // Valid flags: Silent, DisallowNullAuthtok
 func (t *Transaction) AcctMgmt(f Flags) error {
-	t.status = C.pam_acct_mgmt(t.handle, C.int(f))
-	if t.status != C.PAM_SUCCESS {
-		return t
-	}
-	return nil
+	return t.handlePamStatus(C.pam_acct_mgmt(t.handle, C.int(f)))
 }
 
 // ChangeAuthTok is used to change the authentication token.
 //
 // Valid flags: Silent, ChangeExpiredAuthtok
 func (t *Transaction) ChangeAuthTok(f Flags) error {
-	t.status = C.pam_chauthtok(t.handle, C.int(f))
-	if t.status != C.PAM_SUCCESS {
-		return t
-	}
-	return nil
+	return t.handlePamStatus(C.pam_chauthtok(t.handle, C.int(f)))
 }
 
 // OpenSession sets up a user session for an authenticated user.
 //
 // Valid flags: Slient
 func (t *Transaction) OpenSession(f Flags) error {
-	t.status = C.pam_open_session(t.handle, C.int(f))
-	if t.status != C.PAM_SUCCESS {
-		return t
-	}
-	return nil
+	return t.handlePamStatus(C.pam_open_session(t.handle, C.int(f)))
 }
 
 // CloseSession closes a previously opened session.
 //
 // Valid flags: Silent
 func (t *Transaction) CloseSession(f Flags) error {
-	t.status = C.pam_close_session(t.handle, C.int(f))
-	if t.status != C.PAM_SUCCESS {
-		return t
-	}
-	return nil
+	return t.handlePamStatus(C.pam_close_session(t.handle, C.int(f)))
 }
 
 // PutEnv adds or changes the value of PAM environment variables.
@@ -345,11 +363,7 @@ func (t *Transaction) CloseSession(f Flags) error {
 func (t *Transaction) PutEnv(nameval string) error {
 	cs := C.CString(nameval)
 	defer C.free(unsafe.Pointer(cs))
-	t.status = C.pam_putenv(t.handle, cs)
-	if t.status != C.PAM_SUCCESS {
-		return t
-	}
-	return nil
+	return t.handlePamStatus(C.pam_putenv(t.handle, cs))
 }
 
 // GetEnv is used to retrieve a PAM environment variable.
@@ -372,9 +386,10 @@ func (t *Transaction) GetEnvList() (map[string]string, error) {
 	env := make(map[string]string)
 	p := C.pam_getenvlist(t.handle)
 	if p == nil {
-		t.status = C.PAM_BUF_ERR
-		return nil, t
+		t.lastStatus.Store(int32(ErrBuf))
+		return nil, ErrBuf
 	}
+	t.lastStatus.Store(success)
 	for q := p; *q != nil; q = next(q) {
 		chunks := strings.SplitN(C.GoString(*q), "=", 2)
 		if len(chunks) == 2 {

transaction_test.go

@@ -2,10 +2,42 @@ package pam
 
 import (
 	"errors"
+	"fmt"
+	"os"
 	"os/user"
+	"path/filepath"
+	"runtime"
+	"sync/atomic"
 	"testing"
+	"time"
+	"unsafe"
 )
 
+func maybeEndTransaction(t *testing.T, tx *Transaction) {
+	t.Helper()
+
+	if tx == nil {
+		return
+	}
+	err := tx.End()
+	if err != nil {
+		t.Fatalf("end #error: %v", err)
+	}
+}
+
+func ensureTransactionEnds(t *testing.T, tx *Transaction) {
+	t.Helper()
+
+	runtime.SetFinalizer(tx, func(tx *Transaction) {
+		// #nosec:G103 - the pointer conversion is checked.
+		handle := atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&tx.handle)))
+		if handle == nil {
+			return
+		}
+		t.Fatalf("transaction has not been finalized")
+	})
+}
+
 func TestPAM_001(t *testing.T) {
 	u, _ := user.Current()
 	if u.Uid != "0" {
@@ -15,6 +47,8 @@ func TestPAM_001(t *testing.T) {
 	tx, err := StartFunc("", "test", func(s Style, msg string) (string, error) {
 		return p, nil
 	})
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
@@ -46,6 +80,8 @@ func TestPAM_002(t *testing.T) {
 		}
 		return "", errors.New("unexpected")
 	})
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
@@ -80,6 +116,8 @@ func TestPAM_003(t *testing.T) {
 		Password: "secret",
 	}
 	tx, err := Start("", "", c)
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
@@ -98,6 +136,8 @@ func TestPAM_004(t *testing.T) {
 		Password: "secret",
 	}
 	tx, err := Start("", "test", c)
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
@@ -115,9 +155,18 @@ func TestPAM_005(t *testing.T) {
 	tx, err := StartFunc("passwd", "test", func(s Style, msg string) (string, error) {
 		return "secret", nil
 	})
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
+	service, err := tx.GetItem(Service)
+	if err != nil {
+		t.Fatalf("GetItem #error: %v", err)
+	}
+	if service != "passwd" {
+		t.Fatalf("Unexpected service: %v", service)
+	}
 	err = tx.ChangeAuthTok(Silent)
 	if err != nil {
 		t.Fatalf("chauthtok #error: %v", err)
@@ -132,6 +181,8 @@ func TestPAM_006(t *testing.T) {
 	tx, err := StartFunc("passwd", u.Username, func(s Style, msg string) (string, error) {
 		return "secret", nil
 	})
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
@@ -153,6 +204,8 @@ func TestPAM_007(t *testing.T) {
 	tx, err := StartFunc("", "test", func(s Style, msg string) (string, error) {
 		return "", errors.New("Sorry, it didn't work")
 	})
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
@@ -164,6 +217,9 @@ func TestPAM_007(t *testing.T) {
 	if len(s) == 0 {
 		t.Fatalf("error #expected an error message")
 	}
+	if !errors.Is(err, ErrAuth) {
+		t.Fatalf("error #unexpected error %v", err)
+	}
 }
 
 func TestPAM_ConfDir(t *testing.T) {
@@ -173,6 +229,11 @@ func TestPAM_ConfDir(t *testing.T) {
 		Password: "wrongsecret",
 	}
 	tx, err := StartConfDir("permit-service", u.Username, c, "test-services")
+	defer func() {
+		if tx != nil {
+			_ = tx.End()
+		}
+	}()
 	if !CheckPamHasStartConfdir() {
 		if err == nil {
 			t.Fatalf("start should have errored out as pam_start_confdir is not available: %v", err)
@@ -180,6 +241,13 @@ func TestPAM_ConfDir(t *testing.T) {
 		// nothing else we do, we don't support it.
 		return
 	}
+	service, err := tx.GetItem(Service)
+	if err != nil {
+		t.Fatalf("GetItem #error: %v", err)
+	}
+	if service != "permit-service" {
+		t.Fatalf("Unexpected service: %v", service)
+	}
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
@@ -190,18 +258,31 @@ func TestPAM_ConfDir(t *testing.T) {
 }
 
 func TestPAM_ConfDir_FailNoServiceOrUnsupported(t *testing.T) {
+	if !CheckPamHasStartConfdir() {
+		t.Skip("this requires PAM with Conf dir support")
+	}
 	u, _ := user.Current()
 	c := Credentials{
 		Password: "secret",
 	}
-	_, err := StartConfDir("does-not-exists", u.Username, c, ".")
+	tx, err := StartConfDir("does-not-exists", u.Username, c, ".")
 	if err == nil {
 		t.Fatalf("authenticate #expected an error")
 	}
+	if tx != nil {
+		t.Fatalf("authenticate #unexpected transaction")
+	}
 	s := err.Error()
 	if len(s) == 0 {
 		t.Fatalf("error #expected an error message")
 	}
+	var pamErr Error
+	if !errors.As(err, &pamErr) {
+		t.Fatalf("error #unexpected type: %#v", err)
+	}
+	if pamErr != ErrAbort {
+		t.Fatalf("error #unexpected status: %v", pamErr)
+	}
 }
 
 func TestPAM_ConfDir_InfoMessage(t *testing.T) {
@@ -216,9 +297,18 @@ func TestPAM_ConfDir_InfoMessage(t *testing.T) {
 			}
 			return "", errors.New("unexpected")
 		}), "test-services")
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
+	service, err := tx.GetItem(Service)
+	if err != nil {
+		t.Fatalf("GetItem #error: %v", err)
+	}
+	if service != "echo-service" {
+		t.Fatalf("Unexpected service: %v", service)
+	}
 	err = tx.Authenticate(0)
 	if err != nil {
 		t.Fatalf("authenticate #error: %v", err)
@@ -229,11 +319,23 @@ func TestPAM_ConfDir_InfoMessage(t *testing.T) {
 }
 
 func TestPAM_ConfDir_Deny(t *testing.T) {
+	if !CheckPamHasStartConfdir() {
+		t.Skip("this requires PAM with Conf dir support")
+	}
 	u, _ := user.Current()
 	tx, err := StartConfDir("deny-service", u.Username, Credentials{}, "test-services")
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
+	service, err := tx.GetItem(Service)
+	if err != nil {
+		t.Fatalf("GetItem #error: %v", err)
+	}
+	if service != "deny-service" {
+		t.Fatalf("Unexpected service: %v", service)
+	}
 	err = tx.Authenticate(0)
 	if err == nil {
 		t.Fatalf("authenticate #expected an error")
@@ -242,6 +344,9 @@ func TestPAM_ConfDir_Deny(t *testing.T) {
 	if len(s) == 0 {
 		t.Fatalf("error #expected an error message")
 	}
+	if !errors.Is(err, ErrAuth) {
+		t.Fatalf("error #unexpected error %v", err)
+	}
 }
 
 func TestPAM_ConfDir_PromptForUserName(t *testing.T) {
@@ -251,6 +356,8 @@ func TestPAM_ConfDir_PromptForUserName(t *testing.T) {
 		Password: "wrongsecret",
 	}
 	tx, err := StartConfDir("succeed-if-user-test", "", c, "test-services")
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if !CheckPamHasStartConfdir() {
 		if err == nil {
 			t.Fatalf("start should have errored out as pam_start_confdir is not available: %v", err)
@@ -273,6 +380,8 @@ func TestPAM_ConfDir_WrongUserName(t *testing.T) {
 		Password: "wrongsecret",
 	}
 	tx, err := StartConfDir("succeed-if-user-test", "", c, "test-services")
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if !CheckPamHasStartConfdir() {
 		if err == nil {
 			t.Fatalf("start should have errored out as pam_start_confdir is not available: %v", err)
@@ -288,12 +397,20 @@ func TestPAM_ConfDir_WrongUserName(t *testing.T) {
 	if len(s) == 0 {
 		t.Fatalf("error #expected an error message")
 	}
+	if !errors.Is(err, ErrAuth) {
+		t.Fatalf("error #unexpected error %v", err)
+	}
 }
 
 func TestItem(t *testing.T) {
-	tx, _ := StartFunc("passwd", "test", func(s Style, msg string) (string, error) {
+	tx, err := StartFunc("passwd", "test", func(s Style, msg string) (string, error) {
 		return "", nil
 	})
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
+	if err != nil {
+		t.Fatalf("start #error: %v", err)
+	}
 
 	s, err := tx.GetItem(Service)
 	if err != nil {
@@ -328,6 +445,8 @@ func TestEnv(t *testing.T) {
 	tx, err := StartFunc("", "", func(s Style, msg string) (string, error) {
 		return "", nil
 	})
+	ensureTransactionEnds(t, tx)
+	defer maybeEndTransaction(t, tx)
 	if err != nil {
 		t.Fatalf("start #error: %v", err)
 	}
@@ -390,6 +509,139 @@ func TestEnv(t *testing.T) {
 	}
 }
 
+func Test_Error(t *testing.T) {
+	t.Parallel()
+	if !CheckPamHasStartConfdir() {
+		t.Skip("this requires PAM with Conf dir support")
+	}
+
+	statuses := map[string]error{
+		"success":               nil,
+		"open_err":              ErrOpen,
+		"symbol_err":            ErrSymbol,
+		"service_err":           ErrService,
+		"system_err":            ErrSystem,
+		"buf_err":               ErrBuf,
+		"perm_denied":           ErrPermDenied,
+		"auth_err":              ErrAuth,
+		"cred_insufficient":     ErrCredInsufficient,
+		"authinfo_unavail":      ErrAuthinfoUnavail,
+		"user_unknown":          ErrUserUnknown,
+		"maxtries":              ErrMaxtries,
+		"new_authtok_reqd":      ErrNewAuthtokReqd,
+		"acct_expired":          ErrAcctExpired,
+		"session_err":           ErrSession,
+		"cred_unavail":          ErrCredUnavail,
+		"cred_expired":          ErrCredExpired,
+		"cred_err":              ErrCred,
+		"no_module_data":        ErrNoModuleData,
+		"conv_err":              ErrConv,
+		"authtok_err":           ErrAuthtok,
+		"authtok_recover_err":   ErrAuthtokRecovery,
+		"authtok_lock_busy":     ErrAuthtokLockBusy,
+		"authtok_disable_aging": ErrAuthtokDisableAging,
+		"try_again":             ErrTryAgain,
+		"ignore":                nil, /* Ignore can't be returned */
+		"abort":                 ErrAbort,
+		"authtok_expired":       ErrAuthtokExpired,
+		"module_unknown":        ErrModuleUnknown,
+		"bad_item":              ErrBadItem,
+		"conv_again":            ErrConvAgain,
+		"incomplete":            ErrIncomplete,
+	}
+
+	type Action int
+	const (
+		account Action = iota + 1
+		auth
+		password
+		session
+	)
+	actions := map[string]Action{
+		"account":  account,
+		"auth":     auth,
+		"password": password,
+		"session":  session,
+	}
+
+	c := Credentials{}
+
+	servicePath := t.TempDir()
+
+	for ret, expected := range statuses {
+		ret := ret
+		expected := expected
+		for actionName, action := range actions {
+			actionName := actionName
+			action := action
+			t.Run(fmt.Sprintf("%s %s", ret, actionName), func(t *testing.T) {
+				t.Parallel()
+				serviceName := ret + "-" + actionName
+				serviceFile := filepath.Join(servicePath, serviceName)
+				contents := fmt.Sprintf("%[1]s requisite pam_debug.so "+
+					"auth=%[2]s cred=%[2]s acct=%[2]s prechauthtok=%[2]s "+
+					"chauthtok=%[2]s open_session=%[2]s close_session=%[2]s\n"+
+					"%[1]s requisite pam_permit.so\n", actionName, ret)
+
+				if err := os.WriteFile(serviceFile,
+					[]byte(contents), 0600); err != nil {
+					t.Fatalf("can't create service file %v: %v", serviceFile, err)
+				}
+
+				tx, err := StartConfDir(serviceName, "user", c, servicePath)
+				ensureTransactionEnds(t, tx)
+				defer maybeEndTransaction(t, tx)
+				if err != nil {
+					t.Fatalf("start #error: %v", err)
+				}
+
+				switch action {
+				case account:
+					err = tx.AcctMgmt(0)
+				case auth:
+					err = tx.Authenticate(0)
+				case password:
+					err = tx.ChangeAuthTok(0)
+				case session:
+					err = tx.OpenSession(0)
+				}
+
+				if !errors.Is(err, expected) {
+					t.Fatalf("error #unexpected status %#v vs %#v", err,
+						expected)
+				}
+
+				if err != nil {
+					var status Error
+					if !errors.As(err, &status) || err.Error() != status.Error() {
+						t.Fatalf("error #unexpected status %#v vs %#v", err.Error(),
+							status.Error())
+					}
+				}
+			})
+		}
+	}
+}
+
+func Test_Finalizer(t *testing.T) {
+	if !CheckPamHasStartConfdir() {
+		t.Skip("this requires PAM with Conf dir support")
+	}
+
+	func() {
+		tx, err := StartConfDir("permit-service", "", nil, "test-services")
+		ensureTransactionEnds(t, tx)
+		defer maybeEndTransaction(t, tx)
+		if err != nil {
+			t.Fatalf("start #error: %v", err)
+		}
+	}()
+
+	runtime.GC()
+	// sleep to switch to finalizer goroutine
+	time.Sleep(5 * time.Millisecond)
+}
+
 func TestFailure_001(t *testing.T) {
 	tx := Transaction{}
 	_, err := tx.GetEnvList()
@@ -461,3 +713,11 @@ func TestFailure_009(t *testing.T) {
 		t.Fatalf("getenvlist #expected an error")
 	}
 }
+
+func TestFailure_010(t *testing.T) {
+	tx := Transaction{}
+	err := tx.End()
+	if err != nil {
+		t.Fatalf("end #unexpected error %v", err)
+	}
+}