New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`Connection_.init.forward_agent.defaults_to_False` fails when ssh-config enables agent forwarding for `*` #1753

Closed
offbyone opened this Issue May 14, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@offbyone

offbyone commented May 14, 2018

For example, ssh-config contains this:

Host *
    ForwardAgent=yes

This manifests as follows:

================================================================================================================= FAILURES ==================================================================================================================
_____________________________________________________________________________________________ Connection_.init.forward_agent.defaults_to_False ______________________________________________________________________________________________

self = <connection.Connection_.init.forward_agent object at 0x10c59c048>

    def defaults_to_False(self):
>       assert Connection('host').forward_agent is False
E       AssertionError: assert True is False
E        +  where True = <Connection host=host>.forward_agent
E        +    where <Connection host=host> = Connection('host')

tests/connection.py:161: AssertionError
============================================================================================== 1 failed, 229 passed, 6 skipped in 2.63 seconds ==============================================================================================
@offbyone

This comment has been minimized.

Show comment
Hide comment
@offbyone

offbyone May 14, 2018

This specifically references code in https://github.com/fabric/fabric/blob/master/fabric/connection.py#L309-L311

That bit there pulls in global SSH config, which isn't always False for Host: *.

offbyone commented May 14, 2018

This specifically references code in https://github.com/fabric/fabric/blob/master/fabric/connection.py#L309-L311

That bit there pulls in global SSH config, which isn't always False for Host: *.

@offbyone

This comment has been minimized.

Show comment
Hide comment
@offbyone

offbyone May 16, 2018

Okay, so... once paramiko/paramiko#1209 and paramiko/paramiko#1212 are merged in this can go forward :)

offbyone commented May 16, 2018

Okay, so... once paramiko/paramiko#1209 and paramiko/paramiko#1212 are merged in this can go forward :)

offbyone pushed a commit to offbyone/fabric that referenced this issue May 16, 2018

Chris Rose
Fix fabric#1753
The tests have a bug when a developer's host has its own ssh config
that sets agent forwarding to the nondefault.

offbyone pushed a commit to offbyone/fabric that referenced this issue May 16, 2018

Chris Rose
Fix fabric#1753
The tests have a bug when a developer's host has its own ssh config
that sets agent forwarding to the nondefault.
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jun 14, 2018

Member

Those issues are now closed, leaving me trying to remember exactly what we need to do here:

  • IIRC the proximate problem was that Chris was simply unable to run our tests, period, because his test environment has an SSH config that violates the test...
    • So a proximate solution would be e.g. to update the test/suite so it forces SSH config loading off by default
    • I don't see another, deeper solution offhand given the rest of the system is operating as intended (SSH config loading enabled by default, purposefully; and Connection.forward_agent reflecting the union of regular config and SSH config, with latter winnin)
  • An orthogonal problem was a few lines of ugly code at the Fabric level that is now fixed by the new Paramiko feature added in paramiko/paramiko#1212
    • Though if we want to rely on that, we need to bump up the required Paramiko version to 2.5. Ugh!
    • Probably just wants to be a "for Fabric 3.0" TODO instead. I need to settle on a better way to manage that anyways, even if it's just literally # TODO 3.0: xxx
Member

bitprophet commented Jun 14, 2018

Those issues are now closed, leaving me trying to remember exactly what we need to do here:

  • IIRC the proximate problem was that Chris was simply unable to run our tests, period, because his test environment has an SSH config that violates the test...
    • So a proximate solution would be e.g. to update the test/suite so it forces SSH config loading off by default
    • I don't see another, deeper solution offhand given the rest of the system is operating as intended (SSH config loading enabled by default, purposefully; and Connection.forward_agent reflecting the union of regular config and SSH config, with latter winnin)
  • An orthogonal problem was a few lines of ugly code at the Fabric level that is now fixed by the new Paramiko feature added in paramiko/paramiko#1212
    • Though if we want to rely on that, we need to bump up the required Paramiko version to 2.5. Ugh!
    • Probably just wants to be a "for Fabric 3.0" TODO instead. I need to settle on a better way to manage that anyways, even if it's just literally # TODO 3.0: xxx
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jun 14, 2018

Member

I note that almost all other SSH config related connection/config params in tests/connection.py already state load_ssh_configs=False in their overrides. So this is almost just an oversight in that regard.

We test the default behavior of SSH config loading (i.e. the fact that it is on by default) over in tests/config.py, so I suspect a global "no ssh config loading please" in tests/connection.py would be appropriate. Will see how much of a refactoring pain that is.

Member

bitprophet commented Jun 14, 2018

I note that almost all other SSH config related connection/config params in tests/connection.py already state load_ssh_configs=False in their overrides. So this is almost just an oversight in that regard.

We test the default behavior of SSH config loading (i.e. the fact that it is on by default) over in tests/config.py, so I suspect a global "no ssh config loading please" in tests/connection.py would be appropriate. Will see how much of a refactoring pain that is.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jun 15, 2018

Member

Nice, in setting that up I found a real shite bug, namely that gatewaying does not persist the outer connection's config object. So eg if you turn off SSH config loading but specify a jump host, the inner connection for the jump host...will load SSH config...!

(And in my case, I had a 'Host *' ProxyJump with no other PRoxyJump set up, as a temporary way of making these tests break, meaning infinite recursion! TOOT TOOT)

Member

bitprophet commented Jun 15, 2018

Nice, in setting that up I found a real shite bug, namely that gatewaying does not persist the outer connection's config object. So eg if you turn off SSH config loading but specify a jump host, the inner connection for the jump host...will load SSH config...!

(And in my case, I had a 'Host *' ProxyJump with no other PRoxyJump set up, as a temporary way of making these tests break, meaning infinite recursion! TOOT TOOT)

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jun 15, 2018

Member

I think this is Fixed Enough™ and now future edits to the connection test module will default to not loading SSH conf files at all. Not beautiful, but whatever.

Member

bitprophet commented Jun 15, 2018

I think this is Fixed Enough™ and now future edits to the connection test module will default to not loading SSH conf files at all. Not beautiful, but whatever.

bitprophet added a commit that referenced this issue Jun 15, 2018

@bitprophet bitprophet referenced this issue Jun 15, 2018

Closed

Fix #1753 #1763

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