Skip to content

Commit 6f0d3b0

Browse files
author
epriestley
committed
Add a query/policy layer on top of SSH keys for Almanac
Summary: Ref T5833. Currently, SSH keys are associated only with users, and are a bit un-modern. I want to let Almanac Devices have SSH keys so devices in a cluster can identify to one another. For example, with hosted installs, initialization will go something like this: - A request comes in for `company.phacility.com`. - A SiteSource (from D10787) makes a Conduit call to Almanac on the master install to check if `company` is a valid install and pull config if it is. - This call can be signed with an SSH key which identifies a trusted Almanac Device. In the cluster case, a web host can make an authenticated call to a repository host with similar key signing. To move toward this, put a proper Query class on top of SSH key access (this diff). In following diffs, I'll: - Rename `userPHID` to `objectPHID`. - Move this to the `auth` database. - Provide UI for device/key association. An alternative approach would be to build some kind of special token layer in Conduit, but I think that would be a lot harder to manage in the hosting case. This gives us a more direct attack on trusting requests from machines and recognizing machines as first (well, sort of second-class) actors without needing things like fake user accounts. Test Plan: - Added and removed SSH keys. - Added and removed SSH keys from a bot account. - Tried to edit an unonwned SSH key (denied). - Ran `bin/ssh-auth`, got sensible output. - Ran `bin/ssh-auth-key`, got sensible output. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5833 Differential Revision: https://secure.phabricator.com/D10790
1 parent 3ea31c9 commit 6f0d3b0

File tree

7 files changed

+293
-111
lines changed

7 files changed

+293
-111
lines changed

scripts/ssh/ssh-auth-key.php

Lines changed: 12 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -4,48 +4,28 @@
44
$root = dirname(dirname(dirname(__FILE__)));
55
require_once $root.'/scripts/__init_script__.php';
66

7-
$cert = file_get_contents('php://stdin');
8-
9-
if (!$cert) {
10-
exit(1);
11-
}
12-
13-
$parts = preg_split('/\s+/', $cert);
14-
if (count($parts) < 2) {
7+
try {
8+
$cert = file_get_contents('php://stdin');
9+
$public_key = PhabricatorAuthSSHPublicKey::newFromRawKey($cert);
10+
} catch (Exception $ex) {
1511
exit(1);
1612
}
1713

18-
list($type, $body) = $parts;
19-
20-
$user_dao = new PhabricatorUser();
21-
$ssh_dao = new PhabricatorUserSSHKey();
22-
$conn_r = $user_dao->establishConnection('r');
23-
24-
$row = queryfx_one(
25-
$conn_r,
26-
'SELECT userName FROM %T u JOIN %T ssh ON u.phid = ssh.userPHID
27-
WHERE ssh.keyType = %s AND ssh.keyBody = %s',
28-
$user_dao->getTableName(),
29-
$ssh_dao->getTableName(),
30-
$type,
31-
$body);
32-
33-
if (!$row) {
34-
exit(1);
35-
}
36-
37-
$user = idx($row, 'userName');
38-
39-
if (!$user) {
14+
$key = id(new PhabricatorAuthSSHKeyQuery())
15+
->setViewer(PhabricatorUser::getOmnipotentUser())
16+
->withKeys(array($public_key))
17+
->executeOne();
18+
if (!$key) {
4019
exit(1);
4120
}
4221

43-
if (!PhabricatorUser::validateUsername($user)) {
22+
$object = $key->getObject();
23+
if (!($object instanceof PhabricatorUser)) {
4424
exit(1);
4525
}
4626

4727
$bin = $root.'/bin/ssh-exec';
48-
$cmd = csprintf('%s --phabricator-ssh-user %s', $bin, $user);
28+
$cmd = csprintf('%s --phabricator-ssh-user %s', $bin, $object->getUsername());
4929
// This is additional escaping for the SSH 'command="..."' string.
5030
$cmd = addcslashes($cmd, '"\\');
5131

scripts/ssh/ssh-auth.php

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -4,39 +4,40 @@
44
$root = dirname(dirname(dirname(__FILE__)));
55
require_once $root.'/scripts/__init_script__.php';
66

7-
$user_dao = new PhabricatorUser();
8-
$ssh_dao = new PhabricatorUserSSHKey();
9-
$conn_r = $user_dao->establishConnection('r');
10-
11-
$rows = queryfx_all(
12-
$conn_r,
13-
'SELECT userName, keyBody, keyType FROM %T u JOIN %T ssh
14-
ON u.phid = ssh.userPHID',
15-
$user_dao->getTableName(),
16-
$ssh_dao->getTableName());
17-
18-
if (!$rows) {
7+
$keys = id(new PhabricatorAuthSSHKeyQuery())
8+
->setViewer(PhabricatorUser::getOmnipotentUser())
9+
->execute();
10+
11+
foreach ($keys as $key => $ssh_key) {
12+
// For now, filter out any keys which don't belong to users. Eventually we
13+
// may allow devices to use this channel.
14+
if (!($ssh_key->getObject() instanceof PhabricatorUser)) {
15+
unset($keys[$key]);
16+
continue;
17+
}
18+
}
19+
20+
if (!$keys) {
1921
echo pht('No keys found.')."\n";
2022
exit(1);
2123
}
2224

2325
$bin = $root.'/bin/ssh-exec';
24-
foreach ($rows as $row) {
25-
$user = $row['userName'];
26+
foreach ($keys as $ssh_key) {
27+
$user = $ssh_key->getObject()->getUsername();
2628

2729
$cmd = csprintf('%s --phabricator-ssh-user %s', $bin, $user);
2830
// This is additional escaping for the SSH 'command="..."' string.
2931
$cmd = addcslashes($cmd, '"\\');
3032

3133
// Strip out newlines and other nonsense from the key type and key body.
3234

33-
$type = $row['keyType'];
35+
$type = $ssh_key->getKeyType();
3436
$type = preg_replace('@[\x00-\x20]+@', '', $type);
3537

36-
$key = $row['keyBody'];
38+
$key = $ssh_key->getKeyBody();
3739
$key = preg_replace('@[\x00-\x20]+@', '', $key);
3840

39-
4041
$options = array(
4142
'command="'.$cmd.'"',
4243
'no-port-forwarding',

src/__phutil_library_map__.php

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1317,6 +1317,8 @@
13171317
'PhabricatorAuthProviderConfigTransactionQuery' => 'applications/auth/query/PhabricatorAuthProviderConfigTransactionQuery.php',
13181318
'PhabricatorAuthRegisterController' => 'applications/auth/controller/PhabricatorAuthRegisterController.php',
13191319
'PhabricatorAuthRevokeTokenController' => 'applications/auth/controller/PhabricatorAuthRevokeTokenController.php',
1320+
'PhabricatorAuthSSHKeyQuery' => 'applications/auth/query/PhabricatorAuthSSHKeyQuery.php',
1321+
'PhabricatorAuthSSHPublicKey' => 'applications/auth/storage/PhabricatorAuthSSHPublicKey.php',
13201322
'PhabricatorAuthSession' => 'applications/auth/storage/PhabricatorAuthSession.php',
13211323
'PhabricatorAuthSessionEngine' => 'applications/auth/engine/PhabricatorAuthSessionEngine.php',
13221324
'PhabricatorAuthSessionGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php',
@@ -4381,6 +4383,8 @@
43814383
'PhabricatorAuthProviderConfigTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
43824384
'PhabricatorAuthRegisterController' => 'PhabricatorAuthController',
43834385
'PhabricatorAuthRevokeTokenController' => 'PhabricatorAuthController',
4386+
'PhabricatorAuthSSHKeyQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
4387+
'PhabricatorAuthSSHPublicKey' => 'Phobject',
43844388
'PhabricatorAuthSession' => array(
43854389
'PhabricatorAuthDAO',
43864390
'PhabricatorPolicyInterface',
@@ -5627,7 +5631,10 @@
56275631
'PhabricatorUserProfileEditor' => 'PhabricatorApplicationTransactionEditor',
56285632
'PhabricatorUserRealNameField' => 'PhabricatorUserCustomField',
56295633
'PhabricatorUserRolesField' => 'PhabricatorUserCustomField',
5630-
'PhabricatorUserSSHKey' => 'PhabricatorUserDAO',
5634+
'PhabricatorUserSSHKey' => array(
5635+
'PhabricatorUserDAO',
5636+
'PhabricatorPolicyInterface',
5637+
),
56315638
'PhabricatorUserSchemaSpec' => 'PhabricatorConfigSchemaSpec',
56325639
'PhabricatorUserSearchIndexer' => 'PhabricatorSearchDocumentIndexer',
56335640
'PhabricatorUserSinceField' => 'PhabricatorUserCustomField',
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
<?php
2+
3+
final class PhabricatorAuthSSHKeyQuery
4+
extends PhabricatorCursorPagedPolicyAwareQuery {
5+
6+
private $ids;
7+
private $objectPHIDs;
8+
private $keys;
9+
10+
public function withIDs(array $ids) {
11+
$this->ids = $ids;
12+
return $this;
13+
}
14+
15+
public function withObjectPHIDs(array $object_phids) {
16+
$this->objectPHIDs = $object_phids;
17+
return $this;
18+
}
19+
20+
public function withKeys(array $keys) {
21+
assert_instances_of($keys, 'PhabricatorAuthSSHPublicKey');
22+
$this->keys = $keys;
23+
return $this;
24+
}
25+
26+
protected function loadPage() {
27+
$table = new PhabricatorUserSSHKey();
28+
$conn_r = $table->establishConnection('r');
29+
30+
$data = queryfx_all(
31+
$conn_r,
32+
'SELECT * FROM %T %Q %Q %Q',
33+
$table->getTableName(),
34+
$this->buildWhereClause($conn_r),
35+
$this->buildOrderClause($conn_r),
36+
$this->buildLimitClause($conn_r));
37+
38+
return $table->loadAllFromArray($data);
39+
}
40+
41+
protected function willFilterPage(array $keys) {
42+
$object_phids = mpull($keys, 'getObjectPHID');
43+
44+
$objects = id(new PhabricatorObjectQuery())
45+
->setViewer($this->getViewer())
46+
->setParentQuery($this)
47+
->withPHIDs($object_phids)
48+
->execute();
49+
$objects = mpull($objects, null, 'getPHID');
50+
51+
foreach ($keys as $key => $ssh_key) {
52+
$object = idx($objects, $ssh_key->getObjectPHID());
53+
if (!$object) {
54+
unset($keys[$key]);
55+
continue;
56+
}
57+
$ssh_key->attachObject($object);
58+
}
59+
60+
return $keys;
61+
}
62+
63+
protected function buildWhereClause(AphrontDatabaseConnection $conn_r) {
64+
$where = array();
65+
66+
if ($this->ids !== null) {
67+
$where[] = qsprintf(
68+
$conn_r,
69+
'id IN (%Ld)',
70+
$this->ids);
71+
}
72+
73+
if ($this->objectPHIDs !== null) {
74+
$where[] = qsprintf(
75+
$conn_r,
76+
'userPHID IN (%Ls)',
77+
$this->objectPHIDs);
78+
}
79+
80+
if ($this->keys !== null) {
81+
// TODO: This could take advantage of a better key, and the hashing
82+
// scheme for this table is a bit nonstandard and questionable.
83+
84+
$sql = array();
85+
foreach ($this->keys as $key) {
86+
$sql[] = qsprintf(
87+
$conn_r,
88+
'(keyType = %s AND keyBody = %s)',
89+
$key->getType(),
90+
$key->getBody());
91+
}
92+
$where[] = implode(' OR ', $sql);
93+
}
94+
95+
$where[] = $this->buildPagingClause($conn_r);
96+
97+
return $this->formatWhereClause($where);
98+
}
99+
100+
public function getQueryApplicationClass() {
101+
return 'PhabricatorAuthApplication';
102+
}
103+
104+
}
Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
<?php
2+
3+
/**
4+
* Data structure representing a raw public key.
5+
*/
6+
final class PhabricatorAuthSSHPublicKey extends Phobject {
7+
8+
private $type;
9+
private $body;
10+
private $comment;
11+
12+
private function __construct() {
13+
// <internal>
14+
}
15+
16+
public static function newFromRawKey($entire_key) {
17+
$entire_key = trim($entire_key);
18+
if (!strlen($entire_key)) {
19+
throw new Exception(pht('No public key was provided.'));
20+
}
21+
22+
$parts = str_replace("\n", '', $entire_key);
23+
24+
// The third field (the comment) can have spaces in it, so split this
25+
// into a maximum of three parts.
26+
$parts = preg_split('/\s+/', $parts, 3);
27+
28+
if (preg_match('/private\s*key/i', $entire_key)) {
29+
// Try to give the user a better error message if it looks like
30+
// they uploaded a private key.
31+
throw new Exception(pht('Provide a public key, not a private key!'));
32+
}
33+
34+
switch (count($parts)) {
35+
case 1:
36+
throw new Exception(
37+
pht('Provided public key is not properly formatted.'));
38+
case 2:
39+
// Add an empty comment part.
40+
$parts[] = '';
41+
break;
42+
case 3:
43+
// This is the expected case.
44+
break;
45+
}
46+
47+
list($type, $body, $comment) = $parts;
48+
49+
$recognized_keys = array(
50+
'ssh-dsa',
51+
'ssh-dss',
52+
'ssh-rsa',
53+
'ecdsa-sha2-nistp256',
54+
'ecdsa-sha2-nistp384',
55+
'ecdsa-sha2-nistp521',
56+
);
57+
58+
if (!in_array($type, $recognized_keys)) {
59+
$type_list = implode(', ', $recognized_keys);
60+
throw new Exception(
61+
pht(
62+
'Public key type should be one of: %s',
63+
$type_list));
64+
}
65+
66+
$public_key = new PhabricatorAuthSSHPublicKey();
67+
$public_key->type = $type;
68+
$public_key->body = $body;
69+
$public_key->comment = $comment;
70+
71+
return $public_key;
72+
}
73+
74+
public function getType() {
75+
return $this->type;
76+
}
77+
78+
public function getBody() {
79+
return $this->body;
80+
}
81+
82+
public function getComment() {
83+
return $this->comment;
84+
}
85+
86+
}

0 commit comments

Comments
 (0)