Conflation of login and sudo passwords problematic with 2FA #1491

Closed
bitprophet opened this Issue Jul 22, 2016 · 3 comments

Projects

None yet

1 participant

@bitprophet
Member
bitprophet commented Jul 22, 2016 edited

Can't find open tickets about exactly this, probably because it's actually kind of a corner case.


"Normally", one's SSH login password and sudo password are the same value, which is why Fabric has always reused the one for the other by default. (See e.g. http://docs.fabfile.org/en/1.11/usage/execution.html#password-management)

So e.g. env.password (or the value obtained from env.passwords for current host), if set, is always handed to the Paramiko-level SSHClient.connect; this is then used to submit a password-auth request to the server (unless key auth succeeds first).

If the subsequent session includes calls to sudo, the same value is submitted in response to the sudo prompt.


A problem appears when one enables two-factor auth - e.g. key + Duo Security or other TOTP or push based 2FA, implemented as SSH's keyboard-interactive auth type - on the server:

  • As before, env.password is filled in, because that's required for happy sudoing
  • As before, key auth is tried first, and succeeds. So far so good.
  • But then the server says "you still need to satisfy another auth type"
  • Paramiko's auth handling notes that auth has not fully succeeded, sees that password has been set, and submits it - first as password auth (which will fail in this scenario) and then falling back to keyboard-interactive.
  • Unless your password just so happens to match the current timestamp's TOTP (staggeringly unlikely), this will cause a final auth failure, instead of surfacing the 2FA request to the interactive user.
  • Fabric then notices the auth exception and naively (thanks to other tickets) decides the user's password is bunk and prompts for a Login password.
    • Technically, if you knew that all this was happening, you could whip out your authenticator and submit the next TOTP at this prompt, and things would then connect happily.
    • But then, your TOTP is not your sudo password...so then you'd have to enter it by hand when that side of things fails.
    • And of course, you're not going to know this is happening - you'll just be confused about lack of 2FA prompt.

My initial instinct was that this represented a Paramiko problem. We could potentially streamline things there in some way, but it's not really Paramiko's fault - we gave Paramiko a "login password" so it's not unfair to expect that to be used for auto-submitting keyboard-interactive auth (and the way things are implemented now, clearly some users do use it that way - even if it seems to me unlikely to be useful for true 2FA).

Instead, the problem is simply that it's impossible to give Fabric distinct values for login and sudo passwords. Once that's possible, the above scenario is fixable by ensuring that the login password is None and the sudo password is filled in.

How to do this?

  • Obvious approach: add new env.sudo_password, preferentially check it over env.password at sudo time.
    • Downside: doesn't extend to env.passwords and I don't see a backwards compatible way to mutate that particular structure (e.g. to {<host-string>: {'sudo': 'blah', 'login':None}}).
      • Could mimic the top level change and also add env.sudo_passwords...
    • Backwards compatible
    • Don't see a need for the inverse/counterpart, env.login_password, because of the property that login passwords are by definition also sudo passwords.
  • Some additional setting like env.no_login_passwords that prevents use of env.password/etc at connect time - making it effectively only useful for sudo.
    • This seems far less intuitive than the first option & doesn't seem to have any benefits, but I'm noting it for shits and giggles.
  • Backwards incompatible change: literally split the two, no more env.password, only env.login_password and env.sudo_password, with the latter defaulting to the former if unset.
    • No good reason for this either, the first option works just as well and is backwards compatible.
    • In v2 we'll probably go this route; also want to have distinct key-passphrase "passwords" as that fixes other issues like #213 (and I think I saw dupes of that too).
      • E.g. config.auth.passwords = {'login': xxx, 'sudo': yyy, 'passphrase': zzz} or suchlike
@bitprophet
Member

One minor wrinkle, realized there's ambiguity about where user-entered sudo passwords get cached after this is implemented.

Given existing behavior is to cache in env.password/s, it feels like strict backwards compat requires us to continue doing that, even though the "sensible" thing would be to only cache sudo passwords into env.sudo_password/s. (Certainly, 2.x would be set up to do the equivalent.)

@bitprophet
Member

Also, rummaging around, don't see that we have good structures for testing sudo passwords in the fake test server. Given our spotty tests I'm likely to punt testing this for 2.x :(

@bitprophet bitprophet added a commit that referenced this issue Jul 22, 2016
@bitprophet bitprophet DDD re #1491 1b0f5a9
@bitprophet bitprophet added a commit that closed this issue Jul 22, 2016
@bitprophet bitprophet Changelog closes #1491 74b1d0a
@bitprophet
Member

097c224 nicely highlighting why I wish the 1.x codebase was more testable. Sigh.

Anyway, this works for me now, toot toot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment