default SSH host key policy is insecure

[anarcat]

I was quite surprised to find that code in Fabric's Connection class:

        #: The `paramiko.client.SSHClient` instance this connection wraps.
        client = SSHClient()
        client.set_missing_host_key_policy(AutoAddPolicy())
        self.client = client

This changes the default connect policy from RejectPolicy() to AutoAddPolicy which means that a ssh connexion will not warn when an SSH key changes or on new keys. I find this behavior questionable, especially since it's actually very hard to change, because it's embedded deep inside Fabric (inside the Connection constructor).

I tried to monkeypatch this away in a a prototype.py but could not: make this work:

#!/usr/bin/python3

import fabric
import fabric.connection

class SaferConnection(Connection):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.client.set_missing_host_key_policy(RejectPolicy())

fabric.Connection = SaferConnection
fabric.connection.Connection = SaferConnection

c = fabric.Connection('root@88.99.194.57')

c.run('uptime')

for some weird reason, this crashes with:

anarcat@curie:tsa-misc(master)$ ./spike.py 
Traceback (most recent call last):
  File "./spike.py", line 10, in <module>
    c = fabric.Connection('root@88.99.194.57')
  File "/home/anarcat/src/tor/tsa-misc/fabric_tpa/__init__.py", line 56, in __init__
    super().__init__(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/fabric/connection.py", line 371, in __init__
    super(Connection, self).__init__(config=config)
TypeError: __init__() missing 1 required positional argument: 'host'

It looks like the pre-py3 super() call is causing problems here. I tried to fix that in cd004d4 but it seems it is a recurring problem in the code and, besides, it's hardly an intuitive way to make a change.

Shouldn't the default here be safer? Maybe have a prompt like SSH does instead of just gobbling up any SSH key change? I understand if we would want to do TOFU and just trust unknown fingerprints, but from what I can tell, Fabric, by default, will trust any SSH server fingerprint whatsoever.

I find this terrifying.

[anarcat]

i found that adding this to my fabfile.py will at least make it "safe" insofar as it will just crash if Fabric isn't properly patched to support it:

# monkeypatch the default fabric connexion to workaround
# https://github.com/fabric/fabric/issues/2071
logging.warning('Fabric Connection monkeypatched, will crash without a patch')
fabric.Connection = SaferConnection
fabric.connection.Connection = SaferConnection

[anarcat]

i ended up doing this in the short term:

# hack to enforce a different key policy: https://github.com/fabric/fabric/issues/2071
def safe_open(self):
    self.client.set_missing_host_key_policy(RejectPolicy())
    Connection.open_orig(self)


Connection.open_orig = Connection.open
Connection.open = safe_open

in the long term, i would much prefer if the policy was set to Warning or at least customizable somewhat.

[RedKrieg]

After some digging, I think this happens because when the connection is made, nothing ever calls paramiko's client.load_system_host_keys() function, but by using AutoAddPolicy() we are relying on those keys to be loaded by default for security. I would add a --no-known-hosts flag to fabric to conditionally skip calling client.load_system_host_keys() for each Connection() instead of changing to RejectPolicy() by default.

[anarcat]

indeed, we are not loading any host keys at all, which makes AutoAddPolicy() fail open (which is pretty bad for Paramiko to do, if you ask me), but that's a different question.

i started working on this problem in #2072, maybe we can followup there. my approach is to:

1. make the policy customizable
2. load the (proper) known_hosts file

that should be sufficient for my use case.

[harshitgupta1337]

Has there been any updates on this issue? Any plans for merging it?

[anarcat]

this is still a problem, latest is in #2076

[anarcat]

@bitprophet i'm sorry to ping you about this one, but it seems to me this is a capital one to fix. At the very least make the default policy warning! i made a more minimal change in #2280 to change the policy to RejectPolicy by default, hopefully that will be easier to merge?

at least we should be warning here... but I think WarningPolicy is too light...