Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Download add-logging-agent-repo.sh and verify checksum before running #355

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

osalau
Copy link
Member

@osalau osalau commented Aug 16, 2021

curl | bash of "unvendored" scripts is a security risk. This change address that by downloading the image and verifying the checkup before running.

b/193812194

 curl | bash of "unvendored" scripts is a security risk. This change
 address that by downloading the image and verifying the checkup before
 running.
Copy link
Member

@igorpeshansky igorpeshansky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically a trusted script, so we may not need this, but I'll let @qingling128 make that decision.

&& curl -sS https://dl.google.com/cloudagents/add-logging-agent-repo.sh | REPO_SUFFIX="$REPO_SUFFIX" REPO_CODENAME=stretch DO_NOT_INSTALL_CATCH_ALL_CONFIG=true bash /dev/stdin --also-install \
&& curl -sS -o add-logging-agent-repo.sh https://dl.google.com/cloudagents/add-logging-agent-repo.sh \
&& (echo "499cbd999bc9cc26aebd2516d4428623ec7bb9e56a559239cd21b41ae38f0d95 add-logging-agent-repo.sh" | sha256sum --check) \
&& cat add-logging-agent-repo.sh | REPO_SUFFIX="$REPO_SUFFIX" REPO_CODENAME=stretch DO_NOT_INSTALL_CATCH_ALL_CONFIG=true bash /dev/stdin --also-install \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just REPO_SUFFIX="$REPO_SUFFIX" REPO_CODENAME=stretch DO_NOT_INSTALL_CATCH_ALL_CONFIG=true bash add-logging-agent-repo.sh --also-install?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could do that as well. Simply followed existing style. Will update if we end up needing this PR.

@@ -19,7 +19,10 @@ RUN apt-get update \
ca-certificates \
adduser \
# Install Logging Agent.
&& curl -sS https://dl.google.com/cloudagents/add-logging-agent-repo.sh | REPO_SUFFIX="$REPO_SUFFIX" REPO_CODENAME=stretch DO_NOT_INSTALL_CATCH_ALL_CONFIG=true bash /dev/stdin --also-install \
&& curl -sS -o add-logging-agent-repo.sh https://dl.google.com/cloudagents/add-logging-agent-repo.sh \
&& (echo "499cbd999bc9cc26aebd2516d4428623ec7bb9e56a559239cd21b41ae38f0d95 add-logging-agent-repo.sh" | sha256sum --check) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require us to update the checksum every time we update the script, which will introduce additional maintenance burden, so I'd like @qingling128 to sign off on that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires changing our build / test / release pipeline to extract and update this checksum whenever we do an install script release. Otherwise the logging agent pre-release validation is gonna fail. We'd need to prioritize that work before this PR could get in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks. Will confirm on the exemption and update the comments.

@qingling128
Copy link

This script is deployed and hosted by Google. Is it possible to get an exemption for that reason?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants