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

Rework CA certificate support to allow rootless containers #538

Merged
merged 2 commits into from
May 9, 2024

Conversation

rassie
Copy link
Contributor

@rassie rassie commented Apr 22, 2024

This PR includes several improvements and simplifications in CA certificate handling.

Support for CA certificates in containers running as a non-root user

After this PR, CA certificates can get applied even if the container is run as an arbitrary user. This helps usage in restricted environments like Red Hat OpenShift and follows best practices for Kubernetes deployments. When running an Adoptium container as non-root user with USE_SYSTEM_CA_CERTS=1, only the JVM truststore will get updated (or more correctly: re-created), while system CA store will not get updated.

Support for CA certificates in containers running with read-only root filesystem

After this PR, CA certificates can get applied even if the container is run with a read-only root filesystem. Running containers with read-only root filesystem is a recommended best practice for Kubernetes. When running an Adoptium container with read-only root filesystem, the same restrictions apply as with non-root users. In addition, /tmp must be mounted as an EmptyDir volume, to allow JVM trust store to get updated.

Unification of Docker entrypoint scripts into one

Since we can't rely on system tools in non-root or read-only root filesystem mode, the certificate procedure has been unified between all Linux platforms and therefore there is now only one entrypoint script left. As a consequence, a conditional has been added in Dockerfile generation script to exclude Windows platforms, which currently do not support CA certificates.

Entrypoint script now exports CACERT environment variable to point to the used truststore file

Since the JVM truststore can't be overwritten, it needs to be recreated on a different path. This path is automatically provided via JAVA_TOOL_OPTIONS by the container entrypoint script, however it's also exported via the environment variable CACERT to facilitate re-use of that truststore in local scripts.

NB: Dockerfile updates are not included in this PR, since they will get updated by the script after the merge.

Docs updates at https://github.com/docker-library/official-images/ pending.

Possibly fixes: #464

@rassie
Copy link
Contributor Author

rassie commented Apr 22, 2024

It seems some tests are failing, which didn't fail locally. Let me look at it.

EDIT: Possibly the same problem as in the last PR on this topic: Dockerfiles are not current, so that the tests fail in the PR. Should I include the Dockerfile updates?

@karianna karianna requested review from gdams and tellison April 22, 2024 14:35
generate_dockerfiles.py Show resolved Hide resolved
docker_templates/entrypoint.sh Outdated Show resolved Hide resolved
cp $CACERT $CACERT_NEW
CACERT=$CACERT_NEW
# If we use a custom truststore, we need to make sure that the JVM uses it
export JAVA_TOOL_OPTIONS="${JAVA_TOOL_OPTIONS} -Djavax.net.ssl.trustStore=${CACERT} -Djavax.net.ssl.trustStorePassword=changeit"
Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, JAVA_TOOL_OPTIONS may be redefined by a higher layer thereby losing the path to the new trust store (unless users are careful to only append as you did here). This would be a documentation notice.

It is also possible for a layer who cannot change the JAVA_TOOL_OPTIONS value to change the ${CACERT} path or file contents - but I'm not sure how much of a problem that would be in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice one would have to override both the entrypoint and JAVA_TOOL_OPTIONS in a higher layer, since if the entrypoint executes at all, it will still append the right parameters to the variable. And if you override the entrypoint without calling the default one, you are on your own anyway.

.test/tests/java-ca-certificates-update/run.sh Outdated Show resolved Hide resolved
@karianna
Copy link
Contributor

It seems some tests are failing, which didn't fail locally. Let me look at it.

EDIT: Possibly the same problem as in the last PR on this topic: Dockerfiles are not current, so that the tests fail in the PR. Should I include the Dockerfile updates?

I think you need to rebase in that case?

@rassie
Copy link
Contributor Author

rassie commented Apr 24, 2024

It seems some tests are failing, which didn't fail locally. Let me look at it.
EDIT: Possibly the same problem as in the last PR on this topic: Dockerfiles are not current, so that the tests fail in the PR. Should I include the Dockerfile updates?

I think you need to rebase in that case?

That's no problem normally. I can try including them.

rassie added 2 commits May 8, 2024 16:41
This patch includes several improvements and simplifications in CA certificate handling:

* Support for CA certificates in containers running as a non-root user
* Support for CA certificates in containers running with read-only filesystem
* Unification of Docker entrypoint scripts into one
* Entrypoint script now exports CACERT environment variable to point to the used truststore file

Docs updates at https://github.com/docker-library/official-images/ pending.

Possibly fixes: adoptium#464
@karianna
Copy link
Contributor

