-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(pam/gdm): Implement JSON protocol in exec module and use it everywhere #292
base: main
Are you sure you want to change the base?
Conversation
39cd5ed
to
c007c7c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #292 +/- ##
==========================================
+ Coverage 84.48% 85.09% +0.61%
==========================================
Files 72 75 +3
Lines 6115 6201 +86
Branches 75 72 -3
==========================================
+ Hits 5166 5277 +111
+ Misses 662 638 -24
+ Partials 287 286 -1 ☔ View full report in Codecov by Sentry. |
As far as I can see, we can just drop the |
Yeah, exactly... With this branch we could just use the same behavior everywhere, and drop various things... I didn't do it yet since I wasn't sure which was the preferred way, but ideally this is more consistent. Indeed it has some overhead, but from my tests it's not really something noticeable. |
This will be superseded by #293, so I'll move it to draft for now. |
…#293) In order to be able to read and save the module data in the same way we should use the very same pam module. In this case the exec module could be skipped explicitly, but this is happening anyways implicitly since no data for it will be set, so no need to add more complexity. --- This is the simplest alternative to #292 UDENG-2646
We may want to able to decode the value externally too, so let's make this more visible
These are exposed as pam.BinaryconvResponse so no internal bit is actually visible
bb55c39
to
065540f
Compare
Fair enough, I've rebased and merged it with it so in theory this is still good if we want to drop a bunch of code and duplications |
ac84d1b
to
1895a6d
Compare
When a binary conversation is requested from the exec, start a JSON binary conversation with GDM if such protocol is currently supported. Add tests handling the conversation and all the possible corner cases
…ction in such case If we're already about to fail because of a timeout don't cleanup the transaction because we may end up stopping ongoing conversations which will lead to a panic, without clear explaination
In this way we are consistent and we always use the same mode to run the exec module, making possible to share module data in all the instances
It's not used anymore so we can delete it
This is the clean version of using env variables (which are less safe to use) to remember the previously used broker and in general to store the module data consistently.
The good side is that now we can just use everywhere the go-exec + the module and we can drop the code for the wrapper loader (not done here) and not expect different behaviors.
Things works pretty well and are fully tested.
e.g. the alternative to #293.
UDENG-2646