-
Notifications
You must be signed in to change notification settings - Fork 8
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
Https subscription and root certs #341
Conversation
5fc78e2
to
a4bb850
Compare
a4bb850
to
0499b8c
Compare
0499b8c
to
dfd13b3
Compare
dfd13b3
to
78a0184
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #341 +/- ##
==========================================
- Coverage 57.15% 57.03% -0.13%
==========================================
Files 69 69
Lines 7310 7324 +14
Branches 2703 2710 +7
==========================================
- Hits 4178 4177 -1
- Misses 1322 1324 +2
- Partials 1810 1823 +13 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general.
I think it would be good to persist the findings about how the certstore is handled. Please update the PR description and commit messages accordingly.
Also this adds quite some new uncovered new code paths which I think should be easy to fix, since RestClient_tests.cpp already has http(s) tests for get/set and a http only test for subscribe.
https subscriptions were not handled, create a SSLCLient and reuse existing code for subscribing. the certificate store was shared between all SslClients in our RestClient, this does not work, because the client owns the store and deletes it on its own destruction. this patch also adds the certificate specified by OPENCMW_REST_CERT_FILE to our RestDefaultCLientCertificates to make it trusted.
ccc993d
to
2bd9294
Compare
Quality Gate failedFailed conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks for the additional documentation.
I've looked into the coverage, it seems most of it is due to coverage not being properly configured in this project, the majority of the missed lines is in the unit-test that seems to be excluded from coverage collection but not coverage calculation :/
Implement https subscriptions and fix a bug related to our handling regarding the https client.
Https subscriptions were just not handled, initialise a client and reuse the existing code for http subscriptions.
There was a bug in our certificate store handling. The SSLClient owns its certificate store and deletes it on destruction.
We can not share one store with all clients, so every client gets its own now.