Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consul_session: ensure certificate is checked when HTTPS is used #58693

Merged
merged 2 commits into from
Jul 26, 2019

Conversation

pilou-
Copy link
Contributor

@pilou- pilou- commented Jul 4, 2019

SUMMARY

consul_session: ensure that:

  • cert is checked when HTTPS is used
  • cert isn't checked when validate_certs is disabled
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

consul_session

ADDITIONAL INFORMATION

Depends on #58692. Currently fails with:

Could not retrieve session info 400 Client sent an HTTP request to an HTTPS server

@ansibot
Copy link
Contributor

ansibot commented Jul 4, 2019

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Jul 4, 2019
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 12, 2019
@pilou- pilou- force-pushed the consul_session_validate_certs branch from 1e50f2f to 1d26884 Compare July 21, 2019 21:29
@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Jul 21, 2019
@pilou-
Copy link
Contributor Author

pilou- commented Jul 21, 2019

pull-request rebased since #58692 has been merged.

As expected, integration test failed:

TASK [consul : ensure SSL certificate isn't checked when validate_certs is disabled] ***
task path: /root/.ansible/test/tmp/consul-2os7by9u-ÅÑŚÌβŁÈ/test/integration/targets/consul/tasks/consul_session.yml:100
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<testhost> EXEC /bin/sh -c 'echo ~root && sleep 0'
<testhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /root/.ansible/tmp/ansible-tmp-1563744820.3596995-8858196606187 `" && echo ansible-tmp-1563744820.3596995-8858196606187="` echo /root/.ansible/tmp/ansible-tmp-1563744820.3596995-8858196606187 `" ) && sleep 0'
Using module file /root/ansible/lib/ansible/modules/clustering/consul_session.py
<testhost> PUT /root/.ansible/tmp/ansible-local-150861doyicl6/tmpv7wj6ink TO /root/.ansible/tmp/ansible-tmp-1563744820.3596995-8858196606187/AnsiballZ_consul_session.py
<testhost> EXEC /bin/sh -c 'chmod u+x /root/.ansible/tmp/ansible-tmp-1563744820.3596995-8858196606187/ /root/.ansible/tmp/ansible-tmp-1563744820.3596995-8858196606187/AnsiballZ_consul_session.py && sleep 0'
<testhost> EXEC /bin/sh -c '/usr/bin/python3.6 /root/.ansible/tmp/ansible-tmp-1563744820.3596995-8858196606187/AnsiballZ_consul_session.py && sleep 0'
<testhost> EXEC /bin/sh -c 'rm -f -r /root/.ansible/tmp/ansible-tmp-1563744820.3596995-8858196606187/ > /dev/null 2>&1 && sleep 0'
The full traceback is:
  File "/tmp/ansible_consul_session_payload_bsftroy_/__main__.py", line 178, in lookup_sessions
    session_by_id = consul_client.session.info(session_id, dc=datacenter)
  File "/usr/local/lib/python3.6/site-packages/consul/base.py", line 1894, in info
    params=params)
  File "/usr/local/lib/python3.6/site-packages/consul/std.py", line 22, in get
    self.session.get(uri, verify=self.verify, cert=self.cert)))
  File "/usr/lib/python3.6/site-packages/requests/sessions.py", line 537, in get
    return self.request('GET', url, **kwargs)
  File "/usr/lib/python3.6/site-packages/requests/sessions.py", line 524, in request
    resp = self.send(prep, **send_kwargs)
  File "/usr/lib/python3.6/site-packages/requests/sessions.py", line 637, in send
    r = adapter.send(request, **kwargs)
  File "/usr/lib/python3.6/site-packages/requests/adapters.py", line 514, in send
    raise SSLError(e, request=request)

fatal: [testhost]: FAILED! => {
    "changed": false,
    "invocation": {
        "module_args": {
            "behavior": "release",
            "checks": null,
            "datacenter": null,
            "delay": 15,
            "host": "localhost",
            "id": "71998cee-d817-0edd-da7a-4dc1b2fa1042",
            "name": null,
            "node": null,
            "port": 8501,
            "scheme": "https",
            "state": "info",
            "validate_certs": false
        }
    },
    "msg": "Could not retrieve session info HTTPSConnectionPool(host='localhost', port=8501): Max retries exceeded with url: /v1/session/info/71998cee-d817-0edd-da7a-4dc1b2fa1042 (Caused by SSLError(SSLError(1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:877)'),))

bugfix will follow.

@ansibot ansibot added clustering Clustering category module This issue/PR relates to a module. small_patch labels Jul 21, 2019
@pilou- pilou- changed the title [WIP] consul_session: ensure certificate is checked when HTTPS is used consul_session: ensure certificate is checked when HTTPS is used Jul 21, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Jul 21, 2019
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Added some comments about the tests.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jul 23, 2019
@pilou- pilou- force-pushed the consul_session_validate_certs branch from 7fa16e3 to d60a3d6 Compare July 23, 2019 14:58
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, one thing that definitely has to be added: a changelog fragment.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 23, 2019
@pilou- pilou- force-pushed the consul_session_validate_certs branch from d60a3d6 to 0cedc49 Compare July 24, 2019 00:21
@pilou-
Copy link
Contributor Author

pilou- commented Jul 24, 2019

Changelog fragment added

@pilou- pilou- force-pushed the consul_session_validate_certs branch from 0cedc49 to 6237624 Compare July 24, 2019 23:57
@felixfontein felixfontein merged commit 65013c4 into ansible:devel Jul 26, 2019
@felixfontein
Copy link
Contributor

@pilou- thanks for fixing this!

@ansible ansible locked and limited conversation to collaborators Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. clustering Clustering category module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants