New secrets loading module: Empty `files` key causes surprising error

[mhjacks]

New secrets loading module: Empty `files` key causes surprising error

[day0hero]

here is the complete errror:

fatal: [localhost]: FAILED! => {"changed": false, "module_stderr": "Traceback (most recent call last):\n  File \"/home/jrickard/.ansible/tmp/ansible-tmp-1664994985.2909513-413311-242601022481252/AnsiballZ_vault_load_secrets.py\", line 107, in <module>\n    _ansiballz_main()\n  File \"/home/jrickard/.ansible/tmp/ansible-tmp-1664994985.2909513-413311-242601022481252/AnsiballZ_vault_load_secrets.py\", line 99, in _ansiballz_main\n    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)\n  File \"/home/jrickard/.ansible/tmp/ansible-tmp-1664994985.2909513-413311-242601022481252/AnsiballZ_vault_load_secrets.py\", line 47, in invoke_module\n    runpy.run_module(mod_name='ansible.modules.vault_load_secrets', init_globals=dict(_module_fqn='ansible.modules.vault_load_secrets', _modlib_path=modlib_path),\n  File \"/usr/lib64/python3.10/runpy.py\", line 224, in run_module\n    return _run_module_code(code, init_globals, run_name, mod_spec)\n  File \"/usr/lib64/python3.10/runpy.py\", line 96, in _run_module_code\n    _run_code(code, mod_globals, init_globals,\n  File \"/usr/lib64/python3.10/runpy.py\", line 86, in _run_code\n    exec(code, run_globals)\n  File \"/tmp/ansible_vault_load_secrets_payload_7srbr3th/ansible_vault_load_secrets_payload.zip/ansible/modules/vault_load_secrets.py\", line 371, in <module>\n  File \"/tmp/ansible_vault_load_secrets_payload_7srbr3th/ansible_vault_load_secrets_payload.zip/ansible/modules/vault_load_secrets.py\", line 367, in main\n  File \"/tmp/ansible_vault_load_secrets_payload_7srbr3th/ansible_vault_load_secrets_payload.zip/ansible/modules/vault_load_secrets.py\", line 353, in run\n  File \"/tmp/ansible_vault_load_secrets_payload_7srbr3th/ansible_vault_load_secrets_payload.zip/ansible/modules/vault_load_secrets.py\", line 206, in sanitize_values\nTypeError: 'NoneType' object is not iterable\n", "module_stdout": "", "msg": "MODULE FAILURE\nSee stdout/stderr for the exact error", "rc": 1}

values-secret.yaml used:

---
secrets:
  # NEVER COMMIT THESE VALUES TO GIT
  config-demo:
    # Secret used for demonstrating vault storage, external secrets, and ACM distribution
    secret: VALUSE

  # Required for automated spoke deployment
  aws:
      access_key_id: VALUE
      secret_access_key: VALUE

# Required for automated spoke deployment
files:
  # # ssh-rsa AAA...
  # publickey: ~/.ssh/id_rsa.pub
  #
  # # -----BEGIN RSA PRIVATE KEY
  # # ...
  # # -----END RSA PRIVATE KEY
  # privatekey: ~/.ssh/id_rsa
  #
  # # {"auths":{"cloud.openshift.com":{"auth":"b3Blb... }}}
  # openshiftPullSecret: ~/.dockerconfigjson
  #
  # azureOsServicePrincipal: ~/osServicePrincipal.json

[claudiol]

Looking at the module I think we need to update the sanitize_values function. Specifically in the if statement:

    secrets = syaml.get("secrets", {})
    files = syaml.get("files", {})
    if len(secrets) == 0 and len(files) == 0:
        module.fail_json(
            f"Neither 'secrets' nor 'files have any secrets to " f"be parsed: {syaml}"
        )

Splitting the if statement to look like this will generate a better error:

    secrets = syaml.get("secrets", {})
    files = syaml.get("files", {})
    if len(secrets) == 0: 
        module.fail_json(
            f"The 'secrets' section is empty and does not have any secrets to " f"be parsed: {syaml}"
        )
    if len(files) == 0:
        module.fail_json(
            f"The 'files' section is empty and does not have any secrets to " f"be parsed: {syaml}"
        )

This will generate the following error if the files section is empty:

$ ansible-playbook load-secrets.yaml 

PLAY [localhost] **********************************************************************************************************************************************************************************************************

TASK [Gathering Facts] ****************************************************************************************************************************************************************************************************
ok: [localhost]

TASK [Loads secrets file into the vault of a cluster] *********************************************************************************************************************************************************************
fatal: [localhost]: FAILED! => {"changed": false, "msg": "The 'files' section is empty and does not have any secrets to be parsed: {'secrets': {'imageregistry': {'username': 'claudiol+ops', 'password': 'REDACTED'}, 'quay': {'account': 'qyadmin', 'password': 'REDACTED', 'email': '[email protected]'}, 'git': {'username': 'claudiol', 'password': 'ghp_REVOKED'}, 'aws': {'s3Secret': 'REDACTED'}, 'config-demo': {'secret': 'MySup3rR3dh4tP@$$W0rd'}, 'files': None}}"}

PLAY RECAP ****************************************************************************************************************************************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0  

[claudiol]

Please look at the following PR: #185

[mhjacks]

I think it's OK for files to not exist (since a pattern may not need them); likewise, it's OK for secrets to not exist. But there has to be at least a secret to load from one of those keys. I think the error we need to check for at that point is the ensure that type(files) is dict, if files exists.

I think the problem is that the current code handles the situation fine if the keys are not present at all, but if the section is present and has nothing under it, that puts None in the variable instead of the default {} (empty dict).

Thus, I think what we need here are explicit dict() tests for both secrets and files, as that will also prevent malformed files in various ways. The len() test will fail on a NoneType.

I'll propose something here shortly

[mhjacks]

Something like this:

    files = syaml.get("files", {})

    if type(secrets) is not dict or type(files) is not dict:
        module.fail_json(
            f"'secrets' and 'files' must both be of dict type if present: " f"{syaml}"
        )

    if len(secrets) == 0 and len(files) == 0:
        module.fail_json(
            f"Neither 'secrets' nor 'files have any secrets to " f"be parsed: {syaml}"
        )

[claudiol]

@mhjacks Just tested your suggestion but it does not quite work. We expect that the files: section is a dictionary as this will always return a dict type:

files = syaml.get("files", {})

The check will go right pass the if check suggested. I still feel that the PR addresses the issue. The length check is sufficient for either a dict or an array. If files: is found, and it's empty, then we should fail.

[mbaldessari]

I am fixing it via #186 I added a bunch of test cases there

[claudiol]

Ack @mbaldessari