engine: update chmod command for gpg keys on debian #17070

dvdksn · 2023-04-08T10:42:32Z

Proposed changes

Related issues (optional)

Follow-up: #16674

netlify · 2023-04-08T10:42:39Z

✅ Deploy Preview for docsdocker ready!

Name	Link
🔨 Latest commit	`d421693`
🔍 Latest deploy log	https://app.netlify.com/sites/docsdocker/deploys/643197d5b8a44300087bd046
😎 Deploy Preview	https://deploy-preview-17070--docsdocker.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

thaJeztah

Left a comment (we may have to check what we do in other examples)

thaJeztah · 2023-04-08T11:43:49Z

engine/install/debian.md

- > $ sudo chmod a+r /etc/apt/keyrings/docker.gpg
+ > $ sudo chmod -R a+rx /etc/apt/keyrings


I think this may be a bit too eager;

mkdir keyrings touch keyrings/one.sh touch keyrings/docker.gpg ls -al keyrings/ total 8 drwxr-xr-x 2 root root 4096 Apr 8 11:41 . drwxr-xr-x 3 root root 4096 Apr 8 11:41 .. -rw-r--r-- 1 root root 0 Apr 8 11:41 docker.gpg -rw-r--r-- 1 root root 0 Apr 8 11:41 one.sh

