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

Fix hostname canonicalization with GSSAPI #1063

Closed
wants to merge 1 commit into
base: 2.2
from

Conversation

Projects
None yet
4 participants
@SebastianDeiss
Contributor

SebastianDeiss commented Sep 13, 2017

I added the parameter gss_trust_dns in client.py and transport.py. Set by default to True the parameter indicates whether or not the DNS is trusted to securely canonicalize the hostname of the target host. If set to False the hostname entered will be passed to GSSAPI.
This option behaves like GSSAPITrustDNS from OpenSSH.

Also, the option gss_host, if set, can be used to override the hostname for GSSAPI and therefore is considered as the equivalent to GSSAPIServerIdentity from OpenSSH.

This pull should fix issue #915 and it supersedes pull #919 .

Invent the parameter 'gss_trust_dns' for Kerberos support
In response to Paramiko issue #915 the parameter 'gss_trust_dns' was
added for Kerberos support. Set by default to 'True' the parameter
indicates whether or not the DNS is trusted to securely canonicalize
the hostname of the target host. If set to 'False' the hostname
entered will be passed to GSSAPI.
This option behaves like GSSAPITrustDNS from OpenSSH.
Also, the parameter 'gss_host' is now always set, regardless if GSSAPI
is used or not.
Further, a minor fix was required to make the SFTP test work again.
@@ -228,6 +228,7 @@ def connect(
gss_host=None,
banner_timeout=None,
auth_timeout=None,
gss_trust_dns=True

This comment has been minimized.

@ploxiln

ploxiln Sep 13, 2017

Contributor

This should probably be next to the other gss args - I think it's pretty unlikely that anyone is depending on the args order instead of using kwargs by midpoint of this args list.

I also wonder if most of the gss args can be boiled down to a single "gss context" argument. (For backwards compat it would still need an additional kwarg here, but it would be the last one for gss ;)

This comment has been minimized.

@bitprophet

bitprophet Sep 13, 2017

Member

I think for now the (yes, minor) benefit of preserving backwards compat arg order is worth leaving this alone, but partly because I agree that later we should probably compress them into a single argument. (Counterpoint: doing that makes it harder to do things like create partial applications of the function...but I'm gonna guess not many people are doing that.)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Sep 13, 2017

Thanks for this. Have a branch with cleanup going; the thing I'm paused on right now is the very similar, but almost inverted, bits of logic around how to call transport.set_gss_host, one in Client and one in Transport. I'd really like these to be refactored because it makes the logic hard to follow & seems like an excellent way for bugs to get introduced.

(This kinda follows from my earlier notes re: just not liking how this API is spread across Client/Transport to begin with, though that is again not the OP's fault :))

@bitprophet

This comment has been minimized.

Member

bitprophet commented Sep 13, 2017

(I'm still working on it, btw!)

@bitprophet

This comment has been minimized.

Member

bitprophet commented Sep 13, 2017

OK I have my changes published at https://github.com/paramiko/paramiko/compare/1063-int - please let me know if the 'new' Transport.set_gss_host seems like it still does the right thing from your perspective, @SebastianDeiss - thanks!

bitprophet added a commit that referenced this pull request Sep 13, 2017

@bitprophet

This comment has been minimized.

Member

bitprophet commented Sep 14, 2017

It's not listed here because my branch isn't technically related (I gotta figure out a better way to handle that...) but Sebastian +1'd my tweaks inline - https://github.com/paramiko/paramiko/compare/1063-int#commitcomment-24299580 - so I'm merging this to master.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Sep 14, 2017

EDIT: merging to master did not trigger this PR as 'merged' since it was originally set on 2.2. (We're adding a new kwarg so it's a new feature, so it'll be in 2.3, not 2.2 :)) Closing manually. Thanks!

@bitprophet bitprophet closed this Sep 14, 2017

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