Add pam account authorization check #19040
Merged
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
zeripath
added
type/bug
type/enhancement
noerw
approved these changes
Mar 9, 2022
GiteaBot
added
the
lgtm/need 1
6543
approved these changes
Mar 9, 2022
GiteaBot
added
lgtm/done
6543
pushed a commit
to 6543-forks/gitea
that referenced
this pull request
Mar 10, 2022
https://huntr.dev/bounties/8d221f92-b2b1-4878-bc31-66ff272e5ceb/ Co-authored-by: ysf <34326+ysf@users.noreply.github.com>
|
-> #19047 |
zjjhot
added a commit
to zjjhot/gitea
that referenced
this pull request
Mar 10, 2022
* giteaofficial/main: fix pam authorization (go-gitea#19040) [skip ci] Updated translations via Crowdin Upgrading binding package (go-gitea#19034) Ensure isSSH is set whenever DISABLE_HTTP_GIT is set (go-gitea#19028)
zeripath
added
the
pr/breaking
zeripath
added a commit
that referenced
this pull request
Mar 10, 2022
Backport #19040 The PAM module has previously only checked the results of the authentication module. However, in normal PAM practice most users will expect account module authorization to also be checked. Without doing this check in almost every configuration expired accounts and accounts with expired passwords will still be able to login. This is likely to represent a significant gotcha in most configurations and cause most users configurations to be potentially insecure. Therefore we should add in the account authorization check. ##⚠️ **BREAKING**⚠️ Users of the PAM module who rely on account modules not being checked will need to change their PAM configuration. However, as it is likely that the vast majority of users of PAM will be expecting account authorization to be checked in addition to authentication we should make this breaking change to make the default behaviour correct for the majority. --- I suggest we backport this despite the BREAKING nature because of the surprising nature of this. Thanks to @ysf for bringing this to our attention. Co-authored-by: zeripath <art27@cantab.net> Co-authored-by: ysf <34326+ysf@users.noreply.github.com>
Chianina
pushed a commit
to Chianina/gitea
that referenced
this pull request
Mar 28, 2022
https://huntr.dev/bounties/8d221f92-b2b1-4878-bc31-66ff272e5ceb/ Co-authored-by: ysf <34326+ysf@users.noreply.github.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The PAM module has previously only checked the results of the authentication module.
However, in normal PAM practice most users will expect account module authorization to also be checked. Without doing this check in almost every configuration expired accounts and accounts with expired passwords will still be able to login.
This is likely to represent a significant gotcha in most configurations and cause most users configurations to be potentially insecure. Therefore we should add in the account authorization check.
Users of the PAM module who rely on account modules not being checked will need to change their PAM configuration.
However, as it is likely that the vast majority of users of PAM will be expecting account authorization to be checked in addition to authentication we should make this breaking change to make the default behaviour correct for the majority.
I suggest we backport this despite the BREAKING nature because of the surprising nature of this.
Thanks to @ysf for bringing this to our attention.