-
Notifications
You must be signed in to change notification settings - Fork 122
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
Conversation
[test] |
[test] |
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. |
[test-all] |
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. |
[test-all] |
@hhorak Can you take a look at the comments? |
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 |
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.
f18cd78
to
b9cede9
Compare
[test-all] |
IOW I think this PR is still valid and ready to merge. |
There was a problem hiding this 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>
Pull Request validationFailed🔴 Review - Missing review from a member Success🟢 CI - All checks have passed |
[test] |
Fixed by c876a95 |
Testing Farm results
|
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.