pam: Use pam.ModuleTransaction to implement pam module by 3v1n0 · Pull Request #89 · canonical/authd

3v1n0 · 2023-10-14T05:09:47Z

Re-implement the PAM module bootstrap code by using pam-go and in particular the code proposed at msteinert/pam#13

That allows us to have a more tested and testable base. Also it will be a prerequisite for the gdm implementation and a pure-PAM protocol implementation.

The module can now generated by just using go generate -C pam that would generate in two steps both the go-glue code and the library itself.

See single commits for further details.

UDENG-1647, UDENG-1604

denisonbarbosa

Hey @3v1n0! I tried to explain as much as possible the reasons behind the changes related to idiomatic Go, so I hope everything is clear. I'm not an expert on PAM behavior, so if I missed or misunderstood something, feel free to let me know in the comments.

I'm still a bit unsure about switching what we already implemented ourselves to an external dependency (as it pulls a lot of weight that I'm not sure we need), but we can discuss it.

pam/pam-module-transaction-dummy.go

pam/pam-module-transaction-dummy_test.go

3v1n0 · 2023-10-17T23:38:49Z

Hey @3v1n0! I tried to explain as much as possible the reasons behind the changes related to idiomatic Go, so I hope everything is clear. I'm not an expert on PAM behavior, so if I missed or misunderstood something, feel free to let me know in the comments.

Thanks, I'm still attached to some C-isms, so happy to learn! :)

I'm still a bit unsure about switching what we already implemented ourselves to an external dependency (as it pulls a lot of weight that I'm not sure we need), but we can discuss it.

So, what we had implemented so far was just a tiny bit of what we actually need to perform everything we care about (some of the wip code is stashed here), but the current code needed still various C implementations and was very hard to test, so I decided that:

Separation of concern, there's a go-pam library that had already various things we needed and a nice abstraction and it was way better to improve things there instead of having everything inside authd
Working in go-pam externelly allowed me to test things fully, so now basically any interaction of go-pam with actual libpam is tested in both ways (i.e. the generator and the modules are creating code on the go and the built libraries loaded via libpam to be tested) in behaviors and memory.
All these abstractions that we now have allow to easily create mocks and testable pam.ModuleTransaction's that we can now re-implement in various way depending on the test we want to do:
- This also includes the possibility (I hope remote) to make this a wrapper to call the libraries in different ways if shared libraries create troubles to PAM.
Avoid touching C as much as we can inside this repo

So basically my goal was to have a library that allowed us to do everything we need, especially in the GDM and pure-PAM (e.g. ssh) modes without having to care how the low-level implementation is done, assuming that is fully tested.

GabrielNagy

Awesome work! My comments are all related to testing :)

pam/pam_test/pam-module-transaction-dummy_test.go

pam/authentication.go

denisonbarbosa

Hey @3v1n0, thanks for addressing the comments and for explaining some others in which I misunderstood the purpose of the changes. I have another request and, even though it's a single comment, it's a big (and also very important) one. If you're unsure how to move on with the table-testing approach, we can talk more about it in the specific comment, MM or HO.

Also, please avoid resolving the comments yourself, if possible. The comments are very useful for the reviewer to navigate through what was requested and to see if they were addressed or not. What we do normally is for the PR author to react with a 👍 on the comment if it was addressed or to discuss the change a bit more on the comment itself before deciding how (and if) to address it. After everything is addressed and discussed, you can re-request the review so the reviewer can take another look at the changes. Here's a short PR as an example: #58.

pam/pam_test/pam-module-transaction-dummy_test.go

denisonbarbosa

Hey @3v1n0, I appreciate you taking the time to address the comments and discuss them. Thanks!
We are not quite there yet, though. I feel like the table tests can be improved quite a bit, as right now they are a bit confusing and hard to read. I wrote more details in the specific comment. Hopefully, it's clearer.

pam/pam_test/module-transaction-dummy_test.go

denisonbarbosa

Hey @3v1n0. Well done! I definitely like the tests a lot more now.
I still think there's more room for improvement though, so I added some final comments and suggestions to improve readability and consistency.

(The nitpicks are also to prepare you for @didrocks review 😂 ...)

pam/pam_test/module-transaction-dummy_test.go

3v1n0 · 2023-11-28T16:07:16Z

Also normally in such cases the way to do is to do is use the Compare link there, to see the changes that happened in between.

The issue with the compare link is that it only compares the heads, which means we won't see any diff before them, that's why going with fixups is better.

You can manually provide the sha from your previous review, but no worries... I'll go with fixups.

pam/return.go

pam/pam_test/module-transaction-dummy_test.go

3v1n0 · 2023-11-29T19:44:41Z

So... I've finally got some upstream reviews on msteinert/pam#15 and so I also updated the branch here, this also implied renaming pam.StatusError -> pam.Error and dropping for good pam.TransactionError and rely on error wrapping everywhere.

Changes are in the fixups and are quite minimal at the end.

didrocks · 2023-11-30T10:47:05Z

So... I've finally got some upstream reviews on msteinert/pam#15 and so I also updated the branch here, this also implied renaming pam.StatusError -> pam.Error and dropping for good pam.TransactionError and rely on error wrapping everywhere.

See, we are not that foreign or make unreasonable requests ;)

denisonbarbosa

Ok, I think we are done now, @3v1n0. Great job! 🚀

If @didrocks has nothing more to say as well, feel free to rebase the branch on the latest changes on main and join the fixups so we can get this merged!

Trusting the other reviews and no time to rereview myself.

Avoid doing all the C manual work inside authd pam, but handle this in a separate library that handles this all and comes with fully tested operations. We could so mock the ModuleTransaction here if we want so that we can make things more testable as they are now. As per this change, the module can be simply be generated with: go generate -C pam

Instead of using our own types for return status, let's just use pam.StatusError to return more complex error values back to the user

We used to store the authentication ID as global value, but pam can handle this natively now, allowing us to store it as module data.

We have a base interface that the pam.ModuleTransaction should implement and so use and pass this interface instead of relying on the actual module transaction implementation, so that we can mock it. As per this introduce a new dummy implementation for it that can be used in local tests

As per this, do not define main() as an actual function when we're generating the module, to avoid adding unwanted code in the library.

PAM code is using CGO quite a lot, so run tests using address sanitizer to catch memory issues and leaks

ASAN in go does not catch memory leaks properly at the end of the test program execution, so force this using a wrapper function that is called when each test is completed.

Current user group membership always returns, with User.Read.All the list of groups, but without their details. To access their details, we need to list all groups that the user has potentially access too. Add a check for it to return a better error for those use cases.

3v1n0 requested a review from a team as a code owner October 14, 2023 05:09

3v1n0 force-pushed the use-pam-moduler branch 4 times, most recently from 87f38eb to 0435964 Compare October 14, 2023 05:31

denisonbarbosa requested changes Oct 17, 2023

View reviewed changes

3v1n0 force-pushed the use-pam-moduler branch 2 times, most recently from e6d13b2 to 0d94233 Compare October 17, 2023 18:23

3v1n0 force-pushed the use-pam-moduler branch from 0d94233 to 45e7096 Compare October 18, 2023 00:33

GabrielNagy reviewed Oct 19, 2023

View reviewed changes

pam/pam_test/pam-module-transaction-dummy_test.go Outdated Show resolved Hide resolved

pam/pam_test/pam-module-transaction-dummy_test.go Outdated Show resolved Hide resolved

pam/authentication.go Show resolved Hide resolved

denisonbarbosa requested changes Oct 19, 2023

View reviewed changes

pam/pam_test/pam-module-transaction-dummy_test.go Outdated Show resolved Hide resolved

3v1n0 force-pushed the use-pam-moduler branch 2 times, most recently from 7775d8d to f95e0b8 Compare October 20, 2023 01:05

3v1n0 requested a review from denisonbarbosa October 20, 2023 01:06

denisonbarbosa requested changes Oct 20, 2023

View reviewed changes

pam/pam_test/module-transaction-dummy_test.go Outdated Show resolved Hide resolved

3v1n0 force-pushed the use-pam-moduler branch 5 times, most recently from b6f3d32 to 4f6ea76 Compare October 25, 2023 23:48

3v1n0 requested a review from denisonbarbosa October 25, 2023 23:49

3v1n0 force-pushed the use-pam-moduler branch 2 times, most recently from eba2215 to 1a24037 Compare October 26, 2023 02:38

denisonbarbosa requested changes Oct 26, 2023

View reviewed changes

3v1n0 force-pushed the use-pam-moduler branch from 1a24037 to 01576be Compare October 26, 2023 16:09

3v1n0 requested a review from denisonbarbosa October 26, 2023 16:09

3v1n0 force-pushed the use-pam-moduler branch 3 times, most recently from 8d48626 to eed3ab5 Compare October 27, 2023 04:05

3v1n0 requested a review from denisonbarbosa November 28, 2023 17:28

denisonbarbosa reviewed Nov 29, 2023

View reviewed changes

pam/return.go Show resolved Hide resolved

3v1n0 requested a review from denisonbarbosa November 29, 2023 14:05

denisonbarbosa reviewed Nov 29, 2023

View reviewed changes

pam/pam_test/module-transaction-dummy_test.go Outdated Show resolved Hide resolved

3v1n0 force-pushed the use-pam-moduler branch from e0de288 to dd6c830 Compare November 29, 2023 19:08

3v1n0 force-pushed the use-pam-moduler branch from 8855148 to 465ac7f Compare November 29, 2023 19:54

denisonbarbosa approved these changes Nov 30, 2023

View reviewed changes

3v1n0 mentioned this pull request Nov 30, 2023

build: add Debian packaging #120

Merged

didrocks removed their request for review November 30, 2023 16:33

3v1n0 added 12 commits November 30, 2023 18:01

pam: Add comments for main package to please CI

0e7469f

pam: Address some linter warnings

1ad064d

pam: Fail if no broker has been found

781de20

log: Add Warning stub definition

9b6d55b

pam: Simplify the return code handling by using pam.StatusError

7dc0c29

Instead of using our own types for return status, let's just use pam.StatusError to return more complex error values back to the user

pam: Send error or info message to PAM if any

17e8585

pam: Save broker authentication ID in the module data

c8749d9

We used to store the authentication ID as global value, but pam can handle this natively now, allowing us to store it as module data.

pam: Use dummy transaction to run PAM module as test cli app

41bbd91

As per this, do not define main() as an actual function when we're generating the module, to avoid adding unwanted code in the library.

CI: Run PAM tests with address sanitizer enabled

33fbd9d

PAM code is using CGO quite a lot, so run tests using address sanitizer to catch memory issues and leaks

pam_tests: Force doing a memory leak check at tests Cleanup

c974f78

ASAN in go does not catch memory leaks properly at the end of the test program execution, so force this using a wrapper function that is called when each test is completed.

3v1n0 force-pushed the use-pam-moduler branch from 7d01515 to c974f78 Compare November 30, 2023 17:01

denisonbarbosa merged commit 9151507 into canonical:main Nov 30, 2023

Conversation

3v1n0 commented Oct 14, 2023 • edited by denisonbarbosa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

denisonbarbosa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

3v1n0 commented Oct 17, 2023

Uh oh!

GabrielNagy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

denisonbarbosa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

denisonbarbosa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

denisonbarbosa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

3v1n0 commented Nov 28, 2023

Uh oh!

Uh oh!

Uh oh!

3v1n0 commented Nov 29, 2023

Uh oh!

didrocks commented Nov 30, 2023

Uh oh!

denisonbarbosa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

3v1n0 commented Oct 14, 2023 •

edited by denisonbarbosa

Loading

denisonbarbosa left a comment •

edited

Loading

denisonbarbosa left a comment •

edited

Loading