feat: infer krb5 ccache from within the PAM module #914
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
GabrielNagy
force-pushed
the
pam-module-infer-krb5cc
branch
from
72205d1 to
644496e
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #914 +/- ##
==========================================
- Coverage 87.65% 87.63% -0.02%
==========================================
Files 76 76
Lines 6713 6755 +42
==========================================
+ Hits 5884 5920 +36
- Misses 492 495 +3
- Partials 337 340 +3 ☔ View full report in Codecov by Sentry. |
GabrielNagy
force-pushed
the
pam-module-infer-krb5cc
branch
from
644496e to
88ba693
Compare
This is actually covered as part of the e2e test but there's no easy way to make it reflect in the coverage report 😕. |
didrocks
requested changes
Feb 16, 2024
There was a problem hiding this comment.
The C part is perfect. I have a few suggestions and also some others on testing that may not be possible but still worth it IMHO.
This is really great to see, we now have a fallback, while still keeping the existing behaviour without the settings turned on. Well done! :)
GabrielNagy
force-pushed
the
pam-module-infer-krb5cc
branch
2 times, most recently
from
5200afc to
ff397e8
Compare
GabrielNagy
force-pushed
the
pam-module-infer-krb5cc
branch
from
ff397e8 to
f19452f
Compare
didrocks
requested changes
Feb 21, 2024
didrocks
approved these changes
Feb 21, 2024
This is because cloud-init creates overrides here which happen during the first run.
In our PAM module we don't want to block authentication if the path reported by libkrb5 does not exist. Rather than checking for the primitive fs.ErrNotExist in the caller, specialize the error in our own API.
Add a debug command under adsysctl policy responsible for returning the default ccache path for the given user. This command is intended to be used by the PAM module, thus we are specializing its edge cases such as: - configuration option not set: we do not error so PAM can proceed with the previous functionality (return PAM_IGNORE) - ccache reported by libkrb5 not found on disk: we do not error since user can be a non-domain user Other scenarios that can raise errors will be treated as such, and auth will be denied.
Leverage the previously created CLI command from within the PAM module to set the KRB5CCNAME variable if found. Because all checks are handled by the CLI command, we only need to read its output (if present) and propagate any non-zero exit codes.
As this is a scenario difficult to test at package or integration level, create an e2e test for it. In broad terms this does the following: - perform pubkey authentication to bypass pam_sss, ensuring KRB5CCNAME is not set - manually initialize a ccache in the default path using kinit - with the detect_cached_setting enabled, ensure KRB5CCNAME is set on subsequent logins - ensure login still works after removing the ccache
This has changed since we migrated our documentation to readthedocs.
We can cover the most common code branches in integration tests, with the exception of when the command: - fails to get current user - fails to call Atoi on the UID - fails to call Setuid (this is achievable if tests do not run as root but I don't think we should complicate the setup too much)
GabrielNagy
force-pushed
the
pam-module-infer-krb5cc
branch
from
29ccfe4 to
2e43d72
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #903 where the functionality to get the default ticket path from
libkrb5was added.The simplest and safest way to use the functionality above fom the PAM module is to delegate most of the responsibility to a CLI command written in Go, that can return the ticket path of any given user. Because the PAM module runs as root, we need to de-escalate to the given user (using
setuidsince libkrb5 treatssetuidandseteuidseparately, so the latter is not enough).The CLI command is specialized for usage from within the PAM module to minimize friction. It reads the configuration file to see if the
detect_cached_ticketsetting is enabled, and avoids returning an error if the ticket path is not present on disk. Thus the PAM module can just execute the command, read its output and silently return if the subprocess exited with 0 and no output was printed.We block authentication if the subprocess returns an error, or if any buffer-allocation or system calls needed to run and read the output are failing.
On top of this being a fully backwards-compatible change on the account of the opt-in setting, the impact of not having a ticket on disk is minimized since adsys will then treat the user as a non-domain user. There is the risk of the ticket cache on disk being outdated, which will end up failing the
adsysctl updateoperation that happens on login, but ensuring the validity of the existing ticket path pointed to bylibkrb5is the responsibility of the process that generates/renews the ticket, not adsys.Fixes UDENG-2191