feat: Add gomplate to the docker image #1893

nabokihms · 2020-12-21T20:51:45Z

Overview

Add gomplate to the Docker image
Add entrypoint.sh script that renders the config file as a template

What this PR does / why we need it

This PR solves a long time issue in Dex, name injecting secrets into the config file (especially on Kubernetes) without actually saving those secrets in less/non-secure storage layers. By using templating (and environment variables) we can create a config file runtime (when the container starts, in the entrypoint).

Obviously, the rendered config file is still saved to ephemeral storage, so it's not all the way secure, but anyone with access to the storage layer would have other means to get the secrets anyway.

A better long-term solution for secrets would be using environment variables directly and/or secret storage solutions (like Vault) or even KMS integration.

Special notes for your reviewer

Does this PR introduce a user-facing change?

Add [gomplate](https://github.com/hairyhenderson/gomplate) to the Dex docker image to provide config file templating features

sagikazarmark

I feel like there is more we can do here:

I'd get rid of the entrypoint entirely
serve should be the default command IMHO
serve requires a config, so we need some sort of default

I'm not sure I'm a fan of reading config from stdin. I guess it's there so we don't write the resulting config to ephemeral storage, but it feels like a complication without benefits: anyone with access to the storage has already access to the container itself.

At least for now, I'd just write the configuration to /etc/dex/config.yaml.

WDYT?

nabokihms · 2020-12-22T13:12:31Z

Everything sounds reasonable to me. The only thing that remains is implementation. I will work on it.

heidemn · 2020-12-31T06:12:55Z

Dockerfile


-CMD ["version"]
+ENTRYPOINT ["dockerize", "-no-overwrite", "-template", "/etc/dex/config.tmpl:/etc/dex/config.yaml"]
+CMD ["dex", "serve", "/etc/dex/config.yaml"]


What about moving "dex" to the end of ENTRYPOINT, to avoid a breaking change for current users of the Docker image?

ENTRYPOINT ["dockerize", "-no-overwrite", "-template", "/etc/dex/config.tmpl:/etc/dex/config.yaml", "dex"] CMD ["serve", "/etc/dex/config.yaml"]

It may look less clean in the Dockerfile, but in my opinion it's better to avoid the breaking change.

If you still decide for the breaking change, at least it should be mentioned in the release notes ("Does this PR introduce a user-facing change?")

Seems reasonable. Thanks!

Personally, I never like when an application is the entrypoint. It just ties the user's hands. Although it is a breaking change, but I don't think it'll affect most users. We could think of some sort of deprecation for this change to warn users in advance, but I'm not sure how effective it would be.

I hardly believe that adding a deprecation warning will help. Mentioning such a change in the RM should be enough. However, for me, it looks better to leave dex at the entry point just because changing behavior is not required by this PR.

heidemn · 2020-12-31T06:57:30Z

Once my PR #1902 is merged, most Dockerize users will probably want to set DEX_EXPAND_ENV = false, to avoid another unexpected level of env expansion.

heidemn · 2021-01-01T11:23:29Z

examples/config.tmpl

+  passwordConnector: {{ .Env.DEX_OAUTH2_PASSWORD_CONNECTOR }}
+{{- end }}
+
+enablePasswordDB: {{ default .Env.DEX_ENABLE_PASSWORD_DB "true" }}


Question: What is the best way to pass an env variable that may contain YAML special characters like '"?
Is there a better solution than manually escaping single quotes?
bindPW: '{{ replace .Env.DEX_LDAP_BINDPW "'" "''" }}'

Since Dockerize uses text/template, I guess it's not aware of YAML syntax.

Of course the env variables could by convention already contain pre-escaped strings, but that would be rather strange.

Good question. I think it should be raised in the dockerize project. Helm has a quote filter that does exactly that.

Also, I see open issues in the project asking to add the sprig template library.

sagikazarmark

I've looked into dockerize a bit more and I'm not sure it's gonna be good for us. The author doesn't answer to issues/PRs. I pinged him, let's if we get an answer.

sagikazarmark · 2021-01-01T14:20:09Z

Dockerfile

+# Install dockerize to provide the way of config parametrization
+ENV DOCKERIZE_VERSION v0.6.1
+RUN wget https://github.com/jwilder/dockerize/releases/download/$DOCKERIZE_VERSION/dockerize-linux-amd64-$DOCKERIZE_VERSION.tar.gz \
+    && tar -C /usr/local/bin -xzvf dockerize-linux-amd64-$DOCKERIZE_VERSION.tar.gz \


amd64 is going to be a problem form arm builds. And sadly it looks like arm builds are not available yet: jwilder/dockerize#154

The kind of a problem we can't solve on our own. Looks like we need to search for a better solution.

The templating code of Dockerize is only 200 lines, since it's basically Go's text/template with some helper functions added.
On first sight, the only other feature from Dockerize that could be helpful for Dex is waiting for other dependencies (-wait , -timeout & co). And that can usually be solved with a crash loop.

Would it be an option to glue together ourselves Go templating with helpers from a well-maintained library like https://github.com/Masterminds/sprig in Dex?
Could work like this for the user:
$ dex serve config.yaml
$ dex serve --template config.tmpl

Sprig already includes env and default functions, similar to Dockerize.

FYI, if you want to use dockerize, I recommend using its fork https://github.com/powerman/dockerize
This fork was created by an active contributor who was not satisfied with Jwilder's project management, communication, and so on.
Since then, a lot of improvements were done in Powerman's fork, incl. arm builds, for example.
The original dockerize seems to be an abandoned project.

I'm familiar with that fork, but I'm not sure I'm quite happy with the democratized maintenance model to implements:

Everyone who has contributed to the project may become a collaborator - just ask for it in PR comments after your PR has being merged.

Also, we really don't need all that functionality, so I'm happy with a simple templating solution. My only concern with gotemplate is its size.

nabokihms · 2021-01-28T13:00:31Z

@sagikazarmark, I replaced dockerize with gomplate in this PR. Some concerns, problems, tasks to do:

entrypoint is back because gomplate can't work as an entrypoint
TARGETVARIANT is empty for arm/v6 and arm/v7 during the build
gomplate binary size is 36.7M
config template is located in /etc/dex, and directory is shared for nobody user
need to change description an title of the PR

Please take a look if you have time 🙏

sagikazarmark

Thanks @nabokihms !

This looks like a good approach.

I had a few comments, can you please take a look at them?

Dockerfile

entrypoint.sh

examples/config-example.tmpl

entrypoint.sh

Dockerfile

docker-entrypoint.sh

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com> Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

docker-entrypoint.sh

heidemn · 2021-01-29T21:19:19Z

docker-entrypoint.sh

+    exec dex $@
+    ;;
+  *)
+    exec $@


I like this option, allowing it to bypass the templating by adding dex in front of the arguments. 👍
Maybe mention it in the "Usage" comment, so it's more obvious.

You should also adapt the section "Does this PR introduce a user-facing change?":

By default, Gomplate is now applied on the config file for docker run quay.io/dexidp/dex serve ....

To opt out, one can either adapt the command: docker run quay.io/dexidp/dex dex serve ...
or change the entrypoint: docker run --entrypoint dex quay.io/dexidp/dex serve ...

Yes, you are right! Whole description has to be rewritten.

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

heidemn · 2021-01-30T12:54:19Z

config.docker.yaml

@@ -0,0 +1,46 @@
+issuer: {{ getenv "DEX_ISSUER" "http://127.0.0.1:5556/dex" }}
+


Maybe add a comment that this config file is only an example / for development purposes?

And maybe mention in another comment that for escaping "unfriendly" input like passwords, this function could be used for escaping, to get valid YAML? https://docs.gomplate.ca/functions/strings/#strings-squote
(Sorry haven't tested it yet, but I'm assuming that such escaping is not done automatically by Gomplate)

I think that makes sense. Let's add a comment

Dockerfile

Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com> Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

sagikazarmark

I think this is good overall, thanks for working on this @nabokihms

There are two comments left, can you please take a look at those? Thanks!

sagikazarmark · 2021-02-10T11:02:14Z

Dockerfile

+
+ENV GOMPLATE_VERSION=v3.9.0
+
+RUN wget -O /usr/local/bin/gomplate \


Maybe use the slim version instead for a smaller binary: https://github.com/hairyhenderson/gomplate/releases

Actually, does this work with every arm version? arm64, armv7? (The first one is GOARCH, the latter is variant AFAIK)

Yes, it does! I have tested with linux/arm/v7 and linux/arm64.

Awesome, thanks!

sagikazarmark · 2021-02-10T11:26:25Z

