[MIR] ec2-instance-connect

Adam mentioned fixing the Pre-Depends on adduser (moving adduser to postinst rather than preinst) and the ancient coreutils Depends; I'll take care of fixing those.

Aside from that, AFAIK the package is good to go, but asking for a second opinion so I don't just approve my own MIR.

$ shellcheck ec2-instance-connect/eic_curl_authorized_keys ec2-instance-connect/eic_parse_authorized_keys ec2-instance-connect/eic_run_authorized_keys

In ec2-instance-connect/eic_curl_authorized_keys line 36:
    elif [ ! $(cat /sys/devices/virtual/dmi/id/board_asset_tag) = $instance ] ; then
             ^-- SC2046: Quote this to prevent word splitting.
                                                                  ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_curl_authorized_keys line 41:
elif [ ! $(cat /sys/hypervisor/uuid | cut -c1-3) = "ec2" ] ; then
         ^-- SC2046: Quote this to prevent word splitting.
               ^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

In ec2-instance-connect/eic_curl_authorized_keys line 58:
if [ $? -ne 0 ] ; then
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

In ec2-instance-connect/eic_curl_authorized_keys line 64:
if [ $(eval "${curl_cmd} -o /dev/null -I -w %{http_code} http://169.254.169.254/latest/meta-data/managed-ssh-keys/active-keys/${1}/") -eq 404 ]
     ^-- SC2046: Quote this to prevent word splitting.

In ec2-instance-connect/eic_curl_authorized_keys line 72:
if [ $? -ne 0 ]
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

In ec2-instance-connect/eic_curl_authorized_keys line 79:
region=$(echo $zone | sed -n 's/\(\([a-z]\+-\)\+[0-9]\+\).*/\1/p')
              ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_curl_authorized_keys line 81:
if [ $? -ne 0 ]
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

In ec2-instance-connect/eic_curl_authorized_keys line 90:
chmod 700 $userpath # Disallow any other writes to tempdir
          ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_curl_authorized_keys line 91:
signerkeyfile=signer-cert.pem
^-- SC2034: signerkeyfile appears unused. Verify it or export it.

In ec2-instance-connect/eic_curl_authorized_keys line 92:
keysfile=allowed-keys
^-- SC2034: keysfile appears unused. Verify it or export it.

In ec2-instance-connect/eic_curl_authorized_keys line 105:
if [ $? -ne 0 ]
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

In ec2-instance-connect/eic_curl_authorized_keys line 110:
ocsp_path=$(mktemp -d $userpath/eic-ocsp-XXXXXXXX)
                      ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_curl_authorized_keys line 111:
chmod 700 $ocsp_path # Disallow any other writes to tempdir
          ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_curl_authorized_keys line 114:
    eval "${curl_cmd}" "http://169.254.169.254/latest/meta-data/managed-ssh-keys/signer-ocsp/${word}" | base64 -d > $ocsp_path/$word
                                                                                                                    ^-- SC2086: Double quote to prevent globbing and word splitting.
                                   ...

In ec2-instance-connect/eic_harvest_hostkeys line 21:
elif [ $(cat /sys/hypervisor/uuid | cut -c1-3) != "ec2" ] ; then
       ^-- SC2046: Quote this to prevent word splitting.
             ^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

In ec2-instance-connect/eic_harvest_hostkeys line 35:
    printf "$2" | openssl dgst -binary -hex -sha256 -mac HMAC -macopt hexkey:$1 | sed 's/.* //'
           ^-- SC2059: Don't use variables in the printf format string. Use printf "..%s.." "$foo".
                                                                             ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_harvest_hostkeys line 41:
    local base=$(echo -n "AWS4${1}" | od -A n -t x1 | sed ':a;N;$!ba;s/[\n ]//g')
          ^-- SC2155: Declare and assign separately to avoid masking return values.

In ec2-instance-connect/eic_harvest_hostkeys line 42:
    local kdate=$(sign "${base}" $2)
          ^-- SC2155: Declare and assign separately to avoid masking return values.
                                 ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_harvest_hostkeys line 43:
    local kregion=$(sign "${kdate}" $3)
          ^-- SC2155: Declare and assign separately to avoid masking return values.
                                    ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_harvest_hostkeys line 44:
    local kservice=$(sign "${kregion}" $4)
          ^-- SC2155: Declare and assign separately to avoid masking return values.
                                       ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_harvest_hostkeys line 51:
if [ $(eval "${curl_cmd} -o /dev/null -I -w %{http_code} http://169.254.169.254/latest/meta-data/identity-credentials/ec2/security-credentials/ec2-instance/") -ne 200 ]
     ^-- SC2046: Quote this to prevent word splitting.

In ec2-instance-connect/eic_harvest_hostkeys line 60:
        key=$(cat $file | awk '{$1=$1};1')
                  ^-- SC2086: Double quote to prevent globbing and word splitting.
                  ^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.

In ec2-instance-connect/eic_harvest_hostkeys line 61:
        keys="${keys:+${keys},}\"${key}\""
                               ^-- SC2089: Quotes/backslashes will be treated literally. Use an array.

In ec2-instance-connect/eic_harvest_hostkeys line 66:
trap "rm -rf '${userpath}'" EXIT
              ^-- SC2064: Use single quotes, otherwise this expands now rather than when signalled.

In ec2-instance-connect/eic_harvest_hostkeys line 69:
chmod 700 $userpath
          ^-- SC2086: Double quote to prevent globbing and word splitting.

In ec2-instance-connect/eic_harvest_hostkeys line 73:
if [ $? -ne 0 ]
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

In ec2-instance-connect/eic_harvest_hostkeys line 86:
if [ $? -ne 0 ]
     ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.

In ec2-instance-connect/eic_harvest_hostkeys l...

The code is four shell scripts which still have a lot of findings in shellcheck.
As usual the majority are minor issues, but unused variables and word split can often lead to real bad/unexpected behavior later on.

I'm continuing the review, but do you think we could rais this upstream to get at least the worse parts of it fixed?

[Summary]
It seems mostly to me packaging wise, but I think there are a bunch of things
needed to be doen to complete this. We need:
- an ack by the cloud-init Team that this does not conflict with our usual
  services provided through cloud init
  - I'm subscribing the cloud-init Team to give it a look
- Security review as "getting a key that then is allowed for local login" has
  one of the biggest "unintended backdoor" potential I ever seen
  - assigning security to the bug to evaluate this further
- I really would like upstream to pass and fix most of the shellcheck findings
  - @cyphermox do you have a relation with them, could you ask them to do that?
- a few improvements for better maintenance e.g. changes in postinst described
  in detail later on in this review
  - @cyphermox would you do those or who do we need to ask for that?
- the service should not run as root, use PrivateTmp and maybe a few other
  systemd service isolations
  - @cyphermox - this is part of the upstream, will you ask them to improve
    this?
- need to add package subscribing team
- it is not strictly required, but would be great to have some test on this
  Not sure if one can set up a compatible backend on the expected static IP
  in an autopkgtest.
  But if one could do so that would be a great (albeit optional) addition.
- please add d/watch and/or at least upstream VCS references

Status: incomplete until the issues above are resolved.
I subscribed ubuntu-security as they can already take a look / put it in their queue.
Once the findings are resolved AND security AND cloud-init acks as well this would be complete.

[Duplication]
Ok:
- to some extend you'd think that cloud-init would do that.
- But I know that the new sevrice isn't in there yet.
  So the MIR is not blocked for being a duplicate, but we should ask the
  cloud-init team so that this will not conflict.

[Embedded sources and static linking]
Ok:
- no embedeed libs/sources
- no static linking
- no golang

[Security]
Ok:
- no history of CVEs
- does not use webkit1,2
- does not use lib*v8 directly
- does not open a port
- does not processes arbitrary web content
- does not integrate arbitrary javascript into the desktop

Although it (needs security ack):
- parses data formats
- uses centralized online accounts
- deals with system authentication (eg, pam), etc)

Further (should be improved):
- The package runs a service as root wihch only does processing of remote data
  This script/service gets data from remote endpoints, I see no need to run
  it as root
  The package creates a custom user, why not use this
  Furthermore privateTmp should maybe used as well as some other service
  lockdowns if they apply.

[Common blockers]
Ok:
- does not FTBFS currently?
- not a python package

not perfect but ok:
- code has output in logs, no translations
- it has neither an upstream not a autopkgtest testsuite

needs to be fixed:
- There is no package subscriber yet, I assume Foundations is doing that?

[Packaging red flags]
Ok:
- no Ubuntu delta atm
- no library/symbols concerns
- it isn't very old, from the bit I see it seems upstream updates are ok
- no MOTU relation
- all dependencies are already in main
- ...

Since before we had a lot of text @cloud-nit team - please review and ack that this is no conflict with what/how cloud-init is/will provide.

Also while thinking about it, ~5-8 curl calls fro every SSH login can be quite expensive.
I know it fortunately has an early exit but that still is 2 curl requests.

If this is installed in any place without the endpoint at 169.254.169.254 being responsive and super fast this could lead to a very bad user experience.

Examples:
1. it checks the instance-id via curl, only then locally if it runs on EC2
   I think it really should check that ahead of time

2. (more of a general design issue); doing that on every login feels like a massive overhead.
   Think of remote configuration management software that expects to run hundreds of ssh calls
   per second. We were bitten in the past by issues there e.g. slow MOTD generated on login.
   I really would want all those scripts to do some rate-limiting.
   That is either a full design change away from AuthorizedKeysCommand (probably too complex),
   or and that might be more doable a rate limit. Let it timestamp itself and do any execution
   except this check only once per 5 seconds. For an example load with 100 logins per second for
   10 seconds that would drop the overhead from 1000 to 2. And I think it would be fine to wait 5
   sec for a new key to be active.

@cyphermox can you bring that up with the developers who write on this as well?

At a high level I'm concerned about several parts of this tool's design:

- First, it puts an incredibly high level of trust in the metadata
  service. This may make sense in the context of executing on the Amazon
  platform, but is positively dangerous outside the Amazon platform. It's
  extremely risky in the event that someone's networking routes are
  incorrect on AWS -- consider a VPN that is configured to send 1.0.0.0/1
  and 128.0.0.0/1 to a remote peer, in order to send *all* data elsewhere.

  This tool will directly execute anything returned from the metadata
  service with no logging or accounting or access controls.

  This is far beyond what users may reasonably expect: that this would
  manage authorized_keys and nothing else.

  The pervasive use of eval is inappropriate.

- Second, this is implemented in shell and is beyond the complexity level
  appropriate to shell. Many of the commands in shell pipelines could
  fail without the scripts noticing.

  (It's a significantly smaller point, but the CPU and disk use of this
  tool would be larger than a purpose-built executable in a higher-level
  language like Go, Python, etc.)

- Third, this is re-implementing much of the functionality of the OpenSSH
  certificate support introduced in 2010. What features does this provide
  that's not present in OpenSSH already?

- Fourth, shellcheck found many issues, most of which deserve to be fixed.
  There's repeated instances of missing "" quotes around variables.

Some of the issues in the programs:

- Incorrect use of printf in eic_harvest_hostkeys -- variables interpolated
  into the format string.

- eic_harvest_hostkeys uses eval for string to number conversion, rather
  than using string comparisons

- eic_harvest_hostkeys uses eval to execute something straight from the
  metadata service

- what does awk '{$1=$1};1' do? Why use cat? This should work: key="$(< $file)"

- what's the point of unsetting environment variables when bash is exiting?

- eic_parse_authorized_keys uses CN string comparisons -- why? Is this a
  useful security control? If the openssl command fails entirely, the CN
  string check is bypassed. Is this a problem?

- eic_parse_authorized_keys appears to use $tmpdir extensively but if none
  is given on the command line, it does not fail. Probably its behaviour
  if -d tmpdir is forgotten will be poor. There's no trap to clean up
  temporary files or directories on unexpected exits.

- eic_parse_authorized_keys: if CN contains \r \n \t \v whitespace tokens can
  be inserted into future commands lines via use of 'echo'. Other escape
  sequences may have other consequences.

- eic_parse_authorized_keys use of hardcoded /tmp/sigline is a security
  vulnerability on kernels without protected symlinks support.

- eic_parse_authorized_keys doesn't check if fingerprint's commands succeed

- eic_parse_authorized_keys doesn't have else clauses if the base64 or the
  openssl dgst commands don't work

- eic_curl_authorized_keys uses eval for string to number conversion

- eic_curl_authorized_keys uses eval to execute something straight from
  metadata service

- eic_curl_authorized_keys unused variables: signerkeyfile key...

[Expired for ec2-instance-connect (Ubuntu) because there has been no activity for 60 days.]

If this is going to be addressed via code changes rather than a rewrite, I'd like to suggest the following order:

- remove all evals
- add set -o pipefail to help catch errors in pipelines
- add set -u to help catch unset variables
- replace /tmp/sigline with mktemp -d

Thanks

I've been reminded that set -o pipefail is not perfect. I'm going to quote from the excellent bash faq: http://mywiki.wooledge.org/BashFAQ/105

> though with pipefail in effect, code like this will sometimes cause an
> error, depending on whether the output of somecmd exceeds the size of the
> pipe buffer or not:
>$
> 1 set -e -o pipefail
> 2 somecmd | head -n1
> 3 # The following command will sometimes be executed, depending on how much output somecmd writes:
> 4 echo survived

Thanks

Hi,
this was waiting for updates/fixes to a bunch of things as identified in former reviews before being re-reviewed.

The correct state of this is "incomplete" until all that was requested is provided.
(And even then it just means to restart review without guarantees to pass cleanly).

@paelzer I attempt to answer the review comments below

> [Summary]
> It seems mostly to me packaging wise, but I think there are a bunch of things
> needed to be doen to complete this. We need:
> - an ack by the cloud-init Team that this does not conflict with our usual
> services provided through cloud init
> - I'm subscribing the cloud-init Team to give it a look

I've pinged the Team.

> - Security review as "getting a key that then is allowed for local login" has
> one of the biggest "unintended backdoor" potential I ever seen
> - assigning security to the bug to evaluate this further

@sarnold reviewed the latest changes, but he will share his observations here, too.

> - I really would like upstream to pass and fix most of the shellcheck findings
> - @cyphermox do you have a relation with them, could you ask them to do that?

The findings were communicated to upstream and they've fixed most of them.
I've opened an issue for the remaining ones but I believe those issues are minor:
https://github.com/aws/aws-ec2-instance-connect-config/issues/13

> - a few improvements for better maintenance e.g. changes in postinst described
> in detail later on in this review
> - @cyphermox would you do those or who do we need to ask for that?

I've simplified the maintainer scripts which I detail later.

> - the service should not run as root, use PrivateTmp and maybe a few other
> systemd service isolations
> - @cyphermox - this is part of the upstream, will you ask them to improve
> this?

I've forwarded this recommendation, too:
https://github.com/aws/aws-ec2-instance-connect-config/issues/14

> - need to add package subscribing team

Foundations Team is now subscribed.

> - it is not strictly required, but would be great to have some test on this
> Not sure if one can set up a compatible backend on the expected static IP
> in an autopkgtest.
> But if one could do so that would be a great (albeit optional) addition.

I believe the most practical way of testing the package would be adding tests to the CPC EC2 AMI tests.
There is a guide on performing integration test on running EC2 instances in README.md.

> - please add d/watch and/or at least upstream VCS references

The debian/watch file is now present.

> Status: incomplete until the issues above are resolved.
> I subscribed ubuntu-security as they can already take a look / put it in their queue.
> Once the findings are resolved AND security AND cloud-init acks as well this would be complete.
>
> [Duplication]
> Ok:
> - to some extend you'd think that cloud-init would do that.
> - But I know that the new sevrice isn't in there yet.
> So the MIR is not blocked for being a duplicate, but we should ask the
> cloud-init team so that this will not conflict.

...

> needs to be fixed:
> - There is no package subscriber yet, I assume Foundations is doing that?

Done.

...

> Not so great, needs some fixes/adaptions:
> - it lacks all references to get the code
> - no upstream VCS, no debian/watch file, ...
> Please add something so that a drive by maintainer can still help if all
> people that did it before are gone.
> - due to that it is for example hard to ...

From a cloud-init perspective, one area of concern is around manipulation of sshd config.

Cloud-init does make changes to sshd config, in particular it controls the PasswordAuthentication value based on cloud-config values, ssh_pwauth: <yes/no/unchanged>.

Cloud-init will read in sshd, update a key/value and write out the file and then restart the service. See the code for details.

https://github.com/canonical/cloud-init/blob/master/cloudinit/config/cc_set_passwords.py#L97

The second area of concern user confusion. Typical work-flows for cloud-init user-data include importing and setting ssh keys for users. In the ec2-instance-connect workflow; the user-data cloud-init writes to the instance won't actually be used, rather ec2-instance-connect will query ec2 for what the current set of authorized keys. The sshd man page suggests that if the response does not include output that can verify the key it will fallback to reading .ssh keys. It's not clear to me (or users) which keys were used to authorize access to the system.

I would also express additional concern in the area of time to ssh into instances; with this feature enabled, there is additional overhead for each and every ssh connection.

On Wed, Feb 5, 2020 at 10:55 PM Balint Reczey <email address hidden>
wrote:

> @paelzer I attempt to answer the review comments below
>

Thank you @Rbalint for picking this up and driving it.
I'll skip sections that are ok now - thanks for adding/fixing all those!

> > - an ack by the cloud-init Team that this does not conflict with our
> usual
> > services provided through cloud init
> > - I'm subscribing the cloud-init Team to give it a look
>
> I've pinged the Team.
>

The feedback was added to the bug now and the mentioning of of conflicting
definitions isn't good IMHO.

> - Security review as "getting a key that then is allowed for local login"
> has
> > one of the biggest "unintended backdoor" potential I ever seen
> > - assigning security to the bug to evaluate this further
>
> @sarnold reviewed the latest changes, but he will share his observations
> here, too.
>

Lets see if that is better than comment #8 which wasn't too positive.

...

I've simplified the maintainer scripts which I detail later.
>

Thank you, while still not perfect the simplicity is at least easier to
manage.
Thanks for outlining the relevant use cases, I mostly agree but
invite everyone else to chime in if you see a case that breaks these
assumptions.

> > - the service should not run as root, use PrivateTmp and maybe a few
> other
> > systemd service isolations
>
> I've forwarded this recommendation, too:
> https://github.com/aws/aws-ec2-instance-connect-config/issues/14
>

Thanks for forwarding, but IMHO it needs to be resolved before promotion.
I'm sure security would prefer having that as well - @sarnold - opinions on
this detail?

@Rbalint if you can, please drive this pro-actively @upstream e.g. by
providing them a suggestive patch.

...

> > in an autopkgtest.
> > But if one could do so that would be a great (albeit optional)
> addition.
>
> I believe the most practical way of testing the package would be adding
> tests to the CPC EC2 AMI tests.
> There is a guide on performing integration test on running EC2 instances
> in README.md.
>

Yeah that might be a good place, I'm not aware of the details of those
tests.
But it should not just be an idea, if we skip adding autopkgtest we should
do so on the base of having those tests in CPC-EC2-AMI tests.
Can you please work with the CPC Team to get those tests added?

...

> Users installing the package manually on older AMIs are installing
> it exactly to change the behaviour of SSH thus some care is expected on
> their side if they had local changes to ssh configuration.
>

Yeah, I agree that users installing it later kind of opt-in and those
should be ok in any case.
I'm more concerned about the effects it has by adding it to the images and
behavior changing e.g. due to collisions with cloud init as mentioned
by @rharper

I've added a check to postinst checking if there is other override
> for AuthorizedKeysCommand in /etc/ssh/sshd_config printing an error
> and asking for restarting ssh manually after they made sure that the
> new configuration with the drop-in fits their needs.
>

Thanks for reworking so much of the former complexity!

> > - There are more minor things like the p...

I'm sorry that I have not yet returned to review the new version; this
is written without having read the new changes.

On Mon, Feb 10, 2020 at 11:33:27AM -0000, Christian Ehrhardt  wrote:
> > > - the service should not run as root, use PrivateTmp and maybe a few
> > > other systemd service isolations
> >
> > I've forwarded this recommendation, too:
> > https://github.com/aws/aws-ec2-instance-connect-config/issues/14
> >
>
> Thanks for forwarding, but IMHO it needs to be resolved before promotion.
> I'm sure security would prefer having that as well - @sarnold - opinions on
> this detail?

I'm less sure: I also have the instinct to run new services in new user
ids but this authentication mechanism will allow (or forbid) logins root
privileges. If it is compromised it can grant root privileges. If it is
broken it can prevent legitimate users from gaining root privileges when
needed. It's very nearly root-equivalent regardless of how it runs.

Using a different user account increases the complexity, which this
service already has in spades.

However, a different user account may limit what resources are silently or
invisibly used by the service, which may limit future complexity growth.

> If "it will only be on EC2" would be a hard fact we can rely upon it would
> not need the majority of pre-checks at all.

I'm concerned about system images being shared amongst private and public
clouds, or different public clouds, or between public clouds and local
development. I know those checks are burdensome but I would rather have
them than not.

