Allow ansible-review to read yaml containing inline vault strings

[benagricola]

When passing var files to ansible-review which contain inlined vault-encrypted values, like the following:

password: !vault |
  $ANSIBLE_VAULT;1.1;AES256
  ...

The default standards will throw an error:

yaml.constructor.ConstructorError: could not determine a constructor for the tag '!vault'
  in "<unicode string>", line 29, column 24:
    password: !vault |

This is because yaml.safe_load is used to avoid remote code execution from running on untrusted yaml.

This patch adds a stub constructor to the yaml SafeLoader for !vault and !vault-encrypted items, and simply returns the scalar value of the node (i.e. the encrypted string).

It may or may not be necessary to return the (still-encrypted) value for the purposes of ansible-review but I've left it in for the moment.

[willthames]

Apologies for not having spotted this before (I haven't looked at ansible-review in an age)

This looks great - I'd really like it if there were a test case for this (I know the test suite is shockingly low in terms of coverage but I would prefer to add tests as they become apparent)

If you can't create a test, let me know and I'll sort it out

[willthames]

I've included this in the fix_ansible_inventory branch

This is now available for testing on 0.14.0rc1

I'd still like to see some test cases, mostly because I haven't hit this issue at all so would be good to see whether it's still needed with other changes, and just to prevent future regressions

[ansiblejunky]

I concur on this error. I have a basic role structure in a repo:

.
├── LICENSE
├── README.md
├── defaults
│   └── main.yml
├── files
├── handlers
│   └── main.yml
├── library
├── meta
│   └── main.yml
├── tasks
│   ├── debian.yml
│   ├── main.yml
│   ├── redhat.yml
│   └── windows.yml
├── templates
├── tests
│   ├── inventory
│   └── test.yml
└── vars
    ├── main.yml
    ├── redhat.yml
    └── windows.yml

I created a vaulted variable using:

ansible-vault encrypt_string 'password' --name=password

Which produced this output:

password: !vault |
          $ANSIBLE_VAULT;1.1;AES256
          62376538346165303539376161396331666234623531333462383133646335363766646561376336
          3030666362333863333466383462383633616361366661300a343738653466663236343532643536
          62613462326430396637363063373635636633306233633662336538353032323563303764326134
          3130346162333061350a626338666566323638373239363937306664313265383862633635633738
          3463

I added this to vars/main.yml and ran ansible-review vars/main.yml and I get a huge stack trace.

Traceback (most recent call last):
  File "/usr/local/bin/ansible-review", line 10, in <module>
    sys.exit(main())
  File "/usr/local/lib/python2.7/site-packages/ansiblereview/__main__.py", line 99, in main
    errors = errors + candidate.review(options, lines)
  File "/usr/local/lib/python2.7/site-packages/ansiblereview/__init__.py", line 76, in review
    return utils.review(self, settings, lines)
  File "/usr/local/lib/python2.7/site-packages/ansiblereview/utils/__init__.py", line 122, in review
    result = standard.check(candidate, settings)
  File "/usr/local/lib/python2.7/site-packages/ansiblereview/examples/standards.py", line 37, in files_should_have_actual_content
    content = yaml.safe_load(f.read())
  File "/Users/jwadleig/Library/Python/2.7/lib/python/site-packages/yaml/__init__.py", line 93, in safe_load
    return load(stream, SafeLoader)
  File "/Users/jwadleig/Library/Python/2.7/lib/python/site-packages/yaml/__init__.py", line 71, in load
    return loader.get_single_data()
  File "/Users/jwadleig/Library/Python/2.7/lib/python/site-packages/yaml/constructor.py", line 39, in get_single_data
    return self.construct_document(node)
  File "/Users/jwadleig/Library/Python/2.7/lib/python/site-packages/yaml/constructor.py", line 48, in construct_document
    for dummy in generator:
  File "/Users/jwadleig/Library/Python/2.7/lib/python/site-packages/yaml/constructor.py", line 398, in construct_yaml_map
    value = self.construct_mapping(node)
  File "/Users/jwadleig/Library/Python/2.7/lib/python/site-packages/yaml/constructor.py", line 208, in construct_mapping
    return BaseConstructor.construct_mapping(self, node, deep=deep)
  File "/Users/jwadleig/Library/Python/2.7/lib/python/site-packages/yaml/constructor.py", line 133, in construct_mapping
    value = self.construct_object(value_node, deep=deep)
  File "/Users/jwadleig/Library/Python/2.7/lib/python/site-packages/yaml/constructor.py", line 88, in construct_object
    data = constructor(self, node)
  File "/Users/jwadleig/Library/Python/2.7/lib/python/site-packages/yaml/constructor.py", line 414, in construct_undefined
    node.start_mark)
yaml.constructor.ConstructorError: could not determine a constructor for the tag '!vault'
  in "<unicode string>", line 3, column 11:
    password: !vault |
              ^

I am using version v0.13.9, which I also use in my pre-commit. This bug is a problem for my customer who uses these vaulted strings all the time. I didn't know about it until now. I hope we can get this PR approved and promoted to an official release that can also be used with pre-commit.

[benagricola]

Hi @willthames,

Apologies for the delay getting tests added - I missed this a bit 😃

I'm trying to work out the most appropriate place to put new tests for this, since in theory you could put vault encrypted vars pretty much anywhere in the ansible structure, and the changes are part of the example standards which don't appear to be directly tested.

There seems to be some inclusion of the standard tests in test/standards/standards.py, which I think is probably where it makes most sense to pull in the files_should_have_actual_content check which triggers this error, but AFAICT those test standards aren't actually used (unless I'm missing part of the tests where ansible-review is executed directly in the test suite pointing to that standards file).

If the changes in the PR weren't specific to the example standards then I'd probably just create an extra test case and yaml file in test/TestYamlReview.py but this seems like the wrong place given the nature of the change...

[willthames]

@benagricola that's a fair point, we don't actually have tests for including those standards.

I'll double check this works fine on my own repo at work and at some point release 0.14