httpd-container icon indicating copy to clipboard operation
httpd-container copied to clipboard

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

Open hhorak opened this issue 3 years ago • 11 comments

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 avatar Mar 31 '22 09:03 hhorak

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

hhorak avatar Mar 31 '22 09:03 hhorak

[test]

hhorak avatar Mar 31 '22 09:03 hhorak

[test]

hhorak avatar Mar 31 '22 09:03 hhorak

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:

https://github.com/sclorg/httpd-container/commit/b18438a1efefcc78bfc5d4180a54743e1fcd2dd5#diff-e5ca019280a9762c6e79e75ecd328dbcb2d016535ce116eaf709edca0bc46813R49

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

uhliarik avatar Apr 26 '22 10:04 uhliarik

[test-all]

phracek avatar Apr 26 '22 10:04 phracek

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.

uhliarik avatar Apr 26 '22 11:04 uhliarik

[test-all]

phracek avatar Jul 27 '22 12:07 phracek

@hhorak Can you take a look at the comments?

pkubatrh avatar Aug 02 '23 08:08 pkubatrh

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.

hhorak avatar Dec 15 '23 10:12 hhorak

[test-all]

hhorak avatar Dec 15 '23 10:12 hhorak

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

hhorak avatar Dec 15 '23 10:12 hhorak

Pull Request validation

Failed

🔴 Review - Missing review from a member

Success

🟢 CI - All checks have passed

github-actions[bot] avatar Sep 25 '24 07:09 github-actions[bot]

[test]

phracek avatar Sep 25 '24 07:09 phracek

Could we do this change for micro images too?

Otherwise LGTM.

Fixed by https://github.com/sclorg/httpd-container/pull/141/commits/c876a9592a763a5276e2a8843aa48fc7f5321335

phracek avatar Sep 25 '24 07:09 phracek

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

github-actions[bot] avatar Sep 25 '24 07:09 github-actions[bot]