Paths

Table of Contentst

Differential D10400

Implement storage of a host ID and a public key for authorizing Conduit between servers
ClosedPublic
Actions

Authored by hach-que on Sep 2 2014, 10:42 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T4209: Multiserver / High-Availability Configuration

Commits

Restricted Diffusion Commit
rP8fbebce501a4: Implement storage of a host ID and a public key for authorizing Conduit between…

Summary

Ref T4209. This creates storage for public keys against authorized hosts, such that servers can be authorized to make Conduit calls as the omnipotent user.

Servers are registered into this system by running the following command once:

bin/almanac register

NOTE: This doesn't implement authorization between servers, just the storage of public keys.

Placing this against Almanac seemed like the most sensible place, since I'm imagining in future that the register command will accept more information (like the hostname of the server so it can be found in the service directory).

Test Plan

Ran bin/almanac register and saw the host (and public key information) appear in the database.

Diff Detail

Repository

rP Phabricator

Branch

hostidentifiation

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 2408
Build 2412: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

hach-que updated this revision to Diff 25024.Sep 2 2014, 10:42 AM

hach-que retitled this revision from to Implement storage of a host ID and a public key for authorizing Conduit between servers.

hach-que updated this object.

hach-que edited the test plan for this revision. (Show Details)

hach-que added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptSep 2 2014, 10:42 AM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B2408: Diff 25024.Sep 2 2014, 10:43 AM

hach-que updated this object.Sep 2 2014, 10:44 AM

hach-que edited edge metadata.

hach-que added a task: T4209: Multiserver / High-Availability Configuration.

hach-que edited the test plan for this revision. (Show Details)Sep 2 2014, 10:54 AM

hach-que updated this object.Sep 2 2014, 10:56 AM

A couple of thoughts on this:

Hosts usually have a key already (e.g., /etc/ssh_host_rsa_key). I vaguely wonder if we should try to use this? It seems like we probably should not (one thought is that it's probably common for multiple hosts to have the same key), but maybe this is an interaction we should think about.
Maybe we should just create the AlmanacDevice table now? And some kind of AlmanacDeviceProperty, I guess?

This basically seems fine but won't extend very cleanly. Let me check the next diff...

In D10400#10, @epriestley wrote:

one thought is that it's probably common for multiple hosts to have the same key

I think this is a highly likely scenario, at least for Docker instances where you might have 3 Docker instances using the same "instance configuration" directory; the "instance configuration" directory stores the machine SSH keys.

If I'm not mistaken, those machine SSH keys have to be the same on the web tier so that when people connect to Git over SSH, they don't get different fingerprints when connecting?

And yeah, I wasn't quite sure how to relate this data to devices for the moment (i.e the exact table structure of devices and their related information). I was also slightly worried that the knock-on effect of declaring the actual device table would mean setting up the other DAOs as well.

I'd say let's take a shot at AlmanacDevice and I guess AlmanacDeviceProperty (which might end up being wrong, but whatever). I don't think we have to do too much -- Device can just have $name (initialized to php_uname('n')) and DeviceProperty can have like devicePHID + type + value or something. That shouldn't be too bad and should extend a lot more gracefully later. Seem reasonable?

If I'm not mistaken, those machine SSH keys have to be the same on the web tier so that when people connect to Git over SSH, they don't get different fingerprints when connecting?

Yeah, I'm mostly thinking we might want to store this stuff at some point so, e.g., Drydock can fill in AuthorizedHosts files correctly, although that's pretty far away and we shouldn't really be SSH'ing from one cluster host to another.

we shouldn't really be SSH'ing from one cluster host to another.

Well, maybe with bin/almanac ssh web003, I guess? But way out.

Yup that sounds like a good plan. That will also allow us later to just add an additional row (instead of a column) to AlmanacDeviceProperty that explicitly enables / disables Conduit access so that we can store devices without giving them omnipotent Conduit access.

Changes based on code review feedback

Harbormaster completed remote builds in B2532: Diff 25217.Sep 13 2014, 3:11 AM

hach-que updated this object.Sep 13 2014, 3:11 AM

hach-que edited the test plan for this revision. (Show Details)

Some minor inlines, mostly interaction with T1191. It's fine if you aren't able to fix them all perfectly (no docs on anything yet) since the adjustment scripts aren't being run by users yet.

resources/sql/autopatches/20140902.almanacdevice.1.sql
3–4 ↗	(On Diff #25217)	There are some changes after T1191: Instead of `utf8_bin`, use `{$COLLATE_TEXT}`. Instead of `utf8_general_ci`, use `{$COLLATE_TEXT}`, unless the column needs case insensitivity (e.g., a unique key on something like a project name). For these, use `{$COLLATE_SORT}`. These variables are similar to `{$NAMESPACE}`. Otherwise, the `phid` column is now `VARBINARY(64)`. Config > Database Status should walk you through most of this: your table should be fully green after the patch runs. If there are warnings, fix the patch rather than letting `storage adjust` fix them. This stuff has zero documentation yet so yell if you get into trouble.
7 ↗	(On Diff #25217)	This should be `UNIQUE KEY ...`
12 ↗	(On Diff #25217)	This should be VARBINARY(64) now.
17 ↗	(On Diff #25217)	This should probably be `(devicePHID, key)` so we can look up specific properties of a device.
src/applications/almanac/application/PhabricatorAlmanacApplication.php
30–32	This is now `isPrototype()`.
src/applications/almanac/management/AlmanacManagementRegisterWorkflow.php
10	pht(), ideally
src/applications/almanac/query/AlmanacDeviceQuery.php
37–44 ↗	(On Diff #25217)	These checks should be `$this->ids !== null` explicitly so that `setIDs(array())` selects nothing (and throws) instead of everything.
src/applications/almanac/storage/AlmanacDevice.php
7 ↗	(On Diff #25217)	This is implicit; omit it.
12–13 ↗	(On Diff #25217)	This will need `self::CONFIG_COLUMN_SCHEMA` now to go green in Config > Database Status. Likely: 'name' => 'text255',
src/applications/almanac/storage/AlmanacDeviceProperty.php
13 ↗	(On Diff #25217)	As above. We'll be able to figure out the types of `devicePHID` based on the name (it's a PHID) and `value` because it's serialized, but not `key`: 'key' => 'text128',
14 ↗	(On Diff #25217)	Because this table has a special key, you'll also need `self::CONFIG_KEY_SCHEMA`, with something like: 'key_device' => array( 'columns' => array('devicePHID', 'key'), )