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

Add documentation for custom CA certificates for Temurin #2338

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

rassie
Copy link
Contributor

@rassie rassie commented Jun 15, 2023

Will be available after adoptium/containers#392 is merged.

eclipse-temurin/content.md Outdated Show resolved Hide resolved
@tianon
Copy link
Member

tianon commented Jun 15, 2023

Oof, I don't love the feature, but I don't think I dislike it strongly enough to block it at the official images level. 😬

(It feels like scope creep IMO, since it's something users would generally want from "any" container, but it's not super reasonable for every container to implement this type of entrypoint behavior, and it certainly isn't likely to be implemented in debian, alpine, etc. 😅)

@rassie
Copy link
Contributor Author

rassie commented Jun 15, 2023

FWIW, it's a feature that has been available in the old openjdk images for a long time. Adding CA certificates in general to the OS's certificate store hasn't been a problem, it's all about Java's truststore, so I think I can't agree on the "scope creep" part. Either way, thanks for not blocking :)

@tianon
Copy link
Member

tianon commented Jun 15, 2023

Not like this, no - the only part that was in openjdk was making sure that if the system CA store was updated, that the Java store was kept in sync. There wasn't any functionality there to automatically import user-provided certs (I wrote the code for it 😅).

@tianon
Copy link
Member

tianon commented Jun 15, 2023

(ie, there was never an ENTRYPOINT defined in the openjdk images, and this is introducing one in eclipse-temurin that I imagine is not necessarily desirable for a lot of the images downstream from this that might define their own entrypoint scripts)

@rassie
Copy link
Contributor Author

rassie commented Jun 16, 2023

(ie, there was never an ENTRYPOINT defined in the openjdk images, and this is introducing one in eclipse-temurin that I imagine is not necessarily desirable for a lot of the images downstream from this that might define their own entrypoint scripts)

Yeah, I've hoped we could avoid introducing the entrypoint script, but with ubi9 there was simply no way to add an update-ca-certificates-style hook like in openjdk. Best I could do is make the entrypoint a noop in "normal" circumstances, that's why there is an opt-in variable, which also means that nothing changes for those downstream images.

(on a more self-centered note, the entrypoint solves a quite large pain point for us -- with openjdk we needed to overwrite the entrypoint with bash -c "update-ca-certificatest; <whatever the original entrypoint did>", which varied depending on the image, i.e. if the product teams changed the invocation (e.g. from java -jar product-fatjar.jar to java org.springframework.boot.loader.JarLauncher), we had to synchronize that with deployment scripts)

@tellison
Copy link

tellison commented Aug 3, 2023

Just noting that the implementation has been merged, so this documentation issue can continue.

@gdams
Copy link
Contributor

gdams commented Aug 9, 2023

@tianon please can we get this merged

@yosifkit yosifkit marked this pull request as ready for review August 9, 2023 17:40
@yosifkit
Copy link
Member

yosifkit commented Aug 9, 2023

Close/open in attempt to get tests to run.

@yosifkit yosifkit closed this Aug 9, 2023
@yosifkit yosifkit reopened this Aug 9, 2023
eclipse-temurin/content.md Outdated Show resolved Hide resolved
@yosifkit
Copy link
Member

yosifkit commented Aug 9, 2023

I had to copy the test output to see it, but there is an extra whitespace at the end of line that markdownfmt is complaining about.

diff ./eclipse-temurin/content.md markdownfmt/./eclipse-temurin/content.md
--- /tmp/markdownfmt1406235651
+++ /tmp/markdownfmt2757242851
@@ -14,13 +14,13 @@
 
 # Can I add my internal CA certificates to the truststore?
 
-Yes! Add your certificates to `/certificates` inside the container (e.g. by using a volume) and set the environment variable `USE_SYSTEM_CA_CERTS` on the container to any value. With Docker CLI this might look like this: 
+Yes! Add your certificates to `/certificates` inside the container (e.g. by using a volume) and set the environment variable `USE_SYSTEM_CA_CERTS` on the container to any value. With Docker CLI this might look like this:
 
 ```console
 $ docker run -v $(pwd)/certs:/certificates/ -e USE_SYSTEM_CA_CERTS=1 %%IMAGE%%:11 
 ```
 
-The certificates would get added to the system CA store, which would in turn be converted to Java's truststore. The format of the certificates depends on what the OS of the base image used expects, but PEM format with a `.crt` file extension is a good bet. **Please note**: this feature is currently not available for Windows-based images. 
+The certificates would get added to the system CA store, which would in turn be converted to Java's truststore. The format of the certificates depends on what the OS of the base image used expects, but PEM format with a `.crt` file extension is a good bet. **Please note**: this feature is currently not available for Windows-based images.

@rassie
Copy link
Contributor Author

rassie commented Aug 9, 2023

Let me take a look at it tomorrow and rebase it while we are it.

@rassie rassie changed the title Add documentation for custom CA certificates für Temurin Add documentation for custom CA certificates for Temurin Aug 10, 2023
@rassie rassie force-pushed the patch-1 branch 2 times, most recently from 3ada4df to 8d00355 Compare August 10, 2023 15:57
@rassie
Copy link
Contributor Author

rassie commented Aug 10, 2023

I've rebased on current master branch, fixed Markdown so that .ci/check-markdownfmt.sh doesn't complain anymore and squashed the commits. Should be good to go (fingers crossed).

@yosifkit yosifkit merged commit 9fa338f into docker-library:master Aug 10, 2023
5 checks passed
yosifkit added a commit to infosiftr/tomcat that referenced this pull request Aug 11, 2023
The upstream entrypoint is `sh` and so loses dotted environment variables, lets prevent that from happening by just skipping it as the `tomcat` images isn't reliant on its functionality (docker-library/docs#2338).

Fixes docker-library#302 which is a recurrence of docker-library#77
yosifkit added a commit to infosiftr/tomcat that referenced this pull request Aug 11, 2023
The upstream entrypoint is `sh` and so loses dotted environment variables, lets prevent that from happening by just skipping it as the `tomcat` images isn't reliant on its functionality (docker-library/docs#2338).

Fixes docker-library#302 which is a recurrence of docker-library#77
yosifkit added a commit to infosiftr/tomcat that referenced this pull request Aug 11, 2023
The upstream entrypoint is `sh` and so loses dotted environment variables, lets prevent that from happening by just skipping it as the `tomcat` images are not reliant on its functionality. See docker-library/docs#2338 and adoptium/containers#392 for info about what it provides.

Fixes docker-library#302 which is a recurrence of docker-library#77
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.

5 participants