pam icon indicating copy to clipboard operation
pam copied to clipboard

Add PAM module stub generator, modules implmentations and integration tests

Open 3v1n0 opened this issue 2 years ago • 2 comments

For the way go-pam is structured it made easy to create pam applications, but most of the code can be useful also for implementing modules in go.

As per this:

  • Define ModuleTransaction struct that is similar to Transaction, but with less methods
  • Add a pam-moduler (github.com/msteinert/pam/cmd/pam-moduler) to generate pam-module cgo stubs
  • Add a go (simpler) implementation of pam_debug.so and test it
  • Add an interaction testing module that uses an unix socket to dialog with a loader and that can be instructed to perform any operation on module side
  • Add module methods to set/get module data, and to initiate text-based and binary conversations
  • Test all these cases via integration tests (the only way to do that, since we need a pam-module-side handler).

Keeping this as a draft for now as it requires some refactors that were part of #12, but also it may be adapted a bit if we want to change Transaction errors to be less prone to potential races.

3v1n0 avatar Oct 05 '23 04:10 3v1n0

This is now rebased on #15 instead as it makes few things better to handle.

3v1n0 avatar Oct 12 '23 01:10 3v1n0

Codecov Report

Attention: Patch coverage is 79.86705% with 212 lines in your changes are missing coverage. Please review.

Project coverage is 80.72%. Comparing base (e707495) to head (f199038).

:exclamation: Current head f199038 differs from pull request most recent head e0753f6. Consider uploading reports for the commit e0753f6 to get more accurate results

Files Patch % Lines
...r/tests/integration-tester-module/communication.go 58.19% 36 Missing and 15 partials :warning:
...gration-tester-module/integration-tester-module.go 64.81% 33 Missing and 5 partials :warning:
cmd/pam-moduler/tests/internal/utils/test-utils.go 72.64% 25 Missing and 7 partials :warning:
cmd/pam-moduler/tests/internal/utils/test-setup.go 69.86% 16 Missing and 6 partials :warning:
app-transaction.go 83.87% 17 Missing and 3 partials :warning:
module-transaction-mock.go 85.82% 13 Missing and 6 partials :warning:
module-transaction.go 93.85% 16 Missing and 2 partials :warning:
cmd/pam-moduler/tests/debug-module/debug-module.go 75.00% 7 Missing and 1 partial :warning:
...r/tests/integration-tester-module/serialization.go 81.81% 4 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
- Coverage   82.31%   80.72%   -1.59%     
==========================================
  Files           2       13      +11     
  Lines         147     1100     +953     
==========================================
+ Hits          121      888     +767     
- Misses         23      167     +144     
- Partials        3       45      +42     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Nov 30 '23 04:11 codecov-commenter

Any progress on this? Looks very promising.

StephenBrown2 avatar Mar 24 '25 14:03 StephenBrown2

@StephenBrown2 I think we can basically merge this since it's something we've been using actively for some time in https://github.com/ubuntu/authd

However, there are also problems when the modules are strictly done in go, since the go runtime may break other imported modules (dead-locking in SSH as per its specific thread implementation), so it's probably safer to have a different approach:

  1. A dbus transaction, that implements the interface defined here
  2. A "simple" thin C module that uses it by using go module as program.

3v1n0 avatar May 06 '25 17:05 3v1n0

Agreed, I've experience the dead-locking in SSH first-hand with my own PAM module implementation, and have found several work-arounds for that, including in pam_fscrypt, by comparing the PID value between the Auth and session Open calls, gravitational-teleport invokes runtime.LockOSThread() and pam-ussh calls runtime.GOMAXPROCS(1).

I wanted to see if I could write a PAM module that communicates with another local service via gRPC, and it looks like authd does just that. Do you do that via DBUS or the C wrapper during SSH authentication due to the forking issue?

StephenBrown2 avatar May 07 '25 17:05 StephenBrown2

and have found several work-arounds for that, including in pam_fscrypt, by comparing the PID value between the Auth and session Open calls, gravitational-teleport invokes runtime.LockOSThread() and pam-ussh calls runtime.GOMAXPROCS(1)

Yeah, but google one is just avoiding the calls and it's really not a solution (and still may be affected), LockOSThread didn't really help us, same with process limitation as we still may end up being affected on re-entries (the same module may be "re-invoked" multiple times during the same transaction.

So, eventually we decided that it was too risky to put unknown go runtime behavior in the PAM applications (since it was not reliable and too versions-dependent), and just go simple with a C wrapper that:

  • Creates a local dbus server exposing the PAM APIs
  • Invokes a PAM application that is written in Go and follows interface of this very same PR but that uses a dbus implementation of the pam.ModuleTransaction defined in this PR

All the rest of the gRPC communication happens in go though, the C side only invokes actions in the go binary.

So we can have in the same code both a go-library (that we use in safe cases such as GDM) and a PAM-module client application that is invoked through the module.

We used DBus because it was the easier way to handle this from the C side (using GLib's Gio), given that we also want to support binary conversations, but in general it could have been using simple fd's or something like varlink.

The C-module is simple and generic enough that can be used by anyone else though. In fact I was considering including it in this repo too, although it's a bit complex to expose it well in a go world.

Thus the code of this PR this matters, since all the abstraction that is here can be used in both ways.

See also:

  • https://github.com/ubuntu/authd/blob/main/pam/Hacking.md
  • https://github.com/ubuntu/authd/commit/7841af2f503593657c99908446613d73aa59a327

3v1n0 avatar May 07 '25 18:05 3v1n0

Thanks for the detailed description, that's a lot to think about. Sorry for slightly going off topic for this PR. Best wishes on getting it merged!

StephenBrown2 avatar May 07 '25 19:05 StephenBrown2