Feature/vault support #991
Conversation
|
This pull request introduces 1 alert when merging e505d0f into aeef022 - view on LGTM.com new alerts:
|
|
I am really curious what you are trying to achieve with this change considering that we already have a vault testing file at https://github.com/ansible/ansible-lint/blob/master/test/contains_secrets.yml -- which is used by our testing. There is no need to add any explicit vaulting options to the linter itself because the vault loading is supposed to be done by ansible itself. |
I was creating a PR in response to this issue that was logged: |
|
This pull request introduces 1 alert when merging fcc1bb2 into 118772b - view on LGTM.com new alerts:
|
| # TODO: Support loading vaulted file and returning text | ||
| # It appears ansible DataLoader only returns dataset of elements | ||
| with open(playbookfile['path'], mode='r', encoding='utf-8') as f: | ||
| text = f.read() |
There was a problem hiding this comment.
ansible-lint attempts to load the yaml file at this point and only grab the text in order to call the following matchlines() and matchtasks() and matchyaml() functions that basically call the rules so they can parse/check the text and reference specific lines in the original file text. However, to support a vaulted file (not encrypt_strings in a file!) I was trying to use the ansible.parsing.dataloader to get the decrypted text but the load_from_file function only returns the parsed_data which is a dataset of elements. The function does initially get the text and stores this in file_data variable before getting the parsed data but not sure on best practice here. Looking for some ideas since the load_from_file function uses some private functions to get the file_data (text). I suppose we can pull that code and write our own to do the same thing, but wanted to get feedback before doing that.
The code at the moment fails here since of course it returns text that cannot be parsed since its simply the ansible vault string that is generated by ansible-vault.
There was a problem hiding this comment.
What counts as a vault? How do you detect that a file is a vault?
In general we should avoid using any private ansible APIs, this only got us into problems. Due to this is better to reimplement some of the functionality in order to avoid extra dependency on ansible.
From another point of view, I would even skip looking inside the vaults. I do not see vaults them important enough to be lint their content. The real issue is that in order to lint other files we need to allow ansible to load them.
There was a problem hiding this comment.
So the Ansible code appears to first load the file as binary, then it checks if it's a vaulted file by looking at the header or first line of the binary file to see if it matches the expected binary string b_HEADER = b'$ANSIBLE_VAULT'. If it does, then it attempts to decrypt the file and returns the binary contents of the file (b_data). So if we wanted to support linting vaulted files then we would need to implement some of this logic in ansiblelint and lastly get the binary data converted to text so we can reference line numbers, etc.
I agree, definitely no intentions of referencing or using private ansible APIs.
As to your question on whether or not to even support vaulted files and the argument around whether they are important enough to be linted. Well, I suppose that can be argued by different users in different ways. For me I generally recommend using encrypt_string instead of vaulting an entire file - it seems best practice to me, since the vaulted file hides everything even the variable name itself which has no point. So if ansible best practice agrees with that, then we "could" go the direction of not supporting vaulted files and simply add a lint rule that detects it, does not decrypt it, but simply warns or raises an error that the file is vaulted and developer should instead use encrypted strings. I'm cool with that to be honest, but I think this is a good question around how we want to approach this topic with respect to best practice. After all, that's what the lint tool is doing. It's advising on best practice.
If we do choose to decrypt and lint the vaulted files, it first of all shows support for vaulted files which is something supported by ansible... but second, we typically are going to lint variable files and these "should" sit inside the inventory hierarchy. I consider it bad practice to have vaulted file (let alone vaulted encrypt strings) inside an Ansible Role (defaults/vars) since the point of Roles is to be flexible and environment-independent. Again, this can be yet another lint rule if we choose to push best practice versus push the ability to lint every possible way of doing things with ansible.
|
I am going to close this due to lack of updates and because the codebase was heavily modified since. I do think that once https://github.com/ansible-community/ansible-lint/pull/950/files is merged, we would get vault support for free, as a side effect. I would make sense to investigate. |
In response to the following logged issue: #115
tests/vaulted.ymlas a vaulted yaml file for testing (not an encrypt_string!)optionson various functions to get access to the arguments for vaultingTODO: Need to cleanup various functions that callout
options.somethingand instead just pushoptionsthroughISSUE: Runner() is not behaving correctly yet...