1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-04-06 17:38:29 +02:00
Commit graph

8 commits

Author SHA1 Message Date
epriestley
69053a40f9 Dirty the SSH key cache when usernames change
Summary:
Fixes T12554. The SSH key cache contains usernames, but is not currently dirtied on username changes.

An alternative solution would be to use user PHIDs instead of usernames in the file, which would make this unnecessary, but that would make debugging a bit harder. For now, I think this small added complexity is worth the easier debugging, but we could look at this again if cache management gets harder in the future.

Test Plan:
  - Added a key as `ducksey`, ran `bin/ssh-auth`, saw key immediately.
  - Renamed `ducksey` to `ducker`, ran `bin/ssh-auth`, saw username change immediately.
  - Added another key as `ducker`, ran `bin/ssh-auth`, saw key immediately.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12554

Differential Revision: https://secure.phabricator.com/D17687
2017-04-14 08:03:00 -07:00
epriestley
c21a71f024 Cache generation of the SSH authentication keyfile for sshd
Summary:
Ref T11469. This isn't directly related, but has been on my radar for a while: building SSH keyfiles (particular for installs with a lot of keys, like ours) can be fairly slow.

At least one cluster instance is making multiple clone requests per second. While that should probably be rate limited separately, caching this should mitigate the impact of these requests.

This is pretty straightforward to cache since it's exactly the same every time, and only changes when users modify SSH keys (which is rare).

Test Plan:
  - Ran `bin/auth-ssh`, saw authfile generate.
  - Ran it again, saw it read from cache.
  - Changed an SSH key.
  - Ran it again, saw it regenerate.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11469

Differential Revision: https://secure.phabricator.com/D16744
2016-10-21 07:29:40 -07:00
epriestley
0308d580d7 Deactivate SSH keys instead of destroying them completely
Summary:
Ref T10917. Currently, when you delete an SSH key, we really truly delete it forever.

This isn't very consistent with other applications, but we built this stuff a long time ago before we were as rigorous about retaining data and making it auditable.

In partiular, destroying data isn't good for auditing after security issues, since it means we can't show you logs of any changes an attacker might have made to your keys.

To prepare to improve this, stop destoying data. This will allow later changes to become transaction-oriented and show normal transaction logs.

The tricky part here is that we have a `UNIQUE KEY` on the public key part of the key.

Instead, I changed this to `UNIQUE (key, isActive)`, where `isActive` is a nullable boolean column. This works because MySQL does not enforce "unique" if part of the key is `NULL`.

So you can't have two rows with `("A", 1)`, but you can have as many rows as you want with `("A", null)`. This lets us keep the "each key may only be active for one user/object" rule without requiring us to delete any data.

Test Plan:
- Ran schema changes.
- Viewed public keys.
- Tried to add a duplicate key, got rejected (already associated with another object).
- Deleted SSH key.
- Verified that the key was no longer actually deleted from the database, just marked inactive (in future changes, I'll update the UI to be more clear about this).
- Uploaded a new copy of the same public key, worked fine (no duplicate key rejection).
- Tried to upload yet another copy, got rejected.
- Generated a new keypair.
- Tried to upload a duplicate to an Almanac device, got rejected.
- Generated a new pair for a device.
- Trusted a device key.
- Untrusted a device key.
- "Deleted" a device key.
- Tried to trust a deleted device key, got "inactive" message.
- Ran `bin/ssh-auth`, got good output with unique keys.
- Ran `cat ~/.ssh/id_rsa.pub | ./bin/ssh-auth-key`, got good output with one key.
- Used `auth.querypublickeys` Conduit method to query keys, got good active keys.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10917

Differential Revision: https://secure.phabricator.com/D15943
2016-05-18 14:54:28 -07:00
Nick Zheng
8eec9e2c0e Provide a more straightforward way to revoke SSH keys by finding and destroying the objects
Summary: Ref T9967

Test Plan:
Ran migrations.
Verified database populated properly with PHIDs (SELECT * FROM auth_sshkey;).
Ran auth.querypublickeys conduit method to see phids show up
Ran bin/remove destroy <phid>.
Viewed the test key was gone.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T9967

Differential Revision: https://secure.phabricator.com/D14823
2015-12-19 11:52:26 -08:00
Evan Priestley
32cdc23efc Separate SSH key management from the settings panel
Summary:
Ref T5833. I want to add SSH keys to Almanac devices, but the edit workflows for them are currently bound tightly to users.

Instead, decouple key management from users and the settings panel.

Test Plan:
  - Uploaded, generated, edited and deleted SSH keys.
  - Hit missing name, missing key, bad key format, duplicate key errors.
  - Edited/generated/deleted/etc keys for a bot user as an administrator.
  - Got HiSec'd on everything.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5833

Differential Revision: https://secure.phabricator.com/D10824
2014-11-11 08:18:26 -08:00
epriestley
bf17b12daf Standardize SSH key storage
Summary:
Ref T5833. This fixes a few weird things with this table:

  - A bunch of columns were nullable for no reason.
  - We stored an MD5 hash of the key (unusual) but never used it and callers were responsible for manually populating it.
  - We didn't perform known-key-text lookups by using an index.

Test Plan:
  - Ran migrations.
  - Faked duplicate keys, saw them clean up correctly.
  - Added new keys.
  - Generated new keys.
  - Used `bin/auth-ssh` and `bin/auth-ssh-key`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5833

Differential Revision: https://secure.phabricator.com/D10805
2014-11-07 15:34:44 -08:00
epriestley
e29955b48d Move SSHKey table to Auth database
Summary: Ref T5833. Since these will no longer be bound specifically to users, bring them to a more central location.

Test Plan:
  - Edited SSH keys.
  - Ran `bin/ssh-auth` and `bin/ssh-auth-key`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5833

Differential Revision: https://secure.phabricator.com/D10791
2014-11-06 12:37:22 -08:00
epriestley
6f0d3b0796 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
2014-11-06 12:37:02 -08:00