Custom promise type for managing global sshd configuration#128
Custom promise type for managing global sshd configuration#128larsewi merged 2 commits intocfengine:masterfrom
Conversation
Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
larsewi
force-pushed
the
sshd
branch
5 times, most recently
from
57e9aac to
8156d8c
Compare
|
Thanks for submitting a PR! Maybe @craigcomstock can review this? |
larsewi
force-pushed
the
sshd
branch
3 times, most recently
from
dbc704a to
f9ee09c
Compare
olehermanse
left a comment
olehermanse
left a comment
There was a problem hiding this comment.
I think it's good enough for an experimental / first iteration, but there's many things we could consider doing differently. Left some comments in the code.
promise-types/sshd/sshd.py
Outdated
| def validate_promise( # pyright: ignore[reportImplicitOverride] | ||
| self, promiser: str, attributes: dict[str, object], metadata: dict[str, str] | ||
| ): | ||
| for attr, value in attributes.items(): | ||
| if not isinstance(value, (str, list)): | ||
| raise ValidationError(f"Attribute '{attr}' must be a string or slist") |
There was a problem hiding this comment.
I think we should do a better attempt at validating the promise - is it possible to get a list of all valid configuration options from sshd and/or write to a temporary file and use sshd to validate this file?
There was a problem hiding this comment.
Could not find any nice way to do it
larsewi
force-pushed
the
sshd
branch
2 times, most recently
from
12567d4 to
a300170
Compare
olehermanse
left a comment
olehermanse
left a comment
There was a problem hiding this comment.
Looks good. Some small comments / suggestions.
| if not isinstance(value, (str, list)): | ||
| raise ValidationError("Attribute 'value' must be a string or an slist") | ||
|
|
||
| # Make sure 'value' attribute is not empty |
There was a problem hiding this comment.
Can check some things about promiser too, right? IIRC it should:
- Not contain whitespace,
#or/. - Not be empty
- First letter is always uppercase?
Last letter is always lowercase?- No. (PermitTTYfor example).
| def validate_config(self, filename: str) -> bool: | ||
| """Validate the sshd syntax on a file""" | ||
| r = subprocess.run( | ||
| ["/usr/sbin/sshd", "-t", "-f", filename], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| if r.returncode != 0: | ||
| self.log_error(f"Configuration validation failed: {r.stderr.strip()}") | ||
| return False | ||
| return True |
There was a problem hiding this comment.
Why can't we do this as part of validate? With a temporary file.
From your code it looks like it should be possible / easy, if not, please explain in a comment.
There was a problem hiding this comment.
Validation should not fail if you don't have sshd installed for example. Also this command does a sanity of the keys, which do not belong here.
There was a problem hiding this comment.
Does man sshd_config give all the possible options?
man sshd_config | col -b | grep -E '^ {5}[A-Z][a-zA-Z]+$' | sed 's/^ *//'I really think it would be nice if we could do proper validation.
There was a problem hiding this comment.
$ man sshd_config | col -b | grep -E '^ {5}[A-Z][a-zA-Z]+$' | sed 's/^ *//'
troff:<standard input>:810: warning [p 8, 7.2i]: cannot break line
That command does not work for me. And parsing a man page sounds very fragile. How about compiling a hardcoded list of accepted keywords? However, this list would vary based on sshd versions and quickly become a maintenance burden.
| try: | ||
| from cfengine_module_library import PromiseModule, ValidationError, Result | ||
| except ImportError: | ||
| sys.path.append(os.path.join(os.path.dirname(__file__), "../../libraries/python")) | ||
| from cfengine_module_library import PromiseModule, ValidationError, Result |
There was a problem hiding this comment.
I could easily see this becoming confusing, if I have it installed, and they are different. On the other hand, we change the library so rarely that I guess it's okay.
| # When run by CFEngine, the module library is installed. For local development | ||
| # and testing, fall back to the in-tree copy. |
There was a problem hiding this comment.
Could maybe add a docstring / comment here (or in README) about how to run tests.
Ticket: ENT-13797 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>
| self, promiser: str, attributes: dict[str, object], metadata: dict[str, str] | ||
| ): | ||
| # Check that promiser is a valid sshd keyword | ||
| if not re.fullmatch(r"[a-zA-Z0-9]+", promiser): |
There was a problem hiding this comment.
@larsewi Which sshd options can start with a lowercase letter? Which of them can contain numbers? If those exist, please add examples in a comment.
3 participants
No description provided.