Re: [Cloud-init-dev] [Merge] ~florian-mueller-v/cloud-init:docs-fix into cloud-init:master
Hey Florian, thanks for submitting this! It looks like you haven't signed the CLA; please check out the instructions at https://cloudinit.readthedocs.io/en/latest/topics/hacking.html for how to do so. Thanks! -- https://code.launchpad.net/~florian-mueller-v/cloud-init/+git/cloud-init/+merge/371298 Your team cloud-init commiters is requested to review the proposed merge of ~florian-mueller-v/cloud-init:docs-fix into cloud-init:master. ___ 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] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
Inline comment asking for clarification on AWS DHCP lease contents,
requirements for routing traffic to IMDS, DNS, and off-box, as with Azure.
Let's document what's needed; we may need to add secondary ips with a metric.
See
https://github.com/aws/ec2-net-utils/blob/master/ec2net-functions
For AmazonLinux net implementation; I do see some setting of source_ip and
route table/metrics in use.
Diff comments:
> diff --git a/tests/unittests/test_datasource/test_ec2.py
> b/tests/unittests/test_datasource/test_ec2.py
> index 20d59bf..8ed4c18 100644
> --- a/tests/unittests/test_datasource/test_ec2.py
> +++ b/tests/unittests/test_datasource/test_ec2.py
> @@ -302,21 +385,45 @@ class TestEc2(test_helpers.HttprettyTestCase):
> platform_data=self.valid_platform_data,
> sys_cfg={'datasource': {'Ec2': {'strict_id': True}}},
> md={'md': DEFAULT_METADATA})
> -find_fallback_path = (
> -'cloudinit.sources.DataSourceEc2.net.find_fallback_nic')
> +find_fallback_path = M_PATH_NET + 'find_fallback_nic'
> with mock.patch(find_fallback_path) as m_find_fallback:
> m_find_fallback.return_value = 'eth9'
> ds.get_data()
>
> mac1 = '06:17:04:d7:26:0A' # IPv4 only in DEFAULT_METADATA
> -expected = {'version': 1, 'config': [
> -{'mac_address': '06:17:04:d7:26:0A', 'name': 'eth9',
> - 'subnets': [{'type': 'dhcp4'}],
> - 'type': 'physical'}]}
> -patch_path = (
> -'cloudinit.sources.DataSourceEc2.net.get_interfaces_by_mac')
> -get_interface_mac_path = (
> -'cloudinit.sources.DataSourceEc2.net.get_interface_mac')
> +expected = {'version': 2, 'ethernets': {'eth9': {
> +'match': {'macaddress': mac1.lower()}, 'set-name': 'eth9',
> +'dhcp4': True}}}
> +patch_path = M_PATH_NET + 'get_interfaces_by_mac'
> +get_interface_mac_path = M_PATH_NET + 'get_interface_mac'
> +with mock.patch(patch_path) as m_get_interfaces_by_mac:
> +with mock.patch(find_fallback_path) as m_find_fallback:
> +with mock.patch(get_interface_mac_path) as m_get_mac:
> +m_get_interfaces_by_mac.return_value = {mac1: 'eth9'}
> +m_find_fallback.return_value = 'eth9'
> +m_get_mac.return_value = mac1
> +self.assertEqual(expected, ds.network_config)
> +
> +def test_network_config_property_secondary_private_ips(self):
> +"""network_config property configures any secondary ipv4 addresses.
> +
> +Only one device is configured even when multiple exist in metadata.
> +"""
> +ds = self._setup_ds(
> +platform_data=self.valid_platform_data,
> +sys_cfg={'datasource': {'Ec2': {'strict_id': True}}},
> +md={'md': SECONDARY_IP_METADATA_2018_09_24})
> +find_fallback_path = M_PATH_NET + 'find_fallback_nic'
> +with mock.patch(find_fallback_path) as m_find_fallback:
> +m_find_fallback.return_value = 'eth9'
> +ds.get_data()
> +
> +mac1 = '0a:07:84:3d:6e:38' # IPv4 with 1 secondary IP
> +expected = {'version': 2, 'ethernets': {'eth9': {
> +'match': {'macaddress': mac1}, 'set-name': 'eth9',
> +'addresses': ['172.31.45.70/20'], 'dhcp4': True}}}
Do we know if:
1) dhcp response from AWS DHCP server includes classless static routes?
2) if the secondary IPs have to be from the same subnet as the primary
interface (DHCP)?
3) if the IMDS, DNS, or off-box routing requires the source-ip to be the value
from DHCP (Note, I think we know that IMDS requires the source-ip to be from
the DHCP response, IIRC).
> +patch_path = M_PATH_NET + 'get_interfaces_by_mac'
> +get_interface_mac_path = M_PATH_NET + 'get_interface_mac'
> with mock.patch(patch_path) as m_get_interfaces_by_mac:
> with mock.patch(find_fallback_path) as m_find_fallback:
> with mock.patch(get_interface_mac_path) as m_get_mac:
--
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/369792
Your team cloud-init commiters is requested to review the proposed merge of
~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master.
___
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] [Merge] ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master
I think Chad meant after our next SRU[1], which will begin shortly. I'd expect this come in the SRU after the current planned one. 1. https://wiki.ubuntu.com/StableReleaseUpdates -- https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/369792 Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:feature/ec2-secondary-nics into cloud-init:master. ___ 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] [Merge] ~florian-mueller-v/cloud-init:docs-fix into cloud-init:master
Florian Müller has proposed merging ~florian-mueller-v/cloud-init:docs-fix into cloud-init:master. Commit message: Add missing #cloud-config comment on first example in documentation. Since this is the first code snippet some users might see, they could end up in the same situation like me today when they wonder why their yaml user config is not working at all. Requested reviews: cloud-init commiters (cloud-init-dev) For more details, see: https://code.launchpad.net/~florian-mueller-v/cloud-init/+git/cloud-init/+merge/371298 -- Your team cloud-init commiters is requested to review the proposed merge of ~florian-mueller-v/cloud-init:docs-fix into cloud-init:master. diff --git a/doc/examples/cloud-config-user-groups.txt b/doc/examples/cloud-config-user-groups.txt index 6a363b7..f588bfb 100644 --- a/doc/examples/cloud-config-user-groups.txt +++ b/doc/examples/cloud-config-user-groups.txt @@ -1,3 +1,4 @@ +#cloud-config # Add groups to the system # The following example adds the ubuntu group with members 'root' and 'sys' # and the empty group cloud-users. ___ 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
** 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
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
