URL: https://github.com/SSSD/sssd/pull/744 Title: #744: ssh: sssd_ssh fails completely on p11_child timeout

jhrozek commented: """ Currently both the timeout and e.g. a failure to set up child process handlers returns ERR_P11_CHILD. Would it make more sense to perhaps return a timeout error and then only ignore that error?

Or was the idea behind the path that since the output is just a public key, we can go on? But shouldn't the code fall through in that case instead of ending with success? """

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

URL: https://github.com/SSSD/sssd/pull/744 Title: #744: ssh: sssd_ssh fails completely on p11_child timeout

sumit-bose commented: """ Hi,

I was thinking back and forth but finally I think it is best to solve the issue this way.

Originally I thought that the timeout handling should be changed in the cert_to_ssh_key-request so that the timeout is treated as a failed validation and the next certificate in the list is send for validation. And that in ssh_get_output_keys_done() the cert_to_ssh_key-request is run with list of certificates from state->user_cert_override as well. With this we would try to get as much keys as possible from the available certificates.

But if there are still certificates left in the list there is a fair chance that the validation of those will run into a timeout as well if e.g. the system is offline and the time the user has to wait for ssh authentication to continue would increase quite a bit.

So I agree that it is a good idea to just stop and continue as the patch does.

I like the @jhrozek's suggestion to let the timeout handler return a dedicated timeout error. With this a more specific log message can be added saying that ssh-key from certificate are ignored due to a timeout while trying to validate the certificates. And that p11_child_timeout option can be increased if it is expect that validation is slow.

Finally I would change the comment to something like 'ignore ssh keys from certificates and return what we already have'. As you can see in ssh_get_output_keys_send() there might already be plain ssh keys from the user's LDAP object or from an override in the elements array which is later returned to the caller.

bye, Sumit """

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

URL: https://github.com/SSSD/sssd/pull/744 Title: #744: ssh: sssd_ssh fails completely on p11_child timeout

Label: +Changes requested

URL: https://github.com/SSSD/sssd/pull/744 Title: #744: ssh: sssd_ssh fails completely on p11_child timeout

Label: -Changes requested

URL: https://github.com/SSSD/sssd/pull/744 Author: thalman Title: #744: ssh: sssd_ssh fails completely on p11_child timeout Action: synchronized

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

URL: https://github.com/SSSD/sssd/pull/744 Title: #744: ssh: sssd_ssh fails completely on p11_child timeout

thalman commented: """ Added second commit with new ERR_P11_CHILD_TIMEOUT code """

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

URL: https://github.com/SSSD/sssd/pull/744 Title: #744: ssh: sssd_ssh fails completely on p11_child timeout

Label: +Accepted

URL: https://github.com/SSSD/sssd/pull/744 Author: thalman Title: #744: ssh: sssd_ssh fails completely on p11_child timeout Action: closed

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