@rassie Are you able to address the feedback from @tianon (he's our partner from DockerHub :-)) in a separate PR?

@rassie
Copy link
Contributor Author

rassie commented May 30, 2024

@rassie Are you able to address the feedback from @tianon (he's our partner from DockerHub :-)) in a separate PR?

Yes, sure, should be possible in the next 24 hours or so.

@rassie
Copy link
Contributor Author

rassie commented May 31, 2024

@tianon @karianna I've been writing a comment addressing @tianon's point about scope creep and might have found a way to keep multiple sides happy, while reducing pain points, in particular:

  • People feeling the scope creep and maintenance burden
  • People not liking the entrypoint requirement
  • People wanting to inject CA certificates into JRE trust stores.

I'm thinking about using a companion container image, maintained either inside or outside or Adoptium project, which would contain most of the logic in the current entrypoint and could be used as an init container.

It would work somewhat like this:

  1. Freeze and deprecate current entrypoint-based certificate implementation, add a deprecation warning
  2. Document the following recommended process:
    • First init-container: your eclipse-temurin-based target container; copy the cacerts file from it to a temporary volume
    • Second init-container: this new companion container, with your custom certificates and temporary volume mounted, which would combine both to a new cacerts using the logic in this PR
    • Main container: your eclipse-temurin-based target container with added volume mount containing the new cacerts file and an added environment variable JAVA_TOOL_OPTIONS=-Djavax.net.ssl.trustStore=/volume/cacerts -Djavax.net.ssl.trustStorePassword=changeit.
  3. At some point in the future: remove current certificate functionality, possibly remove the entrypoint altogether.

This would probably make the Kubernetes camp happy, since it directly maps onto Kubernetes proceedings. Docker camp would need something like docker compose with service_completed_successfully, but this is probably manageable.

Should I try and implement this before this PR is included in a released image?

@rassie
Copy link
Contributor Author

rassie commented May 31, 2024

Thinking further: if this companion image were to be maintained inside the Adoptium project, we might get away with just including the default cacerts and thus only needing one init container instead of two. No idea how dependant this is on the actual JRE version, maybe there is some common source for the cacerts file?

@karianna
Copy link
Contributor

karianna commented Jun 1, 2024

@tianon @karianna I've been writing a comment addressing @tianon's point about scope creep and might have found a way to keep multiple sides happy, while reducing pain points, in particular:

  • People feeling the scope creep and maintenance burden
  • People not liking the entrypoint requirement
  • People wanting to inject CA certificates into JRE trust stores.

I'm thinking about using a companion container image, maintained either inside or outside or Adoptium project, which would contain most of the logic in the current entrypoint and could be used as an init container.

It would work somewhat like this:

  1. Freeze and deprecate current entrypoint-based certificate implementation, add a deprecation warning

  2. Document the following recommended process:

    • First init-container: your eclipse-temurin-based target container; copy the cacerts file from it to a temporary volume
    • Second init-container: this new companion container, with your custom certificates and temporary volume mounted, which would combine both to a new cacerts using the logic in this PR
    • Main container: your eclipse-temurin-based target container with added volume mount containing the new cacerts file and an added environment variable JAVA_TOOL_OPTIONS=-Djavax.net.ssl.trustStore=/volume/cacerts -Djavax.net.ssl.trustStorePassword=changeit.
  3. At some point in the future: remove current certificate functionality, possibly remove the entrypoint altogether.

This would probably make the Kubernetes camp happy, since it directly maps onto Kubernetes proceedings. Docker camp would need something like docker compose with service_completed_successfully, but this is probably manageable.

Should I try and implement this before this PR is included in a released image?

I'd probably split this into it's own issue and we can build ontop of what we've already merged here.

rassie added a commit to rassie/containers-1 that referenced this pull request Jun 3, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
rassie added a commit to rassie/containers-1 that referenced this pull request Jun 3, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
rassie added a commit to rassie/containers-1 that referenced this pull request Jun 3, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
karianna pushed a commit to rassie/containers-1 that referenced this pull request Jun 10, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
rassie added a commit to rassie/containers-1 that referenced this pull request Jun 12, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
rassie added a commit to rassie/containers-1 that referenced this pull request Jun 26, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
rassie added a commit to rassie/containers-1 that referenced this pull request Jul 23, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
rassie added a commit to rassie/containers-1 that referenced this pull request Jul 24, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
6. Support multi-certificate files (again)
7. Make output less verbose
rassie added a commit to rassie/containers-1 that referenced this pull request Jul 24, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
6. Support multi-certificate files (again)
7. Make output less verbose
rassie added a commit to rassie/containers-1 that referenced this pull request Jul 24, 2024
Address the following problems with adoptium#538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
6. Support multi-certificate files (again)
7. Make output less verbose
gdams pushed a commit that referenced this pull request Jul 24, 2024
Address the following problems with #538:

1. Correct the shell selection for entrypoint, Ubuntu flavours still need explicit `bash` for variables with dots in
their names
2. Change unhelpful exported variable name (changed from `CACERT` to `JRE_CACERTS_PATH`)
3. Change `which` to more-POSIX-compatible `command -v`
4. More cleanup
5. Explicitely use `TMPDIR` when available instead of hard-coded `/tmp`
6. Support multi-certificate files (again)
7. Make output less verbose
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.

[Bug]: internal CA certificate mechanism does not work with non-root user
5 participants