Skip to content

Conversation

@audunmg
Copy link
Contributor

@audunmg audunmg commented Jan 19, 2024

Running apt-get remove in a loop is probably the slowest possible way to uninstall packages.
apt-get will not fail if any of the packages are already uninstalled, so this is unneccesary.

Changing the file extension for the GPG key to .asc lets apt-get know the GPG key is in ascii armor format, no need to dearmor.

@audunmg audunmg requested a review from dvdksn as a code owner January 19, 2024 00:09
@netlify
Copy link

netlify bot commented Jan 19, 2024

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 33befd6
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/65b2cbad4903180008a39b23
😎 Deploy Preview https://deploy-preview-19138--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@github-actions github-actions bot added the area/install Relates to installing a product label Jan 19, 2024
@thaJeztah thaJeztah requested a review from tianon January 19, 2024 00:15
Copy link
Contributor

@tianon tianon left a comment

Choose a reason for hiding this comment

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

I think APT has supported .asc files since ~1.4 (and the oldest Ubuntu LTS, 20.04/Focal has APT 2.0) 👍

LGTM

@tianon
Copy link
Contributor

tianon commented Jan 19, 2024

(even the oldest LTS-supported release of Debian has APT 1.8 🚀)

@audunmg
Copy link
Contributor Author

audunmg commented Jan 19, 2024

Thank you for verifying the .asc support in APT and for looking at it so quickly

Copy link
Contributor

@dvdksn dvdksn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @audunmg , could I ask you not to remove the for loop for apt? It's by design, see #17405

@tianon
Copy link
Contributor

tianon commented Jan 19, 2024

Oh right, it'll still error for things it doesn't know at all (just not for things it knows but aren't installed) 🤦

We could "fix" that by getting clever with patterns, but that's also somewhat dangerous (as the patterns then might match unrelated things). The loop is probably best. 😅

Changing the file extension for the GPG key to .asc lets apt-get know
the GPG key is in ascii armor format, no need to dearmor.

Co-authored-by: audunmg <audun@gangsto.org>
Co-authored-by: David Karlsson <35727626+dvdksn@users.noreply.github.com>
Signed-off-by: David Karlsson <35727626+dvdksn@users.noreply.github.com>
@dvdksn
Copy link
Contributor

dvdksn commented Jan 25, 2024

I patched the commit to revert the for loop change, and replicated the change to raspian/debian as well. ptal @tianon

Copy link
Contributor

@tianon tianon left a comment

Choose a reason for hiding this comment

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

🥳

@dvdksn dvdksn merged commit 5db1e07 into docker:main Jan 25, 2024
@dvdksn dvdksn changed the title Fix apt-get remove and ascii GPG key Fix apt-get ascii GPG key Jan 25, 2024
@thaJeztah
Copy link
Member

Thanks for looking, @tianon ! <3

sudo install -m 0755 -d /etc/apt/keyrings
curl -fsSL {{% param "download-url-base" %}}/gpg | sudo gpg --dearmor -o /etc/apt/keyrings/docker.gpg
sudo chmod a+r /etc/apt/keyrings/docker.gpg
curl -fsSL {{% param "download-url-base" %}}/gpg -O /etc/apt/keyrings/docker.asc
Copy link
Member

Choose a reason for hiding this comment

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

Oh, erm, silly question; would need sudo curl now? (or | sudo tee)? Looks like /etc/apt/keyrings may not be accessible by everyone;

ls -l /etc/apt/keyrings
total 4
-rw-r--r-- 1 root root 2760 Jan 25 21:02 docker.gpg

The sudo chmod a+r /etc/apt/keyrings/docker.gpg was added for some cloud systems where permissions were not allowing traversing the directories, causing things to fail; #17070 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, yes. Would sudo curl do or do we need tee?

Copy link
Member

Choose a reason for hiding this comment

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

sudo curl is probably the easy one, but thinking if cURL depends on user-directories (therefore sudo potentially looking for config in root's home-dir and such. running cURL as root may have a slightly bigger attack surface (vs tee) as well, but perhaps that's just looking for issues.

I'm sure @tianon has opinions as well if curl | sudo tee is preferred over sudo curl

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, attack surface is all that came to mind for me (generally, running curl as root is probably safe, but not the best idea). That being said, if we use tee, we should probably also send the output to /dev/null, so I'd personally think sudo curl is probably the simpler answer to keep the docs easier to understand (users that paranoid are hopefully not even using this line and are instead doing things like fetching the key by full fingerprint from elsewhere and/or doing deeper verification after download).

Copy link
Member

Choose a reason for hiding this comment

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

Yup, sudo curl is definitely easier to grasp, so perhaps it's an ok trade-off (readability over "fully correct").

We should look at the directory permissions though (the sudo chmod a+r /etc/apt/keyrings/docker.gpg)

as I recall there were some real-life scenarios outside of the user's control where things broke without #17070

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install Relates to installing a product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants