Page MenuHomePhabricator

Make SSH key revocation actually prevent adding the same key back
ClosedPublic

Authored by epriestley on Jan 25 2018, 2:44 PM.
Tags
None
Referenced Files
F12852743: D18928.diff
Fri, Mar 29, 7:43 AM
Unknown Object (File)
Wed, Mar 27, 1:01 AM
Unknown Object (File)
Sat, Mar 23, 11:35 PM
Unknown Object (File)
Wed, Mar 13, 5:36 PM
Unknown Object (File)
Feb 26 2024, 4:33 PM
Unknown Object (File)
Feb 26 2024, 2:01 PM
Unknown Object (File)
Jan 12 2024, 5:13 PM
Unknown Object (File)
Jan 1 2024, 4:57 PM
Subscribers
None

Details

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

Diff Detail

Repository
rP Phabricator
Branch
revoke1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 19178
Build 25899: Run Core Tests
Build 25898: arc lint + arc unit