cloud-init-dev team mailing list archive

[Balint Reczey <balint.reczey@xxxxxxxxxxxxx>]

@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 ensure right now if the current
>     release is packaged

Agreed and done.

> - One of the red-flags usually is "modifying the config of another package".
>   This is done, but also the purpose of this package, so we can't "not do it".
>   Although I'd have a suggestion, to make this even safer.
>   The postinst already checks for conflicting configs and overrides (great)
>   But we all know users are not always perfect, I'd like to also address:
>   1. users modified /lib/systemd/system/ssh.service instead (as they should)
>      add an override. You might store expected content of the original
>      ExecStart and bail out if the current one differs.
>   This would also provide some protection from e.g. a security fix that adds
>   an option in that line being lost when ec2-instance-connect is installed.

I see at least three problems here:
1: It checks other .service configuration in /lib/systemd/system, but
there could be local overrides in /etc/systemd/system and also in
/run/systemd/system.
2: It places the drop-in dynamically to
/lib/systemd/system/ssh.service.d while local configuration should be
placed under /etc.
3: The package does not take ownership of the (while the placed file
name gives a good enough hint).

The first problem could have been solved by looking at other dirs on the unit
search path and the second and third could have been solved by handling the
drop-in with ucf, but I believe the added complexity is not worth it.

I've changed the package to simply shipping the drop-in in
/lib/systemd/system/ssh.service.d unconditionally to avoid most of the
complexity in the maintainer scripts, because most complex checks are
not really needed for the following reasons:

1. The plan is shipping the package pre-installed on EC2 images, thus
we can be sure that initially no other override is present for ssh.

2. The postinst checks looking for other overrides in
/lib/systemd/system/* always passed, since no other package in the
official archive provided similar drop-ins for ssh there.

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.

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.

Regarding having locally changed files in /lib/systemd/system/ I agree
that having them is possible, but is a bad practice and changes to those
files are generally not promised to be kept.

> - There are more minor things like the preinst reporting "Created system user
>   ec2-instance-connect" even when it already existed.

Well, this is still present. At least it is printed only on new installs
and not on upgrades, when most likely the user is not present.

...

> 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

The package is planned to be present only on EC2 instances, thus I
believe this would not be a important change (for upstream).

> 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.

I think is is a valid issue, but I also think this is not in the scope
of the MIR. Could you please open a separate bug to track it?

There is a similar issue causing connectivity delay tracked at upstream:

Instance with no outbound connectivity seems to suffer from startup delay
https://github.com/aws/aws-ec2-instance-connect-config/issues/8

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

I'm not sure if the discussion with upstream already covered the two
latter points, but we can continue investingating 2. in a separate bug.


** Bug watch added: github.com/aws/aws-ec2-instance-connect-config/issues #13
   https://github.com/aws/aws-ec2-instance-connect-config/issues/13

** Bug watch added: github.com/aws/aws-ec2-instance-connect-config/issues #14
   https://github.com/aws/aws-ec2-instance-connect-config/issues/14

** Bug watch added: github.com/aws/aws-ec2-instance-connect-config/issues #8
   https://github.com/aws/aws-ec2-instance-connect-config/issues/8

-- 
You received this bug notification because you are a member of cloud-
init Commiters, which is 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 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.

To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/ec2-instance-connect/+bug/1835114/+subscriptions