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 3 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.

This will allow us to make it optional in next steps
@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.53%. Comparing base (e304624) to head (6702205).

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   84.53%   84.53%   -0.01%     
==========================================
  Files          79       82       +3     
  Lines        7036     7048      +12     
  Branches       75       75              
==========================================
+ Hits         5948     5958      +10     
- Misses        759      761       +2     
  Partials      329      329              

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

@3v1n0 3v1n0 force-pushed the module-gdm-split branch 3 times, most recently from 18557f3 to 521bae5 Compare October 2, 2024 22:49
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.
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!

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