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 inline encryption of multiline strings #337

Merged
merged 5 commits into from Nov 24, 2021

Conversation

jeinwag
Copy link
Contributor

@jeinwag jeinwag commented Nov 19, 2021

I noticed that the inline encryption of multiline strings does not give the expected results.

This PR fixes the issue and implements proper handling of the different types of multiline blocks in YAML.

@jeinwag jeinwag requested review from tomaciazek and a team as code owners November 19, 2021 08:34
@ssbarnea ssbarnea added the bug Something isn't working label Nov 19, 2021
@ssbarnea ssbarnea changed the title fix inline encryption of multiline strings Fix inline encryption of multiline strings Nov 19, 2021
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

If you would not mind, next time you find such a bug file a ticket before starting to work on fixing it and mention that you will try to fix it yourself. That is better for several reasons: letting others be aware of existing bug and also avoiding having two working on fixing the same bug.

I mentioned that because your PR went in while I was running the release pipeline. If I knew about that one hour earlier I could have delayed it and include the fix in 0.7.0 release.

PS. I am currently testing the code.

@ssbarnea
Copy link
Member

@jeinwag Can you please mention on clear operation where this failed before your patch and that started working correctly after your patch.

Based on my testing I seen that round-trip operation worked all time, even if the original block was a multiline one, with | or with > yaml forms. Maybe I did not run the test correctly but I would really want to ensure it before merging it.

@jeinwag
Copy link
Contributor Author

jeinwag commented Nov 19, 2021

@ssbarnea sorry, I should have clarified.
En- and decryption of multiline strings does work fine, but the encrypted value is the literal string - including the multiline indicator (e.g. | or >) and indentation.

Running the following example with the value of multiline in encrypted vs decrypted state should make the issue obvious:

- name: test multiline inline encryption
  hosts: localhost
  vars:
    multiline: |
      this is a
      multiline block
  tasks:
  - name: output multiline
    debug: msg="{{ multiline }}"

Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

Now that I understand it better I will approve it but not merge it yet, mainly because I want to merge first the change that adds extra testing.

With extra testing it should become quite easy to add a test for this bugfix, so we would prevent regressions.

src/features/vault.ts Outdated Show resolved Hide resolved
@ssbarnea ssbarnea enabled auto-merge (squash) November 24, 2021 17:09
@ssbarnea ssbarnea merged commit 060925c into ansible:main Nov 24, 2021
ssbarnea added a commit to ssbarnea/vscode-ansible that referenced this pull request Dec 15, 2021
Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
Co-authored-by: Sorin Sbarnea <ssbarnea@redhat.com>
Co-authored-by: Sorin Sbarnea <sorin.sbarnea@gmail.com>
ssbarnea pushed a commit that referenced this pull request Mar 15, 2024
Bumps [ansible-compat](https://github.com/ansible-community/ansible-compat) from 2.0.4 to 2.1.0.
- [Release notes](https://github.com/ansible-community/ansible-compat/releases)
- [Commits](ansible/ansible-compat@v2.0.4...v2.1.0)

---
updated-dependencies:
- dependency-name: ansible-compat
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants