[Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect
[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? ** Ch
[Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect
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
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
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 -- 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
Re: [Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect
On Wed, Feb 5, 2020 at 10:55 PM Balint Reczey 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 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. > It is just a perception thing, not bound to a particular single issue. But it seems to have so many rough edges left from minot things like this to design issues (net latency amplification) that it feels a bit irresponsible to add it to the image by default as-is. ... > > > 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). > 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 guess this belongs to my "please cache results" request that is a general design question. > > > 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 c
[Cloud-init-dev] [Bug 1835114] Re: [MIR] ec2-instance-connect
** Bug watch added: github.com/aws/aws-ec2-instance-connect-config/issues #15 https://github.com/aws/aws-ec2-instance-connect-config/issues/15 -- 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
** Changed in: ec2-instance-connect (Ubuntu) Assignee: (unassigned) => Matthias Klose (doko) -- 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
