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

Need guardrails re: recursion in ProxyJump chained Connections #1850

Closed
bitprophet opened this Issue Aug 15, 2018 · 9 comments

Comments

Projects
None yet
2 participants
@bitprophet
Member

bitprophet commented Aug 15, 2018

Moving pyinvoke/invoke#564 over here - @acdha is getting recursion errors on line 331 of latest public release, which is where we create new Connection objects from ProxyJump in SSH configs:

fabric/fabric/connection.py

Lines 316 to 331 in 4b1a2ab

if gateway is None:
# SSH config wins over Invoke-style config
if "proxyjump" in self.ssh_config:
# Reverse hop1,hop2,hop3 style ProxyJump directive so we start
# with the final (itself non-gatewayed) hop and work up to
# the front (actual, supplied as our own gateway) hop
hops = reversed(self.ssh_config["proxyjump"].split(","))
prev_gw = None
for hop in hops:
# Happily, ProxyJump uses identical format to our host
# shorthand...
if prev_gw is None:
# TODO: this isn't persisting config! which among other
# things can lead to not honoring skipping ssh config
# file loads...
cxn = Connection(hop)

Chris, can you share your (sanitized) ~/.ssh/config, as well as the actual CLI invocation you're using to arrive at the traceback? This functionality has been tested in a basic sense so I'm guessing there might be an angle to your setup that is either a legit mistake on your part (in which case we still need some sort of detection/exception guardrail) or a bug on my end (99.9% possible) re: some edge case.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 15, 2018

Member

As you can see in that snippet, we don't currently persist (inv/fab) config objects from the outer to inner Connections, but right now that should probably only matter if one was trying to avoid loading SSH configs entirely.

Member

bitprophet commented Aug 15, 2018

As you can see in that snippet, we don't currently persist (inv/fab) config objects from the outer to inner Connections, but right now that should probably only matter if one was trying to avoid loading SSH configs entirely.

@acdha

This comment has been minimized.

Show comment
Hide comment
@acdha

acdha Aug 15, 2018

Contributor

Oh, I bet I know what it is: I have a ProxyJump host which would match the wildcard. OpenSSH knows not to endlessly nest but that logic would need to be present on the Fabric side as well:

Host bastion-host.tld
    ProxyCommand none

Host *.tld
    ProxyJump bastion-host.tld
Contributor

acdha commented Aug 15, 2018

Oh, I bet I know what it is: I have a ProxyJump host which would match the wildcard. OpenSSH knows not to endlessly nest but that logic would need to be present on the Fabric side as well:

Host bastion-host.tld
    ProxyCommand none

Host *.tld
    ProxyJump bastion-host.tld
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 15, 2018

Member

Perfect, that makes a lot of sense, should be fixable (might need to be on the Paramiko side, depending...)

Also, re: why disabling SSH config loading didn't help (as noted in #1851) - see above - we don't persist our own config which means the gateways will go back to loading SSH configs. I might as well fix that while I'm in here...

Member

bitprophet commented Aug 15, 2018

Perfect, that makes a lot of sense, should be fixable (might need to be on the Paramiko side, depending...)

Also, re: why disabling SSH config loading didn't help (as noted in #1851) - see above - we don't persist our own config which means the gateways will go back to loading SSH configs. I might as well fix that while I'm in here...

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 15, 2018

Member

Suspect it probably does need to be fixed here; I'm assuming SSH config files should still represent their literal contents & not try to be "smart", meaning that we should get this:

  • You do fab -H somehost.tld or equivalent
  • We get a Connection for somehost.tld
  • Its SSH config is derived as having ProxyJump bastion-host.tld due to the wildcard (Paramiko level logic)
  • Connection sees this, generates gateway Connection object for bastion-host.tld
  • That loads its own SSH config, which will have both ProxyCommand none and ProxyJump bastion-host.tld
  • We then need to detect that we should not be using the ProxyJump because its target is ourselves. (Versus: having the SSH config itself strip out such a self-referential ProxyJump line.)
    • Needs care re: fully qualified such strings, but presumably ProxyJump user@host.tld should still be considered a distinct jump target than just ProxyJump host.tld... 🤔

@acdha I'm also curious about the ProxyCommand none, is that disabling some other ProxyCommand not visible in your snippet? It seems like it shouldn't be relevant here otherwise.

