Paths

Table of Contentst

Differential D21445

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

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 Passed

Unit

No Test Coverage

Build Status

Buildable 24859
Build 34292: arc lint + arc unit

Event Timeline

kuba-orlik created this revision.Aug 12 2020, 7:04 PM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptAug 12 2020, 7:04 PM

Herald added a required legal document: L28 Phacility Individual Contributor License Agreement. · View Herald Transcript

Herald added a subscriber: epriestley. · View Herald Transcript

kuba-orlik requested review of this revision.Aug 12 2020, 7:04 PM

Harbormaster completed remote builds in B24859: Diff 51044.Aug 12 2020, 7:04 PM

kuba-orlik edited the summary of this revision. (Show Details)Aug 12 2020, 7:12 PM

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.

hskiba added a subscriber: hskiba.May 16 2022, 2:13 PM

Revision Contents
Changeset List

Path

Size

resources/

sshd/

phabricator-ssh-hook.sh

2 lines

sshd_config.phabricator.example

2 lines

Diff 51044

View Options

resources/sshd/phabricator-ssh-hook.sh