[MIR] ec2-instance-connect
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
ec2-instance-connect (Ubuntu) |
Fix Released
|
Undecided
|
Matthias Klose |
Bug Description
[Availability]
ec2-instance-
[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 only useful on Amazon
instances, and its support is required as a default deployment on such
instances when deployed with Ubuntu.
[UI Standards]
Not applicable. This service is command-line only and has no configuration options.
[Dependencies]
There are no special dependencies to speak of.
[Standards Compliance]
This package has been thoroughly reviewed by a few Canonical engineers, there are no standards violations known.
[Maintenance]
This package is to be owned by the Ubuntu Foundations team.
[Background Information]
This is Amazon-specific, as previously mentioned.
Related branches
- Ubuntu Core Development Team: Pending requested
-
Diff: 12 lines (+1/-0)1 file modifiedapisupported-cloud (+1/-0)
- Ubuntu Core Development Team: Pending requested
-
Diff: 12 lines (+1/-0)1 file modifiedapisupported-cloud (+1/-0)
- Ubuntu Core Development Team: Pending requested
-
Diff: 12 lines (+1/-0)1 file modifiedapisupported-cloud (+1/-0)
Mathieu Trudel-Lapierre (cyphermox) wrote : | #1 |
Christian Ehrhardt (paelzer) wrote : | #2 |
$ shellcheck ec2-instance-
In ec2-instance-
elif [ ! $(cat /sys/devices/
^-- SC2046: Quote this to prevent word splitting.
In ec2-instance-
elif [ ! $(cat /sys/hypervisor
^-- SC2046: Quote this to prevent word splitting.
^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
In ec2-instance-
if [ $? -ne 0 ] ; then
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In ec2-instance-
if [ $(eval "${curl_cmd} -o /dev/null -I -w %{http_code} http://
^-- SC2046: Quote this to prevent word splitting.
In ec2-instance-
if [ $? -ne 0 ]
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In ec2-instance-
region=$(echo $zone | sed -n 's/\(\(
^-- SC2086: Double quote to prevent globbing and word splitting.
In ec2-instance-
if [ $? -ne 0 ]
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In ec2-instance-
chmod 700 $userpath # Disallow any other writes to tempdir
^-- SC2086: Double quote to prevent globbing and word splitting.
In ec2-instance-
signerkeyfile=
^-- SC2034: signerkeyfile appears unused. Verify it or export it.
In ec2-instance-
keysfile=
^-- SC2034: keysfile appears unused. Verify it or export it.
In ec2-instance-
if [ $? -ne 0 ]
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In ec2-instance-
ocsp_path=$(mktemp -d $userpath/
In ec2-instance-
chmod 700 $ocsp_path # Disallow any other writes to tempdir
^-- SC2086: Double quote to prevent globbing and word splitting.
In ec2-instance-
eval "${curl_cmd}" "http://
Christian Ehrhardt (paelzer) wrote : | #3 |
In ec2-instance-
elif [ $(cat /sys/hypervisor
^-- SC2046: Quote this to prevent word splitting.
^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.
In ec2-instance-
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".
In ec2-instance-
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-
local kdate=$(sign "${base}" $2)
^-- SC2155: Declare and assign separately to avoid masking return values.
In ec2-instance-
local kregion=$(sign "${kdate}" $3)
^-- SC2155: Declare and assign separately to avoid masking return values.
In ec2-instance-
local kservice=$(sign "${kregion}" $4)
^-- SC2155: Declare and assign separately to avoid masking return values.
In ec2-instance-
if [ $(eval "${curl_cmd} -o /dev/null -I -w %{http_code} http://
^-- SC2046: Quote this to prevent word splitting.
In ec2-instance-
key=$(cat $file | awk '{$1=$1};1')
In ec2-instance-
In ec2-instance-
trap "rm -rf '${userpath}'" EXIT
^-- SC2064: Use single quotes, otherwise this expands now rather than when signalled.
In ec2-instance-
chmod 700 $userpath
^-- SC2086: Double quote to prevent globbing and word splitting.
In ec2-instance-
if [ $? -ne 0 ]
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In ec2-instance-
if [ $? -ne 0 ]
^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.
In ec2-instance-
Christian Ehrhardt (paelzer) wrote : | #4 |
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?
Christian Ehrhardt (paelzer) wrote : | #5 |
[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
- ...
Changed in ec2-instance-connect (Ubuntu): | |
status: | New → Incomplete |
assignee: | nobody → Ubuntu Security Team (ubuntu-security) |
Christian Ehrhardt (paelzer) wrote : | #6 |
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.
tags: | added: id-5cbf801e21a2a0662e2718a9 |
Christian Ehrhardt (paelzer) wrote : | #7 |
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 AuthorizedKeysC
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?
Seth Arnold (seth-arnold) wrote : | #8 |
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_
into the format string.
- eic_harvest_
than using string comparisons
- eic_harvest_
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_
useful security control? If the openssl command fails entirely, the CN
string check is bypassed. Is this a problem?
- eic_parse_
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_
be inserted into future commands lines via use of 'echo'. Other escape
sequences may have other consequences.
- eic_parse_
vulnerability on kernels without protected symlinks support.
- eic_parse_
- eic_parse_
openssl dgst commands don't work
- eic_curl_
- eic_curl_
metadata service
- eic_curl_
Seth Arnold (seth-arnold) wrote : | #9 |
Changed in ec2-instance-connect (Ubuntu): | |
assignee: | Ubuntu Security Team (ubuntu-security) → nobody |
Launchpad Janitor (janitor) wrote : | #10 |
[Expired for ec2-instance-
Changed in ec2-instance-connect (Ubuntu): | |
status: | Incomplete → Expired |
Seth Arnold (seth-arnold) wrote : | #11 |
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
Seth Arnold (seth-arnold) wrote : | #12 |
I've been reminded that set -o pipefail is not perfect. I'm going to quote from the excellent bash faq: http://
> 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
Changed in ec2-instance-connect (Ubuntu): | |
status: | Expired → New |
assignee: | nobody → Balint Reczey (rbalint) |
description: | updated |
tags: | added: id-5e1e340b6338410899d33213 |
Changed in ec2-instance-connect (Ubuntu): | |
assignee: | Balint Reczey (rbalint) → nobody |
tags: | added: id-5e21ca0949c79659969a46bd |
Christian Ehrhardt (paelzer) wrote : | #13 |
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).
Changed in ec2-instance-connect (Ubuntu): | |
status: | New → Incomplete |
Balint Reczey (rbalint) wrote : | #14 |
@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:/
> - 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:/
> - 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 ...
Ryan Harper (raharper) wrote : | #15 |
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 PasswordAuthent
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:/
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-
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.
Christian Ehrhardt (paelzer) wrote : Re: [Bug 1835114] Re: [MIR] ec2-instance-connect | #16 |
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:/
>
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 AuthorizedKeysC
> 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...
Seth Arnold (seth-arnold) wrote : | #17 |
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:/
> >
>
> 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
Balint Reczey (rbalint) wrote : | #18 |
@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/
Regarding the potential user confusion when the user also sets ssh keys using cloud-init eic_run_
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.
Ryan Harper (raharper) wrote : Re: [Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect | #19 |
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/
> 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 AuthorizedKeyCo
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 AuthorizedKeyCo
the
ec2-instance-
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_
> 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 AuthorizedKeyCo
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
Balint Reczey (rbalint) wrote : | #20 |
@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.
Seth Arnold (seth-arnold) wrote : | #21 |
Is the postrm script missing a systemctl daemon-reload?
==> postrm <==
#!/bin/sh
set -e
#DEBHELPER#
case "$1" in
purge|
# Delete system user
deluser --system --quiet ec2-instance-
echo "Deleted system user ec2-instance-
# restart ssh since the drop-in disappeared
;;
*)
exit 0
;;
esac
The postinst file has:
systemctl --system daemon-reload >/dev/null || true
as part of starting the service.
Thanks
Balint Reczey (rbalint) wrote : | #22 |
@seth-arnold The daemon-reload is added automatically by debhelper:
...
# Automatically added by dh_systemd_
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.
Seth Arnold (seth-arnold) wrote : | #23 |
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=
kdate="$(sign "${base}" "${2}")"
kregion="$(sign "${kdate}" "${3}")"
kservice=
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
Ryan Harper (raharper) wrote : | #24 |
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.
Seth Arnold (seth-arnold) wrote : | #25 |
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'
616263646566303
$ aa-decode 616263646566303
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_
which is from:
creds=$
and IMDS_TOKEN appears to come from:
IMDS_TOKEN=
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
Seth Arnold (seth-arnold) wrote : | #26 |
This new version of ec2-instance-
I was wrong about the dedicated user: using the ec2-instance-
My one specific concern:
- AWS_SECRET_
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/
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
Balint Reczey (rbalint) wrote : | #27 |
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/
> > 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 AuthorizedKeyCo
> 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 AuthorizedKeyCo
> the
> ec2-instance-
>
> 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/
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/
preset: enabled)
Drop-In: /lib/systemd/
Active: active (running) since Mon 2020-02-24 16:58:25 UTC; 22h ago
Main PID: 27725 (sshd)
Tasks: 1 (limit: 1152)
CGroup: /system.
└─27725 /usr/sbin/sshd -D -o AuthorizedKeysC
/usr/share/
AuthorizedKeysC
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-
/etc/systemd/
> > Regarding the potential user confusion when the user also sets ssh keys
> > using cloud-init eic_run_
> > 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 AuthorizedKeyCo
> 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...
Balint Reczey (rbalint) wrote : Re: [Bug 1835114] Re: [MIR] ec2-instance-connect | #28 |
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-
> for all the work.
>
> I was wrong about the dedicated user: using the ec2-instance-
> user is definitely an improvement.
>
> My one specific concern:
>
> - AWS_SECRET_
> 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:/
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/
> 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:/
>
> Title:
> [MIR] ec2-instance-
>
> Status in ec2-instance-
> Incomplete
>
> Bug description:
> [Availability]
> ec2-instance-
>
> [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...
Seth Arnold (seth-arnold) wrote : | #29 |
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:/
>
> 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-
the understanding that it shouldn't be seeded in generic media.
Thanks
Balint Reczey (rbalint) wrote : | #30 |
@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/
> > 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 AuthorizedKeyCo
> 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 AuthorizedKeyCo
> the
> ec2-instance-
>
> 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/
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/
preset: enabled)
Drop-In: /lib/systemd/
Active: active (running) since Mon 2020-02-24 16:58:25 UTC; 22h ago
Main PID: 27725 (sshd)
Tasks: 1 (limit: 1152)
CGroup: /system.
└─27725 /usr/sbin/sshd -D -o AuthorizedKeysC
/usr/share/
AuthorizedKeysC
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-
/etc/systemd/
> > Regarding the potential user confusion when the user also sets ssh keys
> > using cloud-init eic_run_
> > 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 AuthorizedKeyCo
> will fall back to AuthorizedKey files specifiedin sshd_config; but I've not
> seen any
> discussion on the merging you suggest.
I w...
Changed in ec2-instance-connect (Ubuntu): | |
assignee: | nobody → Matthias Klose (doko) |
Robert C Jennings (rcj) wrote : | #31 |
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:/
Balint Reczey (rbalint) wrote : | #32 |
@raharper Could you please check if you find my recent answers in #30 satisfying for your concerns?
Balint Reczey (rbalint) wrote : | #33 |
@raharper Could you please check if you find my recent answers in #30 satisfying for your concerns?
Ryan Harper (raharper) wrote : | #35 |
@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-
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.
Robert C Jennings (rcj) wrote : | #36 |
@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.
Joshua Powers (powersj) wrote : | #37 |
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.
Christian Ehrhardt (paelzer) wrote : | #38 |
While there are still plenty of concerns left this formally fulfills the requirements now.
Therefore it is ready to be promoted.
Changed in ec2-instance-connect (Ubuntu): | |
status: | Incomplete → In Progress |
Christian Ehrhardt (paelzer) wrote : | #39 |
@Rbaling - This isn't in https:/
Once seeded or depended on set it to Fix Committed and ask ubuntu-archive to promote.
Christian Ehrhardt (paelzer) wrote : | #40 |
Sorry for rolling down one key obviously Rbalint not Rbaling ...
Changed in ec2-instance-connect (Ubuntu): | |
status: | In Progress → Fix Committed |
Matthias Klose (doko) wrote : | #41 |
Override component to main
ec2-instance-
ec2-instance-
ec2-instance-
ec2-instance-
ec2-instance-
ec2-instance-
ec2-instance-
ec2-instance-
8 publications overridden.
Changed in ec2-instance-connect (Ubuntu): | |
status: | Fix Committed → Fix Released |
tags: | added: id-5e61f7e9c860ee02a619532e |
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.