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

Generate a cleaner ssh config file #980

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

SphericalHam
Copy link

These changes make the config file generated a bit cleaner in some cases. It's also now possible to determine that no config need be generated at all.

This way, if no lines are added, the config file doesn't end with a host
block header.
Wait until we've added all the lines we're going to before we decide to
remove the files.

Previously, this would never happen because we unconditionally added a
host block header to the lines - so in practice, this never caused any
strange behaviour.
@pjcdawkins
Copy link
Collaborator

Theoretically none of the Host lines should be necessary, because the SSH manual says:

https://www.freebsd.org/cgi/man.cgi?ssh_config(5)

Include directive may appear inside a Match or Host
block to perform conditional inclusion.

and the root ~/.ssh/config already wraps the Include inside a Host block.

I'm not sure if this conditional inclusion actually works.

@SphericalHam
Copy link
Author

You're quite right, the entire file is only included for matching hosts - a quick look at the openssh source confirms that it does indeed work that way.

Personally, I think it's wise to avoid assuming that root ssh config is set in a particular way - the user is asked if the config should be modified, after all.

One thing I do with my ssh config files is to avoid inclusions at all. I do this because paramiko, the python ssh library, doesn't support inclusions in config files at all. Generally, when I manage config files, I use a script to join all config fragments together to form the final config file. This approach can play poorly with config fragments that have an empty host block at the end.

If we do assume that the file is included conditionally, we could do away with generating host blocks altogether - just use the match exec. I can re-work this PR for that if that's what we prefer.

@pjcdawkins
Copy link
Collaborator

The match exec is only there if there is a cert.

Sounds like you'd need the file to end in Host *, then there isn't a risk from an empty host block? As well as the tidying you suggest

@SphericalHam
Copy link
Author

I guess my thinking is kinda backwards.

If the file is included with openssh's Include directive, then it doesn't matter what Host blocks there are in the included file - the Host blocks only apply to that file.

If the file is concatenated the way I describe, then it sort of doesn't matter how the file ends - empty host block or not, the next file would have to start with a Host line to set the state to something known.

So I guess, then, that the only thing this does is generate a more tidy file.

@pjcdawkins
Copy link
Collaborator

The Host *.platform.sh within this file (whether it's "empty" or not) will continue applying to any statements except those after another Host or Match, so if you're concatenating any SSH config files, you'll have unpredictable results. That's why it'd need Host * or Match all at the end

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

Successfully merging this pull request may close these issues.

2 participants