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

uri module set string with masked content into content and json output #68400

Closed
hungluong5791 opened this issue Mar 23, 2020 · 12 comments · Fixed by #69653
Closed

uri module set string with masked content into content and json output #68400

hungluong5791 opened this issue Mar 23, 2020 · 12 comments · Fixed by #69653
Labels
affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. easyfix This issue is considered easy to fix by aspiring contributors. module This issue/PR relates to a module. net_tools Net-tools category P3 Priority 3 - Approved, No Time Limitation support:core This issue/PR relates to code supported by the Ansible Engineering Team.

Comments

@hungluong5791
Copy link

SUMMARY

uri module set string with masked content into content and json output

ISSUE TYPE
  • Bug Report
COMPONENT NAME

uri

ANSIBLE VERSION
ansible 2.9.0
  config file = None
  configured module search path = ['/Users/hungluong/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/hungluong/Library/Python/3.7/lib/python/site-packages/ansible
  executable location = /Users/hungluong/Library/Python/3.7/bin/ansible
  python version = 3.7.6 (default, Dec 30 2019, 19:38:26) [Clang 11.0.0 (clang-1100.0.33.16)]
CONFIGURATION
N/A
OS / ENVIRONMENT
STEPS TO REPRODUCE
- hosts: localhost
  connection: local
  tasks:
    - name: send request
      uri:
        url: "https://postman-echo.com/get?name=something-with-admin"
        user: admin
        password: admin
        method: GET
        force_basic_auth: yes
        return_content: yes
        status_code: 200
      register: response
    
    - name: extract value
      vars:
        query: args.name
      set_fact:
        value_content: "{{ response.content }}"
        value_content_parsed: "{{ response.content | from_json | json_query(query) }}"
        value_json: "{{ response.json.args.name }}"

    - name: debug
      debug:
        msg:
          - "{{ 'something-with-admin' in value_json }}"
          - "{{ 'something-with-admin' in value_content }}"
          - "{{ 'something-with-admin' in value_content_parsed }}"
          - "{{ 'something-with-********' in value_json }}"
          - "{{ 'something-with-********' in value_content }}"
          - "{{ 'something-with-********' in value_content_parsed }}"
EXPECTED RESULTS

The module should return the json/content value with the correct values

ACTUAL RESULTS

The module seems to apply sensitive info masking ('********') to value matching username/password in its output

"msg": [
        false,
        false,
        false,
        true,
        false,
        true
    ]
@ansibot
Copy link
Contributor

ansibot commented Mar 23, 2020

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.

click here for bot help

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. net_tools Net-tools category support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Mar 23, 2020
@bcoca bcoca added P3 Priority 3 - Approved, No Time Limitation and removed needs_triage Needs a first human triage before being processed. labels Apr 14, 2020
@bcoca
Copy link
Member

bcoca commented Apr 14, 2020

Actually we should be masking the non json keys also.

@bcoca bcoca added the easyfix This issue is considered easy to fix by aspiring contributors. label Apr 14, 2020
@ansibot
Copy link
Contributor

ansibot commented May 15, 2020

Files identified in the description:

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

@rubber-side-down
Copy link

Actually we should be masking the non json keys also.

It's masking the non-json (response.content) value as well. Adding the facts to the debug msg confirms this:

- hosts: localhost
  connection: local
  tasks:
    - name: send request
      uri:
        url: "https://postman-echo.com/get?name=something-with-admin"
        user: admin
        password: admin
        method: GET
        force_basic_auth: yes
        return_content: yes
        status_code: 200
      register: response

    - name: extract value
      vars:
        query: args.name
      set_fact:
        value_json: "{{ response.json.args.name }}"
        value_content: "{{ response.content }}"
        value_content_parsed: "{{ response.content | from_json | json_query (query) }}"

    - name: debug
      debug:
        msg:
          - "{{ value_json }}"
          - "{{ value_content }}"
          - "{{ value_content_parsed }}"
          - "{{ 'something-with-admin' in value_json }}"
          - "{{ 'something-with-admin' in value_content }}"
          - "{{ 'something-with-admin' in value_content_parsed }}"
          - "{{ 'something-with-********' in value_json }}"
          - "{{ 'something-with-********' in value_content }}"
          - "{{ 'something-with-********' in value_content_parsed }}"
ok: [localhost] => {
    "msg": [
        "something-with-********",
        {
            "args": {
                "name": "something-with-********"
            },
            "headers": {
                "accept-encoding": "identity",
                "authorization": "Basic YWRtaW46YWRtaW4=",
                "host": "postman-echo.com",
                "user-agent": "ansible-httpget",
                "x-amzn-trace-id": "Root=1-5ec4de30-6007601b0412140ca002d5eb",
                "x-forwarded-port": "443",
                "x-forwarded-proto": "https"
            },
            "url": "https://postman-echo.com/get?name=something-with-********"
        },
        "something-with-********",
        false,
        false,
        false,
        true,
        false,
        true
    ]
}

'something-with-********' in value_content returns False because the test is incorrect. It tests whether something-with-******** is a key in value_content rather than whether the string exists in the content. Changing the test to 'something-with-********' in value_content.__str__() returns True.

@bcoca, is there work to do here? It sounds like the request is to stop masking the values, but your comment indicates that these values should be masked.

@hungluong5791
Copy link
Author

It tests whether something-with-******** is a key in value_content rather than whether the string exists in the content

@rubber-side-down Why is that the case? The module example seems to indicate that it is just a string:

- name: Check that a page returns a status 200 and fail if the word AWESOME is not in the page contents
  uri:
    url: http://www.example.com
    return_content: yes
  register: this
  failed_when: "'AWESOME' not in this.content"

@rubber-side-down
Copy link

@hungluong5791 I'm uncertain why this is the case, but it's pretty clear from modifying the debug msg spec that this is the case.

My best guess is that the response.content is being decoded automatically based on the content-type header. The content-type header is "text/html; charset=UTF-8" for http://www.example.com, but it is "application/json; charset=utf-8" for https://postman-echo.com/get?name=something-with-admin.

@sivel
Copy link
Member

sivel commented May 21, 2020

@bcoca, is there work to do here? It sounds like the request is to stop masking the values, but your comment indicates that these values should be masked.

Yes, the request to not mask the values is invalid. In fact this issues identified that we need to mask more data.

@hungluong5791
Copy link
Author

hungluong5791 commented May 22, 2020

@samdoran @sivel Hi, I'm a bit confused. Why is the data need to be masked? They are logically irrelevant from each other, and this actually blocks our API integration workflow.

@hungluong5791
Copy link
Author

@rubber-side-down I see, that might be it. But do you think that behaviour is inherently confusing and unnecessary? That's the entire purpose of the response's json key, no?

@rubber-side-down
Copy link

@hungluong5791, I could go either way on the behavior of request content marshaling/deserialization, but it's beyond the scope of this issue.

@sivel
Copy link
Member

sivel commented Jun 1, 2020

@hungluong5791 We purposefully mask return values to hide no log values. Because you have chosen a password that also appears in the output, the values are masked. You pointed out a security flaw in our code, and as such, we addressed it.

Consider changing your password to some value that is not included in the return values from the API.

@hungluong5791
Copy link
Author

@sivel Thank you for getting back. Yes I understand the behaviour and I think I understand where it is coming from. But please consider the following points:

  • First, my specific use case:

    • we are provisioning an off-the-shelf system and having to create users & groups in that system through the API.
    • The default credentials is admin/admin (which cannot be changed, only disabled)
    • We have to create admin groups belonging to departments in the form of group1-admin, group2-admin etc., checking first to see if they exists for idempotency
    • The API does not support directly querying if a group exists, instead only provides a fuzzy search API - searching admin yields admin1, admin2 and such.
      --> thus the need of looking for values containing the credentials (admin) in the response, which currently blocked.
  • Secondly, uri is a generic API-calling module - as such it should not "police" external API behaviours. If an API decide to return the credentials in its response (like say httpbin), it is up to the user to decide if they should use that API, not the module. Not logging the value to output is perfectly sensible, but the user should still be able to use the data. Aggressively managing and requiring users to change their data just to use the module is backward, imo.

  • Thirdly, while I haven't tried it, I'd presume I could achieve what I want by using shell and curl (or just curl in a shell script) - which is neither portable, convenient nor any more secure. Pardon if I am ignorant about all the security implications from this, but if I could already do it, shouldn't Ansible make it easier, not blocking it?

Thanks

@ansible ansible locked and limited conversation to collaborators Jun 18, 2020
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. easyfix This issue is considered easy to fix by aspiring contributors. module This issue/PR relates to a module. net_tools Net-tools category P3 Priority 3 - Approved, No Time Limitation support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants