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

Postgres tls/sslmode "verify-full" as default #248

Merged

Conversation

maab
Copy link
Contributor

@maab maab commented Oct 7, 2022

With this PR verify-full will be the default postgres sslmode instead of disable. A user then has to actively disable transport layer security. This improves security and hopefully awareness of users for it.

@maab maab requested a review from iegomez as a code owner October 7, 2022 08:50
@iegomez iegomez changed the base branch from release-2.0.0 to master October 7, 2022 14:40
@iegomez
Copy link
Owner

iegomez commented Oct 7, 2022

Thanks, @maab!

I retargeted it to master because 2.0.0 was out of date and I'km gonna do the release anyway.

Never mind, I rebased that branch.

Copy link
Owner

@iegomez iegomez left a comment

Choose a reason for hiding this comment

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

Oh, forgot about one thing: this needs to get documented. Could you change the Readme to be explicit about it?

Also, I'll recreate the 2.0.0 branch.

@iegomez iegomez changed the base branch from master to release-2.0.0 October 7, 2022 14:43
@iegomez
Copy link
Owner

iegomez commented Oct 21, 2022

@maab would you be available to document the change? I can throw something in the readme if not.

@maab
Copy link
Contributor Author

maab commented Oct 28, 2022

@maab would you be available to document the change? I can throw something in the readme if not.

Sorry for the late reply, I was on vacation. I'll block time to extend the readme next week.

@iegomez
Copy link
Owner

iegomez commented Oct 28, 2022

Sure, @maab, no rush. Sorry for bugging you on your vacation, hope you enjoyed your time off!

@maab
Copy link
Contributor Author

maab commented Nov 3, 2022

Thank you @iegomez and absolutely no problem.

I've pushed a suggestion for a extension of the readme.

@iegomez
Copy link
Owner

iegomez commented Nov 3, 2022

Thanks, @maab!

I'll get it merged and release the new version later today.

@iegomez
Copy link
Owner

iegomez commented Nov 3, 2022

One question, @maab: is this passing tests? I'd think this test case would need to have ssl_mode explicitly set to disable to pass: https://github.com/iegomez/mosquitto-go-auth/blob/master/backends/postgres_test.go#L12

I'm out of the country and only have my work machine with me, which is not configured to either have working services for running tests, nor the Docker test script, so I can't reliably check.

@iegomez
Copy link
Owner

iegomez commented Nov 11, 2022

@maab I tried running test through Docker but there's something weird going on in this machine and I failed to do so.
So yeah, I'll have to ask you to run them to see if the test case needs to be adjusted for this default value change.

@maab
Copy link
Contributor Author

maab commented Nov 11, 2022

Again, sorry for the late feedback. You're right, some of the tests failed because of the verify-full sslmode. I've fixed them by adding sslmode disable to the tests.

@iegomez iegomez merged commit fb6a5b8 into iegomez:release-2.0.0 Nov 11, 2022
@iegomez
Copy link
Owner

iegomez commented Nov 11, 2022

@maab It takes me some times weeks to even respond, so no worries at all.
Thanks for checking and adjusting it, I've already merged and will release 2.0.0.

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.

2 participants