community.general
community.general copied to clipboard
ansible-collections
Docs for validating certificates in consul_kv_lookup are inaccurate
Summary
The documentation for the consul_kv_lookup plugin is not correct. Per the docs as well as the code the validate_certs parameter should be a boolean value which dictates if certs should be verified or not.
While setting this value to False does bypass validation, it's not clear in the documentation that in order to validate a self-signed certificate the value should be set to the path to the CA certificate. The name and/or the documentation should be updated to reflect that this value is not just a boolean.
Issue Type
Bug Report
Component Name
consul_kv_lookup
Ansible Version
$ ansible --version
ansible [core 2.11.2]
config file = None
configured module search path = ['/root/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
ansible python module location = /usr/local/lib/python3.8/dist-packages/ansible
ansible collection location = /root/.ansible/collections:/usr/share/ansible/collections
executable location = /usr/local/bin/ansible
python version = 3.8.5 (default, May 27 2021, 13:30:53) [GCC 9.3.0]
jinja version = 3.0.1
libyaml = True```
### Configuration
```console (paste below)
$ ansible-config dump --only-changed
(blank)
OS / Environment
Ubuntu 20.04 (Docker)
Steps to Reproduce
---
- hosts: localhost
tasks:
- debug:
msg: "{{ lookup('community.general.consul_kv', 'my/key', validate_certs='/certs/ca.crt') }}"
Expected Results
The above playbook seems like it should fail since the validate_certs parameter defaults to a Boolean value.
Actual Results
The plugin executes and successfully validates. Note that providing True for the parameter will result in the module failing with a self-signed certificate error. This is what originally confused me because it wasn't apparent how we were intended to pass the actual CA certificate to the module. It seems to me there should be two separate parameters: one for performing the validation and one for passing the CA certificate. However, after digging through the underlying Python module being used (python_consul), it appears everything is combined into a single parameter as well.
With that being said, I would recommend at least updating the documentation to reflect this confusing behavior.
Code of Conduct
- [X] I agree to follow the Ansible Code of Conduct
Files identified in the description: None
If these files are inaccurate, please update the component name section of the description or use the !component bot command.
@jmgilman Thanks for reporting this, would you like to open a PR adding this to documentation?
Files identified in the description:
If these files are inaccurate, please update the component name section of the description or use the !component bot command.
I don't think this should be documented, but prevented. validte_certs should be a boolean parameter, and not a "different behavior on different parameter types" parameter. If it should be possible to specify a path to the CA cert(s), this should be a separate option.
It's not really a breaking change, since the documented behavior does not change. validate_certs should always be a boolean parameter.
Users may expect and use this behaviour (even not documented), changing this will break their playbooks.
Users can also rely on a bug, and their playbooks can break when the bug is fixed. Not having type: bool here is IMO a bug.
The correct way to fix this is probably add a new option ca_path (of type string), deprecate non-boolean values for validate_certs, and add type: bool once the deprecation expires.
Users can also rely on a bug, and their playbooks can break when the bug is fixed. Not having
type: boolhere is IMO a bug.
DO NOT BREAK THEIR WORKFLOW!
The problem with this is that this behavior of the consul lib is an undocumented implementation detail. The documentation for the verify parameter says verify is whether to verify the SSL certificate for HTTPS requests (https://python-consul.readthedocs.io/en/latest/#consul); that the verify parameter has this double meaning because it is eventually forwarded to requests's verify parameter (https://docs.python-requests.org/en/latest/api/#requests.request). This is mentioned in an issue in the consul client's repo (https://github.com/cablehead/python-consul/issues/245), but since there has never been a reaction by the maintainer(s), it is not clear to me whether they actually know about it and would pledge to keep supporting this behavior or not.
Also: basically the client's development stopped in August 2018. It looks like there's a fork that's somewhat active (https://github.com/poppyred/python-consul2/, see also https://github.com/cablehead/python-consul/issues/250) though.
That was the problem I was trying to bring light to towards the end of my submission: the underlying problem can be tracked all the way back to the python_consul dependency. It wasn't until I looked through the code and saw the eventual call to requests that I attempted to pass a path to the verify parameter and realized it worked. I found nothing in the documentation that suggested this was the expected behavior.
So, even if the desire is to break up this operation into two parameters, the underlying dependency doesn't support it (I originally thought the cert parameter was for passing the CA certificate but that is for client certificates only). That's why I suggested at least possibly documenting this behavior to ease the burden on people in the future - but perhaps updating the dependency to an active fork and implementing the correct solution would be better.