ansible-review
ansible-review copied to clipboard
Allow ansible-review to read yaml containing inline vault strings
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.
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
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
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.
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...
@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