fluves / pywaterinfo

Python package to download time series data from waterinfo.be
https://fluves.github.io/pywaterinfo/
MIT License
17 stars 9 forks source link

Fix SSL network CA certificate handling #9

Closed stijnvanhoey closed 4 years ago

stijnvanhoey commented 4 years ago

Currently, the usage of the CA certificate (which was required in the VITO network) is commented out in the code. Should be fixed/controlled/checked (preferably by someone from VITO). The method is still present: https://github.com/fluves/pywaterinfo/blob/master/src/pywaterinfo/waterinfo.py#L123

binomaiheu commented 4 years ago

Just checked the package & works perfectly on my home linux box, inside VITO VPN i indeed get the SSL error back. Since this is an issue which seems specifically related to VITO network security, I propose to fix this simply by leaving it as it is and adding a small note in the documentation on how to add the certificate manually, referring to https://stackoverflow.com/questions/27835619/urllib-and-ssl-certificate-verify-failed-error ? If you agree, will do this tomorrow or so (perfect morning coffee activity :-)).

stijnvanhoey commented 4 years ago

That is fine for me too, although others my experience the same issue in corporate networks ;-)

Another alternative I see is to refactor the code a bit, something like...

class Waterinfo:
    def __init__(self, provider: str = "vmm", token: str = None, ssl_cert: str = None:
        """
        ...
        ssl_cert : str
             Path to the ssl certificate to use.
        """
        ...
        if ssl_cert:
            self._verify_ssl(ssl_cert)

    @staticmethod
    def _verify_ssl(ssl_cert):
        """check for network SSL issues"""
        try:
            logger.debug("Adding custom certs to Certifi store...")
            cafile = certifi.where()
            with open(ssl_cert, "rb") as infile:
                customca = infile.read()
            with open(cafile, "ab") as outfile:
                outfile.write(customca)
            logger.debug("SSL certificate added to store.")
        except SSLAdditionException:
            logger.debug("SSL custom certificates failed.")

?

stijnvanhoey commented 4 years ago

Or, we could maybe get it out of the class, make this available in a utility file and document it as such?

def verify_ssl(ssl_cert):
    """check for network SSL issues"""
    try:
        logger.debug("Adding custom certs to Certifi store...")
        cafile = certifi.where()
        with open(ssl_cert, "rb") as infile:
            customca = infile.read()
        with open(cafile, "ab") as outfile:
            outfile.write(customca)
        logger.debug("SSL certificate added to store.")
    except SSLAdditionException:
        logger.debug("SSL custom certificates failed.")

For the user, this requires an additional step on import and function run:

from pywaterinfo.utils import verify_ssl
fromp pywaterinfo import Waterinfo

verify_ssl("path_to_my_ca_file")
vmm = Waterinfo("vmm")

The Waterinfo code should just work and we do not need the additional input parameter of the class that is useless to most users...

binomaiheu commented 4 years ago

created pull request for implementation of last suggestion #10

stijnvanhoey commented 4 years ago

Thanks! all fine.

I used the moment to tryout the github action when creating a new release, see https://github.com/fluves/pywaterinfo/runs/611912777?check_suite_focus=true

When release is created, the CI (github actions):