config.docker.yaml

@@ -0,0 +1,46 @@
+issuer: {{ getenv "DEX_ISSUER" "http://127.0.0.1:5556/dex" }}
+


I think that makes sense. Let's add a comment

Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com> Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

sagikazarmark

Thanks! I think this is a great improvement!

nabokihms · 2021-02-10T17:02:12Z

I have no time to change the description yet. Hope that I will be able to do it tonight.

sagikazarmark · 2021-02-10T19:02:55Z

No worries, I've updated the PR description.

@HEllRZA

The official docker release for this release can be pulled from ``` ghcr.io/dexidp/dex:v2.28.0 ``` **Features:** - Add c_hash to id_token, issued on /auth endpoint, when in hybrid flow (dexidp#1773, @HEllRZA) - Allow configuration of returned auth proxy header (dexidp#1839, @seuf) - Allow to disable os.ExpandEnv for storage + connector configs by env variable DEX_EXPAND_ENV = false (dexidp#1902, @heidemn-faro) - Added the possibility to activate lowercase for UPN-Strings (dexidp#1888, @VF-mbrauer) - Add "Cache-control: no-store" and "Pragma: no-cache" headers to token responses (dexidp#1948, @nabokihms) - Add gomplate to the docker image (dexidp#1893, @nabokihms) - Graceful shutdown (dexidp#1963, @nabokihms) - Allow public clients created with API to have no client_secret (dexidp#1871, @spohner) **Bugfixes:** - Fix the etcd PKCE AuthCode deserialization (dexidp#1908, @bnu0) - Fix garbage collection logging of device codes and device request (dexidp#1918, @nabokihms) - Discovery endpoint contains updated claims and auth methods (dexidp#1951, @nabokihms) - Return invalid_grant error if auth code is invalid or expired (dexidp#1952, @nabokihms) - Return an error to auth requests with the "request" parameter (dexidp#1956, @nabokihms) **Minor changes:** - Change default themes to light/dark (dexidp#1858, @nabokihms) - Various developer experience improvements - Dependency upgrades - Tons of small fixes and changes

nabokihms force-pushed the add-dockerize branch from 53d2fce to c56c38c Compare December 22, 2020 04:56

sagikazarmark reviewed Dec 22, 2020

View reviewed changes

heidemn reviewed Dec 31, 2020

View reviewed changes

heidemn reviewed Jan 1, 2021

View reviewed changes

sagikazarmark reviewed Jan 1, 2021

View reviewed changes

sagikazarmark mentioned this pull request Jan 14, 2021

Create Dex helm chart dexidp/helm-charts#2

Closed

3 tasks

nabokihms mentioned this pull request Jan 28, 2021

feat: gotemplate formatting for Dex config file #1965

Closed

nabokihms force-pushed the add-dockerize branch from 8c798cf to b0d2a1c Compare January 28, 2021 11:26

nabokihms requested a review from sagikazarmark January 28, 2021 13:06

nabokihms force-pushed the add-dockerize branch 2 times, most recently from 1cb0da1 to 8afe5f0 Compare January 28, 2021 13:46

sagikazarmark changed the title ~~feat: Add dockerize to the Dex docker image~~ feat: Add gotemplate to the docker image Jan 28, 2021

sagikazarmark requested changes Jan 28, 2021

View reviewed changes

nabokihms force-pushed the add-dockerize branch 2 times, most recently from e62e270 to 394d7e5 Compare January 28, 2021 23:24

nabokihms requested a review from sagikazarmark January 28, 2021 23:29

nabokihms force-pushed the add-dockerize branch 4 times, most recently from 46cc8d5 to 0c1feba Compare January 29, 2021 04:50

sagikazarmark reviewed Jan 29, 2021

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

docker-entrypoint.sh Outdated Show resolved Hide resolved

nabokihms and others added 6 commits January 29, 2021 13:48

feat: Add dockerize to the Dex docker image

7784a47

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

Remove entrypoint.sh, add config template example

891fa17

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

Switch to gomplate

e13aac4

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

Download gomplate during building phase

d43053e

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

Apply suggestions from code review

226c91d

Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com> Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

Add docker build args

a6cb627

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

Move downloading gomplate to separate stage

3241fd4

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

nabokihms force-pushed the add-dockerize branch from 0c1feba to 3241fd4 Compare January 29, 2021 09:49

nabokihms requested a review from sagikazarmark January 29, 2021 10:05

heidemn reviewed Jan 29, 2021

View reviewed changes

docker-entrypoint.sh Outdated Show resolved Hide resolved

heidemn reviewed Jan 29, 2021

View reviewed changes

Add detailed description to docker-entrypoint.sh

7f74459

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

heidemn reviewed Jan 30, 2021

View reviewed changes

sagikazarmark reviewed Feb 4, 2021

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

Apply suggestions from code review

dd4a62e

Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com> Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

nabokihms force-pushed the add-dockerize branch from b599b16 to dd4a62e Compare February 4, 2021 17:17

sagikazarmark changed the title ~~feat: Add gotemplate to the docker image~~ feat: Add gomplate to the docker image Feb 8, 2021

sagikazarmark added this to the v2.28.0 milestone Feb 10, 2021

sagikazarmark requested changes Feb 10, 2021

View reviewed changes

Switch to slim version of a gomplate and add a comment to docker config

715fee7

Co-authored-by: Márk Sági-Kazár <sagikazarmark@users.noreply.github.com> Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>

sagikazarmark approved these changes Feb 10, 2021

View reviewed changes

sagikazarmark merged commit 10597cf into dexidp:master Feb 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add gomplate to the docker image #1893

feat: Add gomplate to the docker image #1893

nabokihms commented Dec 21, 2020 •

edited by sagikazarmark

Loading

sagikazarmark left a comment

nabokihms commented Dec 22, 2020

heidemn Dec 31, 2020 •

edited

Loading

nabokihms Dec 31, 2020

sagikazarmark Dec 31, 2020

nabokihms Jan 2, 2021

heidemn commented Dec 31, 2020

heidemn Jan 1, 2021 •

edited

Loading

sagikazarmark Jan 1, 2021

sagikazarmark left a comment

sagikazarmark Jan 1, 2021

nabokihms Jan 2, 2021

heidemn Jan 2, 2021 •

edited

Loading

mouchar Feb 4, 2021

sagikazarmark Feb 4, 2021

nabokihms commented Jan 28, 2021

sagikazarmark left a comment

heidemn Jan 29, 2021 •

edited

Loading

heidemn Jan 31, 2021 •

edited

Loading

nabokihms Jan 31, 2021

heidemn Jan 30, 2021 •

edited

Loading

sagikazarmark Feb 10, 2021

sagikazarmark left a comment

sagikazarmark Feb 10, 2021

sagikazarmark Feb 10, 2021

nabokihms Feb 10, 2021 •

edited

Loading

sagikazarmark Feb 10, 2021

sagikazarmark Feb 10, 2021

sagikazarmark left a comment

nabokihms commented Feb 10, 2021

sagikazarmark commented Feb 10, 2021

		@@ -0,0 +1,46 @@
		issuer: {{ getenv "DEX_ISSUER" "http://127.0.0.1:5556/dex" }}


		ENV GOMPLATE_VERSION=v3.9.0

		RUN wget -O /usr/local/bin/gomplate \

feat: Add gomplate to the docker image #1893

feat: Add gomplate to the docker image #1893

Conversation

nabokihms commented Dec 21, 2020 • edited by sagikazarmark Loading

Overview

What this PR does / why we need it

Special notes for your reviewer

Does this PR introduce a user-facing change?

sagikazarmark left a comment

Choose a reason for hiding this comment

nabokihms commented Dec 22, 2020

heidemn Dec 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heidemn commented Dec 31, 2020

heidemn Jan 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heidemn Jan 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabokihms commented Jan 28, 2021

sagikazarmark left a comment

Choose a reason for hiding this comment

heidemn Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

heidemn Jan 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heidemn Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabokihms Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark left a comment

Choose a reason for hiding this comment

nabokihms commented Feb 10, 2021

sagikazarmark commented Feb 10, 2021

nabokihms commented Dec 21, 2020 •

edited by sagikazarmark

Loading

heidemn Dec 31, 2020 •

edited

Loading

heidemn Jan 1, 2021 •

edited

Loading

heidemn Jan 2, 2021 •

edited

Loading

heidemn Jan 29, 2021 •

edited

Loading

heidemn Jan 31, 2021 •

edited

Loading

heidemn Jan 30, 2021 •

edited

Loading

nabokihms Feb 10, 2021 •

edited

Loading