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

feat: set 755 permissions for custom ca paths #6041

Merged
merged 3 commits into from
Mar 20, 2025

Conversation

UtheMan
Copy link
Contributor

@UtheMan UtheMan commented Mar 17, 2025

Adjusts permissions for /opt/certs directory used by custom ca feature to 755
We use 755 to match permissions used for usr/local/share/ca-certficates path, which is the path that is checked by the update-ca-certificates tool that updates the trust store. This is to stop anyone from being able to add a file to the path that installs custom CAs in the trust store when feature is used on the node.

@UtheMan UtheMan force-pushed the mikolaj/adjust-custom-ca-perms branch from 58506bc to 94423e9 Compare March 19, 2025 20:42
@@ -135,6 +135,7 @@ configureHTTPProxyCA() {

configureCustomCaCertificate() {
mkdir -p /opt/certs
chmod 1755 /opt/certs
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we put anything else in this dir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know we only use this for custom ca feature. I checked RP/AB and it only appears in paths related to custom ca.

Copy link
Collaborator

Choose a reason for hiding this comment

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

might be worth leaving a comment clarifying that we don't use this dir for anything else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, added

Copy link
Collaborator

@zachary-bailey zachary-bailey left a comment

Choose a reason for hiding this comment

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

This dir is created in pre-install-dependencies, we could also look changing it there

@@ -52,7 +52,7 @@ systemctlEnableAndStart disk_queue 30 || exit 1
capture_benchmark "${SCRIPT_NAME}_copy_packer_files_and_enable_logging"

mkdir /opt/certs
chmod 1666 /opt/certs
chmod 1755 /opt/certs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachary-bailey I modified this here as well, that should be enough right? Or is there some other place you were thinking of?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah thats it!

Copy link
Contributor

No changes to cached containers or packages on Windows VHDs

@UtheMan UtheMan enabled auto-merge (squash) March 20, 2025 16:56
@UtheMan UtheMan merged commit 9b3eb02 into master Mar 20, 2025
20 checks passed
@UtheMan UtheMan deleted the mikolaj/adjust-custom-ca-perms branch March 20, 2025 17:12
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.

4 participants