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

More robust reading of sshd configuration #2330

Merged
merged 4 commits into from Mar 23, 2024

Conversation

kiekerjan
Copy link
Contributor

This pull request uses the command sshd -T to obtain the sshd configuration instead of directly reading the configuration files. This has the advantage of:

  • Taking into account actually running configuration
  • Taking into account configurations using /etc/ssh/sshd_config.d/ as a configuration directory

I think I got all places where sshd configuration is used, e.g. for port number and for checking passwordauthentication.

break
port = get_ssh_port()
# If nothing returned, assume default
if not port:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think get_ssh_port can return None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. But I think it should. I'll work on the get_ssh_port function

@kiekerjan
Copy link
Contributor Author

Updated utils.py:

  • get_ssh_config_value
    • replace spaces with tabs to match rest of file
    • comments were incorrect
    • make sure provided parameter is compared lower case, as ssh -T returns lower case configuration parameters
  • get_ssh_port
    • replace spaces with tabs to match rest of file
    • when int() gets None as input, it'll throw an exception, fixed that

return None

returnNext = False
for e in output.split():
Copy link

@dms00 dms00 Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure of your primary processing loop, compares every item in the output to the "parameter_name" that is being sought. This also means you compare the parameter_name to values, not just keys. Also, the logic assumes there's only 1 value belonging to a key. That is true most, but not all, of the time.

The output of ssh -T is as follows:

key1 val1 [val2 ... valN]
key2 val1 [val2 ... valN]
...

Most keys can only have 1 value, but some can have multiple values, so I would propose 2 changes to your loop logic here.

First, you should loop over each line of the output, treating each as a record instead of looping over every item in the output. For each line, the first item is the key and all items after that are values. Then you would only compare the parameter_name to the first item (the key), if they match return the list of values (see the next point). If the parameter_name does not match, move on to the next line.

Second, the function (to be complete) really should return a list of values, even though 1 value is far more common. Alternatively, your function could just return a single value, but then you will need to come up with a way of dealing with the few cases where there are more than 1 value.

Here's a possible outline to handle both single and multiple value returns:

def get_ssh_config_value(parameter_name):
    r = get_ssh_config_values(parameter_name)
    if r is None:
        return None
    elif len(r) != 1:
        # throw exception
    else:
        return r[0]

def get_ssh_config_values(parameter_name):
    # Do the work here
    # return None if key not found, else return list containing 1 or more values

This suggestion leaves it up to the caller to decide (and to know) if the key being sought has 1 or more than 1 values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I just reused some code that was already there 😄
But you are right of course, by generalizing the function name, the code should also have been generalised. It'll work now for the usage within MiaB, but not for all cases.
When I have some more time I'll look into it.

@JoshData
Copy link
Member

I pushed some improvements. Let me know if it looks ok. If so, I'll merge. Thanks.

@kiekerjan
Copy link
Contributor Author

Just tested it, it seems to work.

@JoshData JoshData merged commit 1a239c5 into mail-in-a-box:main Mar 23, 2024
@JoshData
Copy link
Member

Thanks!

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.

None yet

3 participants