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

dockerTools ca-certificates.crt helper #170906

Merged
merged 2 commits into from
Aug 31, 2022

Conversation

Sohalt
Copy link
Contributor

@Sohalt Sohalt commented Apr 29, 2022

Description of changes

Add a helper to install ca-bundle.crt in containers built with dockerTools at /etc/ssl/certs/ca-certificates.crt, where different programs such as wget look for them.

Add documentation.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@Sohalt Sohalt requested a review from roberth as a code owner April 29, 2022 12:59
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Apr 29, 2022
@Sohalt Sohalt changed the title Docker tools.ca certificates.crt dockerTools ca-certificates.crt helper Apr 29, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 29, 2022
@@ -773,6 +773,11 @@ rec {
ln -s ${bashInteractive}/bin/bash $out/bin/sh
'';

# This provides /etc/ssl/certs/ca-certificates.crt
caCertificates = runCommand "ca-certificates" { } ''
ln -s ${cacert}/etc/ssl/certs/ca-bundle.crt $out/etc/ssl/certs/ca-certificates.crt
Copy link
Member

Choose a reason for hiding this comment

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

I thought the cacert package already provided the extra symlink. Wouldn't it be simpler to add it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The symlinks are setup by the nixos ca module. If the package provided the symlinks itself, it wouldn't be of much help, since they would still be under the full store path, wouldn't they? It would work with the docker specific use case, since the image builders create the symlinks at the root directory, but eg for NixOS you'd still need the extra logic outside the package to provide the links.

Copy link
Member

Choose a reason for hiding this comment

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

The whole premise of this is that non-store paths are created at the container root from the paths below store paths, but you're right. What I was suggesting would run the risk of duplicating store paths. There's too many ways to add stuff and we should probably use the module system to take care of this stuff anyway. (poc: #148456). Your derivation will be helpful in the meanwhile.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this provide both names, like NixOS does? ca-bundle.crt and ca-certificates.crt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ca-bundle.crt is the name of the bundle in $out of cacert, so it's already set up by the dockerTools symlink. We might want to add the other symlink from the ca module for Fedora compatibility though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait it isn't. Nvm. We should add it.

doc/builders/images/dockertools.section.md Outdated Show resolved Hide resolved
doc/builders/images/dockertools.section.md Outdated Show resolved Hide resolved
@@ -773,6 +773,11 @@ rec {
ln -s ${bashInteractive}/bin/bash $out/bin/sh
'';

# This provides /etc/ssl/certs/ca-certificates.crt
caCertificates = runCommand "ca-certificates" { } ''
ln -s ${cacert}/etc/ssl/certs/ca-bundle.crt $out/etc/ssl/certs/ca-certificates.crt
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this provide both names, like NixOS does? ca-bundle.crt and ca-certificates.crt.

Various tools (e.g. wget) expect the ca bundle to be available at
/etc/ssl/certs/ca-certificates.crt
@Sohalt Sohalt force-pushed the dockerTools.ca-certificates.crt branch from 5c17f7c to f93491a Compare May 3, 2022 09:58
@Sohalt
Copy link
Contributor Author

Sohalt commented Jul 6, 2022

Anything left to do here? @roberth

Some packages expect certain files to be available globally.
When building an image from scratch (i.e. without `fromImage`), these files are missing.
`pkgs.dockerTools` provides some helpers to set up an environment with the necessary files.
You can include them in the `contents` like this:
Copy link
Member

Choose a reason for hiding this comment

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

contents has changed to copyToRoot. Otherwise lgtm.

@Sohalt Sohalt force-pushed the dockerTools.ca-certificates.crt branch from d7bcb84 to c9d8e34 Compare July 7, 2022 12:50
@Sohalt
Copy link
Contributor Author

Sohalt commented Aug 31, 2022

What's missing to get this merged?

@roberth roberth merged commit 62b25a2 into NixOS:master Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants