URL: https://github.com/SSSD/sssd/pull/911 Author: alexal Title: #911: Update pam_sss.8.xml Action: opened
PR body: """ pam_sss: Added return values on a man page
Resolves: https://pagure.io/SSSD/sssd/issue/3672 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/911/head:pr911 git checkout pr911
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
centos-ci commented: """ Can one of the admins verify this patch? """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-544642148
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
mzidek-rh commented: """ I plan to review this today or tomorrow. """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-544867903
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
alexey-tikhonov commented: """ ok to test """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-544928286
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
mzidek-rh commented: """ Here are my comments for each entry you added:
- PAM_SUCCESS - OK
- PAM_USER_UNKNOWN - ... or the SSSD's PAM responder is not running.
- PAM_IGNORE - Simply write "See options ignore_unknown_user and ignore_authinfo_unavail."
- PAM_AUTHTOK_ERR - I think this is OK... maybe add that it could be returned also when user authenticates with certificates and multiple certificates are available but installed version of gdm does not support selection from multiple certificates.
- PAM_AUTHINFO_UNAVAIL - OK (I think)
- PAM_BUF_ERR - Ok, but also add that this could be returned also when options use_first_pass or use_authtok were set, but no password from previously stacked pam module was found."
- PAM_SYSTEM_ERR - OK
- PAM_CRED_ERR - OK
- PAM_CRED_INSUFFICIENT Ok, but add "For example missing PIN during smartcard authentication or missing factor during 2-factor authentication." ```
I hope it is understandable. Ask if something is not clear. """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-545645733
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
mzidek-rh commented: """ Here are my comments for each entry you added:
- PAM_SUCCESS - OK
- PAM_USER_UNKNOWN - ... or the SSSD's PAM responder is not running.
- PAM_IGNORE - Simply write "See options ignore_unknown_user and ignore_authinfo_unavail."
- PAM_AUTHTOK_ERR - I think this is OK... maybe add that it could be returned also when user authenticates with certificates and multiple certificates are available but installed version of gdm does not support selection from multiple certificates.
- PAM_AUTHINFO_UNAVAIL - OK (I think)
- PAM_BUF_ERR - Ok, but also add that this could be returned also when options use_first_pass or use_authtok were set, but no password from previously stacked pam module was found."
- PAM_SYSTEM_ERR - OK
- PAM_CRED_ERR - OK
- PAM_CRED_INSUFFICIENT Ok, but add "For example missing PIN during smartcard authentication or missing factor during 2-factor authentication."
I hope it is understandable. Ask if something is not clear. """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-545645733
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
mzidek-rh commented: """ Here are my comments for each entry you added:
- PAM_SUCCESS - OK
- PAM_USER_UNKNOWN - ... or the SSSD's PAM responder is not running.
- PAM_IGNORE - Simply write "See options ignore_unknown_user and ignore_authinfo_unavail."
- PAM_AUTHTOK_ERR - I think this is OK... maybe add that it could be returned also when user authenticates with certificates and multiple certificates are available but installed version of gdm does not support selection from multiple certificates.
- PAM_AUTHINFO_UNAVAIL - OK (I think)
- PAM_BUF_ERR - Ok, but also add that this could be returned also when options use_first_pass or use_authtok were set, but no password from previously stacked pam module was found.
- PAM_SYSTEM_ERR - OK
- PAM_CRED_ERR - OK
- PAM_CRED_INSUFFICIENT Ok, but add "For example missing PIN during smartcard authentication or missing factor during two-factor authentication."
I hope it is understandable. Ask if something is not clear. """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-545645733
URL: https://github.com/SSSD/sssd/pull/911 Author: alexal Title: #911: Update pam_sss.8.xml Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/911/head:pr911 git checkout pr911
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
alexal commented: """ @mzidek-rh thanks for your comments. I've made those changes, please see the updated commit. """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-545666034
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
mzidek-rh commented: """ @alexal Thanks, it looks good to me now.
@sumit-bose Can I borrow your pair of eyes for these pam_sss man page changes? """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-545805841
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
sumit-bose commented: """ Hi,
thank you for the patch.
you wording is mainly about authentication, but pam_sss handles authorization and session setup as well. Maybe you can check if you can make at least some of the descriptions more general.
You didn't mention return codes like e.g. `PAM_PERM_DENIED` or `PAM_AUTH_ERR`. Is this because you didn't find a good description or are you planning to add them in a later pull-request?
bye, Sumit """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-546306056
URL: https://github.com/SSSD/sssd/pull/911 Author: alexal Title: #911: Update pam_sss.8.xml Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/911/head:pr911 git checkout pr911
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
alexal commented: """ @sumit-bose ,
I think it would be good to say that this means an error on the SSSD side and the SSSD logs should be checked why it occured.
I've added, "The SSSD log files may contain additional information about the error."
You didn't mention return codes like e.g. PAM_PERM_DENIED or PAM_AUTH_ERR
I just missed these two return values. Added them, please see the updated commit.
you wording is mainly about authentication, but pam_sss handles authorization and session setup as well. Maybe you can check if you can make at least some of the descriptions more general.
I would appreciate any suggestions. """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-547006780
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
sumit-bose commented: """
@sumit-bose ,
I think it would be good to say that this means an error on the SSSD side and the SSSD logs should be checked why it occured.
I've added, "The SSSD log files may contain additional information about the error."
You didn't mention return codes like e.g. PAM_PERM_DENIED or PAM_AUTH_ERR
I just missed these two return values. Added them, please see the updated commit.
Thanks, I think there were important to mention as well.
For the record the following PAM return codes are used in the SSSD code as well, but please do not feel obliged to add those to your PR as well, I think you covered all that are important. I just want to record them somewhere to make it more easy to add them later:
- PAM_SERVICE_ERR - PAM_NEW_AUTHTOK_REQD - PAM_ACCT_EXPIRED - PAM_SESSION_ERR - PAM_CRED_UNAVAIL - PAM_NO_MODULE_DATA - PAM_CONV_ERR - PAM_AUTHTOK_LOCK_BUSY - PAM_ABORT - PAM_MODULE_UNKNOWN - PAM_BAD_ITEM
you wording is mainly about authentication, but pam_sss handles authorization and session setup as well. Maybe you can check if you can make at least some of the descriptions more general.
I would appreciate any suggestions.
This is mostly about `PAM_SUCCESS`. Here something like `The PAM operation finished successfully.` would be better imo.
bye, Sumit
"""
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-547370620
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
mzidek-rh commented: """ Adding changes requested as per @sumit-bose's comment. """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-547831486
URL: https://github.com/SSSD/sssd/pull/911 Author: alexal Title: #911: Update pam_sss.8.xml Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/911/head:pr911 git checkout pr911
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
alexal commented: """ @sumit-bose , @mzidek-rh I apologize for the delay. I had too much work. I've updated a man page with all of the requested changes. Please take a look and let me know if something needs to be changed. """
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-564090022
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: +Waiting for review
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: -Waiting for review
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: +Ready to push
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
pbrezina commented: """ * `master` * 192eadaaf8ac6a427533612362f012dcbd7ce776 - Update pam_sss.8.xml
"""
See the full comment at https://github.com/SSSD/sssd/pull/911#issuecomment-581364133
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/911 Title: #911: Update pam_sss.8.xml
Label: -Ready to push
URL: https://github.com/SSSD/sssd/pull/911 Author: alexal Title: #911: Update pam_sss.8.xml Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/911/head:pr911 git checkout pr911
sssd-devel@lists.fedorahosted.org
-
alexal -
alexey-tikhonov
-
centos-ci
-
mzidek-rh
-
pbrezina
-
sumit-bose