Notice that all files in the directory now have their executable bit set (including files we don't own);

chmod -R a+rx keyrings ls -l keyrings/ total 8 drwxr-xr-x 2 root root 4096 Apr 8 11:41 . drwxr-xr-x 3 root root 4096 Apr 8 11:41 .. -rwxr-xr-x 1 root root 0 Apr 8 11:41 docker.gpg -rwxr-xr-x 1 root root 0 Apr 8 11:41 one.sh

I think we need two steps;

one to set the executable bit on the directory to make it browsable (using capital X to only touch the directory itself)

one to set the permissinos on the docker.gpg (we should not touch other files that we don't own)

mkdir keyrings2 touch keyrings2/one.sh touch keyrings2/docker.gpg chmod -R a+rX keyrings2 chmod -R a+r keyrings2/docker.gpg ls -al keyrings2/ total 8 drwxr-xr-x 2 root root 4096 Apr 8 11:41 . drwxr-xr-x 4 root root 4096 Apr 8 11:41 .. -rw-r--r-- 1 root root 0 Apr 8 11:41 docker.gpg -rw-r--r-- 1 root root 0 Apr 8 11:41 one.sh

thaJeztah · 2023-04-08T12:01:31Z

I'm actually wondering, perhaps we could update the install steps to use install instead of mkdir; #16654 updated the instructions for creating the keyrings to set the right permissions, but if the directory already existed, it won't update those permissions (which is probably the issue we're running into here).

Here, I mimic a situation where the directory already exists with 0700 perms, and use mkdir -p (current installation steps); in that case, the -p just makes the mkdir stop early (directory exists, nothing to do), and it won't update any existing permissions;

mkdir -m 0700 keyrings

ls -l
total 4
drwx------ 2 root root 4096 Apr  8 11:54 keyrings

mkdir -m 0755 -p keyrings

ls -l
total 4
drwx------ 2 root root 4096 Apr  8 11:54 keyrings

install, on the other hand, allows you to do the same (create the directory if missing), but (unlike mkdir) also sets the permissions if they don't match;

ls -l
total 8
drwx------ 2 root root 4096 Apr  8 11:54 keyrings

install -m 0755 -d keyrings

ls -l
total 8
drwxr-xr-x 2 root root 4096 Apr  8 11:54 keyrings

install -m 0755 -d keyrings2

ls -l
total 8
drwxr-xr-x 2 root root 4096 Apr  8 11:54 keyrings
drwxr-xr-x 2 root root 4096 Apr  8 11:55 keyrings2

thaJeztah · 2023-04-08T12:01:57Z

I think with the install approach, we could even drop the "note" altogether 🤔

Signed-off-by: David Karlsson <david.karlsson@docker.com>

dvdksn · 2023-04-08T15:59:31Z

Nice, I like it. I had a go at updating to use install. (It's the first time I hear about this command 🙈 you live and you learn!)

Please take a look!

thaJeztah · 2023-04-08T16:05:24Z

(It's the first time I hear about this command 🙈 you live and you learn!)

I didn't know about it until "not too long ago", when I stumbled upon it when working on some of our packaging scripts. It's definitely not a command I think of "all the time", but it's pretty useful actually!

thaJeztah · 2023-04-08T16:09:04Z

engine/install/debian.md

- > Receiving a GPG error when running `apt-get update`?
- >
- > Your default [umask](https://en.wikipedia.org/wiki/Umask){: target="blank"
- > rel="noopener" } may be incorrectly configured, preventing detection of the
- > repository public key file. Try granting read permission for the Docker
- > public key file before updating the package index:
- >
- > ```console
- > $ sudo chmod a+r /etc/apt/keyrings/docker.gpg
- > $ sudo apt-get update
- > ```


🎉

before we fully celebrate, let's also get some more eyes on this in case we're overlooking scenarios where someone could still run into this.

Overall I think we should be fine (assuming users just ran the steps above), and I guess if someone ran those steps with the old variant, they would already have run into the issue.

Oh! 🤦 I think there is still a situation; we now fixed the directory permissions, but we don't explicitly set read permissions on the docker.gpg itself.

I guess we could either keep a note (but I hate "too many notes" as they distract from the overall flow), or we could add a line to always set the permissions;

$ curl -fsSL {{ download-url-base }}/gpg | sudo gpg --dearmor -o /etc/apt/keyrings/docker.gpg $ sudo chmod a+r /etc/apt/keyrings/docker.gpg

I double-checked and yes, looks like we still may have to (here's with a 0066 umask);

umask 0066 curl -fsSL https://download.docker.com/linux/ubuntu/gpg | gpg --dearmor -o /etc/apt/keyrings/docker2.gpg ls -l /etc/apt/keyrings/ total 8 -rw-r--r-- 1 root root 2760 Jun 7 2022 docker.gpg -rw------- 1 root root 2760 Apr 8 16:20 docker2.gpg umask 0022

added chmod to the previous step - I agree that too many notes creates clutter and makes it hard to read

Signed-off-by: David Karlsson <david.karlsson@docker.com>

thaJeztah

LGTM

@tianon @neersighted perhaps you want to give this a sanity check?

tianon

Seems reasonable to me -- I worried for a moment about users who might have had different permissions on /etc/apt/keyrings on purpose, but those users aren't likely to be blindly copy/pasting that install line from these docs (just recording the thought process 👍).

neersighted

Lost this in my notifications; seems reasonable to me as well/matches what I do on my systems.

dvdksn requested a review from thaJeztah April 8, 2023 10:42

dvdksn force-pushed the engine/update-chmod-gpgkeys-deb branch from 8b5074d to 2d244d0 Compare April 8, 2023 10:43

thaJeztah requested changes Apr 8, 2023

View reviewed changes

thaJeztah added area/engine Issue affects Docker engine/daemon area/install Relates to installing a product labels Apr 8, 2023

engine: replace mkdir with install for gpg setup

7ad91f1

Signed-off-by: David Karlsson <david.karlsson@docker.com>

dvdksn force-pushed the engine/update-chmod-gpgkeys-deb branch from 2d244d0 to 7ad91f1 Compare April 8, 2023 15:58

dvdksn requested a review from thaJeztah April 8, 2023 16:00

thaJeztah reviewed Apr 8, 2023

View reviewed changes

engine: add chmod a+r for docker.gpg key

d421693

Signed-off-by: David Karlsson <david.karlsson@docker.com>

thaJeztah approved these changes Apr 8, 2023

View reviewed changes

dvdksn mentioned this pull request Apr 10, 2023

Update ubuntu.md #16536

Closed

tianon approved these changes Apr 11, 2023

View reviewed changes

neersighted approved these changes Apr 11, 2023

View reviewed changes

dvdksn merged commit ce05003 into docker:main Apr 11, 2023

thaJeztah mentioned this pull request Apr 26, 2023

ci/cirrus: use vagrant from hashicorp repo, bump Fedora to 38, bump bats opencontainers/runc#3848

Merged

dvdksn deleted the engine/update-chmod-gpgkeys-deb branch May 3, 2023 13:47

thaJeztah mentioned this pull request May 7, 2023

Assorted fixes and improvements docker/docker-install#359

Merged

thaJeztah mentioned this pull request Jan 25, 2024

Fix apt-get ascii GPG key #19138

Merged

dvdksn mentioned this pull request Jan 26, 2024

engine: add chmod a+r for gpg key #19232

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

engine: update chmod command for gpg keys on debian #17070

engine: update chmod command for gpg keys on debian #17070

dvdksn commented Apr 8, 2023

netlify bot commented Apr 8, 2023 •

edited

Loading

thaJeztah left a comment

thaJeztah Apr 8, 2023

thaJeztah commented Apr 8, 2023

thaJeztah commented Apr 8, 2023

dvdksn commented Apr 8, 2023

thaJeztah commented Apr 8, 2023

thaJeztah Apr 8, 2023

thaJeztah Apr 8, 2023

dvdksn Apr 8, 2023

thaJeztah left a comment

tianon left a comment

neersighted left a comment

		> $ sudo chmod a+r /etc/apt/keyrings/docker.gpg
		> $ sudo chmod -R a+rx /etc/apt/keyrings

engine: update chmod command for gpg keys on debian #17070

engine: update chmod command for gpg keys on debian #17070

Conversation

dvdksn commented Apr 8, 2023

Proposed changes

Related issues (optional)

netlify bot commented Apr 8, 2023 • edited Loading

✅ Deploy Preview for docsdocker ready!

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Apr 8, 2023

Choose a reason for hiding this comment

thaJeztah commented Apr 8, 2023

thaJeztah commented Apr 8, 2023

dvdksn commented Apr 8, 2023

thaJeztah commented Apr 8, 2023

thaJeztah Apr 8, 2023

Choose a reason for hiding this comment

thaJeztah Apr 8, 2023

Choose a reason for hiding this comment

dvdksn Apr 8, 2023

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

tianon left a comment

Choose a reason for hiding this comment

neersighted left a comment

Choose a reason for hiding this comment

netlify bot commented Apr 8, 2023 •

edited

Loading