Make TLS certificate verification configurable

[smarsching]

As dicussed in #37, the verification of remote TLS certificates should be configurable, being enabled by default. This PR makes the necessary changes.

[tynanford]

This took me a bit to figure out. Setting verify=false in ~/.channelfinderapi.conf didn't have the desired effect since verify defaults to True in the constructor for ChannelFinderClient and self.__getDefaultConfig will never check the configuration if the default value is not None.

So the user has to change all their ChannelFinderClient object calls to include the verify keyword. Personally I would rather just change the verify setting in one place and then it can be overridden in the calls to ChannelFinderClient if needed.

What about setting the verify default value to None and then check for None after calling self.__getDefaultConfig? Then if not present in the config or passed via an argument, default to True

[smarsching]

Yes, you are right, I overlooked this myself. The behavior should be:

• verify has a default value of None.
• If None, the value from the configuration is used.
• If no value is set in the configuration, True is used.

So, explicitly specifying verify=True or verify=False would override the value from the configuration, just like explicitly specifying BaseURL, username, or password does, but code that does not specify it explicitly would use the settings from the configuration, which will be the right thing in 99 % of cases.

I will amend this PR to implement this behavior correctly.

[smarsching]

After investigating this a bit closer, I think that a part of the problem is that the documentation of __getDefaultConfig is a bit misleading: From the documentation, I expected that ref is only used when key is not present in the configuration, but actually the key is only looked up if ref is None.

Maybe this could be clarified by rephrasing the :return: documentation in the following way:

:return: ``ref`` if not ``None``, otherwise the value associated with ``key`` in the configuration or ``None`` if ``key`` is not present.

Or it might make sense to change this method altogether, always trying to read the value from the configuration and using ref if it is not set there:

return basecfg["DEFAULT"].get(key, ref)

However, this would change the behavior, so that configuration values override the values passed by the calling code, which might not be desirable. Therefore, simply clarifying the documentation might be the better option.

Regarding the documentation, it might also be a good idea to add something like the following sentence to the documentation of ChannelFinderClient.__init__:

If any of the arguments is None, the corresponding value from the DEFAULT section of the configuration is used.

[tynanford]

I was also surprised at how __getDefaultConfig works while trying to figure out what was going on. Updating both documentation pieces would be great to clarify.

However, this would change the behavior, so that configuration values override the values passed by the calling code, which might not be desirable. Therefore, simply clarifying the documentation might be the better option.

I think the behavior would be the same since BaseURL, username, and password were always set to None by default in the argument list? And those are the only 3 calls to __getDefaultConfig in ChannelFinderClient.py

[anderslindho]

I think fixing this through documentation feels like a workaround. The real issue is that verify: bool | None = None makes the API unclear - None isn’t an actual value the caller should care about, it’s just being overloaded to mean “use config”.

A cleaner approach would IMO be to use a private sentinel, so the public API stays bool and we can still detect “not provided”:

_UNSET = object()

def cls_or_fn(obj, verify: bool: _UNSET, config=None):
  if verify is _UNSET:
    if config and config.verify:
      obj.verify = config.verify
    else:
      obj.verify = True
  else:
    obj.verify = verify

Then we can have the presedence be:

1. explicit method arg
2. explicit ctor arg
3. config setting
4. default

[tynanford]

I like this idea and think it is fine to default to verify SSL certs. Maybe we should create a release before merging this since it could be a breaking change for some users?

Also it looks like the pre-commit needs to be run to make ruff-format happy

[jacomago]

I like this idea and think it is fine to default to verify SSL certs. Maybe we should create a release before merging this since it could be a breaking change for some users?

Also it looks like the pre-commit needs to be run to make ruff-format happy

Yeah, looks fine to me. Just needs to run pre-commit.

I made a new release to support.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[smarsching]

I just force-pushed an amended commit. The pre-commit issues seem to be resolved now, and I changed the behavior so that verify=None is the default now, resulting in the value from the configuration file being used. I also improved the documentation for both __init__ and __getDefaultConfig, so that the behavior in regards to explicitly specifying configuration values should be clearer now.

[anderslindho]

I maintain my stance as written in a comment reply. I think we can make the API more clear than using None to signal "read from config or apply default".

[tynanford]

This looks good to me, we should just make it clear in the next release note that there is this change.

IMO refactoring the __getDefaultConfig more can be a separate PR

[jacomago]

You can simplify a lot of this using cfg.getboolean() see https://github.com/ChannelFinder/pyCFClient/tree/38-sky

How getboolean works: https://docs.python.org/3/library/configparser.html#configparser.ConfigParser.getboolean

[jacomago]

@anderslindho I think this satisfies some of your annoyances?

[anderslindho]

Parts thereof, yes, but not my main concern. This works functionally, but it still leaves us with an ambiguous public API. Using a private sentinel would give us clear semantics and avoids optional booleans entirely.

But I can concede that further changes are out of scope, so if both you and @tynanford are fine with this, feel free to proceed. There is (unfortunately...) already an established pattern of using None this way in this code base.