Pass OIDC config from .env to Backend and Frontend

[matthew-white]

I'm pushing this PR mostly in order to start a conversation about where/how Central users should configure SSO and how that config will be passed through to Backend and Frontend.

Related to the issue for SSO, #449.

What has been done to verify that this works as intended?

I haven't tested this yet, but I wanted to get initial thoughts from @alxndrsn.

Why is this the best possible solution? Were any other approaches considered?

This is similar to how we pass other config to Backend. We haven't had to pass config to Frontend before, so that part is new.

Before submitting this PR, please make sure you have:

• verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

[matthew-white]

The Vue build has access to environment variables via process.env, so I'm thinking that we can set oidcEnabled to process.env.OIDC_ENABLED in src/config.js. I don't remember the details of that mechanism (I'll look more into it later this week), I think we may need to add a Vue-specific prefix to the name of the environment variable.

[matthew-white]

I checked, and it looks like the build mostly just has access to environment variables prefixed with VUE_APP_: https://cli.vuejs.org/guide/mode-and-env.html#environment-variables. I think I'm going to keep it named OIDC_ENABLED in docker-compose.yml, then add the prefix just when calling npm run build. It looks like the VUE_APP_ prefix may need to change to VITE_ once we move to Vite (getodk/central-frontend#671).

Also, I'm not sure, but for nginx, I think OIDC_ENABLED needs to be specified in docker-compose.yml under build.args rather than environment in order for it to make its way to build-frontend.sh. The variable journeys through a couple of layers before eventually reaching npm build.

[matthew-white]

Most examples of ARG that I've seen have ARG after FROM. But service.dockerfile has ARG before FROM. Maybe because there are two FROMs? Things seem to be working here with ARG before FROM, but I could look more into this.

[alxndrsn]

This seems like a strange use of ARG, or perhaps a strange position for it in the dockerfile. Will setting it here mean every single layer in the dockerfile needs rebuilding when OIDC_ENABLED is toggled?

Is it even necessary to set this in the dockerfile at all? Presumably RUN files/prebuild/build-frontend.sh can't pick up standard env vars set in docker-compose.yml(?)

[matthew-white]

perhaps a strange position for it in the dockerfile

I've moved ARG to after the first FROM. Based on how I understand the docs, I think that's what's needed here.

Will setting it here mean every single layer in the dockerfile needs rebuilding when OIDC_ENABLED is toggled?

This question makes me wonder, should I move ARG even further down, to right before RUN files/prebuild/build-frontend.sh? I don't think what comes before build-frontend.sh takes much time, so I doubt there'd be a big difference. But to be explicit, if an OIDC_* config is changed, the Vue build will need to be rerun.

Presumably RUN files/prebuild/build-frontend.sh can't pick up standard env vars set in docker-compose.yml(?)

That's my impression from reading some of the docs, but I'm definitely no Docker expert. I tried it out in a couple of commits here, and it does look like variables set in environment aren't available in the build. From the build for 735cbe7:

$OIDC_DISCOVERY_URL in the Dockerfile: [blank]
...
$OIDC_DISCOVERY_URL in build-frontend.sh: [blank]

From the build for 8171f6c (using build.args instead):

$OIDC_DISCOVERY_URL in the Dockerfile: [foobar]
...
$OIDC_DISCOVERY_URL in build-frontend.sh: [foobar]

[alxndrsn]

Seems like it matters where the ARG is declared:

Impact on build caching

ARG variables are not persisted into the built image as ENV variables are. However, ARG variables do impact the build cache in similar ways. If a Dockerfile defines an ARG variable whose value is different from a previous build, then a “cache miss” occurs upon its first usage, not its definition. In particular, all RUN instructions following an ARG instruction use the ARG variable implicitly (as an environment variable), thus can cause a cache miss. All predefined ARG variables are exempt from caching unless there is a matching ARG statement in the Dockerfile.
-https://docs.docker.com/engine/reference/builder/#impact-on-build-caching

I think declaring directly before use is probably better:

• while build times may not be affected much, the number of local images will be, and associated disk usage
• it may make it clearer to future maintainers where the ARG is used

[matthew-white]

Done! I've moved ARG to right before build-frontend.sh.

[alxndrsn]

Approved if we're happy with current secret management.

[matthew-white]

if we're happy with current secret management

I think what this PR does is consistent with what we've done in the past. Feel free to file an issue though if you think we should rethink that approach!

[matthew-white]

OK, I'm feeling more confident about this approach! @alxndrsn, would you mind taking another look?

[matthew-white]

I wanted to note this warning I'm seeing about ARG:

It is not recommended to use build-time variables for passing secrets like GitHub keys, user credentials etc. Build-time variable values are visible to any user of the image with the docker history command.

I don't think this is a particular problem for us, because anyone who could run docker history on the machine would also be able to read the .env file. Passing this information via build.args doesn't seem riskier than passing it via environment. However, I wanted to mention the warning in case I'm misunderstanding something about it.

[matthew-white]

Right now, build-frontend.sh derives VUE_APP_OIDC_ENABLED from the other three OIDC_* variables. It'd be nice if we could derive that value somewhere prior to the Dockerfile, then pass only that value to the Dockerfile as an ARG. However, I don't know of an easy way to do that with our current setup. And again, my understanding is that this approach isn't a particular risk for us.

[matthew-white]

@alxndrsn and I discussed this earlier today. Instead all three OIDC_* variables being passed to nginx.dockerfile, now only OIDC_DISCOVERY_URL will be passed. Unlike OIDC_CLIENT_SECRET, OIDC_DISCOVERY_URL isn't sensitive, so there shouldn't be an issue that it is passed via ARG. build-frontend.sh will derive VUE_APP_OIDC_ENABLED based only on the presence of OIDC_DISCOVERY_URL.

[alxndrsn]

Even if derived from OIDC_DISCOVERY_URL, I think it would still be helpful to call this OIDC_ENABLED or similar in the frontend image, as that is what it's used for. I think seeing OIDC_DISCOVERY_URL in the frontend would raise questions about why it's being passed there.

[matthew-white]

OIDC_DISCOVERY_URL won't be visible in the JS that's built, just VUE_APP_OIDC_ENABLED. Is there somewhere other than the JS that you're thinking about? I think we can assume that anyone examining the source will be able to determine why the variable is passed to nginx.dockerfile.

[matthew-white]

Alex and I chatted about this earlier today. We decided to reintroduce a separate OIDC_ENABLED variable so that we don't need to pass the discovery/issuer URL to the Frontend build. The user will specify the OIDC_ENABLED variable: it's not derived from the other variables. I've made this change in a commit I just pushed.

[sadiqkhoja]

looks good to me

[sadiqkhoja]

create an issue about this #473

[matthew-white]

I'm going to go ahead and merge! In addition to @sadiqkhoja's approval, the final version of the code is very similar to the version that @alxndrsn approved. @alxndrsn, let me know if you have additional thoughts about anything here!