Change admin container logic to use public keys from user-data #19

jpculp · 2021-02-18T20:11:05Z

Issue number:

N/A

Description of changes:

Adds jq to the container image.

Replaces IMDS logic with fetching public keys from user-data.

This change allows the admin container to be platform agnostic as it
retrieves its .ssh/authorized_keys from a base64-encoded
JSON-structured block in user-data rather than relying on the AWS
instance metadata service (IMDS) directly.

Example JSON: {"ssh":{"authorized_keys":[]}}

Also adds support for TrustedUserCAKeys.

This allows users who want to set TrustedUserCAKeys to do so by adding
them to the base64-encoded JSON-structured user-data block.

Example JSON: {"ssh":{"trusted_user_ca_keys":[]}}

Bumps version to v0.6.0.

Testing done:
Launched the new admin container on a bottlerocket instance containing shibaken and verified that SSH access was still available.

After that, I went through several test cases of disabling the admin container, posting a new base64-encoded block to host-containers.admin.user-data, re-enabling the admin container, and checking the connection.

✅ Empty JSON Structure (expected fail)
✅ Custom authorized key
✅ Several keys, some good, some bad (only the strings that could be parsed by ssh-keygen were present in authorized_keys)
✅ Authorized key AND trusted user CA key (.ssh/authorized_keys, /etc/ssh/trusted_user_ca_keys.pub, and /etc/ssh/sshd_config looked as expected)
✅ Removed authorized key so that only trusted user CA key was available

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.

start_admin_sshd.sh

README.md

jahkeup · 2021-02-19T22:14:47Z

Related to changes in PR bottlerocket-os/bottlerocket#1331

jahkeup

Small scripty changes - thanks for rolling this up with bottlerocket-os/bottlerocket#1331 👍🏽

README.md

start_admin_sshd.sh

jahkeup · 2021-02-19T22:37:39Z

start_admin_sshd.sh

+    if ! grep -q "^TrustedUserCAKeys: ${ssh_trusted_user_ca_keys}$" ${sshd_config}; then
+      echo "Failed to write TrustedUserCAKeys to sshd_config" >&2
+    else
+      ((++available_auth_methods))


Typically ++n is used in very specific use cases and tickles my spideysenses - can we switch to n++?

Suggested change

((++available_auth_methods))

((available_auth_methods++))

I gave this a shot, but the container seemed to stop immediately after the first instance:

+ [[ ! -s /home/ec2-user/.ssh/authorized_keys ]] + (( available_auth_methods++ ))

I think this is due to the set -e at the top of the script.

Ah, yeah this would have to written as : (( available_auth_methods++ )) or let available_auth_methods++.

It seems those also are causing an early exit.

It seems those also are causing an early exit.

TIL:

from bash -c 'help let'

Exit Status: If the last ARG evaluates to 0, let returns 1; let returns 0 otherwise.

The : ((available_auth_methods++)) expression should be okay no matter what (because of : noop).

( x=0; set -x; : x++; echo $?; x=0; let x++; echo $?; x=0; let ++x; echo $?) # + : x++ # + echo 0 # 0 # + x=0 # + let x++ # + echo 1 # 1 # + x=0 # + let ++x # + echo 0 # 0

start_admin_sshd.sh

zmrow

I think this looks good - can you update the testing section in the PR description with the testing you've done?

🏐

start_admin_sshd.sh

README.md

start_admin_sshd.sh

jahkeup · 2021-02-23T18:14:14Z

start_admin_sshd.sh

+  echo "${trusted_user_ca_keys}" > "${ssh_trusted_user_ca_keys}"
+
+  cat <<EOF >> "${sshd_config}"
+


This extra newline seems unneeded - the comment adds context if the intent is to break up the appended config from the prior. This isn't a sticking point though, if you want to keep this: feel free to leave it.

jahkeup · 2021-02-23T18:22:15Z

start_admin_sshd.sh

+    # Extract the keys from user-data json
+    local raw_keys
+    if ! raw_keys=$(jq -e -r '.["ssh"]["authorized_keys"][]' "${user_data}" 2>/dev/null); then
+      echo "Failed to parse authorized_keys from ${user_data}" >&2


The echo msg >&2 is repeated throughout, instead of relying on the convention of redirection a function would make the intent clear and safer overall. It would reduce the chances that extra output is sent to subshell substitutes that are expecting data in stdout.

log() { echo "$*" >&2 }

Suggested change

echo "Failed to parse authorized_keys from ${user_data}" >&2

log "Failed to parse authorized_keys from ${user_data}"

Or, alternatively (using my go to log definition)

# log with date prefix. # # Suggested use: `log $LEVEL: $msg` log() { echo "[$(date --iso-8601=s --utc)] $*" >&2 } # with use as: log "INFO: installed ssh authorized keys" log "WARN: unable to parse trusted user CA certificates" log "ERROR: unable to setup ssh with provided user data"

jahkeup · 2021-02-23T18:28:09Z

start_admin_sshd.sh

-    echo "Failed to write key with index ${public_key_index} to authorized_keys" >&2
-    continue
+declare -r ssh_host_key_dir="/.bottlerocket/host-containers/admin/etc/ssh"
+declare -r ssh_config_dir="/home/ec2-user/.ssh"


Let's put this above the mkdir and use it there to make sure we're being consistent throughout with directory creation and their early setup.

I'd also like to call this something else.. this isn't /etc/ssh/ssh_config (which would have been my guess once I lost this off my mental stack). May I suggest: user_ssh_dir. To me, this is far clearer in contexts like "$user_ssh_dir/authorized_keys" (vs. "$ssh_config_dir/authorized_keys", which could have been a system level ssh config directory).

jahkeup · 2021-02-23T18:32:55Z

start_admin_sshd.sh

+  if [[ -f "${ssh_authorized_keys}" ]]; then
+    echo "AuthorizedKeysFile ${ssh_authorized_keys}" >> "${sshd_config}"
  fi


I don't think we should be configuring an absolute path to the authorized keys. I haven't tried this myself, but I'd suspect that an absolute path would authorize all users with the given file, meaning any user could login as any user. Even so, sshd_config(5) has this to say:

AuthorizedKeysFile Specifies the file that contains the public keys used for user authentication. The format is described in the AUTHORIZED_KEYS FILE FORMAT section of sshd(8). Arguments to AuthorizedKeysFile accept the tokens described in the TOKENS section. After expansion, AuthorizedKeysFile is taken to be an absolute path or one relative to the user's home direc‐ tory. Multiple files may be listed, separated by whitespace. Alternately this option may be set to none to skip checking for user keys in files. The default is ".ssh/authorized_keys .ssh/authorized_keys2".

I'm of a mind that we do not try to set or modify the option. The location we're installing the keys will work without adding the explicit path.

jahkeup · 2021-02-23T18:34:41Z

start_admin_sshd.sh

+
+# If there were no successful auth methods, then users cannot authenticate
+if [[ "${available_auth_methods}" -eq 0 ]]; then
+  echo "Failed to configure ssh authentication" >&2
  exit 1


The ${ssh_config_dir} was created by the script, so we should make sure its chown'd before exiting. If users are doing anything to customize their image (and still using this script) then the directory may be inaccessible to them.

jahkeup · 2021-02-23T18:37:03Z

README.md

