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 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.
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_authorize d_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 fail-on- non-empty /lib/systemd/ system/ ssh.service. d
rmdir --ignore-
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