Member

bitprophet commented Aug 15, 2018

Suspect it probably does need to be fixed here; I'm assuming SSH config files should still represent their literal contents & not try to be "smart", meaning that we should get this:

  • You do fab -H somehost.tld or equivalent
  • We get a Connection for somehost.tld
  • Its SSH config is derived as having ProxyJump bastion-host.tld due to the wildcard (Paramiko level logic)
  • Connection sees this, generates gateway Connection object for bastion-host.tld
  • That loads its own SSH config, which will have both ProxyCommand none and ProxyJump bastion-host.tld
  • We then need to detect that we should not be using the ProxyJump because its target is ourselves. (Versus: having the SSH config itself strip out such a self-referential ProxyJump line.)
    • Needs care re: fully qualified such strings, but presumably ProxyJump user@host.tld should still be considered a distinct jump target than just ProxyJump host.tld... 🤔

@acdha I'm also curious about the ProxyCommand none, is that disabling some other ProxyCommand not visible in your snippet? It seems like it shouldn't be relevant here otherwise.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 15, 2018

Member

lmao, the other side bug re: not persisting config is also making it impossible to reproduce the main bug (since my test setup relies on a blanket disabling of SSH config loading, then explicitly enabling specific support-level SSH config files for specific tests...)

Guess I'm doing that one first.

Member

bitprophet commented Aug 15, 2018

lmao, the other side bug re: not persisting config is also making it impossible to reproduce the main bug (since my test setup relies on a blanket disabling of SSH config loading, then explicitly enabling specific support-level SSH config files for specific tests...)

Guess I'm doing that one first.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 15, 2018

Member

Cool, fixing #1852 gets my test for this bug failing as anticipated. On to the fix. EDIT: oh actually it's a different RecursionError. That's...interesting. Hopefully still related. EDIT 2: hrm no I think maybe recursion error tracebacks, or maybe pytest's own black magic around this, just acts funny; the stack itself still shows the expected recursion (creating new Connections forever).

Member

bitprophet commented Aug 15, 2018

Cool, fixing #1852 gets my test for this bug failing as anticipated. On to the fix. EDIT: oh actually it's a different RecursionError. That's...interesting. Hopefully still related. EDIT 2: hrm no I think maybe recursion error tracebacks, or maybe pytest's own black magic around this, just acts funny; the stack itself still shows the expected recursion (creating new Connections forever).

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 16, 2018

Member

In a rabbit hole surrounding proxyjump target identity re: user/port values; technically speaking one might want their proxy to be very specific and wouldn't want this recursion detection to be too widely applied.

However, this adds a surprising amount of extra effort around testing all the angles, and I can't see it being that frequent a problem (seriously, how often are you going to be trying to get to, say, bastion.tld:123 via bastion.tld:2222 or whatever the heck?)

So I'm punting on that for now 😝

Member

bitprophet commented Aug 16, 2018

In a rabbit hole surrounding proxyjump target identity re: user/port values; technically speaking one might want their proxy to be very specific and wouldn't want this recursion detection to be too widely applied.

However, this adds a surprising amount of extra effort around testing all the angles, and I can't see it being that frequent a problem (seriously, how often are you going to be trying to get to, say, bastion.tld:123 via bastion.tld:2222 or whatever the heck?)

So I'm punting on that for now 😝

bitprophet added a commit that referenced this issue Aug 16, 2018

bitprophet added a commit that referenced this issue Aug 16, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 16, 2018

Member

Another thing I am punting on is when somebody REALLY goofs up and does something like Connection("user@host:123", gateway="user@host:123"), which would technically be a similar class of error.

Anyway, I got my tests to pass, so this should be good now? Will be out in a bugfix release soon™.

Member

bitprophet commented Aug 16, 2018

Another thing I am punting on is when somebody REALLY goofs up and does something like Connection("user@host:123", gateway="user@host:123"), which would technically be a similar class of error.

Anyway, I got my tests to pass, so this should be good now? Will be out in a bugfix release soon™.

@bitprophet bitprophet closed this Aug 16, 2018

@acdha

This comment has been minimized.

Show comment
Hide comment
@acdha

acdha Aug 16, 2018

Contributor

Thanks!

Contributor

acdha commented Aug 16, 2018

Thanks!

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