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.

Sign up for GitHub

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

Jump to bottom

ssl: look for ~/.postgresql/root.crt #891

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

ssl: look for ~/.postgresql/root.crt #891

wants to merge 2 commits into from

Conversation

pschultz
Copy link

If the sslrootcert option hasn't been specified, use ~/.postgresql/root.crt if it exists.

This is what libpq does.

@pschultz
Copy link
Author

The failing test seems unrelated to this patch.

--- FAIL: TestCopyRespLoopConnectionError (0.03s)
    copy_test.go:402: expected error
[...]
FAIL
FAIL	github.com/lib/pq	2.734s

@xoxys
Copy link

xoxys commented May 1, 2023

Any chance to get this PR done?

@rafiss
Copy link
Collaborator

rafiss commented Jun 28, 2023 •
edited

This project is in maintenance mode (https://github.com/lib/pq#status), but I can merge this patch if it gets rebased to resolve conflicts, and tests are added.

@xoxys
Copy link

xoxys commented Jun 28, 2023

Sorry, missed that information. In that case, I guess it's not worth it.

@pschultz
ssl: look for ~/.postgresql/root.crt
2f89da5 
If the sslrootcert option hasn't been specified, use
~/.postgresql/root.crt if it exists (or %APPDATA%\postgresql\root.crt on
Windows).

This is what libpq does. See
- https://www.postgresql.org/docs/11/libpq-connect.html#LIBPQ-CONNECT-SSLROOTCERT
- https://www.postgresql.org/docs/current/libpq-ssl.html#LIBQ-SSL-CERTIFICATES
@pschultz
Copy link
Author

pschultz commented Jun 28, 2023 •
edited

I rewrote the patch on top of master and added a test. The patch is now simpler than before and will now also work on Windows.

@rafiss rafiss self-requested a review June 28, 2023 15:12
@pschultz
Copy link
Author

I'm currently changing the existing tests and only just now realized that this PR will change the behavior when sslrootcert is absent or empty from using system CAs to failing (unless ~/.postgresql/root.crt exists).

I'm not convinced anymore that that is a good idea, even if it matches libpq's behavior (which never considers system CAs as far as I can tell).

WDYT?

@pschultz
update SSL tests
2315506 
TestDefaultRootCert removed because it is a complex test that is
redundant with TestSSLVerifyCA* and TestSSLVerifyFull*.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafiss rafiss Awaiting requested review from rafiss

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pschultz @xoxys @rafiss