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

key_filename value from Collection.config and config file is not passed on connect to paramiko #1762

Closed
garu57 opened this Issue May 15, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@garu57

garu57 commented May 15, 2018

Python 3.6.5, Invoke 1.0.0, fabric 2.0.0, paramiko 2.4.1

First of all the argument is called key_filenames in documentation, but if used that way gives:
TypeError: connect() got an unexpected keyword argument 'key_filenames'

But the real problem is that either using it in Collection.confg or fabric.yaml or both, key_filename value is not passed to paramiko.
It looks like during the config merge phase it is detected, but overridden with an empty one {'connect_kwargs': {'key_filename': []}}, leading to exception:
paramiko.ssh_exception.SSHException: No authentication methods available

Launching fab -d -H c04801029 check-login > c:\tmp\fab.log 2>&1.
Debug log: fab.log

Here you see the key_filename initialized from fabric.yaml

connect_kwargs:
  key_filename: 
    - C:/home/ProjectsDoc/Tech/ssh_keys/21047012@ib024.priv
user: root

but initializing it from Collection.config or both together gives the very same result.

@garu57

This comment has been minimized.

Show comment
Hide comment
@garu57

garu57 May 15, 2018

Looks like the problem is with overrides because launching
fab -d -H c04801029 -i C:/home/ProjectsDoc/Tech/ssh_keys/21047012@ib024.priv check-login
leads to correct login.

garu57 commented May 15, 2018

Looks like the problem is with overrides because launching
fab -d -H c04801029 -i C:/home/ProjectsDoc/Tech/ssh_keys/21047012@ib024.priv check-login
leads to correct login.

@garu57 garu57 changed the title from key_filename value is not passed on connect to paramiko to key_filename value from Collection.config and config file is not passed on connect to paramiko May 15, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jun 21, 2018

Member

Thanks for the report, just fixed the docs typo, and the rest I'll have to look at. Hoping it doesn't represent a design-level issue with the config system, but it might 😒

EDIT: to clarify the latter, the config system ought to be merging between levels without issue, and as you saw, -i manages to do that somehow. I cannot recall offhand if overrides are special and stomp on stuff on purpose; or if something else that's a more simple bug is causing this; or what.

This is absolutely a bug that needs fixing either way, since configuring one's default auth/cxn params is a top tier piece of functionality.

Member

bitprophet commented Jun 21, 2018

Thanks for the report, just fixed the docs typo, and the rest I'll have to look at. Hoping it doesn't represent a design-level issue with the config system, but it might 😒

EDIT: to clarify the latter, the config system ought to be merging between levels without issue, and as you saw, -i manages to do that somehow. I cannot recall offhand if overrides are special and stomp on stuff on purpose; or if something else that's a more simple bug is causing this; or what.

This is absolutely a bug that needs fixing either way, since configuring one's default auth/cxn params is a top tier piece of functionality.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jun 22, 2018

Member

Also this may need to move to Invoke if it's only at that level, but investigation is needed first.

Member

bitprophet commented Jun 22, 2018

Also this may need to move to Invoke if it's only at that level, but investigation is needed first.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

Looking at this now; yea, the debug log (which I hadn't looked at initially) definitely confirms the OP's suspicion that the overrides level is winning:

invoke.config.merge: Overrides: {'run': {}, 'tasks': {}, 'sudo': {}, 'connect_kwargs': {'key_filename': []}}

That comes after the per-project config file load line, which does show the user's desired value appearing there. So this has something to do with how we're populating the overrides layer, probably at the time when we take over from Invoke's default config stuff and add fabric-level specifics.


What's weird is that we have a big ol' block of tests around all the various config layer interactions, specifically aimed at key_filename because it's got its own related CLI option (--identity) and thus needs special care.

Those tests:

fabric/tests/connection.py

Lines 651 to 749 in 3ab2d55

class connect_kwargs_key_filename:
"connect_kwargs(key_filename=...)"
# TODO: it'd be nice to truly separate CLI from regular (non override
# level) invoke config; as it is, invoke config comes first in expected
# outputs since otherwise there's no way for --identity to "come
# first".
@pytest.mark.parametrize(
"ssh, invoke, kwarg, expected",
[
param(
True,
True,
True,
[
"configured.key",
"kwarg.key",
"ssh-config-B.key",
"ssh-config-A.key",
],
id="All sources",
),
param(False, False, False, [], id="No sources"),
param(
True,
False,
False,
["ssh-config-B.key", "ssh-config-A.key"],
id="ssh_config only",
),
param(
False,
True,
False,
["configured.key"],
id="Invoke-level config only",
),
param(
False,
False,
True,
["kwarg.key"],
id="Connection kwarg only",
),
param(
True,
True,
False,
["configured.key", "ssh-config-B.key", "ssh-config-A.key"],
id="ssh_config + invoke config, no kwarg",
),
param(
True,
False,
True,
["kwarg.key", "ssh-config-B.key", "ssh-config-A.key"],
id="ssh_config + kwarg, no Invoke-level config",
),
param(
False,
True,
True,
["configured.key", "kwarg.key"],
id="Invoke-level config + kwarg, no ssh_config",
),
],
)
def merges_sources(self, client, ssh, invoke, kwarg, expected):
config_kwargs = {}
if ssh:
# SSH config with 2x IdentityFile directives.
config_kwargs["runtime_ssh_path"] = join(
support, "ssh_config", "runtime_identity.conf"
)
if invoke:
# Use overrides config level to mimic --identity use NOTE: (the
# fact that --identity is an override, and thus overrides eg
# invoke config file values is part of invoke's config test
# suite)
config_kwargs["overrides"] = {
"connect_kwargs": {"key_filename": ["configured.key"]}
}
conf = Config_(**config_kwargs)
connect_kwargs = {}
if kwarg:
# Stitch in connect_kwargs value
connect_kwargs = {"key_filename": ["kwarg.key"]}
# Tie in all sources that were configured & open()
Connection(
"runtime", config=conf, connect_kwargs=connect_kwargs
).open()
# Ensure we got the expected list of keys
kwargs = client.connect.call_args[1]
if expected:
assert kwargs["key_filename"] == expected
else:
# No key filenames -> it's not even passed in as connect_kwargs
# is gonna be a blank dict
assert "key_filename" not in kwargs

Output currently:

» inv test -m connection -k connect_kwargs_key_filename                                                                                                 
================================================================== test session starts ===================================================================
platform darwin -- Python 3.6.4, pytest-3.2.5, py-1.5.3, pluggy-0.4.0 -- /Users/jforcier/.virtualenvs/fabric/bin/python3.6
cachedir: .cache
rootdir: /Users/jforcier/Code/oss/fabric, inifile: setup.cfg
plugins: relaxed-1.0.0, cov-2.5.1
collected 130 items                                                                                                                                       

Connection

    connect kwargs key filename

        merges sources[All sources]
        merges sources[No sources]
        merges sources[ssh config only]
        merges sources[Invoke-level config only]
        merges sources[Connection kwarg only]
        merges sources[ssh config + invoke config, no kwarg]
        merges sources[ssh config + kwarg, no Invoke-level config]
        merges sources[Invoke-level config + kwarg, no ssh config]

================================================================== 122 tests deselected ==================================================================
======================================================== 8 passed, 122 deselected in 0.23 seconds ========================================================

Notice how we're explicitly weaving in the 'Invoke config'. I'm guessing that this is too high-level and we're feeding in an explicit Config instance instead of testing interactions between config levels (the emphasis was on ssh_config data vs CLI/kwarg vs Invoke config; in the real world example, the Invoke config is likely coming up with that incorrect empty list.)


Going to try writing some more focused tests in the Config test suite to expose whatever's really going on.

Member

bitprophet commented Jul 24, 2018

Looking at this now; yea, the debug log (which I hadn't looked at initially) definitely confirms the OP's suspicion that the overrides level is winning:

invoke.config.merge: Overrides: {'run': {}, 'tasks': {}, 'sudo': {}, 'connect_kwargs': {'key_filename': []}}

That comes after the per-project config file load line, which does show the user's desired value appearing there. So this has something to do with how we're populating the overrides layer, probably at the time when we take over from Invoke's default config stuff and add fabric-level specifics.


What's weird is that we have a big ol' block of tests around all the various config layer interactions, specifically aimed at key_filename because it's got its own related CLI option (--identity) and thus needs special care.

Those tests:

fabric/tests/connection.py

Lines 651 to 749 in 3ab2d55

class connect_kwargs_key_filename:
"connect_kwargs(key_filename=...)"
# TODO: it'd be nice to truly separate CLI from regular (non override
# level) invoke config; as it is, invoke config comes first in expected
# outputs since otherwise there's no way for --identity to "come
# first".
@pytest.mark.parametrize(
"ssh, invoke, kwarg, expected",
[
param(
True,
True,
True,
[
"configured.key",
"kwarg.key",
"ssh-config-B.key",
"ssh-config-A.key",
],
id="All sources",
),
param(False, False, False, [], id="No sources"),
param(
True,
False,
False,
["ssh-config-B.key", "ssh-config-A.key"],
id="ssh_config only",
),
param(
False,
True,
False,
["configured.key"],
id="Invoke-level config only",
),
param(
False,
False,
True,
["kwarg.key"],
id="Connection kwarg only",
),
param(
True,
True,
False,
["configured.key", "ssh-config-B.key", "ssh-config-A.key"],
id="ssh_config + invoke config, no kwarg",
),
param(
True,
False,
True,
["kwarg.key", "ssh-config-B.key", "ssh-config-A.key"],
id="ssh_config + kwarg, no Invoke-level config",
),
param(
False,
True,
True,
["configured.key", "kwarg.key"],
id="Invoke-level config + kwarg, no ssh_config",
),
],
)
def merges_sources(self, client, ssh, invoke, kwarg, expected):
config_kwargs = {}
if ssh:
# SSH config with 2x IdentityFile directives.
config_kwargs["runtime_ssh_path"] = join(
support, "ssh_config", "runtime_identity.conf"
)
if invoke:
# Use overrides config level to mimic --identity use NOTE: (the
# fact that --identity is an override, and thus overrides eg
# invoke config file values is part of invoke's config test
# suite)
config_kwargs["overrides"] = {
"connect_kwargs": {"key_filename": ["configured.key"]}
}
conf = Config_(**config_kwargs)
connect_kwargs = {}
if kwarg:
# Stitch in connect_kwargs value
connect_kwargs = {"key_filename": ["kwarg.key"]}
# Tie in all sources that were configured & open()
Connection(
"runtime", config=conf, connect_kwargs=connect_kwargs
).open()
# Ensure we got the expected list of keys
kwargs = client.connect.call_args[1]
if expected:
assert kwargs["key_filename"] == expected
else:
# No key filenames -> it's not even passed in as connect_kwargs
# is gonna be a blank dict
assert "key_filename" not in kwargs

Output currently:

» inv test -m connection -k connect_kwargs_key_filename                                                                                                 
================================================================== test session starts ===================================================================
platform darwin -- Python 3.6.4, pytest-3.2.5, py-1.5.3, pluggy-0.4.0 -- /Users/jforcier/.virtualenvs/fabric/bin/python3.6
cachedir: .cache
rootdir: /Users/jforcier/Code/oss/fabric, inifile: setup.cfg
plugins: relaxed-1.0.0, cov-2.5.1
collected 130 items                                                                                                                                       

Connection

    connect kwargs key filename

        merges sources[All sources]
        merges sources[No sources]
        merges sources[ssh config only]
        merges sources[Invoke-level config only]
        merges sources[Connection kwarg only]
        merges sources[ssh config + invoke config, no kwarg]
        merges sources[ssh config + kwarg, no Invoke-level config]
        merges sources[Invoke-level config + kwarg, no ssh config]

================================================================== 122 tests deselected ==================================================================
======================================================== 8 passed, 122 deselected in 0.23 seconds ========================================================

Notice how we're explicitly weaving in the 'Invoke config'. I'm guessing that this is too high-level and we're feeding in an explicit Config instance instead of testing interactions between config levels (the emphasis was on ssh_config data vs CLI/kwarg vs Invoke config; in the real world example, the Invoke config is likely coming up with that incorrect empty list.)


Going to try writing some more focused tests in the Config test suite to expose whatever's really going on.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

Also, looking at the code, I have a hunch this applies to all connect_kwargs since that's the level the Fabric config subclass cares about; it doesn't look inside at key_filename specifically. EDIT: except, we're seeing it ending up specifically set, which is...odd. Maybe I'm wrong. It's happened at least once or twice before... 🙃

Member

bitprophet commented Jul 24, 2018

Also, looking at the code, I have a hunch this applies to all connect_kwargs since that's the level the Fabric config subclass cares about; it doesn't look inside at key_filename specifically. EDIT: except, we're seeing it ending up specifically set, which is...odd. Maybe I'm wrong. It's happened at least once or twice before... 🙃

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

Seems the proximate cause is some too-strict value checking in Fabric's extension of Program.update_config (at the end of this highlighted block):

  • The comment about not losing data is clearly incorrect. LOL?
  • Code is filling in the overrides layer with data sourced from the CLI: key filename/identity, login password, key passphrase.
  • Because it's the overrides layer, the config system itself is doing its job as designed: overrides win over anything below them.
  • The primary problem is simply testing the CLI-derived value for None-ness instead of truthiness; it's what allows the default empty list value to always get slotted in.
  • A secondary problem is that we've documented --identity as appending; this is correct insofar as it appends to itself (one can give fab -i key1 -i key2 to build a list) but as written, even if the None bit gets fixed, we'll still end up overriding any configured values.
    • On one hand, this could be arguably correct, because CLI
    • However, every other config source for key_filename specifically is documented as appending too - SSH config, runtime kwarg, etc. So this really ought to append anyways.
Member

bitprophet commented Jul 24, 2018

Seems the proximate cause is some too-strict value checking in Fabric's extension of Program.update_config (at the end of this highlighted block):

  • The comment about not losing data is clearly incorrect. LOL?
  • Code is filling in the overrides layer with data sourced from the CLI: key filename/identity, login password, key passphrase.
  • Because it's the overrides layer, the config system itself is doing its job as designed: overrides win over anything below them.
  • The primary problem is simply testing the CLI-derived value for None-ness instead of truthiness; it's what allows the default empty list value to always get slotted in.
  • A secondary problem is that we've documented --identity as appending; this is correct insofar as it appends to itself (one can give fab -i key1 -i key2 to build a list) but as written, even if the None bit gets fixed, we'll still end up overriding any configured values.
    • On one hand, this could be arguably correct, because CLI
    • However, every other config source for key_filename specifically is documented as appending too - SSH config, runtime kwarg, etc. So this really ought to append anyways.
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

Hrm, so yea first fix easy, second fix tho...harder, because overrides are what they are. No good way to say "actually merge this one key instead of overriding with it".

I'd bet most people giving -i would want it to remove, not merge with, configured keys. But the docs do say 'appends to' right now. Bleh. (This is due for a mild shakeup once paramiko/paramiko#387 finally lands, since that would allow the possibility of explicit different auth sources – such as giving more than one key, separately, instead of being forced into the single key_filenames list.)

Given that the actual behavior (due to this bug) resulted in "CLI wins" anyways and nobody could be relying on "CLI merges with config", I'm thinking I'll just tweak the docs and say we meant this all along. Yes...clearly. steeples fingers

Member

bitprophet commented Jul 24, 2018

Hrm, so yea first fix easy, second fix tho...harder, because overrides are what they are. No good way to say "actually merge this one key instead of overriding with it".

I'd bet most people giving -i would want it to remove, not merge with, configured keys. But the docs do say 'appends to' right now. Bleh. (This is due for a mild shakeup once paramiko/paramiko#387 finally lands, since that would allow the possibility of explicit different auth sources – such as giving more than one key, separately, instead of being forced into the single key_filenames list.)

Given that the actual behavior (due to this bug) resulted in "CLI wins" anyways and nobody could be relying on "CLI merges with config", I'm thinking I'll just tweak the docs and say we meant this all along. Yes...clearly. steeples fingers

bitprophet added a commit that referenced this issue Jul 24, 2018

bitprophet added a commit that referenced this issue Jul 24, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 24, 2018

Member

Will be out in next bugfix release. #soon

Member

bitprophet commented Jul 24, 2018

Will be out in next bugfix release. #soon

@bitprophet bitprophet closed this Jul 24, 2018

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