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

Do not send "Connection: close" when requesting a tunnel #46070

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
4 participants
@sivel
Member

sivel commented Sep 24, 2018

SUMMARY

When validating SSL certs using an a non-SSL proxy, do not send "Connection: close" when requesting a tunnel. This prevents some proxy servers from dropping the connection

RFC2817 section 5.2 (or any other section) does not specify use of sending Connection: close and specifically squid seems to have issues with sending this.

Fixes #32750

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lib/ansible/module_utils/urls.py

ANSIBLE VERSION
2.5
2.6
2.7
2.8
ADDITIONAL INFORMATION

@ansibot

This comment has been minimized.

Contributor

ansibot commented Sep 24, 2018

The test ansible-test sanity --test changelog [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/changelog.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "packaging/release/changelogs/changelog.py", line 814, in <module>
    main()
  File "packaging/release/changelogs/changelog.py", line 98, in main
    args.func(args)
  File "packaging/release/changelogs/changelog.py", line 109, in command_lint
    lint_fragments(fragments, exceptions)
  File "packaging/release/changelogs/changelog.py", line 227, in lint_fragments
    errors += linter.lint(fragment)
  File "packaging/release/changelogs/changelog.py", line 307, in lint
    errors += [(fragment.path, 0, 0, result[1]) for result in results]
  File "packaging/release/changelogs/changelog.py", line 307, in <listcomp>
    errors += [(fragment.path, 0, 0, result[1]) for result in results]
  File "/usr/local/lib/python3.6/dist-packages/rstcheck.py", line 169, in check
    find_ignored_languages(source)
  File "/usr/local/lib/python3.6/dist-packages/rstcheck.py", line 235, in find_ignored_languages
    for (index, line) in enumerate(source.splitlines()):
AttributeError: 'dict' object has no attribute 'splitlines'
Traceback (most recent call last):
  File "test/sanity/code-smell/changelog.py", line 14, in <module>
    main()
  File "test/sanity/code-smell/changelog.py", line 10, in main
    subprocess.check_call(cmd)
  File "/usr/lib/python3.6/subprocess.py", line 291, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['packaging/release/changelogs/changelog.py', 'lint', 'changelogs/fragments/2.8-core-deprecations.yaml', 'changelogs/fragments/2.8-removed-modules.yaml', 'changelogs/fragments/44278-pamd_valid_simple_controls.yaml', 'changelogs/fragments/45628-fetch_url-error-headers.yaml', 'changelogs/fragments/add-elapsed-return-value-to-select-modules.yaml', 'changelogs/fragments/agnostic-become-prompt.yaml', 'changelogs/fragments/async-dir.yaml', 'changelogs/fragments/azure_rm_deployment_fix_45941.yaml', 'changelogs/fragments/copy-diff-text.yaml', 'changelogs/fragments/dd-put-empty-files.yaml', 'changelogs/fragments/dnf-group-removal.yaml', 'changelogs/fragments/docker_container-idempotency.yaml', 'changelogs/fragments/ec2_group_fix_target_containing_list_within_list.yaml', 'changelogs/fragments/elb_target_group_fix_KeyError.yaml', 'changelogs/fragments/fix_ec2_group_target_vpc_precedence.yaml', 'changelogs/fragments/get-url-fix-idempotency.yaml', 'changelogs/fragments/get_url.yaml', 'changelogs/fragments/loop_undefined_delegate_to.yaml', 'changelogs/fragments/mysql-migrate_to_pymysql.yaml', 'changelogs/fragments/no_empty_groups.yml', 'changelogs/fragments/openstack_inventory_fix.yml', 'changelogs/fragments/piped-transfer-empty-files.yaml', 'changelogs/fragments/run-command-expand-shell.yaml', 'changelogs/fragments/script-module-no-file-path.yaml', 'changelogs/fragments/tower_credential_ssh_key_data.yaml', 'changelogs/fragments/urls-proxy-validate.yaml', 'changelogs/fragments/v2.8.0-initial-commit.yaml', 'changelogs/fragments/win_copy-dest-quote.yaml', 'changelogs/fragments/win_group_membership-com-marshal.yaml', 'changelogs/fragments/win_keep_remote_file_python26.yaml', 'changelogs/fragments/win_say-fix.yaml', 'changelogs/fragments/win_scheduled_task-repetition.yaml', 'changelogs/fragments/win_script-become.yaml', 'changelogs/fragments/windows-deprecated-functionality.yaml']' returned non-zero exit status 1.

click here for bot help

@sivel sivel force-pushed the sivel:issue/32750 branch from 21f8a95 to 483033c Sep 24, 2018

@ansibot ansibot removed the needs_revision label Sep 24, 2018

@rseuchter

This comment has been minimized.

rseuchter commented on 483033c Sep 25, 2018

Thanks for picking this up!

I'll be more than happy to test this fix in the environment where I can reproduce this.

@webknjaz webknjaz requested review from mattclay and samdoran Sep 25, 2018

@webknjaz webknjaz removed the needs_triage label Sep 25, 2018

@webknjaz

This comment has been minimized.

Member

webknjaz commented Sep 25, 2018

@rseuchter do you know of any way we could integrate such test in CI to prevent regressions?

@samdoran samdoran requested a review from jimi-c Sep 25, 2018

@samdoran samdoran changed the title from Do not send "Connection: close" when requesting a tunnel. Fixes #32750 to Do not send "Connection: close" when requesting a tunnel Sep 25, 2018

@mattclay mattclay removed their request for review Sep 25, 2018

@rseuchter

This comment has been minimized.

rseuchter commented Sep 26, 2018

Today, I was able to verify the fix. 👍

I used this playbook:

---
- name: bug 32750
  hosts: all
  gather_facts: no
  tasks:
  - name: download nodejs script to add repository
    get_url:
      url: https://deb.nodesource.com/setup_7.x
      dest: /tmp/nodejs_setup_7.x
      mode: 0755

The fixed version worked as expected:

root@8bd8badb23b2:/workspace# ansible --version
ansible 2.8.0.dev0 (issue/32750 483033c0c3) last updated 2018/09/26 08:56:26 (GMT +000)
  config file = None
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /root/ansible/lib/ansible
  executable location = /root/ansible/bin/ansible
  python version = 2.7.15 (default, Sep  5 2018, 04:46:44) [GCC 6.3.0 20170516]
root@8bd8badb23b2:/workspace# ansible-playbook -c local -i hosts.ini 32750.yml

PLAY [bug 32750] *****************************************************************************************************

TASK [download nodejs script to add repository] **********************************************************************
changed: [127.0.0.1]

PLAY RECAP ***********************************************************************************************************
127.0.0.1                  : ok=1    changed=1    unreachable=0    failed=0    skipped=0

Ansible 2.6.4 failed (as was expected):

root@fe7d683c7570:/workspace# ansible --version
ansible 2.6.4
  config file = None
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python2.7/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.15 (default, Sep  5 2018, 04:46:44) [GCC 6.3.0 20170516]
root@fe7d683c7570:/workspace# ansible-playbook -c local -i hosts.ini 32750.yml

PLAY [bug 32750] *****************************************************************************************************

TASK [download nodejs script to add repository] **********************************************************************
fatal: [127.0.0.1]: FAILED! => {"changed": false, "msg": "Connection to proxy failed"}
        to retry, use: --limit @/workspace/32750.retry

PLAY RECAP ***********************************************************************************************************
127.0.0.1                  : ok=0    changed=0    unreachable=0    failed=1

@webknjaz I don't have a way to easily reproduce this end-to-end, see #32750 (comment). So my best guess to avoid regressions would be to write a test with a (pseudo-)proxy that captures the headers and then simply check if the header's absent. That's not very elegant but it's the best I can think of when we cannot reproduce this in a lab environment.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Sep 27, 2018

@sivel is it worth adding some docker container with a proxy to tests?

@sivel

This comment has been minimized.

Member

sivel commented Sep 27, 2018

@sivel is it worth adding some docker container with a proxy to tests?

I'm sure it is, although I have not reproduced the issue. I can spend a little time trying to reproduce with a container.

I however don't think we should hold this up to add the tests, since the linked issue represents a fair number of people this seems to affect.

@rseuchter

This comment has been minimized.

rseuchter commented Sep 28, 2018

I tried reproduce it with containers a few weeks back but didn't succeed. The challenge is to get the proxy configuration right. If anybody managed to reproduce this I'd be interested to know how.

@webknjaz

This comment has been minimized.

Member

webknjaz commented Sep 28, 2018

@sivel agreed

@rseuchter I think I saw something similar in requests repo and the config there was squid+ntlm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment