-
Notifications
You must be signed in to change notification settings - Fork 12
Patch command (OpenSSH issues only) #263
Patch command (OpenSSH issues only) #263
Conversation
881c666
to
333142c
Compare
78e8eb6
to
c34bc8e
Compare
I guess we also need a separate PR for the updated RAs. |
@@ -57,6 +72,9 @@ def main(): | |||
elif args.action == 'daemon': | |||
logger.info("start in daemon mode...") | |||
run_daemon(dev=args.dev) | |||
elif args.action == 'patch': | |||
patch(args.patch_name, dev=args.dev) | |||
run(ping=True, dev=args.dev) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should in theory be safe to run service ssh restart
after, as it has a check-config first:
restart)
check_for_upstart 1
check_privsep_dir
check_config
log_daemon_msg "Restarting OpenBSD Secure Shell server" "sshd" || true
start-stop-daemon --stop --quiet --oknodo --retry 30 --pidfile /var/run/sshd.pid
check_for_no_start log_end_msg
check_dev_null log_end_msg
if start-stop-daemon --start --quiet --oknodo --pidfile /var/run/sshd.pid --exec /usr/sbin/sshd -- $SSHD_OPTS; then
log_end_msg 0 || true
else
log_end_msg 1 || true
fi
;;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry forgot about that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could check it manually (which is what check_config
does above):
/usr/sbin/sshd $SSHD_OPTS -t
And then send a SIGHUP to it.
That way it becomes distro agnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIGHUP won’t restart it, it will reload. This is what “service sshd reload” does. Also I think (need to double-check) reload will fail if config is invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, service ssh reload
will do the same, however, the reason for sending SIGHUP vs using service
is that we don't need to worry about distro specific things (like if it's called "ssh" or "sshd")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure that it is in /usr/sbin in all distros?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. That said, I doesn't seem like the -t
argument requires an absolute path:
$ sudo sshd -t
$ echo $?
0
(Launching sshd will however require an absolute path.)
agent/security_helper.py
Outdated
patched_lines[-1] = param + ' ' + safe_value + '\n' | ||
replaced = True | ||
if replaced: | ||
shutil.copy(SSHD_CONFIG_PATH, SSHD_CONFIG_PATH + '.bup') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we copy this file to /opt/wott/backups/sshd_config.$TIMESTAMP
or similar instead?
agent/security_helper.py
Outdated
value = value.strip('"') | ||
if param == patch_param and value != safe_value: | ||
logger.info('{}: replacing "{}" with "{}"'.format(param, value, safe_value)) | ||
patched_lines[-1] = param + ' ' + safe_value + '\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we annotate the end of this line with something like # Added by wott-agent on $TIMESTAMP
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can only add a comment above or below the line, not to the line itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true:
$ grep Proto /etc/ssh/sshd_config
Protocol 2 #This is a comment
$ sudo service ssh restart
$ sudo /usr/sbin/sshd -t
$ echo $?
0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I agree that praxis is to add it above, so maybe we should stick to that.
Doesn't seem like PasswordAuthentication patch works when the line is commented out:
The same is true if a value isn't set:
|
521e17a
to
2de65c2
Compare
Note: all "actions" are now argparse commands which can have subcommands:
Only "patch" command has a subcommand:
In order to pass a dev or debug flag the
--dev
or--debug
argument should come before the command, for example:#257