[Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect

2019-08-14 Thread Seth Arnold
** Attachment added: "shellcheck.txt"
   
https://bugs.launchpad.net/ubuntu/+source/ec2-instance-connect/+bug/1835114/+attachment/5282470/+files/shellcheck.txt

** Changed in: ec2-instance-connect (Ubuntu)
 Assignee: Ubuntu Security Team (ubuntu-security) => (unassigned)

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

___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help   : https://help.launchpad.net/ListHelp


[Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect

2019-08-14 Thread Seth Arnold
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 keysfile

- eic_curl_authorized_keys calling into $DIR/eic_parse_authorized_keys
  with the complicated mechanism to pass openssl commands, tmpdir, etc
  looks very brittle and prone to error under maintenance

And some issues in the packaging:

- Why does the preinst install a user? I don't believe I saw it used

- prerm could skip the ls -A check and run
  rmdir --ignore-fail-on-non-empty /lib/systemd/system/ssh.service.d


This program has grown beyond what's appropriate for shell scripts.

Furthermore, I believe OpenSSH certificates is the better solution to the
problem that this program is addressing.

I strongly recommend using the OpenSSH certificates instead.

I don't believe that we should ship this package with Ubuntu.

Thanks

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

[Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect

2019-07-03 Thread Christian Ehrhardt 
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?

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

___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help   : https://help.launchpad.net/ListHelp


[Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect

2019-07-03 Thread Francis Ginther
** Tags added: id-5cbf801e21a2a0662e2718a9

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

___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help   : https://help.launchpad.net/ListHelp


[Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect

2019-07-03 Thread Christian Ehrhardt 
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.

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

___
Mailing list: https://launchpad.net/~cloud-init-dev
Post to : cloud-init-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~cloud-init-dev
More help   : https://help.launchpad.net/ListHelp


[Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect

2019-07-03 Thread Christian Ehrhardt 
[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
- no massive Lintian warnings
- d/rules is super clean
- no debian/control use of Built-Using
- no golang specifics needed

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

[Upstream red flags]
Ok:
- no Errors/warnings during the build (isn't a real build)
  - but see shellcheck and other things I mentioned already for the same purpose
- bash only, no malloc/sprintf
- no use of sudo, gksu, pkexec, or LD_LIBRARY_PATH
- no use of User nobody
- no use of setuid
- no known Important bugs (crashers, etc) in Debian or Ubuntu
- no dependency on webkit, qtwebkit, seed or libgoa-*
- not part of Unity Dash / UI in regadr to privacy settings?


**