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

Fix multiple issues with the TSS lookup plugin when using fetch_attachments #6720

Merged
merged 8 commits into from
Jun 20, 2023

Conversation

laszlojau
Copy link
Contributor

@laszlojau laszlojau commented Jun 18, 2023

SUMMARY

There are multiple issues present in the TSS lookup plugin when the recently added fetch_attachments is set to True:

  • All files are written as text with UTF-8 encoding - this does not work for PFX files which have to be written in binary format
  • An AttributeError is raised ('str' object has no attribute 'text') when some attachments are missing from a secret. This is an issue, as some attachments may be optional (e.g. a secret can hold both a PFX and PEM format certificate in separate fields)
  • Empty files get created for missing attachments, so an empty certificate may be uploaded to a target server if we are only checking for the file being present
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

tss

ADDITIONAL INFORMATION
  1. An AttributeError is raised when attachments are missing:

    Before this PR (files get created for missing attachments and the whole task errors out):

    fatal: [localhost]: FAILED! => {
        "msg": "An unhandled exception occurred while templating '{{\n    lookup(\n        'community.general.tss',\n        1234,\n        fetch_attachments=True,\n        file_download_path='files/certs-test',\n    )\n}}'. Error was a <class 'ansible.errors.AnsibleError'>, original message: An unhandled exception occurred while running the lookup plugin 'community.general.tss'. Error was a <class 'AttributeError'>, original message: 'str' object has no attribute 'text'. 'str' object has no attribute 'text'"
    }

    After this PR (files won't be created for missing attachments and the task will raise warnings instead of erroring out):

    TASK [Test secret without all attachments] *************************************************************************
    [WARNING]: Could not read file content for certificate-file-pem
    [WARNING]: Could not read file content for key-file-pem
    [WARNING]: Could not read file content for full-chain-certificate-file-pem
    
  2. Downloaded files are corrupted when using PFX attachments

    Before this PR: PFX files get corrupted when downloading them with the current lookup plugin, which uses the text field for loading the content of the attachment - i.e. they can't be read with openssl pkcs12 -in my-cert.pfx -clcerts -nokeys -noout -text.

    After this PR: Using the content field and writing the file as binary works for PFX files. It also works for PEM format certificates and RSA keys.

3. The return values are different when using fetch_attachments and they cannot be printed
I've reverted the relevant commit as this was an issue on our end, masked by the above the 2 issues. I was receiving errors as our secret template did not have a private-key field.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug lookup lookup plugin new_contributor Help guide this first time contributor plugins plugin (any type) labels Jun 18, 2023
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 18, 2023
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jun 18, 2023
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! I've a first comment on the changelog fragment, the plugin maintainers might have more/other comments.

changelogs/fragments/6720-tss-fix-fetch-attachments.yml Outdated Show resolved Hide resolved
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-6 backport-7 Automatically create a backport for the stable-7 branch labels Jun 19, 2023
laszlojau and others added 2 commits June 19, 2023 16:08
Co-authored-by: Felix Fontein <felix@fontein.de>
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jun 20, 2023
@felixfontein felixfontein merged commit 6bff57e into ansible-collections:main Jun 20, 2023
136 checks passed
@patchback
Copy link

patchback bot commented Jun 20, 2023

Backport to stable-6: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 6bff57e on top of patchback/backports/stable-6/6bff57ee6e85a450540ddd9151b9814cd24d294f/pr-6720

Backporting merged PR #6720 into main

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https://github.com/ansible-collections/community.general.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/stable-6/6bff57ee6e85a450540ddd9151b9814cd24d294f/pr-6720 upstream/stable-6
  4. Now, cherry-pick PR Fix multiple issues with the TSS lookup plugin when using fetch_attachments #6720 contents into that branch:
    $ git cherry-pick -x 6bff57ee6e85a450540ddd9151b9814cd24d294f
    If it'll yell at you with something like fatal: Commit 6bff57ee6e85a450540ddd9151b9814cd24d294f is a merge but no -m option was given., add -m 1 as follows instead:
    $ git cherry-pick -m1 -x 6bff57ee6e85a450540ddd9151b9814cd24d294f
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR Fix multiple issues with the TSS lookup plugin when using fetch_attachments #6720 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/stable-6/6bff57ee6e85a450540ddd9151b9814cd24d294f/pr-6720
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback
Copy link

patchback bot commented Jun 20, 2023

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/6bff57ee6e85a450540ddd9151b9814cd24d294f/pr-6720

Backported as #6751

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@felixfontein
Copy link
Collaborator

@laszlojau thanks for fixing this!
@delinea-sagar @delineaKrehl thanks for reviewing!

patchback bot pushed a commit that referenced this pull request Jun 20, 2023
…hments (#6720)

* Treat files as binary when downloading attachments

* Raise a warning when the attachment can't be read

* Set the 'itemValue' for files, even when they can't be read

* Always return the original secret content

* Add changelog

* Fix changelog

* Update changelog

Co-authored-by: Felix Fontein <felix@fontein.de>

* Revert "Always return the original secret content"

This reverts commit a9fb96e.

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 6bff57e)
felixfontein pushed a commit that referenced this pull request Jun 20, 2023
…SS lookup plugin when using fetch_attachments (#6751)

Fix multiple issues with the TSS lookup plugin when using fetch_attachments (#6720)

* Treat files as binary when downloading attachments

* Raise a warning when the attachment can't be read

* Set the 'itemValue' for files, even when they can't be read

* Always return the original secret content

* Add changelog

* Fix changelog

* Update changelog

Co-authored-by: Felix Fontein <felix@fontein.de>

* Revert "Always return the original secret content"

This reverts commit a9fb96e.

---------

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 6bff57e)

Co-authored-by: laszlojau <49835454+laszlojau@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-7 Automatically create a backport for the stable-7 branch bug This issue/PR relates to a bug lookup lookup plugin new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants