Page MenuHomePhabricator

Change sshd templates so they use the new ssh-auth syntax
Needs ReviewPublic

Authored by kuba-orlik on Aug 12 2020, 7:04 PM.

Details

Reviewers
None
Group Reviewers
Blessed Reviewers
Required Signatures
L28 Phacility Individual Contributor License Agreement
Summary

We've been encountering an issue with phabricator's ssh
daemon being very unreliable on our install. I've been trying to debug
it for a good part of the last week now and finally solved it. The fix
was to update the sshd config to use the new ssh-auth argument syntax.

The issue was that 9 times out of 10 the server didn't accept the
users's keys, responding with:

Permission denied (publickey).

After setting the sshd log level on the server to VERBOSE I could see the following:

...
debug1: matching key found: file /usr/libexec/phabricator-ssh-hook.sh, line 2 <redacted>
error: AuthorizedKeysCommand /usr/libexec/phabricator-ssh-hook.sh git failed, status 255
debug1: restore_uid: 0/0
...

After reading the source of scripts/ssh/ssh-auth.php, I realized
that when given a key as an argument, the script returns only the
matching key instead of all the keys in the database. But the default
sshd config templates mentioned in this article
do not take advantage of that feature.

After changing the sshd config so it passes the user key to the php script, the problems went away.

Test Plan

Used the config templates on a server, restarted the sshd daemon and successfully connected to the server via ssh (e.g. by cloning a repo over ssh)

Diff Detail

Repository
rP Phabricator
Branch
sshd-template-fix
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 24859
Build 34292: arc lint + arc unit

Event Timeline

The root cause is a bug in SSHD which was fixed in January 2017, see T11827.

This change is conceptually fine but I'm not entirely sure the %k change works everywhere and I'm hesitant to risk replacing a well-understood headache rooted in an SSHD bug which tends to be resolved in the obvious way ("upgrade sshd") with a poorly-understood headache rooted in, say, keys which have two or more different variations that SSHD accepts.

%k has been in production in Phacility for a while without issues and I expect to recommend this configuration or some similar configuration in the future, but I'd like to pull production data first and convince myself that all the cases which are falling through are for keys legitimately not present in the key list.

Mostly for my own notes:

In PHI1785 (internal) we did this, which is perhaps slightly simpler:

AuthorizedKeysCommand /usr/libexec/phabricator-ssh-hook.sh %u --sshd-key %k

But I think the whole $VCSUSER check is not really necessary, and it might be reasonable to simplify the indirection hook.