+         "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQClKsfkNkuSevGj3eYhCe53pcjqP3maAhDFcvBS7O6Vhz2ItxCih+PnDSUaw+WNQn/mZphTk/a/gU8jEzoOWbkM4yxyb/wB96xbiFveSFJuOp/d6RJhJOI0iBXrlsLnBItntckiJ7FbtxJMXLvvwJryDUilBMTjYtwB+QhYXUMOzce5Pjz5/i8SeJtjnV3iAoG/cQk+0FzZqaeJAAHco+CY/5WrUBkrHmFJr6HcXkvJdWPkYQS3xqC0+FmUZofz221CBt5IMucxXPkX4rWi+z7wB3RbBQoQzd8v7yeb7OzlPnWOyN0qFU0XA246RA8QFYiCNYwI3f05p6KLxEXAMPLE my-key-pair"
+      ],
+      "trusted_user_ca_keys":[
+         "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQClKsfkNkuSevGj3eYhCe53pcjqP3maAhDFcvBS7O6Vhz2ItxCih+PnDSUaw+WNQn/mZphTk/a/gU8jEzoOWbkM4yxyb/wB96xbiFveSFJuOp/d6RJhJOI0iBXrlsLnBItntckiJ7FbtxJMXLvvwJryDUilBMTjYtwB+QhYXUMOzce5Pjz5/i8SeJtjnV3iAoG/cQk+0FzZaeJAAHco+CY/5WrUBkrHmFJr6HcXkvJdWPkYQS3xqC0+FmUZofz221CBt5IMucxXPkX4rWi+z7wB3RbBQoQzd8v7yeb7OzlPnWOyN0qFU0XA246RA8QFYiCNYwI3f05p6KLxEXAMPLE my-key-pair"


I'd like to see a distinct and (maybe) valid example here instead of duplicated ssh public key (its the same as authorized_keys. At the very least, I think having a dummy value like ssh-rsa example-ssh-ca-public-key-formatted-here authority@ssh-ca.example.com is preferred.

jpculp · 2021-02-24T04:44:47Z

Switched echos "blah" >&2 to log()
Renamed ssh_config_dir to user_ssh_dir
Replaced hardcoded /home/ec2-user/.ssh with user_ssh_dir
Tweaked cat/EOF block
Removed echo "AuthorizedKeysFile ${ssh_authorized_keys}" >> "${sshd_config}"
Moved chown ec2-user -R "${user_ssh_dir}" up to run before the possible exit due to authentication configuration failure

bcressey · 2021-02-24T05:16:57Z

README.md

+{
+   "ssh":{
+      "authorized_keys":[
+         "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQClKsfkNkuSevGj3eYhCe53pcjqP3maAhDFcvBS7O6Vhz2ItxCih+PnDSUaw+WNQn/mZphTk/a/gU8jEzoOWbkM4yxyb/wB96xbiFveSFJuOp/d6RJhJOI0iBXrlsLnBItntckiJ7FbtxJMXLvvwJryDUilBMTjYtwB+QhYXUMOzce5Pjz5/i8SeJtjnV3iAoG/cQk+0FzZqaeJAAHco+CY/5WrUBkrHmFJr6HcXkvJdWPkYQS3xqC0+FmUZofz221CBt5IMucxXPkX4rWi+z7wB3RbBQoQzd8v7yeb7OzlPnWOyN0qFU0XA246RA8QFYiCNYwI3f05p6KLxEXAMPLE my-key-pair"


nit: might be good to give this the same EXAMPLESSHPUBLICKEYFORMATTEDHERE treatment since the line is very long, and we want to be clear that this isn't a valid key that will give us access to the instance of anyone who copy/pastes this.

You're right, it does look a little silly now. I'll change it to be more consistent.

Fun fact, the example key is from the IMDS documentation. (for anyone who is curious)

bcressey · 2021-02-24T05:29:24Z

start_admin_sshd.sh

+# Populate authorized_keys with all the authorized keys found in user-data
+if authorized_keys=$(parse_authorized_keys); then
+  ssh_authorized_keys="${user_ssh_dir}/authorized_keys"
+  install -D -m 0600 /dev/null "${ssh_authorized_keys}"


This results in ~/.ssh getting created with the default umask, but the recommended default is 0700.

bcressey · 2021-02-24T05:30:56Z

start_admin_sshd.sh

+    # Map the keys to avoid improper splitting
+    local mapped_keys
+    mapfile -t mapped_keys <<< "${raw_keys}"
+
+    # Verify the keys are valid
+    local key
+    local -a valid_keys
+    for key in "${mapped_keys[@]}"; do
+      if ! echo "${key}" | ssh-keygen -lf - &>/dev/null; then
+        log "Failed to validate ${key}"
+        continue
+      fi
+      valid_keys+=( "${key}" )
+    done
+
+    ( IFS=$'\n'; echo "${valid_keys[*]}" )


Can this common block be factored out into its own function?

bcressey · 2021-02-24T05:33:04Z

start_admin_sshd.sh

+parse_trusted_user_ca_keys() {
+    # Extract the keys from user-data json
+    local raw_keys
+    if ! raw_keys=$(jq -e -r '.["ssh"]["trusted_user_ca_keys"][]' "${user_data}" 2>/dev/null); then


nit: I'd move this up by parse_authorized_keys so it doesn't interrupt the flow here.

Based on the other feedback, I'll create a single function to take care of both 😃

bcressey · 2021-02-24T05:34:25Z

start_admin_sshd.sh

+# Configured by user data
+TrustedUserCAKeys ${ssh_trusted_user_ca_keys}


Can this be set unconditionally and have sshd not complain too much if the file doesn't exist?

bcressey · 2021-02-24T05:35:42Z

start_admin_sshd.sh

 fi

-chown ec2-user -R "${ssh_config_dir}"
+chown ec2-user -R "${user_ssh_dir}"


ec2-user should be in a variable so it's easier to override and matches how user_ssh_dir is defined

bcressey · 2021-02-24T05:37:30Z

start_admin_sshd.sh

+  if [[ -s "${ssh_host_key_dir}/ssh_host_${key}_key" ]] &&
+  [[ -s "${ssh_host_key_dir}/ssh_host_${key}_key.pub" ]]; then


We don't need to use [[ here; seems like an unnecessary change. I prefer to use [ unless we're taking advantage of specific features that only [[ can provide.

bcressey · 2021-02-24T05:40:11Z

start_admin_sshd.sh

+# If there were no successful auth methods, then users cannot authenticate
+if [[ "${available_auth_methods}" -eq 0 ]]; then
+  log "Failed to configure ssh authentication"
+  exit 1


We should just log a warning and start sshd anyhow, so the container doesn't continually exit and restart - there's no chance that a simple restart will clear up a transient failure.

This change allows the admin container to be platform agnostic as it retrieves its .ssh/authorized_keys from a base64-encoded JSON block in user-data rather than relying on the AWS instance metadata service (IMDS) directly. Example JSON: {"ssh":{"authorized_keys":[]}}

This allows users who want to set TrustedUserCAKeys to do so by adding them as a base64-encoded JSON block in user-data. Example JSON: {"ssh":{"trusted_user_ca_keys":[]}}

This commit bumps the version from v0.5.2 to v0.6.0 and adds a changelog for the admin container.

jpculp · 2021-02-24T23:07:04Z

Consolidated parse_authorized_keys/parse_trusted_user_ca_keys into a single get_user_data_keys function.
Reduced example authorized_key to something shorter.
Added TrustedUserCAKeys directly to sshd_config.
Replaced hardcoded ec2-user with variable local_user
Replaced install with the original touch/chmod to keep things simple.
Removed extraneous brackets.

zmrow

🎮

Add jq to installed packages

95d773e

jpculp requested review from tjkirch, bcressey, zmrow and etungsten February 18, 2021 20:11

zmrow reviewed Feb 19, 2021

View reviewed changes

start_admin_sshd.sh Outdated Show resolved Hide resolved

jpculp force-pushed the userdata-keys branch 3 times, most recently from aadfd0a to 50bc482 Compare February 19, 2021 20:39

jpculp commented Feb 19, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

zmrow reviewed Feb 19, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Show resolved Hide resolved

This was referenced Feb 19, 2021

Add authorized_keys to TrustedUserCAKeys for certificate-based authen… #18

Closed

AWS-independent ssh mechanism or documentation bottlerocket-os/bottlerocket#1326

Closed

jpculp force-pushed the userdata-keys branch from 50bc482 to 23b02e6 Compare February 19, 2021 22:27

jpculp marked this pull request as ready for review February 19, 2021 22:33

jahkeup suggested changes Feb 19, 2021

View reviewed changes

jpculp force-pushed the userdata-keys branch from 23b02e6 to 6f19973 Compare February 20, 2021 04:46

zmrow approved these changes Feb 22, 2021

View reviewed changes

etungsten approved these changes Feb 22, 2021

View reviewed changes

jahkeup reviewed Feb 22, 2021

View reviewed changes

start_admin_sshd.sh Outdated Show resolved Hide resolved

start_admin_sshd.sh Outdated Show resolved Hide resolved

start_admin_sshd.sh Outdated Show resolved Hide resolved

bcressey reviewed Feb 22, 2021

View reviewed changes

start_admin_sshd.sh Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

jpculp force-pushed the userdata-keys branch 2 times, most recently from 687ac66 to 29dca43 Compare February 23, 2021 04:29

jpculp requested review from jahkeup, bcressey, zmrow and etungsten February 23, 2021 04:38

etungsten approved these changes Feb 23, 2021

View reviewed changes

zmrow approved these changes Feb 23, 2021

View reviewed changes

jahkeup reviewed Feb 23, 2021

View reviewed changes

jpculp force-pushed the userdata-keys branch from 29dca43 to f5b253f Compare February 24, 2021 04:38

jpculp requested review from jahkeup, zmrow and etungsten February 24, 2021 04:44

bcressey reviewed Feb 24, 2021

View reviewed changes

jpculp added 3 commits February 24, 2021 22:49

Add support for TrustedUserCAKeys

e8d90e3

This allows users who want to set TrustedUserCAKeys to do so by adding them as a base64-encoded JSON block in user-data. Example JSON: {"ssh":{"trusted_user_ca_keys":[]}}

Release v0.6.0

14064c0

This commit bumps the version from v0.5.2 to v0.6.0 and adds a changelog for the admin container.

jpculp force-pushed the userdata-keys branch from f5b253f to 14064c0 Compare February 24, 2021 22:59

jpculp requested a review from bcressey February 24, 2021 23:01

jahkeup approved these changes Feb 24, 2021

View reviewed changes

zmrow approved these changes Feb 24, 2021

View reviewed changes

etungsten approved these changes Feb 24, 2021

View reviewed changes

webern approved these changes Feb 24, 2021

View reviewed changes

bcressey approved these changes Feb 24, 2021

View reviewed changes

jpculp merged commit d90644f into bottlerocket-os:develop Feb 24, 2021

jpculp deleted the userdata-keys branch February 26, 2021 02:11

jpculp mentioned this pull request Mar 30, 2021

Support using keys from IMDS as TrustedUserCAKeys #17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change admin container logic to use public keys from user-data #19

Change admin container logic to use public keys from user-data #19

jpculp commented Feb 18, 2021 •

edited

jahkeup commented Feb 19, 2021

jahkeup left a comment

jahkeup Feb 19, 2021

jpculp Feb 20, 2021

jahkeup Feb 22, 2021

jpculp Feb 22, 2021

jahkeup Feb 22, 2021

zmrow left a comment

jahkeup Feb 23, 2021

jahkeup Feb 23, 2021

jahkeup Feb 23, 2021

jahkeup Feb 23, 2021

jahkeup Feb 23, 2021

jahkeup Feb 23, 2021

jpculp commented Feb 24, 2021

bcressey Feb 24, 2021

jpculp Feb 24, 2021

bcressey Feb 24, 2021

bcressey Feb 24, 2021

bcressey Feb 24, 2021

jpculp Feb 24, 2021

bcressey Feb 24, 2021

bcressey Feb 24, 2021

bcressey Feb 24, 2021

bcressey Feb 24, 2021

jpculp commented Feb 24, 2021

zmrow left a comment

		echo "${trusted_user_ca_keys}" > "${ssh_trusted_user_ca_keys}"

		cat <<EOF >> "${sshd_config}"

	echo "Failed to parse authorized_keys from ${user_data}" >&2
	log "Failed to parse authorized_keys from ${user_data}"

		# Configured by user data
		TrustedUserCAKeys ${ssh_trusted_user_ca_keys}

		if [[ -s "${ssh_host_key_dir}/ssh_host_${key}_key" ]] &&
		[[ -s "${ssh_host_key_dir}/ssh_host_${key}_key.pub" ]]; then

Change admin container logic to use public keys from user-data #19

Change admin container logic to use public keys from user-data #19

Conversation

jpculp commented Feb 18, 2021 • edited

jahkeup commented Feb 19, 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

zmrow 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

jpculp commented Feb 24, 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

jpculp commented Feb 24, 2021

zmrow left a comment

Choose a reason for hiding this comment

jpculp commented Feb 18, 2021 •

edited