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

Slightly reduce TLS file security checks #132

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

will
Copy link
Contributor

@will will commented Jan 28, 2021

  • Allow files to additionally be owned by root
  • Allow group read permissions on private key file

I don't think these changes make the certs all that much less secure, and they would allow me to reuse the certs the postgres server is using without having to copy them and change the owner away from root and the permissions.

@jesperpedersen jesperpedersen self-assigned this Jan 28, 2021
@jesperpedersen
Copy link
Collaborator

Hi Will,

Maybe we should divide the checks into user and root - just within an overall if probably - and document the checks more. What do you think ?

Thanks for contributing !

will added a commit to will/pgagroal that referenced this pull request Jan 29, 2021
* Allow files to additionally be owned by root
* Allow group read permissions on private key file
@will
Copy link
Contributor Author

will commented Jan 29, 2021

Hi, I've updated this with more documentation. I wasn't able to figure out a way to do the checks differently. It's not that I'm running pgagroal as root, just that the files may or may not be owned by root, so I cant see a good place to do an overall if. I could just be missing it though.

@jesperpedersen
Copy link
Collaborator

I'm thinking about expanding the checks, like

https://github.com/agroal/pgagroal/blob/master/src/libpgagroal/security.c#L3187

into

if (st.st_uid == geteuid())
{
  /* Check for 0600 and log */
}
else if (st.st_uid == 0)
{
  /* Check for 0640 and log */
}
else
{
  /* Log requirements */
  goto error;
}

or similar checks within that function such as that the logging is more specific to each case.

Please, squash your commits and remember to update the AUTHORS file as well :)

@jesperpedersen jesperpedersen added the enhancement Improvement to an existing feature label Feb 1, 2021
* Allow files to additionally be owned by root
* Allow group read permissions on private key file
@will
Copy link
Contributor Author

will commented Feb 9, 2021

@jesperpedersen sorry for the delay, the last week for me was very busy. I think I've done what you were looking for, let me know. It's been rebased on top of the current master.

Also it seems like my editor did a bunch of whitespace trimming and I committed them before noticing. It looks like it was just on a few things, so I think maybe you would want them in. If not, I can work through and drop those from the patch.

@jesperpedersen jesperpedersen merged commit cafe42d into agroal:master Feb 9, 2021
@jesperpedersen
Copy link
Collaborator

Thanks Will !

Looking forward to your next contribution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants