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

Improve SSL support #567

Merged
merged 4 commits into from
Feb 6, 2023
Merged

Improve SSL support #567

merged 4 commits into from
Feb 6, 2023

Conversation

nyurik
Copy link
Member

@nyurik nyurik commented Feb 4, 2023

This is a partial fix for #496

  • BREAKING: Now Martin behaves the same way as psql -- by default, if SSL is available on the server, it will be used, even though it will not verify that the server has a valid SSL certificate
  • Martin now understands PGSSLCERT, PGSSLKEY, and PGSSLROOTCERT env vars (and corresponding config keys) - same as psql.
  • Martin can now process ?sslmode=verify-ca and verify-full (just like psql). The verify modes require root and/or client cert & key.
  • remove danger_accept_invalid_certs -- turns out that behavior is expected by default unless ssl mode is set to verify - which upstream lib does not support - PR submitted.
  • added connection_timeout_ms option for postgres and set it to 5 seconds by default. This way it will fail out earlier.
  • added error reporting to bb8 - but it is currently broken upstream - not sure we can fix it easily, so may need to switch to deadpool later.
  • added docker-based TLS test (horray!) - wasn't trivial at all, despite ending up fairly simple.

@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2023

Codecov Report

Base: 57.92% // Head: 57.89% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (31f48ab) compared to base (3b63b5b).
Patch coverage: 54.38% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #567      +/-   ##
==========================================
- Coverage   57.92%   57.89%   -0.04%     
==========================================
  Files          29       30       +1     
  Lines        2510     2546      +36     
==========================================
+ Hits         1454     1474      +20     
- Misses       1056     1072      +16     
Impacted Files Coverage Δ
src/pg/utils.rs 51.92% <ø> (ø)
src/pg/pool.rs 29.09% <26.08%> (-1.87%) ⬇️
src/pg/tls.rs 60.86% <60.86%> (ø)
src/args/pg.rs 84.82% <100.00%> (+0.76%) ⬆️
src/pg/config.rs 86.59% <100.00%> (-0.78%) ⬇️
src/lib.rs 40.99% <0.00%> (-0.77%) ⬇️
src/pg/pg_source.rs 74.32% <0.00%> (-0.35%) ⬇️
src/mbtiles/mod.rs 0.00% <0.00%> (ø)
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nyurik nyurik force-pushed the fix-ssl branch 6 times, most recently from f0c48e2 to 3eca837 Compare February 4, 2023 22:36
@nyurik nyurik requested a review from stepankuzmin February 4, 2023 22:57
* remove `danger_accept_invalid_certs` -- turns out that behavior is expected by default unless ssl mode is set to verify - which upstream lib does not support (yet) - PR submitted.
* added connection_timeout_ms option for postgres and set it to 5 seconds by default. This way it will fail out earlier.
* added error reporting to bb8 - but it is currently broken upstream - not sure we can fix it easily, so may need to switch to deadpool later.
* added docker-based TLS test (horray!) - wasn't trivial at all, despite ending up fairly simple.
* important: if ca root file is set, it will be verified. The host will never be verified.
@nyurik nyurik marked this pull request as ready for review February 5, 2023 00:41
Copy link
Collaborator

@stepankuzmin stepankuzmin left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

One nit: could you please not remove the "Using with Heroku PostgreSQL" section from the README but drop the danger-accept flag notion?

@nyurik nyurik merged commit fbb282d into maplibre:main Feb 6, 2023
@nyurik nyurik deleted the fix-ssl branch February 6, 2023 19:32
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