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

Explicitly set ownership to nginx for certs created by ssl Certificate() #117

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

nichmoe
Copy link
Contributor

@nichmoe nichmoe commented Jun 15, 2023

When the service user places the cert, the cert is owned by it. If Nginx runs as root, this doesn't create any problems. But if Nginx runs as a separate user, it may be denied access to the cert.
This change sets the ownership of the created certs to nginx, to fix this problem.

@nichmoe nichmoe requested a review from zagy June 15, 2023 07:52
@nichmoe nichmoe self-assigned this Jun 15, 2023
@nichmoe
Copy link
Contributor Author

nichmoe commented Jun 15, 2023

@zagy Could you please check, if this is the correct fix? I want to be sure, that I understood the problem correctly ;)

@zagy
Copy link
Member

zagy commented Jun 15, 2023

It sure looks like it does the right thing.

@@ -248,6 +248,7 @@ def update(self):
-out {{component.fullchain}}
"""
)
self.cmd("setfacl -Rm u:nginx:rX {{componet.key.dir}}")
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 Author

Choose a reason for hiding this comment

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

Unfortunately it only works for files, not folders.

@nichmoe
Copy link
Contributor Author

nichmoe commented Jun 16, 2023

There is a problem, if the cert directory is in a sub directory of directory with missing permissions for nginx.

@nichmoe nichmoe force-pushed the FC-30497-Add-acl-to-nginx-to-ssl-Certificate branch 3 times, most recently from 4fcbbb2 to cc9aed3 Compare June 21, 2023 13:19
@nichmoe nichmoe removed the request for review from zagy June 23, 2023 10:53
@nichmoe nichmoe marked this pull request as ready for review June 23, 2023 10:54
@nichmoe nichmoe removed their assignment Jun 23, 2023
@frlan
Copy link
Member

frlan commented Jun 30, 2023

Works not for Debian e.a (User www-data for Nginx). Worth making the user configurable?

@nichmoe nichmoe force-pushed the FC-30497-Add-acl-to-nginx-to-ssl-Certificate branch from cc9aed3 to c49bdd1 Compare August 10, 2023 11:37
@@ -102,6 +102,8 @@ class Certificate(batou.component.Component):
# You will need something like nrpehost or sensuchecks on the host
enable_check = batou.component.Attribute("literal", default=True)

cert_owner = batou.component.Attribute(str, default="nginx")
Copy link
Member

Choose a reason for hiding this comment

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

I really like to rename the variable. I'm aware of that it makes sense, but we are allowing access to the cert to the user Nginx (or other applications) are running in. Hence I think something like granted_user or whatever (not having a good idea though ;)) would be a better name.

@nichmoe nichmoe force-pushed the FC-30497-Add-acl-to-nginx-to-ssl-Certificate branch from c49bdd1 to cf6d736 Compare August 16, 2023 07:22
@frlan frlan merged commit d7a5d36 into master Aug 17, 2023
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.

3 participants