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

Add option to not verify certificate when calling the real estate WMS #2005

Merged
merged 10 commits into from
Aug 16, 2024

Conversation

jwkaltz
Copy link
Member

@jwkaltz jwkaltz commented Jun 25, 2024

Add option to not verify certificate when calling the real estate WMS. Useful for example if the WMS has a self-signed certificate.

In addition to the CI, this PR was tested locally with
http://localhost:6543/oereb/extract/json?EGRID=CH113928077734&WITHIMAGES=true

with both settings (verify_certificate True and False)

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.51%. Comparing base (3a31466) to head (fab63e2).
Report is 12 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2005   +/-   ##
=======================================
  Coverage   85.51%   85.51%           
=======================================
  Files         120      120           
  Lines        5275     5276    +1     
=======================================
+ Hits         4511     4512    +1     
  Misses        764      764           
Flag Coverage Δ
unittests 85.51% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jwkaltz jwkaltz requested review from domfre and michmuel June 25, 2024 14:51
@jwkaltz jwkaltz marked this pull request as draft July 1, 2024 10:33
Copy link
Collaborator

@michmuel michmuel left a comment

Choose a reason for hiding this comment

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

Thanks @jwkaltz. I added some comments.

It seems to me that it is a bit a partial solution to a very specific problem of a particular oereb server instance. And not a general use case. If an extract with geometry is requested, the wms for a topic could have as well a self-signed certificate. But this is not possible so for with the standard source. In addition, neglecting the certificates is not advised. Do we really need this MR?

pyramid_oereb/contrib/data_sources/standard/sources/plr.py Outdated Show resolved Hide resolved
pyramid_oereb/core/readers/real_estate.py Outdated Show resolved Hide resolved
pyramid_oereb/core/readers/real_estate.py Outdated Show resolved Hide resolved
pyramid_oereb/core/records/view_service.py Outdated Show resolved Hide resolved
@michmuel michmuel requested a review from svamaa July 2, 2024 14:00
@jwkaltz
Copy link
Member Author

jwkaltz commented Jul 3, 2024

Thanks @jwkaltz. I added some comments.

It seems to me that it is a bit a partial solution to a very specific problem of a particular oereb server instance. And not a general use case. If an extract with geometry is requested, the wms for a topic could have as well a self-signed certificate. But this is not possible so for with the standard source. In addition, neglecting the certificates is not advised. Do we really need this MR?

Hi @michmuel , thanks for the review, I will look at your proposed changes!

Regarding the general context:
In fact the requirement does not come from within the oereb instance itself, but from the oereb server which calls a service with a self-signed certificate. I know of at least 2 cantons which use self-signed certificated for their services, because they believe it saves money to use self-signed certificates. I did advise them to not use self-signed certificates, but this is currently not an option.
We did have a similar situation with the oereb_client, which is why this possibility was added there (openoereb/oereb_client@be16b59).

As far as I could tell, adding this option to the requests.get call in view_service.py is in fact a full solution for this issue. There is also the swisstopo/address.py, but swisstopo has proper certificates so this not an issue there.

@jwkaltz jwkaltz force-pushed the add_option_to_not_verify_wms_certificate branch from c4506ae to 99bb454 Compare July 9, 2024 05:40
@jwkaltz jwkaltz requested review from michmuel and voisardf July 9, 2024 11:11
@jwkaltz jwkaltz marked this pull request as ready for review July 9, 2024 13:19
Copy link
Collaborator

@svamaa svamaa left a comment

Choose a reason for hiding this comment

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

I agree with @michmuel, it's not ideal to open pyramid_oereb to use self-signed certificates. Wouldn't it be possible to add a self-signed certificate to the docker image of pyramid_oereb?
However, the changes don't affect the security of users, that use proper certificates.
Though, I would strongly advice in pyramid_oereb.yml.mako not to use verify_certificate: False with a comment.

@jwkaltz jwkaltz requested review from michmuel and svamaa August 13, 2024 08:18
Copy link
Collaborator

@svamaa svamaa 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. Thank you for adding the comment.

Copy link
Collaborator

@michmuel michmuel left a comment

Choose a reason for hiding this comment

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

The branch works fine in our test environment. However, I spotted a few things.

dev/config/pyramid_oereb.yml.mako Show resolved Hide resolved
pyramid_oereb/core/readers/real_estate.py Outdated Show resolved Hide resolved
pyramid_oereb/core/readers/real_estate.py Outdated Show resolved Hide resolved
@jwkaltz jwkaltz requested a review from michmuel August 16, 2024 08:02
@jwkaltz jwkaltz merged commit 5567e13 into master Aug 16, 2024
12 checks passed
@jwkaltz jwkaltz deleted the add_option_to_not_verify_wms_certificate branch August 16, 2024 08:50
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