Skip to content
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

[RFC] PAM: Build GDM support only on the library #566

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Oct 2, 2024

There's quite a bit of code that is GDM related that we don't really need to include into the compiled PAM binary that we use for authentication in non-GDM cases.

We don't really use all the gdm-related code in the PAM Exec child that
we use in normal PAM transactions, so no need to compile it at all by
default while we need it in the PAM library that GDM will consume.

This is basically the opposite of #292, as that tried to unify the logic while here we assumed that we're going to ship two different implementations, and so reducing the logic in one side when is not needed.

@3v1n0 3v1n0 requested a review from a team as a code owner October 2, 2024 22:17
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.11%. Comparing base (87a34c4) to head (d219a3e).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
pam/internal/adapter/gdmmodel.go 71.42% 2 Missing ⚠️
pam/internal/adapter/gdmmodel_disabled.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #566      +/-   ##
==========================================
+ Coverage   83.77%   84.11%   +0.34%     
==========================================
  Files          80       82       +2     
  Lines        7098     7077      -21     
  Branches       75       75              
==========================================
+ Hits         5946     5953       +7     
+ Misses        820      791      -29     
- Partials      332      333       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@3v1n0 3v1n0 force-pushed the module-gdm-split branch 4 times, most recently from 521bae5 to 6702205 Compare October 2, 2024 23:24
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

This will allow us to make it optional in next steps
We don't really use all the gdm-related code in the PAM Exec child that
we use in normal PAM transactions, so no need to compile it at all by
default while we need it in the PAM library that GDM will consume.
@3v1n0
Copy link
Collaborator Author

3v1n0 commented Oct 7, 2024

I've made the integration tests to require the gdm tag now and added to debian side too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants