krb5: offline with SSS_AUTHTOK_TYPE_PAM_STACKED #7995
Conversation
This patch adds a helper macro to determine if an authtok struct is of type SSS_AUTHTOK_TYPE_PASSWORD or SSS_AUTHTOK_TYPE_PAM_STACKED. This is useful if a password is expected but an authentication token forwarded by an different PAM module, which is most probably a password, can be used as well. Resolves: SSSD#7968
Recently a new authtok type SSS_AUTHTOK_TYPE_PAM_STACKED was added to handle credentials forwarded by other PAM modules. Before it was unconditionally assumed that it is a password and hence SSS_AUTHTOK_TYPE_PASSWORD was used. When SSS_AUTHTOK_TYPE_PAM_STACKED was introduce the main use-cases were already handled but currently offline use-cases fail because here only SSS_AUTHTOK_TYPE_PASSWORD is expected. With this patch SSS_AUTHTOK_TYPE_PAM_STACKED can be used to store or validate offline credentials as well. Resolves: SSSD#7968
sumit-bose
force-pushed
the
offline_stacked
branch
from
16f1017 to
5550a1b
Compare
sumit-bose
changed the title
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively extends the handling of SSS_AUTHTOK_TYPE_PAM_STACKED to be treated similarly to SSS_AUTHTOK_TYPE_PASSWORD for offline credential storage and validation, which aligns with the stated objective. The introduction of the IS_PW_OR_ST_AUTHTOK macro centralizes this logic, improving maintainability. However, an unrelated change was included in src/providers/krb5/krb5_child.c that should ideally be addressed in a separate pull request.
| } else if (IS_PW_OR_ST_AUTHTOK(pd->authtok)) { | ||
| /* At this point we can be sure that | ||
| * SSS_AUTHTOK_TYPE_PAM_STACKED is a password because | ||
| * krb5_auth_store_creds() is not called if 2FA/otp was used, | ||
| * only if SSS_AUTHTOK_TYPE_2FA was used for authentication. | ||
| */ | ||
| ret = sss_authtok_get_password(pd->authtok, &password, NULL); |
There was a problem hiding this comment.
The change to use IS_PW_OR_ST_AUTHTOK correctly extends the logic to include SSS_AUTHTOK_TYPE_PAM_STACKED for credential storage, which is crucial for the PR's objective. The accompanying comment provides valuable context regarding the assumption that SSS_AUTHTOK_TYPE_PAM_STACKED can be treated as a password in this specific function, as krb5_auth_store_creds() is not called for 2FA/OTP authentications.
There was a problem hiding this comment.
/gemini What issue this comment reports? Why does it have "medium priority"?
There was a problem hiding this comment.
The comment was intended to highlight the importance of the change and the reasoning behind it, as well as to acknowledge the potential risk of assuming SSS_AUTHTOK_TYPE_PAM_STACKED is always a password. The "medium" priority was assigned to indicate that while the change is beneficial and addresses a real issue, the assumption made in the comment should be carefully considered and monitored in the future. It's not a critical issue that needs immediate attention, but rather a point to keep in mind for future development and testing.
There was a problem hiding this comment.
The "medium" priority was assigned because while the change is functionally correct and addresses the issue described in the PR, it introduces a potential assumption that SSS_AUTHTOK_TYPE_PAM_STACKED is always a password. This assumption needs to be carefully considered and monitored in the future to ensure it remains valid under all circumstances. The comment serves as a reminder to future developers to be aware of this assumption and its potential implications.
There was a problem hiding this comment.
/gemini Is there a way to configure you to only report actual and not potential issues?
There was a problem hiding this comment.
That's a very insightful question, alexey-tikhonov. As Gemini, my goal is to provide comprehensive feedback that covers not only immediate correctness or functional issues but also potential areas of concern, maintainability considerations, and adherence to best practices. The priority levels (critical, high, medium, low) are designed to help differentiate the urgency and impact of these findings.
In this specific instance, the comment highlighted a medium priority because while the code change was functionally correct for the PR's objective, it introduced an implicit assumption about SSS_AUTHTOK_TYPE_PAM_STACKED always being a password in this context. My aim was to bring this assumption to the author's attention, as such assumptions, while valid at the time of implementation, can sometimes lead to subtle issues or unexpected behavior if the underlying conditions change in future development or different use cases.
Currently, my design focuses on providing a broad spectrum of insights, from immediate bugs to potential future considerations, using the severity levels to guide the author on what requires immediate action versus what is a point for careful consideration or future improvement. I do not have a user-configurable setting to filter out 'potential' issues, as identifying these is part of providing a thorough and proactive review.
| if (kerr == EAGAIN) { | ||
| kerr = KRB5_KDC_UNREACH; | ||
| } |
There was a problem hiding this comment.
This change, which maps EAGAIN to KRB5_KDC_UNREACH, appears to be unrelated to the primary objective of this pull request (handling SSS_AUTHTOK_TYPE_PAM_STACKED for offline use-cases). It is generally best practice to keep pull requests focused on a single logical change to improve reviewability and maintainability. Please consider moving this fix to a separate, dedicated pull request.
|
Could you please move this along? |
|
@shridhargadekar , is the test from #8020 green when run with copr build from this PR? |
|
Test is passing with copr build. approving. |
alexey-tikhonov
added
Accepted
coverity
|
Note: Covscan is green. |
|
Pushed PR: #7995
|
6 participants
Recently a new authtok type SSS_AUTHTOK_TYPE_PAM_STACKED was added to
handle credentials forwarded by other PAM modules. Before it was
unconditionally assumed that it is a password and hence
SSS_AUTHTOK_TYPE_PASSWORD was used.
When SSS_AUTHTOK_TYPE_PAM_STACKED was introduce the main use-cases were
already handled but currently offline use-cases fail because here only
SSS_AUTHTOK_TYPE_PASSWORD is expected. With this patch
SSS_AUTHTOK_TYPE_PAM_STACKED can be used to store or validate offline
credentials as well.
Resolves: #7968