[sssd PR#553][opened] Use p11_child to verify certificates for the ssh responder

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/553
Author: sumit-bose
 Title: #553: Use p11_child to verify certificates for the ssh responder
Action: opened

PR body:
"""
This patch set is another step to solve https://pagure.io/SSSD/sssd/issue/3489,
i.e to remove the NSS dependency and allow all Smartcard and certificate
related features to be build with OpenSSL as well.

To have all the code related to certificate verification in one place the
verification code is removed from the ssh responder and the ssh responder will
now call p11_child to verify a certificate before extracting the public key as
ssh key. Another benefit is that the ssh responder is not blocked anymore
during OCSP check since they now run in p11_child and the ssh responder can
process other requests in parallel.

In this context I also added a patch which improves the documentation of the
feature in the sss_ssh_authorizedkeys man page as requested in
https://pagure.io/SSSD/sssd/issue/3688.

Besides adding unit tests for the new calls I added an unit test for the ssh
responder, similar to the ones for the nss and pam responder.
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/553/head:pr553
git checkout pr553

[sssd PR#553][comment] Use p11_child to verify certificates for the ssh responder

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/553
Title: #553: Use p11_child to verify certificates for the ssh responder

jhrozek commented:
"""
Thank you for the patches. Can you help me understand what kind of testing should be done for this PR in addition to running the automated tests you wrote?
"""

See the full comment at https://github.com/SSSD/sssd/pull/553#issuecomment-382320951

[sssd PR#553][comment] Use p11_child to verify certificates for the ssh responder

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/553
Title: #553: Use p11_child to verify certificates for the ssh responder

sumit-bose commented:
"""
@jhrozek, sss_ssh_authorizedkeys should work as before (return the same keys) both if only ssh keys are added to the user entry and if a valid certificate is added to the user entry as well.
"""

See the full comment at https://github.com/SSSD/sssd/pull/553#issuecomment-382329475

[sssd PR#553][comment] Use p11_child to verify certificates for the ssh responder

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/553
Title: #553: Use p11_child to verify certificates for the ssh responder

jhrozek commented:
"""
I started the review by running Coverity:
```
Error: UNREACHABLE (CWE-561):
sssd-1.16.2/src/responder/ssh/ssh_reply.c:166: unreachable: This code cannot be reached: "ret = 12;".
#  164|       if (state->elements == NULL) {
#  165|           goto done;
#  166|->         ret = ENOMEM;
#  167|       }
#  168|

Error: UNINIT (CWE-457):
sssd-1.16.2/src/responder/ssh/ssh_reply.c:135: var_decl: Declaring variable "ret" without initializer.
sssd-1.16.2/src/responder/ssh/ssh_reply.c:258: uninit_use: Using uninitialized value "ret".
#  256|   
#  257|   done:
#  258|->     if (ret != EAGAIN) {
#  259|           if (ret == EOK) {
#  260|               tevent_req_done(req);

Error: COMPILER_WARNING:
sssd-1.16.2/src/responder/ssh/ssh_reply.c: scope_hint: In function 'ssh_get_output_keys_send'
sssd-1.16.2/src/responder/ssh/ssh_reply.c:258:8: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized]
#     if (ret != EAGAIN) {
#        ^
#  256|   
#  257|   done:
#  258|->     if (ret != EAGAIN) {
#  259|           if (ret == EOK) {
#  260|               tevent_req_done(req);

Error: CLANG_WARNING:
sssd-1.16.2/src/responder/ssh/ssh_reply.c:258:13: warning: The left operand of '!=' is a garbage value
#    if (ret != EAGAIN) {
#        ~~~ ^
sssd-1.16.2/src/responder/ssh/ssh_reply.c:135:5: note: 'ret' declared without an initial value
#    errno_t ret;
#    ^~~~~~~~~~~
sssd-1.16.2/src/responder/ssh/ssh_reply.c:139:9: note: Assuming 'req' is not equal to null
#    if (req == NULL) {
#        ^~~~~~~~~~~
sssd-1.16.2/src/responder/ssh/ssh_reply.c:139:5: note: Taking false branch
#    if (req == NULL) {
#    ^
sssd-1.16.2/src/responder/ssh/ssh_reply.c:150:5: note: Taking false branch
#    if (state->ssh_ctx == NULL) {
#    ^
sssd-1.16.2/src/responder/ssh/ssh_reply.c:157:5: note: Taking false branch
#    if (state->name == NULL) {
#    ^
sssd-1.16.2/src/responder/ssh/ssh_reply.c:164:5: note: Taking true branch
#    if (state->elements == NULL) {
#    ^
sssd-1.16.2/src/responder/ssh/ssh_reply.c:165:9: note: Control jumps to line 258
#        goto done;
#        ^
sssd-1.16.2/src/responder/ssh/ssh_reply.c:258:13: note: The left operand of '!=' is a garbage value
#    if (ret != EAGAIN) {
#        ~~~ ^
#  256|   
#  257|   done:
#  258|->     if (ret != EAGAIN) {
#  259|           if (ret == EOK) {
#  260|               tevent_req_done(req);
```

Feel free to fix this just locally, I haven't really done anything else so far.
"""

See the full comment at https://github.com/SSSD/sssd/pull/553#issuecomment-382755929

[sssd PR#553][synchronized] Use p11_child to verify certificates for the ssh responder

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/553
Author: sumit-bose
 Title: #553: Use p11_child to verify certificates for the ssh responder
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/553/head:pr553
git checkout pr553

[sssd PR#553][comment] Use p11_child to verify certificates for the ssh responder

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/553
Title: #553: Use p11_child to verify certificates for the ssh responder

sumit-bose commented:
"""
The latest version fixes the Coverity issue Jakub found above.
"""

See the full comment at https://github.com/SSSD/sssd/pull/553#issuecomment-383503355

[sssd PR#553][synchronized] Use p11_child to verify certificates for the ssh responder

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/553
Author: sumit-bose
 Title: #553: Use p11_child to verify certificates for the ssh responder
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/553/head:pr553
git checkout pr553

[sssd PR#553][synchronized] Use p11_child to verify certificates for the ssh responder

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/553
Author: sumit-bose
 Title: #553: Use p11_child to verify certificates for the ssh responder
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/553/head:pr553
git checkout pr553

[sssd PR#553][+superseded] Use p11_child to verify certificates for the ssh responder

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/553
Title: #553: Use p11_child to verify certificates for the ssh responder

Label: +superseded

[sssd PR#553][comment] Use p11_child to verify certificates for the ssh responder

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/553
Title: #553: Use p11_child to verify certificates for the ssh responder

jhrozek commented:
"""
Since all these fixes are also in PR #570 which was just accepted, I'm closing this one.
"""

See the full comment at https://github.com/SSSD/sssd/pull/553#issuecomment-394312977

[sssd PR#553][closed] Use p11_child to verify certificates for the ssh responder

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/553
Author: sumit-bose
 Title: #553: Use p11_child to verify certificates for the ssh responder
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/553/head:pr553
git checkout pr553