Closed sgherdao closed 3 weeks ago
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
orCML_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 validationI 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.
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
.
I was expecting the same behavior from virl2_client.
@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!
@sgherdao we are going to check if we could modify the behavior to match virlutils behavior.
There seems to be a bug with the way
CA_BUNDLE
andCML_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
:I get the following exception:
It appears that we get the strings
"False"
or"True"
and they should be converted to abool
, ideally case insensitive.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
andCML_VERIFY_CERT
is set toFalse
, one might expectssl_verify
will be set toFalse
butCA_BUNDLE
takes precedence.Note the use of a
bool
here and not astr
CA_BUNDLE
set toTrue
andCML_VERIFY_CERT
to a path could also lead to unexpected behavior: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 ?