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

Do not allow other users to read the generated SSL key/crt #141

Merged
merged 4 commits into from
Sep 25, 2024

Conversation

hhorak
Copy link
Member

@hhorak hhorak commented Mar 31, 2022

We need to maintain the feature of being able to run the container as
any user ID, so we cannot just leave the user to have read permissions
for the generated key and certificate.

However, there seems to be no use case for having the permissions
for reading for other users. While being a different user inside a container
might be not relevant anyway in the container case, let's rather be
super cautious and remove the read permissions that are not needed.

@hhorak
Copy link
Member Author

hhorak commented Mar 31, 2022

@notroj @uhliarik This would deserve your approval, as I might miss some use case where read access to "others" is needed in containers..

@hhorak
Copy link
Member Author

hhorak commented Mar 31, 2022

[test]

@hhorak hhorak changed the title Do not allow any user to read the generated SSL key/crt Do not allow other users to read the generated SSL key/crt Mar 31, 2022
@hhorak
Copy link
Member Author

hhorak commented Mar 31, 2022

[test]

@uhliarik
Copy link
Member

I don't see any reason, why $SSLKEY should have 644 permissions, maybe there is some hidden reason I don't know about, but from my point of view it can definitely have 640.

I was trying to find from where a+r permission came from and it is apparently there from the beginning, so it doesn't give any hint:

b18438a#diff-e5ca019280a9762c6e79e75ecd328dbcb2d016535ce116eaf709edca0bc46813R49

Anyway from my point of view you can lower down the permissions.

@phracek
Copy link
Member

phracek commented Apr 26, 2022

[test-all]

@uhliarik
Copy link
Member

Humm I forgot on case that httpd can be also run as a non-root user. Therefore it won't be possible for httpd to read $SSLKEY. If we wan't to have read permissions for $SSLKEY owner only, we should chown the $SSLKEY file before we execute httpd.

@phracek
Copy link
Member

phracek commented Jul 27, 2022

[test-all]

@pkubatrh
Copy link
Member

pkubatrh commented Aug 2, 2023

@hhorak Can you take a look at the comments?

@hhorak
Copy link
Member Author

hhorak commented Dec 15, 2023

we should chown the $SSLKEY file before we execute httpd

I think this is not an issue. If we run the container under any user, the certificates are generated and thus owned by this user every-time, because it's done just before the httpd is run. The location is $HTTPD_TLS_CERT_PATH/ that is /etc/httpd/tls actually, so it is IMO not possible to bring those auto-generated certificates together with the application and then having problems when the container is re-started under a different user. Even if it was, the g+w permissions should be sufficient here.

We need to maintain the feature of being able to run the container as
any user ID, so we cannot just leave the user to have read permissions
for the generated key and certificate.

However, there seems to be no use case for having the permissions
for reading for other users. While being a different user inside a container
might be not relevant anyway in the container case, let's rather be
super cautious and remove the read permissions that are not needed.
@hhorak hhorak force-pushed the permissions-for-certificates branch from f18cd78 to b9cede9 Compare December 15, 2023 10:26
@hhorak
Copy link
Member Author

hhorak commented Dec 15, 2023

[test-all]

@hhorak
Copy link
Member Author

hhorak commented Dec 15, 2023

IOW I think this PR is still valid and ready to merge.

Copy link
Contributor

@zmiklank zmiklank left a comment

Choose a reason for hiding this comment

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

Could we do this change for micro images too?

Otherwise LGTM.

Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Signed-off-by: Petr "Stone" Hracek <phracek@redhat.com>
Copy link

github-actions bot commented Sep 25, 2024

Pull Request validation

Failed

🔴 Review - Missing review from a member

Success

🟢 CI - All checks have passed

@phracek
Copy link
Member

phracek commented Sep 25, 2024

[test]

@phracek
Copy link
Member

phracek commented Sep 25, 2024

Could we do this change for micro images too?

Otherwise LGTM.

Fixed by c876a95

Copy link

github-actions bot commented Sep 25, 2024

Testing Farm results

namecomposearchstatusstarted (UTC)timelogs
Fedora - 2.4Fedora-latestx86_64✅ passed25.09.2024 07:08:5617min 31stest pipeline
CentOS Stream 9 - 2.4-microCentOS-Stream-9x86_64✅ passed25.09.2024 07:08:5418min 10stest pipeline
RHEL9 - 2.4RHEL-9.4.0-Nightlyx86_64✅ passed25.09.2024 07:08:5523min 38stest pipeline
RHEL8 - 2.4RHEL-8.10.0-Nightlyx86_64✅ passed25.09.2024 07:08:5826min 28stest pipeline

@pkubatrh pkubatrh merged commit 0634601 into sclorg:master Sep 25, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants