From 44c364e364c1eb5ccdfad3b11f574787152adbd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 7 Nov 2023 11:51:27 +0200 Subject: ci: Use golang-ci linter --- .github/workflows/lint.yaml | 22 ++++++++++++++++ .golangci.yaml | 61 +++++++++++++++++++++++++++++++++++++++++++++ transaction.go | 14 ++++++++--- 3 files changed, 93 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/lint.yaml create mode 100644 .golangci.yaml diff --git a/.github/workflows/lint.yaml b/.github/workflows/lint.yaml new file mode 100644 index 0000000..771e735 --- /dev/null +++ b/.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 diff --git a/.golangci.yaml b/.golangci.yaml new file mode 100644 index 0000000..bbfa6b4 --- /dev/null +++ b/.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 diff --git a/transaction.go b/transaction.go index 96bff63..fcba3d5 100644 --- a/transaction.go +++ b/transaction.go @@ -94,6 +94,7 @@ 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 { @@ -102,15 +103,18 @@ func cbPAMConv(s C.int, msg *C.char, c C.uintptr_t) (*C.char, C.int) { return nil, C.PAM_CONV_ERR } return (*C.char)(C.CBytes(bytes)), C.PAM_SUCCESS - } else { - r, err = cb.RespondPAM(style, C.GoString(msg)) } + handler = cb case ConversationHandler: if style == BinaryPrompt { return nil, C.PAM_AUTHINFO_UNAVAIL } - r, err = cb.RespondPAM(style, C.GoString(msg)) + handler = cb } + if handler == nil { + return nil, C.PAM_CONV_ERR + } + r, err = handler.RespondPAM(style, C.GoString(msg)) if err != nil { return nil, C.PAM_CONV_ERR } @@ -118,6 +122,8 @@ func cbPAMConv(s C.int, msg *C.char, c C.uintptr_t) (*C.char, C.int) { } // Transaction is the application's handle for a PAM transaction. +// +//nolint:errname type Transaction struct { handle *C.pam_handle_t conv *C.struct_pam_conv @@ -195,7 +201,7 @@ func start(service, user string, handler ConversationHandler, confDir string) (* } func (t *Transaction) Error() string { - return C.GoString(C.pam_strerror(t.handle, C.int(t.status))) + return C.GoString(C.pam_strerror(t.handle, t.status)) } // Item is a an PAM information type. -- cgit v1.2.3 From 6bb315c571d62ac8b55b6c088d02411ec5844cc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 22 Sep 2023 16:59:03 +0200 Subject: transaction: Add PAM Error types Go definitions And use them instead of C ones. Given that we have strings for them we can easily implement error interfaces for it too. --- errors.go | 94 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ transaction.go | 39 +++++++++++++----------- 2 files changed, 115 insertions(+), 18 deletions(-) create mode 100644 errors.go diff --git a/errors.go b/errors.go new file mode 100644 index 0000000..2f81a9a --- /dev/null +++ b/errors.go @@ -0,0 +1,94 @@ +package pam + +/* +#include +*/ +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))) +} diff --git a/transaction.go b/transaction.go index fcba3d5..1142fdc 100644 --- a/transaction.go +++ b/transaction.go @@ -29,6 +29,9 @@ import ( "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 @@ -100,25 +103,25 @@ func cbPAMConv(s C.int, msg *C.char, c C.uintptr_t) (*C.char, C.int) { 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 + 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) } handler = cb } if handler == nil { - return nil, C.PAM_CONV_ERR + 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. @@ -194,14 +197,14 @@ func start(service, user string, handler ConversationHandler, confDir string) (* defer C.free(unsafe.Pointer(c)) t.status = C.pam_start_confdir(s, u, t.conv, c, &t.handle) } - if t.status != C.PAM_SUCCESS { + if t.status != success { return nil, t } return t, nil } func (t *Transaction) Error() string { - return C.GoString(C.pam_strerror(t.handle, t.status)) + return Error(t.status).Error() } // Item is a an PAM information type. @@ -232,7 +235,7 @@ 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 { + if t.status != success { return t } return nil @@ -242,7 +245,7 @@ func (t *Transaction) SetItem(i Item, item string) error { 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 { + if t.status != success { return "", t } return C.GoString((*C.char)(s)), nil @@ -281,7 +284,7 @@ const ( // 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 { + if t.status != success { return t } return nil @@ -293,7 +296,7 @@ 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 { + if t.status != success { return t } return nil @@ -304,7 +307,7 @@ func (t *Transaction) SetCred(f Flags) error { // 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 { + if t.status != success { return t } return nil @@ -315,7 +318,7 @@ func (t *Transaction) AcctMgmt(f Flags) error { // 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 { + if t.status != success { return t } return nil @@ -326,7 +329,7 @@ func (t *Transaction) ChangeAuthTok(f Flags) error { // 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 { + if t.status != success { return t } return nil @@ -337,7 +340,7 @@ func (t *Transaction) OpenSession(f Flags) error { // 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 { + if t.status != success { return t } return nil @@ -352,7 +355,7 @@ 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 { + if t.status != success { return t } return nil @@ -378,7 +381,7 @@ 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 + t.status = C.int(ErrBuf) return nil, t } for q := p; *q != nil; q = next(q) { -- cgit v1.2.3 From ea51cc0fe42e421ba0eaedded96bfe6448cbc43c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 22 Sep 2023 18:26:02 +0200 Subject: transaction: Add tests for all the possible Status (and error) values Use pam_debug.so to generate pam configurations at test time and check if the returned values expect the ones we want. --- transaction_test.go | 120 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/transaction_test.go b/transaction_test.go index 94aa9c1..809364c 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -2,7 +2,10 @@ package pam import ( "errors" + "fmt" + "os" "os/user" + "path/filepath" "testing" ) @@ -164,6 +167,9 @@ func TestPAM_007(t *testing.T) { if len(s) == 0 { t.Fatalf("error #expected an error message") } + if tx.Error() != ErrAuth.Error() { + t.Fatalf("error #unexpected status %v", tx.Error()) + } } func TestPAM_ConfDir(t *testing.T) { @@ -242,6 +248,9 @@ func TestPAM_ConfDir_Deny(t *testing.T) { if len(s) == 0 { t.Fatalf("error #expected an error message") } + if tx.Error() != ErrAuth.Error() { + t.Fatalf("error #unexpected status %v", tx.Error()) + } } func TestPAM_ConfDir_PromptForUserName(t *testing.T) { @@ -288,6 +297,9 @@ func TestPAM_ConfDir_WrongUserName(t *testing.T) { if len(s) == 0 { t.Fatalf("error #expected an error message") } + if tx.Error() != ErrAuth.Error() { + t.Fatalf("error #unexpected status %v", tx.Error()) + } } func TestItem(t *testing.T) { @@ -390,6 +402,114 @@ 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": Error(success), + "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": Error(success), /* 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) + 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 tx.Error() != expected.Error() { + t.Fatalf("error #unexpected status %v", tx.Error()) + } + if tx.Error() == Error(success).Error() && err != nil { + t.Fatalf("error #unexpected: %v", err) + } else if tx.Error() != Error(success).Error() && err == nil { + t.Fatalf("error #expected an error message") + } + }) + } + } +} + func TestFailure_001(t *testing.T) { tx := Transaction{} _, err := tx.GetEnvList() -- cgit v1.2.3 From a5f5ad6470e625b72c71cac9477173307b005af2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 25 Sep 2023 13:44:45 +0200 Subject: transaction: Return errors wrapping pam.Error values on failure If the transaction fails during start, there's no way to get the error detail in a programmatic way, so let's wrap the pam.Error to allow more per-type checks. --- transaction.go | 11 +++++++---- transaction_test.go | 7 +++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/transaction.go b/transaction.go index 1142fdc..6859817 100644 --- a/transaction.go +++ b/transaction.go @@ -22,7 +22,7 @@ package pam import "C" import ( - "errors" + "fmt" "runtime" "runtime/cgo" "strings" @@ -164,7 +164,9 @@ func StartFunc(service, user string, handler func(Style, string) (string, error) // transaction provides an interface to the remainder of the API. 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) @@ -174,7 +176,8 @@ 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{ @@ -198,7 +201,7 @@ func start(service, user string, handler ConversationHandler, confDir string) (* t.status = C.pam_start_confdir(s, u, t.conv, c, &t.handle) } if t.status != success { - return nil, t + return nil, Error(t.status) } return t, nil } diff --git a/transaction_test.go b/transaction_test.go index 809364c..5bc858e 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -208,6 +208,13 @@ func TestPAM_ConfDir_FailNoServiceOrUnsupported(t *testing.T) { 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) { -- cgit v1.2.3 From 3e4f7f5e4be10027f645e21dd2aae37cd4c580a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 11 Oct 2023 10:25:42 +0200 Subject: transaction: Add an helper function to handle pam functions return status All the pam functions return an integer with the status of the operation so instead of duplicating the same code everywhere, that is quite error prone, use an helper function. It would have been nice to make this more dynamic, but cgo doesn't allow us to do much magic here. This is enough though. --- transaction.go | 76 +++++++++++++++++++++------------------------------------- 1 file changed, 27 insertions(+), 49 deletions(-) diff --git a/transaction.go b/transaction.go index 6859817..642059f 100644 --- a/transaction.go +++ b/transaction.go @@ -22,6 +22,7 @@ package pam import "C" import ( + "errors" "fmt" "runtime" "runtime/cgo" @@ -141,6 +142,15 @@ func transactionFinalizer(t *Transaction) { t.c.Delete() } +// Allows to call pam functions managing return status +func (t *Transaction) handlePamStatus(cStatus C.int) error { + t.status = cStatus + if cStatus != success { + return t + } + return nil +} + // Start initiates a new PAM transaction. Service is treated identically to // how pam_start treats it internally. // @@ -193,15 +203,16 @@ 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 != success { - return nil, Error(t.status) + if err != nil { + return nil, errors.Join(Error(t.status), err) } return t, nil } @@ -237,19 +248,15 @@ const ( 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 != 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 != 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 } @@ -286,11 +293,7 @@ const ( // // Valid flags: Silent, DisallowNullAuthtok func (t *Transaction) Authenticate(f Flags) error { - t.status = C.pam_authenticate(t.handle, C.int(f)) - if t.status != 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 @@ -298,55 +301,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 != 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 != 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 != 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 != 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 != 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. @@ -357,11 +340,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 != success { - return t - } - return nil + return t.handlePamStatus(C.pam_putenv(t.handle, cs)) } // GetEnv is used to retrieve a PAM environment variable. @@ -384,8 +363,7 @@ 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.int(ErrBuf) - return nil, t + return nil, t.handlePamStatus(C.int(ErrBuf)) } for q := p; *q != nil; q = next(q) { chunks := strings.SplitN(C.GoString(*q), "=", 2) -- cgit v1.2.3 From 911a346a003fd5ef80688caf03a0296b95efee69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 29 Sep 2023 23:01:50 +0200 Subject: transaction: Use Atomic to store/load the status Transactions save the status of each operation in a status field, however such field could be written concurrently by various operations, so we need to be sure that: - We always return the status for the current operation - We store the status in a atomic way so that other actions won't create write races In general, in a multi-thread operation one should not rely on Transaction.Error() to get info about the last operation. --- transaction.go | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/transaction.go b/transaction.go index 642059f..7b19f5c 100644 --- a/transaction.go +++ b/transaction.go @@ -27,6 +27,7 @@ import ( "runtime" "runtime/cgo" "strings" + "sync/atomic" "unsafe" ) @@ -129,22 +130,22 @@ func cbPAMConv(s C.int, msg *C.char, c C.uintptr_t) (*C.char, C.int) { // //nolint:errname type Transaction struct { - handle *C.pam_handle_t - conv *C.struct_pam_conv - status C.int - c cgo.Handle + handle *C.pam_handle_t + conv *C.struct_pam_conv + 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) + C.pam_end(t.handle, C.int(t.lastStatus.Load())) t.c.Delete() } // Allows to call pam functions managing return status func (t *Transaction) handlePamStatus(cStatus C.int) error { - t.status = cStatus + t.lastStatus.Store(int32(cStatus)) if cStatus != success { return t } @@ -212,13 +213,13 @@ func start(service, user string, handler ConversationHandler, confDir string) (* err = t.handlePamStatus(C.pam_start_confdir(s, u, t.conv, c, &t.handle)) } if err != nil { - return nil, errors.Join(Error(t.status), err) + return nil, errors.Join(Error(t.lastStatus.Load()), err) } return t, nil } func (t *Transaction) Error() string { - return Error(t.status).Error() + return Error(t.lastStatus.Load()).Error() } // Item is a an PAM information type. @@ -363,8 +364,10 @@ func (t *Transaction) GetEnvList() (map[string]string, error) { env := make(map[string]string) p := C.pam_getenvlist(t.handle) if p == nil { - return nil, t.handlePamStatus(C.int(ErrBuf)) + t.lastStatus.Store(int32(ErrBuf)) + return nil, t } + t.lastStatus.Store(success) for q := p; *q != nil; q = next(q) { chunks := strings.SplitN(C.GoString(*q), "=", 2) if len(chunks) == 2 { -- cgit v1.2.3 From adffdfbbdc356d1ff3a8e3b011a085eed0402062 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 11 Oct 2023 12:16:59 +0200 Subject: transaction: Never return Transaction as error While transaction does implement error, it's not a valid error implementer because it may have bogous values since it's not thread-safe and so we may read the result of Error() when it's into an invalid state As per this never return it as an error, while always return the Status unless when not available, where we still return pam.Error. --- transaction.go | 9 ++++----- transaction_test.go | 32 ++++++++++++++++++-------------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/transaction.go b/transaction.go index 7b19f5c..5957071 100644 --- a/transaction.go +++ b/transaction.go @@ -22,7 +22,6 @@ package pam import "C" import ( - "errors" "fmt" "runtime" "runtime/cgo" @@ -146,8 +145,8 @@ func transactionFinalizer(t *Transaction) { // Allows to call pam functions managing return status func (t *Transaction) handlePamStatus(cStatus C.int) error { t.lastStatus.Store(int32(cStatus)) - if cStatus != success { - return t + if status := Error(cStatus); status != success { + return status } return nil } @@ -213,7 +212,7 @@ func start(service, user string, handler ConversationHandler, confDir string) (* err = t.handlePamStatus(C.pam_start_confdir(s, u, t.conv, c, &t.handle)) } if err != nil { - return nil, errors.Join(Error(t.lastStatus.Load()), err) + return nil, err } return t, nil } @@ -365,7 +364,7 @@ func (t *Transaction) GetEnvList() (map[string]string, error) { p := C.pam_getenvlist(t.handle) if p == nil { t.lastStatus.Store(int32(ErrBuf)) - return nil, t + return nil, ErrBuf } t.lastStatus.Store(success) for q := p; *q != nil; q = next(q) { diff --git a/transaction_test.go b/transaction_test.go index 5bc858e..62b4ce1 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -167,8 +167,8 @@ func TestPAM_007(t *testing.T) { if len(s) == 0 { t.Fatalf("error #expected an error message") } - if tx.Error() != ErrAuth.Error() { - t.Fatalf("error #unexpected status %v", tx.Error()) + if !errors.Is(err, ErrAuth) { + t.Fatalf("error #unexpected error %v", err) } } @@ -255,8 +255,8 @@ func TestPAM_ConfDir_Deny(t *testing.T) { if len(s) == 0 { t.Fatalf("error #expected an error message") } - if tx.Error() != ErrAuth.Error() { - t.Fatalf("error #unexpected status %v", tx.Error()) + if !errors.Is(err, ErrAuth) { + t.Fatalf("error #unexpected error %v", err) } } @@ -304,8 +304,8 @@ func TestPAM_ConfDir_WrongUserName(t *testing.T) { if len(s) == 0 { t.Fatalf("error #expected an error message") } - if tx.Error() != ErrAuth.Error() { - t.Fatalf("error #unexpected status %v", tx.Error()) + if !errors.Is(err, ErrAuth) { + t.Fatalf("error #unexpected error %v", err) } } @@ -416,7 +416,7 @@ func Test_Error(t *testing.T) { } statuses := map[string]error{ - "success": Error(success), + "success": nil, "open_err": ErrOpen, "symbol_err": ErrSymbol, "service_err": ErrService, @@ -441,7 +441,7 @@ func Test_Error(t *testing.T) { "authtok_lock_busy": ErrAuthtokLockBusy, "authtok_disable_aging": ErrAuthtokDisableAging, "try_again": ErrTryAgain, - "ignore": Error(success), /* Ignore can't be returned */ + "ignore": nil, /* Ignore can't be returned */ "abort": ErrAbort, "authtok_expired": ErrAuthtokExpired, "module_unknown": ErrModuleUnknown, @@ -504,13 +504,17 @@ func Test_Error(t *testing.T) { err = tx.OpenSession(0) } - if tx.Error() != expected.Error() { - t.Fatalf("error #unexpected status %v", tx.Error()) + if !errors.Is(err, expected) { + t.Fatalf("error #unexpected status %#v vs %#v", err, + expected) } - if tx.Error() == Error(success).Error() && err != nil { - t.Fatalf("error #unexpected: %v", err) - } else if tx.Error() != Error(success).Error() && err == nil { - t.Fatalf("error #expected an error message") + + 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()) + } } }) } -- cgit v1.2.3 From 71620046687a14acad2ed49317d3003ac1fe84af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 11 Oct 2023 12:21:35 +0200 Subject: transaction: Do not make Transaction to implement error interface anymore As per previous commit, Transaction can't be used anymore as an error value, but we instead we always return the status code. --- transaction.go | 6 ------ transaction_test.go | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/transaction.go b/transaction.go index 5957071..2809614 100644 --- a/transaction.go +++ b/transaction.go @@ -126,8 +126,6 @@ func cbPAMConv(s C.int, msg *C.char, c C.uintptr_t) (*C.char, C.int) { } // Transaction is the application's handle for a PAM transaction. -// -//nolint:errname type Transaction struct { handle *C.pam_handle_t conv *C.struct_pam_conv @@ -217,10 +215,6 @@ func start(service, user string, handler ConversationHandler, confDir string) (* return t, nil } -func (t *Transaction) Error() string { - return Error(t.lastStatus.Load()).Error() -} - // Item is a an PAM information type. type Item int diff --git a/transaction_test.go b/transaction_test.go index 62b4ce1..0e3a481 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -512,7 +512,7 @@ func Test_Error(t *testing.T) { 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(), + t.Fatalf("error #unexpected status %#v vs %#v", err.Error(), status.Error()) } } -- cgit v1.2.3 From c635cfc38a6067d5902215db16c417801e2fe993 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 11 Oct 2023 22:36:28 +0200 Subject: transaction: Add End() method and Remove Transaction finalizer A PAM transaction needs to be ended in order to release the associated resources, however this can't be sadly automated as the go finalizers run in goroutines and this could cause problems to modules that we load. In fact a module code may be called back during pam_end (to cleanup data for example) and the module code could not be thread safe. So let's make this more manual, but safer. The transaction status is still preserved in the transaction so end will be automatically called with the last-known status. Closes: #14 --- transaction.go | 36 ++++++++++++++++++++++++++++-------- transaction_test.go | 49 +++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 10 deletions(-) diff --git a/transaction.go b/transaction.go index 2809614..52ad1be 100644 --- a/transaction.go +++ b/transaction.go @@ -23,7 +23,6 @@ import "C" import ( "fmt" - "runtime" "runtime/cgo" "strings" "sync/atomic" @@ -133,11 +132,17 @@ type Transaction struct { c cgo.Handle } -// transactionFinalizer cleans up the PAM handle and deletes the callback -// function. -func transactionFinalizer(t *Transaction) { - C.pam_end(t.handle, C.int(t.lastStatus.Load())) - 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 @@ -154,11 +159,19 @@ func (t *Transaction) handlePamStatus(cStatus C.int) 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 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)) } @@ -170,6 +183,13 @@ 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, fmt.Errorf( @@ -193,7 +213,6 @@ func start(service, user string, handler ConversationHandler, confDir string) (* 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 @@ -210,6 +229,7 @@ func start(service, user string, handler ConversationHandler, confDir string) (* err = t.handlePamStatus(C.pam_start_confdir(s, u, t.conv, c, &t.handle)) } if err != nil { + var _ = t.End() return nil, err } return t, nil diff --git a/transaction_test.go b/transaction_test.go index 0e3a481..2f620a2 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -9,6 +9,18 @@ import ( "testing" ) +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 TestPAM_001(t *testing.T) { u, _ := user.Current() if u.Uid != "0" { @@ -18,6 +30,7 @@ func TestPAM_001(t *testing.T) { tx, err := StartFunc("", "test", func(s Style, msg string) (string, error) { return p, nil }) + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -49,6 +62,7 @@ func TestPAM_002(t *testing.T) { } return "", errors.New("unexpected") }) + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -83,6 +97,7 @@ func TestPAM_003(t *testing.T) { Password: "secret", } tx, err := Start("", "", c) + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -101,6 +116,7 @@ func TestPAM_004(t *testing.T) { Password: "secret", } tx, err := Start("", "test", c) + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -118,6 +134,7 @@ func TestPAM_005(t *testing.T) { tx, err := StartFunc("passwd", "test", func(s Style, msg string) (string, error) { return "secret", nil }) + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -135,6 +152,7 @@ func TestPAM_006(t *testing.T) { tx, err := StartFunc("passwd", u.Username, func(s Style, msg string) (string, error) { return "secret", nil }) + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -156,6 +174,7 @@ 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") }) + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -179,6 +198,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) @@ -200,10 +224,13 @@ func TestPAM_ConfDir_FailNoServiceOrUnsupported(t *testing.T) { 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") @@ -229,6 +256,7 @@ func TestPAM_ConfDir_InfoMessage(t *testing.T) { } return "", errors.New("unexpected") }), "test-services") + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -244,6 +272,7 @@ func TestPAM_ConfDir_InfoMessage(t *testing.T) { func TestPAM_ConfDir_Deny(t *testing.T) { u, _ := user.Current() tx, err := StartConfDir("deny-service", u.Username, Credentials{}, "test-services") + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -267,6 +296,7 @@ func TestPAM_ConfDir_PromptForUserName(t *testing.T) { Password: "wrongsecret", } tx, err := StartConfDir("succeed-if-user-test", "", c, "test-services") + 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) @@ -289,6 +319,7 @@ func TestPAM_ConfDir_WrongUserName(t *testing.T) { Password: "wrongsecret", } tx, err := StartConfDir("succeed-if-user-test", "", c, "test-services") + 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) @@ -310,9 +341,13 @@ func TestPAM_ConfDir_WrongUserName(t *testing.T) { } 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 }) + defer maybeEndTransaction(t, tx) + if err != nil { + t.Fatalf("start #error: %v", err) + } s, err := tx.GetItem(Service) if err != nil { @@ -347,6 +382,7 @@ func TestEnv(t *testing.T) { tx, err := StartFunc("", "", func(s Style, msg string) (string, error) { return "", nil }) + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -489,6 +525,7 @@ func Test_Error(t *testing.T) { } tx, err := StartConfDir(serviceName, "user", c, servicePath) + defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) } @@ -592,3 +629,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) + } +} -- cgit v1.2.3 From c7ecbf20dcee2221ba50c5b07144f3684121e886 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 11 Oct 2023 23:09:04 +0200 Subject: transaction: Add a test finalizer checking if transaction has ended Check if a transaction is ended in in tests. --- transaction.go | 3 ++- transaction_test.go | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/transaction.go b/transaction.go index 52ad1be..148e786 100644 --- a/transaction.go +++ b/transaction.go @@ -173,7 +173,7 @@ func Start(service, user string, handler ConversationHandler) (*Transaction, err // 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 @@ -212,6 +212,7 @@ func start(service, user string, handler ConversationHandler, confDir string) (* conv: &C.struct_pam_conv{}, c: cgo.NewHandle(handler), } + C.init_pam_conv(t.conv, C.uintptr_t(t.c)) s := C.CString(service) defer C.free(unsafe.Pointer(s)) diff --git a/transaction_test.go b/transaction_test.go index 2f620a2..e5da07b 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -6,7 +6,11 @@ import ( "os" "os/user" "path/filepath" + "runtime" + "sync/atomic" "testing" + "time" + "unsafe" ) func maybeEndTransaction(t *testing.T, tx *Transaction) { @@ -21,6 +25,19 @@ func maybeEndTransaction(t *testing.T, tx *Transaction) { } } +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" { @@ -30,6 +47,7 @@ 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) @@ -62,6 +80,7 @@ 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) @@ -97,6 +116,7 @@ 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) @@ -116,6 +136,7 @@ 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) @@ -134,6 +155,7 @@ 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) @@ -152,6 +174,7 @@ 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) @@ -174,6 +197,7 @@ 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) @@ -256,6 +280,7 @@ 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) @@ -272,6 +297,7 @@ func TestPAM_ConfDir_InfoMessage(t *testing.T) { func TestPAM_ConfDir_Deny(t *testing.T) { 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) @@ -296,6 +322,7 @@ 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 { @@ -319,6 +346,7 @@ 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 { @@ -344,6 +372,7 @@ func TestItem(t *testing.T) { 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) @@ -382,6 +411,7 @@ 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) @@ -525,6 +555,7 @@ func Test_Error(t *testing.T) { } tx, err := StartConfDir(serviceName, "user", c, servicePath) + ensureTransactionEnds(t, tx) defer maybeEndTransaction(t, tx) if err != nil { t.Fatalf("start #error: %v", err) @@ -558,6 +589,25 @@ func Test_Error(t *testing.T) { } } +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() -- cgit v1.2.3 From fe75bbaeee4f29f460fdd7ca795584298e685b80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 10 Oct 2023 11:45:12 +0200 Subject: transaction: Mark Item, Flags and Style const values as Item, Flags and Style types We redefined various PAM constant values for items, flags and style, but only few of them were marked as being Item's or Flag's. This caused go to just consider them as generic integers instead of the actual subtype. --- transaction.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/transaction.go b/transaction.go index 148e786..7f8c2d1 100644 --- a/transaction.go +++ b/transaction.go @@ -42,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 @@ -244,19 +244,19 @@ 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 ) // SetItem sets a PAM information item. @@ -287,21 +287,21 @@ 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 + EstablishCred Flags = C.PAM_ESTABLISH_CRED // DeleteCred inidicates that credentials should be deleted. - DeleteCred = C.PAM_DELETE_CRED + 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. -- cgit v1.2.3 From 31a452ad25b91f3cbdaf33273a40d8693056700b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Wed, 25 Oct 2023 21:03:57 +0200 Subject: transaction: Add missing default PAM item types --- transaction.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/transaction.go b/transaction.go index 7f8c2d1..1bb5229 100644 --- a/transaction.go +++ b/transaction.go @@ -257,6 +257,14 @@ const ( Ruser Item = C.PAM_RUSER // UserPrompt is the string use to prompt for a username. 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. -- cgit v1.2.3 From 01f62f11f7aa7a84b89287737822f2458a5adf40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 22 Sep 2023 14:56:36 +0200 Subject: transaction_test: Add tests checking the loaded services match --- transaction_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/transaction_test.go b/transaction_test.go index e5da07b..b5b807e 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -160,6 +160,13 @@ func TestPAM_005(t *testing.T) { 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) @@ -234,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) } @@ -285,6 +299,13 @@ func TestPAM_ConfDir_InfoMessage(t *testing.T) { 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) @@ -302,6 +323,13 @@ func TestPAM_ConfDir_Deny(t *testing.T) { 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") -- cgit v1.2.3 From e6f817312ad26cbf96c9cd7185e3d7c37e8e2430 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Fri, 29 Sep 2023 20:28:22 +0200 Subject: transaction: Skip some tests requiring confdir if not available --- transaction_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/transaction_test.go b/transaction_test.go index b5b807e..d358b0e 100644 --- a/transaction_test.go +++ b/transaction_test.go @@ -258,6 +258,9 @@ 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", @@ -316,6 +319,9 @@ 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) -- cgit v1.2.3 From 067f634acb780246805728d068f2bfcedcc4a4ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 20 Nov 2023 21:15:43 +0100 Subject: transaction: Fix comment typo --- transaction.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transaction.go b/transaction.go index 1bb5229..9fee575 100644 --- a/transaction.go +++ b/transaction.go @@ -299,7 +299,7 @@ const ( // EstablishCred indicates that credentials should be established // for the user. EstablishCred Flags = C.PAM_ESTABLISH_CRED - // DeleteCred inidicates that credentials should be deleted. + // DeleteCred indicates that credentials should be deleted. DeleteCred Flags = C.PAM_DELETE_CRED // ReinitializeCred indicates that credentials should be fully // reinitialized. -- cgit v1.2.3