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

Run the secrets-provider with a limited user instead of with root #95

Merged
merged 3 commits into from
May 25, 2020

Conversation

sigalsax
Copy link
Contributor

  • A container running as root is not a good security practice so this should be avoided
  • Add group permissions in Conjur directories to avoid UID mismatch upon deploying to Openshift

@sigalsax sigalsax requested a review from orenbm May 24, 2020 12:43
@sigalsax sigalsax self-assigned this May 24, 2020
Dockerfile Outdated
CMD ["secrets-provider"]
COPY --from=secrets-provider-builder /opt/secrets-provider-for-k8s/secrets-provider /usr/local/bin/

ENTRYPOINT [ "/usr/local/bin/secrets-provider"]
Copy link
Member

Choose a reason for hiding this comment

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

missing new line

Dockerfile Outdated
@@ -3,7 +3,7 @@ MAINTAINER CyberArk Software, Ltd.

ENV GOOS=linux \
GOARCH=amd64 \
CGO_ENABLED=0
CGO_ENABLED=1
Copy link
Member

Choose a reason for hiding this comment

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

why do we need cgo_enabled? are you sure it is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about this one. Other projects used it compile source code in Go and C

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you think we need this offering here?

Copy link
Member

Choose a reason for hiding this comment

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

please don't enable cgo if we don't have to. if the build passes without it then leave it like that.

Dockerfile Outdated
CMD ["secrets-provider"]
COPY --from=secrets-provider-builder /opt/secrets-provider-for-k8s/secrets-provider /usr/local/bin/

ENTRYPOINT [ "/usr/local/bin/secrets-provider"]
Copy link
Member

Choose a reason for hiding this comment

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

why did you change from CMD to ENTRYPOINT? can you challenge this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CMD sets default command and can be overwritten from command line when docker container runs (with docker run -it <image> bash). ENTRYPOINT command and parameters will not be overwritten from command line. So I guess in our case it doesn't really matter but thought I would go with a command that cannot be overwritten

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want secrets-provider to be overridden but CMD is safer...

Dockerfile Outdated
@@ -23,7 +23,7 @@ RUN go build -a -installsuffix cgo -o secrets-provider ./cmd/secrets-provider
FROM busybox

# =================== MAIN CONTAINER ===================
FROM scratch as secrets-provider
FROM alpine:3.8 as secrets-provider
Copy link
Member

Choose a reason for hiding this comment

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

why do we use 3.8? why not latest? why not a newer version?

i'm not saying it's incorrect, just want to verify you put thought into it and chose the best one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. i did it for uniformity across projects but 3.11 is available and the local build passes so I will use that one

@orenbm
Copy link
Member

orenbm commented May 24, 2020

i actually prefer the title Run the secrets-provider with a limited user instead with root as it better says what the change was

@sigalsax sigalsax changed the title Prevent container running as root and avoid UID issues during OC deployment Run the secrets-provider with a limited user instead of with root May 24, 2020
@orenbm
Copy link
Member

orenbm commented May 24, 2020

@sigalsax please add a changelog entry for this change.

Dockerfile Outdated

ENV GOOS=linux \
GOARCH=amd64 \
CGO_ENABLED=0
Copy link
Member

Choose a reason for hiding this comment

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

why was this removed? it should still be here (although it's the default value, it's best to have it for explicitness)

@@ -23,8 +22,8 @@ RUN go build -a -installsuffix cgo -o secrets-provider ./cmd/secrets-provider
FROM busybox

# =================== MAIN CONTAINER ===================
FROM scratch as secrets-provider
MAINTAINER CyberArk Software, Ltd.
FROM alpine:3.11 as secrets-provider
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 use latest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I don't like the idea of automatically pulling latest for the same reason you thought; risky. But open to latest if you think this is the way to go

Copy link
Member

Choose a reason for hiding this comment

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

i think that our nightly automation is good enough but i can live with 3.11.

Dockerfile Outdated

COPY --from=secrets-provider-builder /opt/secrets-provider-for-k8s/secrets-provider /usr/local/bin/

CMD [ "/usr/local/bin/secrets-provider"]
Copy link
Member

Choose a reason for hiding this comment

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

so CMD is better than ENTRYPOINT? it seemed that the latter is safer from your previous explanation.

Copy link
Contributor Author

@sigalsax sigalsax May 24, 2020

Choose a reason for hiding this comment

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

CMD can be overriden and ENTRYPOINT cannot. What do you mean by "Safer"?

sigalsax added 3 commits May 24, 2020 17:42
- A container running as root is not a good security practice so this should be avoided
- Add group permissions in Conjur directories to avoid UID mismatch upon deploying to Openshift
Copy link
Member

@orenbm orenbm left a comment

Choose a reason for hiding this comment

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

LGTM

@sigalsax sigalsax merged commit 37c5edd into master May 25, 2020
@sigalsax sigalsax deleted the limited-user branch May 25, 2020 06:29
@sigalsax sigalsax mentioned this pull request Aug 18, 2020
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.

2 participants