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

Add pam account authorization check #19040

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Add pam account authorization check #19040

merged 1 commit into from
Mar 10, 2022

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 9, 2022

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.

@zeripath zeripath added type/bug type/enhancement An improvement of existing functionality topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! backport/v1.16 labels Mar 9, 2022
@zeripath zeripath added this to the 1.17.0 milestone Mar 9, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 9, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 9, 2022
@lunny lunny merged commit 1314f38 into go-gitea:main Mar 10, 2022
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Mar 10, 2022
@6543 6543 added the backport/done All backports for this PR have been created label Mar 10, 2022
@6543
Copy link
Member

6543 commented Mar 10, 2022

-> #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 zeripath added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Mar 10, 2022
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
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/bug type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants