CA_BUNDLE and CML_VERIFY_CERT should be converted to bool

[sgherdao]

There seems to be a bug with the way CA_BUNDLE and CML_VERIFY_CERT are handled, the docstring says:

https://github.com/CiscoDevNet/virl2-client/blob/5bf320f62214c39a5615d19fda9a932f67d332ce/virl2_client/virl2_client.py#L218-L220

For example, with the following .virlrc:

VIRL_HOST=cml-04
VIRL_USERNAME=user
VIRL_PASSWORD=password
CML_VERIFY_CERT=True
CA_BUNDLE=False   

I get the following exception:

from virl2_client import ClientLibrary
client = ClientLibrary()
<snip>
OSError: Could not find a suitable TLS CA certificate bundle, invalid path: False

It appears that we get the strings "False" or "True" and they should be converted to a bool, ideally case insensitive.

    138     if ssl_verify is True:
    139         breakpoint()
--> 140         ssl_verify = _get_prop("CA_BUNDLE") or _get_prop("CML_VERIFY_CERT")
    141
    142     return host, username, password, ssl_verify

ipdb> interact
*interactive*
In [1]: _get_prop("CA_BUNDLE") or _get_prop("CML_VERIFY_CERT")
Out[1]: 'False'

In [2]: _get_prop("CA_BUNDLE")
Out[2]: 'False'

In [3]: _get_prop("CML_VERIFY_CERT")
Out[3]: 'True'

In [4]: _get_prop("CA_BUNDLE") or _get_prop("CML_VERIFY_CERT")
Out[4]: 'False'

Do we need two options for setting ssl_verify? I initially thought it was for backward compatibility, but it seems this is a relatively new addition (#40).

My concern is that this could lead to unexpected behavior in certain cases. For example, if a path is set with CA_BUNDLE and CML_VERIFY_CERT is set to False, one might expect ssl_verify will be set to False but CA_BUNDLE takes precedence.

In [6]: _get_prop("CA_BUNDLE") or False
Out[6]: '/tmp/blah'

Note the use of a bool here and not a str

CA_BUNDLE set to True and CML_VERIFY_CERT to a path could also lead to unexpected behavior:

In [7]: True or "/tmp/path"
Out[7]: True

Could we reconsider the need for both options to avoid potential confusion and ensure consistent/simpler behavior? I understand it might be too late for this, in this case we should probably clarify in the doc.

Maybe not too late for #104 ?

[tmikuska]

The client library has so many options for compatibility and historical reasons. CA_BUNDLE has always been an environment variable supported by the client library, and CML_VERIFY_CERT has been included for compatibility with virlutils. The client library also supports multiple environment variables for username and password for the same reasons.

Neither CA_BUNDLE nor CML_VERIFY_CERT should be set to anything else than a valid path to a cert. Though, we could do some validation around environment variables.

:param ssl_verify: Path of the SSL controller certificate, or True to load from CA_BUNDLE or CML_VERIFY_CERT environment variable, or False to disable.

As the docstrings say, ssl_verify should be set to:

• True (default) to read a cert from a path provided by either CA_BUNDLE or CML_VERIFY_CERT
• False to skip SSL validation
• a path to read a cert from.

I think that this is mostly about clarification, so users don't have to check virl2_client/virl2_client/models/configuration.py. Anyway, I'm open to other opinions.

[sgherdao]

Thanks for the clarifications.

:param ssl_verify: Path of the SSL controller certificate, or True to load from CA_BUNDLE or CML_VERIFY_CERT environment variable, or False to disable.

or True to load from ... this wasn't clear for me whether we load a path or "False"

What led to the confusion, is that virlutils allows CML_VERIFY_CERT to be set to "False" documented here:

CML_VERIFY_CERT - The path to a PEM-encoded certificate file to use to verify the CML controller VM's SSL certificate. >If you do not wish to verify the certificate, set this to "False",

When set to "False" via virlrc or env var, it "converts" it a bool before passing it to ClientLibrary.

https://github.com/CiscoDevNet/virlutils/blob/b3bc0b882dcd1074c0d65babde441aebae9ae212/virl/helpers.py#L271

I was expecting the same behavior from virl2_client.

[sgherdao]

@tmikuska please feel free to close the issue unless you are planning to modify the behavior (i.e. converting "False" to False), thanks again for all the explanations!

[tmikuska]

@sgherdao we are going to check if we could modify the behavior to match virlutils behavior.

[tmikuska]

Resolved in https://github.com/CiscoDevNet/virl2-client/pull/111