If this service runs elsewhere it may represent an instant remote code
execution mechanism.

Thanks

@raharper I agree with the concern regarding the manipulation of sshd config. To minimize the collision with cloud-init this package does not change /etc/ssh/sshd_config like cloud-init does, but overrides the configuration value with a systemd drop-in. The drop-in is placed at the time the AMI is built thus there is no race with cloud-init here, and if upgrade of ec2-instance-connect has a race with cloud-init then there is a race with the potential upgrade of sshd as well.

Regarding the potential user confusion when the user also sets ssh keys using cloud-init eic_run_authorized_keys is designed to _merge_ the keys used by Instance Connect with the other keys in use thus the users can continue to use their keys deployed by cloud-init or the ones deployed by other means.

I also agree that there is additional overhead for each ssh connection, but while testing the package I have not found that excessive. We may need further evaluation of the impact on the ssh service before adding the package to the AMIs by default, but I think this can be done after finishing the MIR process.

Upstream already answered @paelzer's caching proposal, and the package is installed on Amazon Linux 2 by default already, thus I believe upstream's attention is warranted regarding the overhead.

On Thu, Feb 13, 2020 at 6:20 AM Balint Reczey <email address hidden>
wrote:

> @raharper I agree with the concern regarding the manipulation of sshd
> config. To minimize the collision with cloud-init this package does not
> change /etc/ssh/sshd_config like cloud-init does, but overrides the
> configuration value with a systemd drop-in. The drop-in is placed at the
>

This does not avoid a collision with cloud-init. That is, the current
implementation
is to modify the arguments passed to sshd instead of making changes to sshd
config.
Multiple sources of configuration is still an issue. Someone inspecting
sshd config will
not see that the AuthorizedKeyCommand is being passed via arguments,
further they
may not know where this additional override is coming from w.r.t the
systemd drop-in
config. If a user (or program via cloud-init config) were to modify sshd
config and
set their own AuthorizedKeyCommand, this scenario will fail for them since
the
ec2-instance-connect drop in will always take precedence.

Shouldn't the drop-in program examine sshd config?

> Regarding the potential user confusion when the user also sets ssh keys
> using cloud-init eic_run_authorized_keys is designed to _merge_ the keys
> used by Instance Connect with the other keys in use thus the users can
> continue to use their keys deployed by cloud-init or the ones deployed
> by other means.
>

Could you elaborate (or point to documentation) on this merging? I'm
familiar
with how AuthorizedKeyCommand works if it does not produce any output, sshd
will fall back to AuthorizedKey files specifiedin sshd_config; but I've not
seen any
discussion on the merging you suggest.

>
> I also agree that there is additional overhead for each ssh connection,
> but while testing the package I have not found that excessive. We may
>

Instead of vague statements, capturing actual values would be most useful.

> need further evaluation of the impact on the ssh service before adding
> the package to the AMIs by default, but I think this can be done after
> finishing the MIR process.
>

I generally disagree with letting something in to main that we want to
support
and "we can fix this after we agree to MIR".

>
> Upstream already answered @paelzer's caching proposal, and the package
> is installed on Amazon Linux 2 by default already, thus I believe
> upstream's attention is warranted regarding the overhead.
>

The response is dismissive; security and usability override any concern
around
overhead of the "security" feature. This reinforces the need to benchmark
the
actual overhead per connection with synthetic and actual workloakds (I
suspect
something like a juju deployment to Ec2 of something like k8s or the like
mode
with lots of instances would be a reasonable workload to compare with and
without
the instance connect enabled.

Ryan

@seth-arnold could you please review the latest version? It would be nice to have the Security Team's approval or the list of security-related issues to be fixed to gain approval.

Is the postrm script missing a systemctl daemon-reload?

==> postrm <==
#!/bin/sh

set -e

#DEBHELPER#

case "$1" in
    purge|remove|abort-install|disappear)
        deb-systemd-helper purge ec2-instance-connect
        # Delete system user
        deluser --system --quiet ec2-instance-connect
        echo "Deleted system user ec2-instance-connect"

        # restart ssh since the drop-in disappeared
        deb-systemd-invoke restart ssh.service
    ;;
    *)
        exit 0
    ;;
esac

The postinst file has:

systemctl --system daemon-reload >/dev/null || true

as part of starting the service.

Thanks

@seth-arnold The daemon-reload is added automatically by debhelper:

...
# Automatically added by dh_systemd_start/11.1.6ubuntu2
if [ -d /run/systemd/system ]; then
 systemctl --system daemon-reload >/dev/null || true
fi
# End automatically added section
...

Since it is also added to the postinst script that one is obsolete, indeed.

That's got to be my one super-power -- asking a question and finding out that no, I didn't find a bug, but by asking the question someone *else* spots a bug.

How about this?

# Derive a sigv4 signing key for the given secret
# get_sigv4_key [key] [datestamp] [region name] [service name]
getsigv4key () {
    base="$(/bin/echo -n "AWS4${1}" | /usr/bin/od -A n -t x1 | /bin/sed ':a;N;$!ba;s/[\n ]//g')"
    kdate="$(sign "${base}" "${2}")"
    kregion="$(sign "${kdate}" "${3}")"
    kservice="$(sign "${kregion}" "${4}")"
    sign "${kservice}" "aws4_request"
}

This appears to execute /bin/echo with a key as a parameter, where it may be visible to ps(1) output or /proc/*/cmdline.

What's the consequences of exposing this key to all users on the computer?

Thanks

printf is a shell built-in which does not exec a new process like echo; I believe this is a reasonable replacement for use of echo -n

printf "%s" ${1}

Alternatively, if you drop the /bin/echo and use bash's built-in echo; that also will work as it won't exec a new process.

The hex encoded version of the key is also passed to openssl:

$ echo abcdef0123456789 | /usr/bin/od -A n -t x1 | /bin/sed ':a;N;$!ba;s/[\n ]//g'
616263646566303132333435363738390a
$ aa-decode 616263646566303132333435363738390a
Decoded: abcdef0123456789

# Sign a message with a given key
# sign [key] [msg]
sign () {
    /usr/bin/printf "${2}" | /usr/bin/openssl dgst -binary -hex -sha256 -mac HMAC -macopt hexkey:"${1}" | /bin/sed 's/.* //'
}

(See the hexkey: parameter)

This appears to come via:

AWS_SECRET_ACCESS_KEY=$(/bin/echo "${creds}" | /bin/sed -n 's/.*"SecretAccessKey" : "\(.*\)",/\1/p')

which is from:

creds=$(/usr/bin/curl -s -f -m 1 -H "X-aws-ec2-metadata-token: ${IMDS_TOKEN}" "http://169.254.169.254/latest/meta-data/identity-credentials/ec2/security-credentials/ec2-instance/")

and IMDS_TOKEN appears to come from:

IMDS_TOKEN="$(/usr/bin/curl -s -f -m 1 -X PUT "http://169.254.169.254/latest/api/token" -H "X-aws-ec2-metadata-token-ttl-seconds: 5")"

Replacing the echo binary with a shell built-in wouldn't hide this key well.

Can any process on the system simply request such a token itself from the aws metadata service?

What does knowledge of this key represent?

Thanks

This new version of ec2-instance-connect is significantly better, thanks for all the work.

I was wrong about the dedicated user: using the ec2-instance-connect user is definitely an improvement.

My one specific concern:

- AWS_SECRET_ACCESS_KEY (and the ability to get one) appears to be available to all processes on the system. What does possession of this secret key mean? The hypervisor may not care, a guest is a guest is a guest, but users may care deeply. Do they?

And two generic concerns:

- Shell error handling is difficult. This code looks much safer than before but the language is not helpful here.

- SSH access credentials are almost invisible: ps auxw | grep ssh will show the flow, as will an inspection of /lib/systemd/system/ssh.service.d/ec2-instance-connect.conf , but these are fairly subtle.

These last two issues are more business decisions than security purview. Rewriting a tool isn't cheap and the work on this version was extensive. And all this effort must surely be because users have wanted an out-of-band authentication mechanism. Sufficiently advertising the new feature would allay my concern that it's very subtle.

Thanks

On Thu, Feb 13, 2020 at 4:51 PM Ryan Harper <email address hidden> wrote:
>
> On Thu, Feb 13, 2020 at 6:20 AM Balint Reczey <email address hidden>
> wrote:
>
> > @raharper I agree with the concern regarding the manipulation of sshd
> > config. To minimize the collision with cloud-init this package does not
> > change /etc/ssh/sshd_config like cloud-init does, but overrides the
> > configuration value with a systemd drop-in. The drop-in is placed at the
> >
>
> This does not avoid a collision with cloud-init. That is, the current
> implementation
> is to modify the arguments passed to sshd instead of making changes to sshd
> config.
> Multiple sources of configuration is still an issue. Someone inspecting
> sshd config will
> not see that the AuthorizedKeyCommand is being passed via arguments,
> further they
> may not know where this additional override is coming from w.r.t the
> systemd drop-in
> config. If a user (or program via cloud-init config) were to modify sshd
> config and
> set their own AuthorizedKeyCommand, this scenario will fail for them since
> the
> ec2-instance-connect drop in will always take precedence.
>
> Shouldn't the drop-in program examine sshd config?

A agree that having multiple sources of configuration can be confusing
for _some_ users, but using systemd drop-ins is a well established
practice for extending configuration of services:

$ apt-file search -x '^/lib/systemd/system/.*.d/.*.conf' | wc -l
21

Also the drop-in is and the changed configuration is reasonably easy
to discover in ssh's status:

$ service ssh status
● ssh.service - OpenBSD Secure Shell server
   Loaded: loaded (/lib/systemd/system/ssh.service; enabled; vendor
preset: enabled)
  Drop-In: /lib/systemd/system/ssh.service.d
           └─ec2-instance-connect.conf
   Active: active (running) since Mon 2020-02-24 16:58:25 UTC; 22h ago
 Main PID: 27725 (sshd)
    Tasks: 1 (limit: 1152)
   CGroup: /system.slice/ssh.service
           └─27725 /usr/sbin/sshd -D -o AuthorizedKeysCommand
/usr/share/ec2-instance-connect/eic_run_authorized_keys %u %f -o
AuthorizedKeysCommandUser ec2-instance-connect

The drop-in could in theory examine sshd_config, but sshd_config is
not the only possible source of the configuration and I see this
behaviour more confusing than the simple discoverable override. Users
not wanting to use the override can simply remove the
ec2-instance-connect -package or override the drop-in in
/etc/systemd/system/

> > Regarding the potential user confusion when the user also sets ssh keys
> > using cloud-init eic_run_authorized_keys is designed to _merge_ the keys
> > used by Instance Connect with the other keys in use thus the users can
> > continue to use their keys deployed by cloud-init or the ones deployed
> > by other means.
> >
>
> Could you elaborate (or point to documentation) on this merging? I'm
> familiar
> with how AuthorizedKeyCommand works if it does not produce any output, sshd
> will fall back to AuthorizedKey files specifiedin sshd_config; but I've not
> seen any
> discussion on the merging you suggest.

I was pretty sure I saw the key I've set up on the AWS console to
access the machine showing up with the k...

Hi Seth,

Thanks for the new review.

On Fri, Feb 21, 2020 at 5:15 AM Seth Arnold <email address hidden> wrote:
>
> This new version of ec2-instance-connect is significantly better, thanks
> for all the work.
>
> I was wrong about the dedicated user: using the ec2-instance-connect
> user is definitely an improvement.
>
> My one specific concern:
>
> - AWS_SECRET_ACCESS_KEY (and the ability to get one) appears to be
> available to all processes on the system. What does possession of this
> secret key mean? The hypervisor may not care, a guest is a guest is a
> guest, but users may care deeply. Do they?

This is a temporary key and it is indeed available to everyone being
able to run curl on the system:
https://www.reddit.com/r/aws/comments/85vkq6/question_about_accesskeyid_secretaccesskey_in/

The package does not change the availability of the key, so I believe
this is not a concern regarding the package, but a general concern
regarding EC2 instances.

> And two generic concerns:
>
> - Shell error handling is difficult. This code looks much safer than
> before but the language is not helpful here.
>
> - SSH access credentials are almost invisible: ps auxw | grep ssh will
> show the flow, as will an inspection of
> /lib/systemd/system/ssh.service.d/ec2-instance-connect.conf , but these
> are fairly subtle.
>
> These last two issues are more business decisions than security purview.
> Rewriting a tool isn't cheap and the work on this version was extensive.
> And all this effort must surely be because users have wanted an out-of-
> band authentication mechanism. Sufficiently advertising the new feature
> would allay my concern that it's very subtle.

Can I take this as an OK for the MIR, from the Security Team?

Thanks,
Balint

>
> Thanks
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1835114
>
> Title:
> [MIR] ec2-instance-connect
>
> Status in ec2-instance-connect package in Ubuntu:
> Incomplete
>
> Bug description:
> [Availability]
> ec2-instance-connect is in the Ubuntu archive, and available for all supported releases. It is available on all architectures despite only being useful on Amazon EC2 instances.
>
> [Rationale]
> This package is useful on Amazon EC2 instances to make use of a new feature:
> Instance Connect; which allows storing SSH keys for access online in the Amazon systems. These SSH keys are then retrieved to be used by the system's SSH service, collated with pre-existing keys as deployed on the system.
>
> Installing the package enables the use of Instance Connect on an
> instance.
>
> [Security]
> This is a new package, and as such has no security history to speak of.
>
> [Quality Assurance]
> The package consists in a few shell scripts that are difficult to test by
> themselves due to the high reliance on Amazon's Instance Connect service;
> which is online and limited to use on Amazon instances.
>
> Given that it's a new package, there are no long-term outstanding bugs in
> Ubuntu or Debian. The package is only maintained in Ubuntu at the moment.
>
> This package deals with special "hardware"; it is o...

On Tue, Feb 25, 2020 at 04:21:05PM -0000, Balint Reczey wrote:
> This is a temporary key and it is indeed available to everyone being
> able to run curl on the system:
> https://www.reddit.com/r/aws/comments/85vkq6/question_about_accesskeyid_secretaccesskey_in/
>
> The package does not change the availability of the key, so I believe
> this is not a concern regarding the package, but a general concern
> regarding EC2 instances.

Excellent, this is exactly the confirmation that this is expected.

> Can I take this as an OK for the MIR, from the Security Team?

Yes, security team ACK for promoting ec2-instance-connect to main, with
the understanding that it shouldn't be seeded in generic media.

Thanks

@raharper Copying my answer in #27 again because it is not easily readable due to answering in email:

On Thu, Feb 13, 2020 at 4:51 PM Ryan Harper <email address hidden> wrote:
>
> On Thu, Feb 13, 2020 at 6:20 AM Balint Reczey <email address hidden>
> wrote:
>
> > @raharper I agree with the concern regarding the manipulation of sshd
> > config. To minimize the collision with cloud-init this package does not
> > change /etc/ssh/sshd_config like cloud-init does, but overrides the
> > configuration value with a systemd drop-in. The drop-in is placed at the
> >
>
> This does not avoid a collision with cloud-init. That is, the current
> implementation
> is to modify the arguments passed to sshd instead of making changes to sshd
> config.
> Multiple sources of configuration is still an issue. Someone inspecting
> sshd config will
> not see that the AuthorizedKeyCommand is being passed via arguments,
> further they
> may not know where this additional override is coming from w.r.t the
> systemd drop-in
> config. If a user (or program via cloud-init config) were to modify sshd
> config and
> set their own AuthorizedKeyCommand, this scenario will fail for them since
> the
> ec2-instance-connect drop in will always take precedence.
>
> Shouldn't the drop-in program examine sshd config?

I agree that having multiple sources of configuration can be confusing
for _some_ users, but using systemd drop-ins is a well established
practice for extending configuration of services:

$ apt-file search -x '^/lib/systemd/system/.*.d/.*.conf' | wc -l
21

Also the drop-in is and the changed configuration is reasonably easy
to discover in ssh's status:

$ service ssh status
● ssh.service - OpenBSD Secure Shell server
   Loaded: loaded (/lib/systemd/system/ssh.service; enabled; vendor
preset: enabled)
  Drop-In: /lib/systemd/system/ssh.service.d
           └─ec2-instance-connect.conf
   Active: active (running) since Mon 2020-02-24 16:58:25 UTC; 22h ago
 Main PID: 27725 (sshd)
    Tasks: 1 (limit: 1152)
   CGroup: /system.slice/ssh.service
           └─27725 /usr/sbin/sshd -D -o AuthorizedKeysCommand
/usr/share/ec2-instance-connect/eic_run_authorized_keys %u %f -o
AuthorizedKeysCommandUser ec2-instance-connect

The drop-in could in theory examine sshd_config, but sshd_config is
not the only possible source of the configuration and I see this
behaviour more confusing than the simple discoverable override. Users
not wanting to use the override can simply remove the
ec2-instance-connect -package or override the drop-in in
/etc/systemd/system/

> > Regarding the potential user confusion when the user also sets ssh keys
> > using cloud-init eic_run_authorized_keys is designed to _merge_ the keys
> > used by Instance Connect with the other keys in use thus the users can
> > continue to use their keys deployed by cloud-init or the ones deployed
> > by other means.
> >
>
> Could you elaborate (or point to documentation) on this merging? I'm
> familiar
> with how AuthorizedKeyCommand works if it does not produce any output, sshd
> will fall back to AuthorizedKey files specifiedin sshd_config; but I've not
> seen any
> discussion on the merging you suggest.

I w...

On the topic of testing raised in earlier comments. CPC will continue with our baseline testing and for releases earlier than focal where we will be pre-installing the package in AWS images with the service disabled (opt-in) we will test that the service is correctly disabled. This testing is performed on images which contain packages from -updates, not -proposed.

Further conversation regarding SRU testing needs to be had with the AWS instance connect development team to iron out the process and then document[1] the test plan. I believe that team should be involved in SRU testing from -proposed as we have with other packages which enable cloud-specific features.

[1] https://wiki.ubuntu.com/StableReleaseUpdates#Documentation_for_Special_Cases

@raharper Could you please check if you find my recent answers in #30 satisfying for your concerns?

@raharper Could you please check if you find my recent answers in #30 satisfying for your concerns?

@Balint,

Apologies for not responding sooner.

Perf-wise, the delta between with and without worst-case values from your
results:

(0.959 - 0.624) = .335s

is a non-trivial amount (almost 50% more) overhead for a single connection.

Users (or programs) may run concurrent ssh sessions, which I don't see
tested here (you said this was out of scope, I disagree).

We've also not tested induced failure (you can run an iptable rule to map
the IMDS address to be dropped to simulate failure to talk to IMDS) so we
don't know what happens in the case that ec2-instance-connect cannot reach
the end-point. I think we should understand the behavior in best and worst
case scenarios before we enable this.

Given that in the images this will be opt-in (disabled by default), I won't
block this request any longer.

@raharper,

Your statement that this will be opt-in (disabled by default) does not reflect our intentions with this package. Upon review, my earlier statement was not as clear as I wish it was. In comment #31 I said:

> CPC (public cloud team) will continue with our baseline testing and for
> releases earlier than focal where we will be pre-installing the package
> in AWS images with the service disabled (opt-in) we will test that the
> service is correctly disabled.

The goal is to ship this in all supported Ubuntu images on AWS enabled by default for releases where it can be added prior to release. We would ship disabled (opt-in) for releases that are already supported in the field so as not to change behavior mid-release. So my statement regarding being disabled "for releases earlier than focal" reflects the goal of shipping enabled by default in Focal were this MIR accepted prior to Focal's release.

I want to ensure this is reviewed with that intent in mind.

While this potentially valuable feature will impact boot speed performance, the issue is not enough to NACK the MIR. Therefore, this MIR should continue to be processed.

While there are still plenty of concerns left this formally fulfills the requirements now.
Therefore it is ready to be promoted.

@Rbaling - This isn't in https://people.canonical.com/~ubuntu-archive/component-mismatches.html yet.
Once seeded or depended on set it to Fix Committed and ask ubuntu-archive to promote.

Sorry for rolling down one key obviously Rbalint not Rbaling ...

Override component to main
ec2-instance-connect 1.1.12+dfsg1-0ubuntu3 in focal: universe/net -> main
ec2-instance-connect 1.1.12+dfsg1-0ubuntu3 in focal amd64: universe/net/optional/100% -> main
ec2-instance-connect 1.1.12+dfsg1-0ubuntu3 in focal arm64: universe/net/optional/100% -> main
ec2-instance-connect 1.1.12+dfsg1-0ubuntu3 in focal armhf: universe/net/optional/100% -> main
ec2-instance-connect 1.1.12+dfsg1-0ubuntu3 in focal i386: universe/net/optional/100% -> main
ec2-instance-connect 1.1.12+dfsg1-0ubuntu3 in focal ppc64el: universe/net/optional/100% -> main
ec2-instance-connect 1.1.12+dfsg1-0ubuntu3 in focal riscv64: universe/net/optional/100% -> main
ec2-instance-connect 1.1.12+dfsg1-0ubuntu3 in focal s390x: universe/net/optional/100% -> main
8 publications overridden.