[sssd PR#586][opened] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/586
Author: jhrozek
 Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys
Action: opened

PR body:
"""
Resolves: https://pagure.io/SSSD/sssd/issue/3747

Before writing the keys to sshd, ignore SIGPIPE so that if the pipe towards
the authorizedkeys helper is closed, the sss_ssh_authorizedkeys helper is
not terminated with SIGPIPE, but instead proceeds and then the write(2)
calls would non-terminally fail with EPIPE.

The other patch in this PR is not meant to be pushed. It is an easy way to
reproduce the bug. I would also like to write an integration test, but
I'm not sure if I can do that very soon and given that we try to release
in about a week I prefer to send the fix first.

In order to reproduce, load many SSH keys to a user object. I found it was
easiest to cheat during reproducing and do this:
    - first, set a long cache expire so that the cache doesn't expire
      and overwrite your local changes
    - ldbedit the cache
    - copy the ssh public key attribute and each time, change one character
      in the attribute (ldb would otherwise detect the duplicates)
    - save the ldbedit window
    - run the program from the second patch. With the sss_ssh_authorizedkeys
      patch in, the sss_ssh_authorizedkeys binary should finish gracefully,
      without the patch, it would fail with SIGPIPE.

In my testing, I needed about 30 ssh keys to reproduce the bug.
"""

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

[sssd PR#586][comment] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

jhrozek commented:
"""
btw Sumit asked me to check if openssh can be patched to read the output completely instead of aborting in the middle. Turns out this is also what openssh recently did upstream and I filed https://bugzilla.redhat.com/show_bug.cgi?id=1584774 to backport the read-the-whole-output patch to RHEL as well.
"""

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

[sssd PR#586][comment] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[sumit-bose]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

sumit-bose commented:
"""
My idea was to not read the output completely, but let sshd ignore that the child was killed by SIGPIPE if the output was not read completely.

exited_cleanly() has:

if (WIFSIGNALED(status)) {
                error("%s %s exited on signal %d", tag, cmd, WTERMSIG(status));
                return -1;

here something like 'if (WIFSIGNALED(status) && WTERMSIG(status) != SIGPIPE) {' might help to ignore the signal (not tested).

bye,
Sumit
"""

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

[sssd PR#586][comment] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

jhrozek commented:
"""
this PR really does need an integration test and I don't want to rush it in before this week's release so I'm tagging it with Changes Requested
"""

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

[sssd PR#586][+Changes requested] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

Label: +Changes requested

[sssd PR#586][synchronized] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/586
Author: jhrozek
 Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys
Action: synchronized

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

[sssd PR#586][comment] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

jhrozek commented:
"""
Since the PR now has an integration test as well, I'm removing the Changes Requested label.
"""

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

[sssd PR#586][-Changes requested] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

Label: -Changes requested

[sssd PR#586][comment] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[fidencio]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

fidencio commented:
"""
I've fired a CI run with these patches.
"""

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

[sssd PR#586][comment] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[fidencio]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

fidencio commented:
"""
Everything works as expected with the patch, everything fails as expected without the patch.
CI: http://vm-031.$%7Babc%7D/logs/job/90/50/summary.html. The CI failures on debian are being already tracked as https://pagure.io/SSSD/sssd/issue/3762.
Ack!
"""

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

[sssd PR#586][+Accepted] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[fidencio]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

Label: +Accepted

[sssd PR#586][comment] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[fidencio]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

fidencio commented:
"""
master:
  1575ec9
  56cda83
  804c5b5
  cb138d7
  909c16e
  4cc3c1a

"""

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

[sssd PR#586][+Pushed] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[fidencio]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

Label: +Pushed

[sssd PR#586][-Accepted] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[fidencio]

URL: https://github.com/SSSD/sssd/pull/586
Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

Label: -Accepted

[sssd PR#586][closed] SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys

[fidencio]

URL: https://github.com/SSSD/sssd/pull/586
Author: jhrozek
 Title: #586: SSH: Do not exit abruptly if SSHD closes its end of the pipe before reading all the SSH keys
Action: closed

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