Skip to content
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

avoid KeyError when canonicaldomains is missing #2338

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

totaam
Copy link

@totaam totaam commented Dec 19, 2023

paramiko assumes that canonicaldomains is always present when CanonicalizeHostname is set, and it errors out when it is not, preventing the whole host config from being parsed.

Xpra-org/xpra#3868 (comment)

totaam referenced this pull request Dec 19, 2023
paramiko assumes that `canonicaldomains` is always present when `CanonicalizeHostname` is set, and it errors out when it is not, preventing the whole host config from being parsed.

Xpra-org/xpra#3868 (comment)
paramiko/config.py Outdated Show resolved Hide resolved
@bskinn
Copy link
Contributor

bskinn commented Dec 19, 2023

It would be good to add a test case for the bugged situation here, to guard against future regression -- is that something you could tackle, @totaam?

Also, we'll want a CHANGELOG entry for this -- please add a bullet to /sites/www/changelog.rst following the format used there.

Thanks for this!

@totaam
Copy link
Author

totaam commented Dec 20, 2023

I can take a look, not sure when.
(I am travelling, hence the botched conversion of the working patch into a borken PR)

@bskinn
Copy link
Contributor

bskinn commented Dec 20, 2023

No worries on timeline, @totaam, happy to take care of this whenever you're able.

paramiko assumes that `canonicaldomains` is always present when `CanonicalizeHostname` is set, and it errors out when it is not, preventing the whole host config from being parsed.

Xpra-org/xpra#3868 (comment)
@totaam
Copy link
Author

totaam commented Jan 24, 2024

The updated PR adds a test which verifies that paramiko no longer blows up when a config specifies canonicalizehostname without also specifying canonicaldomains.

The failing check is not related to the changes I made so I am ignoring it.

@totaam
Copy link
Author

totaam commented Mar 14, 2024

Please review this PR, the failing test is not caused by it.

@totaam
Copy link
Author

totaam commented May 8, 2024

No worries on timeline, @totaam, happy to take care of this whenever you're able.

@bskinn how about now?

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

Successfully merging this pull request may close these issues.

None yet

2 participants