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

pam/gdm: Keep debugging all the events, sanitizing them if needed #565

Merged
merged 3 commits into from
Oct 3, 2024

Conversation

3v1n0
Copy link
Collaborator

@3v1n0 3v1n0 commented Oct 2, 2024

Instead of hiding the content authentication data completely, show the
whole challenge when pam_gdm_debug build tag is used, or when in
testing mode.

Otherwise, just show a sanitized challenge so that we don't miss the
fact that the event has happened.

In the same way, keep the conversation debugging alive so that we don't miss pieces of what we got from GDM if the content is relevant.

To sanitize the JSON I preferred to go wild and just replace the content, since it's still something happening only when debugging only, so not really something in production code.

Fixes: ca47562, 40fa85d

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

codecov-commenter commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 28 lines in your changes missing coverage. Please review.

Project coverage is 84.19%. Comparing base (0354d40) to head (c608853).

Files with missing lines Patch % Lines
pam/internal/gdm/protocol.go 0.00% 23 Missing ⚠️
pam/internal/adapter/gdmmodel.go 0.00% 2 Missing and 1 partial ⚠️
pam/internal/gdm/conversation.go 66.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
- Coverage   84.43%   84.19%   -0.24%     
==========================================
  Files          79       79              
  Lines        7040     7066      +26     
  Branches       75       75              
==========================================
+ Hits         5944     5949       +5     
- Misses        762      785      +23     
+ Partials      334      332       -2     

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

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.

Looks good, just a small nitpick 🙃

pam/internal/gdm/protocol.go Outdated Show resolved Hide resolved
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! Feel free to rebase and merge once CI passes.

…debug mode

Instead of hiding the content authentication data completely, show the
whole challenge when `pam_gdm_debug` build tag is used, or when in
testing mode.

Otherwise, just show a sanitized challenge so that we don't miss the
fact that the event has happened

Fixes: ca47562
In case the gdm data poll response has content, it's still something we
want to show, even though we should sanitize the value when it contains
the challenge secret

Fixes: 40fa85d
@3v1n0 3v1n0 merged commit b252fc7 into ubuntu:main Oct 3, 2024
8 checks passed
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