Page MenuHomePhabricator

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

Authored by hach-que on Sep 2 2014, 10:42 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 5:49 PM
Unknown Object (File)
Mon, Mar 25, 5:49 PM
Unknown Object (File)
Mon, Mar 25, 5:49 PM
Unknown Object (File)
Mon, Mar 25, 5:49 PM
Unknown Object (File)
Mon, Mar 25, 5:49 PM
Unknown Object (File)
Sat, Mar 16, 4:43 AM
Unknown Object (File)
Tue, Mar 5, 4:07 PM
Unknown Object (File)
Feb 10 2024, 3:37 PM
Subscribers

Details

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
arcpatch-D10400
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 2731
Build 2735: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

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.

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...

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.

hach-que edited edge metadata.

Changes based on code review feedback

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

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
4–5

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.

8

This should be UNIQUE KEY ...

13

This should be VARBINARY(64) now.

18

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
38–45

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
8

This is implicit; omit it.

13–14

This will need self::CONFIG_COLUMN_SCHEMA now to go green in Config > Database Status. Likely:

'name' => 'text255',
src/applications/almanac/storage/AlmanacDeviceProperty.php
14

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',
15

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'),
)
This revision is now accepted and ready to land.Oct 3 2014, 11:22 AM
hach-que edited edge metadata.

Update based on code review feedback

This revision was automatically updated to reflect the committed changes.