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

Verify checksums of downloaded files in Dockerfile + remove curl -k flag in jq download #71

Closed
wants to merge 3 commits into from

Conversation

moritzraho
Copy link

Hi,
This is a simple PR to verify the integrity of downloaded code and binaries in the Dockerfile, also remove uneeded usage of insecure curl -k flag.

Copy link
Member

@atrifan atrifan left a comment

Choose a reason for hiding this comment

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

you can use plain variables for sha and don't store them as env variables in the final image. Do you plan to use the env variables later on?

@moritzraho
Copy link
Author

Ok yes thanks for the comment this makes sense. But I guess same applies to the versions right? I prefer to not separate version and sha to keep it clear that both must be updated when upgrading to a higher version

@moritzraho
Copy link
Author

I've pushed the change, but cannot build the image because of #72 . Any ideas how to solve this ?

@moritzraho
Copy link
Author

moritzraho commented Oct 26, 2018

Solved #72 in #74. This must be merged after #74

Dockerfile Outdated
@@ -135,15 +142,17 @@ RUN echo " ... adding Openresty, NGINX, and PCRE" \
--without-http_scgi_module \
-j${NPROC} \
&& make -j${NPROC} \
&& make install \
&& make install
Copy link
Member

Choose a reason for hiding this comment

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

If we want to complete this docker layer here, I'd clean it up too:

Suggested change
&& make install
&& apk del g++ gcc make \
&& rm -rf /var/cache/apk/* \
&& rm -rf /tmp/api-gateway

Copy link
Author

Choose a reason for hiding this comment

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

pushed the change

ENV OPENRESTY_VERSION 1.13.6.1
ENV PCRE_VERSION 8.37
RUN echo " ... adding Openresty and PCRE" \
&& OPENRESTY_SHA256=d1246e6cfa81098eea56fb88693e980d3e6b8752afae686fab271519b81d696b \
Copy link
Member

Choose a reason for hiding this comment

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

for consistency, should we move OPENRESTY_VERSION here as well, instead of having it as an ENV ? B/c the SHA and the ENV must be changed together when updating a version.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I agree but OPENRESTY_VERSION is used in other layers.

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

Successfully merging this pull request may close these issues.

None yet

3 participants