Clean up start_admin_sshd.sh #22

jpculp · 2021-03-31T16:32:44Z

Issue number:

N/A

Description of changes:

This is just housekeeping to be more consistent with control container and to increase readability.
There are no functional changes.

Testing done:

Launched aws-ecs-1 instance.
Instance connected to ecs cluster.
Test task deployed successfully.
Connected to admin container via ssh.
Ran sudo sheltie to verify root shell was still available.
Checked for failed systemd units.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

webern · 2021-03-31T17:27:39Z

start_admin_sshd.sh

+declare -r USER_SSH_DIR="/home/${LOCAL_USER}/.ssh"
+
+# This is a counter used to verify at least
+# one of the methods of below is available


Maybe you meant this?

Suggested change

# one of the methods of below is available

# one of the methods below is available

jpculp · 2021-03-31T17:35:50Z

Fixed typo

jahkeup · 2021-03-31T17:38:59Z

start_admin_sshd.sh

-  user_data_condensed=$(tr -d '[:space:]' < "${user_data}")
+  user_data_condensed=$(tr -d '[:space:]' < "${USER_DATA}")


This command would delete intentional whitespace as well as the (potential) extraneous space from read JSON. If the goal is to compact the JSON itself (and not user values), then let's use jq to do so - we already know its there and it has a handy flag for this:

# print out condensed json blob to logs. jq --compact-output . "$USER_DATA" # or handle fallback, when not json (I didn't read back to see if we # return early away from this path if its not known to be json): jq --exit-status --compact-output . "$USER_DATA" 2>/dev/null || cat "$USER_DATA"

That's fair. I'll definitely make this change, but I'd like to do so in a different PR to keep this limited to non-functional changes.

Addressed in #25

jahkeup · 2021-03-31T17:41:55Z

start_admin_sshd.sh

@@ -65,31 +67,31 @@ if trusted_user_ca_keys=$(get_user_data_keys "trusted_user_ca_keys"); then
  ((++available_auth_methods))
 fi

-chown "${local_user}" -R "${user_ssh_dir}"
+chown "${LOCAL_USER}" -R "${USER_SSH_DIR}"


nit: flag before the positional arguments is preferred. Added : after the local user to discourage misusing LOCAL_USER=ec2-user:myneatgroup

Suggested change

chown "${LOCAL_USER}" -R "${USER_SSH_DIR}"

chown -R "${LOCAL_USER}:" "${USER_SSH_DIR}"

jahkeup · 2021-03-31T17:43:24Z

start_admin_sshd.sh

  log "Failed to configure ssh authentication with user-data: ${user_data_condensed}"
  exit 1
 fi

 # Generate the server keys
-mkdir -p "${ssh_host_key_dir}"
+mkdir -p "${SSH_HOST_KEY_DIR}"
 for key in rsa ecdsa ed25519; do


Please rename this var while you're here. These aren't "keys" but rather a list of "key algorithms" to generate:

Suggested change

for key in rsa ecdsa ed25519; do

for keyalg in rsa ecdsa ed25519; do

jahkeup · 2021-03-31T17:46:31Z

start_admin_sshd.sh

+
+# This is a counter used to verify at least
+# one of the methods below is available
+available_auth_methods=0


Please declare this as an integer to avoid introducing bugs in later revisions:

Suggested change

available_auth_methods=0

declare -i available_auth_methods=0

Example of why this is important:

#!/usr/bin/env bash unset fooint foostr declare -i fooint=0 foostr=0 fooint+=1 foostr+=1 echo "fooint: $fooint, foostr: $foostr" #stdout: fooint: 1, foostr: 01

jahkeup · 2021-03-31T17:52:29Z

start_admin_sshd.sh

+    if ! raw_keys=$(jq --arg key_type "${key_type}" -e -r '.["ssh"][$key_type][]?' "${USER_DATA}") \
    || [[ -z "${raw_keys}" ]]; then


Rather than combining this in the same if, can you split this out into its own branch?

I've added some logging messages to help demonstrate the difference, not necessarily to suggest adding the messages:

if ! raw_keys=$(jq ...); then # can't extract keys from provided data log "unable to parse configuration from user-data" return 1 fi if [[ -z "$raw_keys" ]]; then # no keys defined log "user-data does not provide $raw_keys" return 1 fi

jahkeup · 2021-03-31T17:53:35Z

start_admin_sshd.sh

@@ -47,9 +35,23 @@ get_user_data_keys() {
    ( IFS=$'\n'; echo "${valid_keys[*]}" )
 }

+declare -r PERSISTENT_STORAGE_BASE_DIR="/.bottlerocket/host-containers/current"
+declare -r SSH_HOST_KEY_DIR="${PERSISTENT_STORAGE_BASE_DIR}/etc/ssh"
+declare -r USER_DATA="${PERSISTENT_STORAGE_BASE_DIR}/user-data"


It's a bit odd to me to see this declared below a user of the variable - was this organized this way intentionally? What your thought on this?

I thought it would easier to read if the functions were at the top and the main logic was below (including constants as part of the main logic)

I'd prefer that we had these constants up top.

There aren't user input changes to them (no flag parsing) or otherwise so they may as well be declared early on. It also loads up the mental context with the constants prior to reading their uses.

There's not too much code and assumption between the functions and constants at this point so I'll leave it up to you.

Sure, I'll go ahead and make this change here, and in the control container script as well since my PR is still open.

jpculp · 2021-04-01T01:11:36Z

Moved constants to the top
Set declare -i for available_auth_methods
Split up the raw_keys jq logic
Fixed the chown
Renamed key as key_alg

jpculp · 2021-04-01T19:29:15Z

Switched #!/bin/bash to #!/usr/bin/env bash
Fixed typo in an old comment

jahkeup

🎉

jahkeup · 2021-04-01T21:34:47Z

start_admin_sshd.sh

-    || [[ -z "${raw_keys}" ]]; then
+    # ? signifies an optional object identifier-index and doesn't return a
+    # 'failed to iterate' error in the event that a key_type is missing.
+    if ! raw_keys=$(jq --arg key_type "${key_type}" -e -r '.["ssh"][$key_type][]?' "${USER_DATA}"); then


I'm getting to the point where I think we ought to add tests in the very very near future (not this PR; maybe with BATS?) for the scripts in the host containers. The functionality is steadily increasing that having some unit tests for processing functions would be great to ensure expected cases are handled appropriately.

I'm a fan of automated testing 🤖

jahkeup · 2021-04-01T21:37:20Z

start_admin_sshd.sh

@@ -65,33 +71,33 @@ if trusted_user_ca_keys=$(get_user_data_keys "trusted_user_ca_keys"); then
  ((++available_auth_methods))


Oh, now that the counter is an int, these can become: available_auth_methods+=1. This isn't necessary, but it is a related cleanup item!

I think we tried this back when we first added it, but it didn't cooperate with set -e. At least I remember that being the case with ((available_auth_methods++)). If you're not strongly opposed I'd rather keep as is.

We tried ((inside++)) which does exit non zero on 0->1 transitions - this behaves coherently:

trap 'echo exit=$? foo=$foo' DEBUG declare -i foo=0 false # the only non-zero exit below foo+=1 foo+=1 echo foo=$foo # should be 2! #stdout: exit=0 foo= #stdout: exit=0 foo=0 #stdout: exit=1 foo=0 #stdout: exit=0 foo=1 #stdout: exit=0 foo=2 #stdout: foo=2

Also: fine to leave as is 👍🏽

Ah nifty! I'm still a fan of the ++, but this gave me a stronger appreciation for the declare -i. I'll be sure to keep that up in the future.

This is just housekeeping to be more consistent with control container and to increase readability. There are no functional changes.

jpculp · 2021-04-01T23:11:24Z

Rebased to fix merge-conflict

jpculp requested review from zmrow, webern, etungsten and jahkeup March 31, 2021 16:32

webern approved these changes Mar 31, 2021

View reviewed changes

jpculp force-pushed the code-cleanup branch from 897e395 to b4eb3f9 Compare March 31, 2021 17:34

etungsten approved these changes Mar 31, 2021

View reviewed changes

jahkeup suggested changes Mar 31, 2021

View reviewed changes

jpculp force-pushed the code-cleanup branch from b4eb3f9 to e78f674 Compare April 1, 2021 01:08

jpculp requested review from jahkeup, webern and etungsten April 1, 2021 01:11

webern approved these changes Apr 1, 2021

View reviewed changes

jpculp force-pushed the code-cleanup branch from e78f674 to 758d9c0 Compare April 1, 2021 19:27

jahkeup approved these changes Apr 1, 2021

View reviewed changes

etungsten approved these changes Apr 1, 2021

View reviewed changes

Clean up start_admin_sshd.sh

a46d771

This is just housekeeping to be more consistent with control container and to increase readability. There are no functional changes.

jpculp force-pushed the code-cleanup branch from 758d9c0 to a46d771 Compare April 1, 2021 23:10

jpculp merged commit 7b08d54 into bottlerocket-os:develop Apr 1, 2021

jpculp deleted the code-cleanup branch April 1, 2021 23:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up start_admin_sshd.sh #22

Clean up start_admin_sshd.sh #22

jpculp commented Mar 31, 2021

webern Mar 31, 2021

jpculp commented Mar 31, 2021

jahkeup Mar 31, 2021

jpculp Mar 31, 2021

jpculp Apr 1, 2021

jahkeup Mar 31, 2021

jahkeup Mar 31, 2021

jahkeup Mar 31, 2021

jahkeup Mar 31, 2021

jahkeup Mar 31, 2021

jpculp Mar 31, 2021

jahkeup Mar 31, 2021

jpculp Mar 31, 2021

jpculp commented Apr 1, 2021

jpculp commented Apr 1, 2021

jahkeup left a comment

jahkeup Apr 1, 2021

jpculp Apr 1, 2021

jahkeup Apr 1, 2021

jpculp Apr 1, 2021

jahkeup Apr 1, 2021

jahkeup Apr 1, 2021

jpculp Apr 1, 2021

jpculp commented Apr 1, 2021

	# one of the methods of below is available
	# one of the methods below is available

		user_data_condensed=$(tr -d '[:space:]' < "${user_data}")
		user_data_condensed=$(tr -d '[:space:]' < "${USER_DATA}")

	chown "${LOCAL_USER}" -R "${USER_SSH_DIR}"
	chown -R "${LOCAL_USER}:" "${USER_SSH_DIR}"

	for key in rsa ecdsa ed25519; do
	for keyalg in rsa ecdsa ed25519; do

		if ! raw_keys=$(jq --arg key_type "${key_type}" -e -r '.["ssh"][$key_type][]?' "${USER_DATA}") \
		\|\| [[ -z "${raw_keys}" ]]; then

		@@ -65,33 +71,33 @@ if trusted_user_ca_keys=$(get_user_data_keys "trusted_user_ca_keys"); then
		((++available_auth_methods))

Clean up start_admin_sshd.sh #22

Clean up start_admin_sshd.sh #22

Conversation

jpculp commented Mar 31, 2021

Choose a reason for hiding this comment

jpculp commented Mar 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpculp commented Apr 1, 2021

jpculp commented Apr 1, 2021

jahkeup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpculp commented Apr 1, 2021