recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Make SSH key revocation actually prevent adding the same key back

Summary:
Ref T13043. In an earlier change I updated this langauge from "Deactivate" to "Revoke", but the behavior doesn't quite match.

This table has a unique key on `<isActive, keyBody>`, which enforces the rule that "a key can only be active for one unique user".

However, we set `isActive` to `null` when we revoke a key, and multiple rows are allowed to have the value `<null, "asdf">` (since a `null` column in a unique key basically means "don't enforce this unique key").

This is intentional, to support this workflow:

- You add key X to bot A.
- Whoops, wrong account.
- You revoke key X from bot A.
- You add key X to bot B.

This isn't necessarily a great workflow -- ideally, you'd throw key X away and go generate a new key after you realize you made a mistake -- but it's the sort of practical workflow that users are likely to expect and want to see work ("I don't want to generate a new key, it's already being used by 5 other services and cycling it is a ton of work and this is just a test install for my dog anyway."), and there's no technical reason we can't support it.

To prevent users from adding keys on the revocation list back to their account, just check explicitly.

(This is probably better in general anyway, because "cert-authority" support from PHI269 may mean that two keys are "equivalent" even if their text differs, and we may not be able to rely on a database test anyway.)

Test Plan:
- Added the key `ssh-rsa asdf` to my account.
- Revoked it.
- Tried to add it again.
- Before patch: worked.
- After patch: error, "this key has been revoked".
- Added it to a different account (the "I put it on the wrong bot" workflow).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18928

epriestley deb754df 1059fae6

+25
+25
src/applications/auth/editor/PhabricatorAuthSSHKeyEditor.php
··· 104 104 array $xactions) { 105 105 106 106 $errors = parent::validateTransaction($object, $type, $xactions); 107 + $viewer = $this->requireActor(); 107 108 108 109 switch ($type) { 109 110 case PhabricatorAuthSSHKeyTransaction::TYPE_NAME: ··· 149 150 pht('Invalid'), 150 151 $ex->getMessage(), 151 152 $xaction); 153 + continue; 154 + } 155 + 156 + // The database does not have a unique key on just the <keyBody> 157 + // column because we allow multiple accounts to revoke the same 158 + // key, so we can't rely on database constraints to prevent users 159 + // from adding keys that are on the revocation list back to their 160 + // accounts. Explicitly check for a revoked copy of the key. 161 + 162 + $revoked_keys = id(new PhabricatorAuthSSHKeyQuery()) 163 + ->setViewer($viewer) 164 + ->withObjectPHIDs(array($object->getObjectPHID())) 165 + ->withIsActive(0) 166 + ->withKeys(array($public_key)) 167 + ->execute(); 168 + if ($revoked_keys) { 169 + $errors[] = new PhabricatorApplicationTransactionValidationError( 170 + $type, 171 + pht('Revoked'), 172 + pht( 173 + 'This key has been revoked. Choose or generate a new, '. 174 + 'unique key.'), 175 + $xaction); 176 + continue; 152 177 } 153 178 } 154 179 }