Warnings from salt.pillar.gpg in Fluorine

[max-arnold]

Description of Issue/Question

It looks like some changes in #45781 or #46542 broke the salt.pillar.gpg https://docs.saltstack.com/en/latest/ref/pillar/all/salt.pillar.gpg.html:

% tail -2 etc/salt/minion
ext_pillar:
  - gpg: {}

% cat pillar/test.sls
key: value

% LANG=C salt-call --local pillar.items
[WARNING ] Could not decrypt cipher value, received: b'gpg: no valid OpenPGP data found.\n[GNUPG:] NODATA 1\n[GNUPG:] NODATA 2\ngpg: decrypt_message failed: eof\n'
[WARNING ] Could not decrypt cipher value, received: b'gpg: no valid OpenPGP data found.\n[GNUPG:] NODATA 1\n[GNUPG:] NODATA 2\ngpg: decrypt_message failed: eof\n'
local:
    ----------
    key:
        value

If I restore the salt/renderers/gpg.py back to the 2013.3.3 state, everything works as expected.

Versions Report

Salt fluorine branch, revision 241741a

CC: @secumod @DmitryKuzmenko

[max-arnold]

This is also slower, because it attempts to decrypt each pillar value, even if there is no PGP signature:

        # Possibly just encrypted data without begin/end marks
        return _decrypt_ciphertext(cipher)

At least two tests need to be added:

1. That there are no attempts to decrypt unencrypted keys
2. That there are no WARNINGs in the log

[ignatk]

Looking at the code I think it is #46542 because when I did #45781 my assumption was we always annotate PGP ciphertext with proper BEGIN/END marks. However, it seems we also want to have unannotated PGP ciphertext, which was fixed in #46542.

I'm not sure what the proper behaviour should be here, because if we want "no attempts to decrypt unencrypted keys" but still want support for "unannotated PGP ciphertext", we need some way to identify OpenPGP messages without BEGIN/END marks. We can do that by trying to parse the data according to RFC 4880, but in your setup that would still make it slow, because it will try to parse every pillar value by default.

In my opinion, gpg renderer should not be enabled by default for all pillars, but rather in specific files in a "shebang" line on as-needed basis. Apart from mentioned performance issues you may as well get security issues, because the salt-master has only one master decryption key. If you by accident stick your encrypted value into the wrong file - it will still be rendered and decrypted - therefore the secret may potentially leak to a less trusted minion. By specifying the gpg renderer explicitly only in files, which really need it, there is less risk it may happen.

And from security perspective with on-needed gpg renderer the WARNING message is justified in my opinion: if your pillar definition does not have any secrets - the renderer for this pillar should be disabled to avoid someone accidentally modifying the definition and leaking some sensitive data.

But all these are just my thoughts - I guess the maintainers should agree what the proper behaviour should be here.

[max-arnold]

In my opinion, gpg renderer should not be enabled by default for all pillars

See the docs I mentioned above: https://docs.saltstack.com/en/latest/ref/pillar/all/salt.pillar.gpg.html
It is not possible to add the GPG renderer to external pillars without enabling it globally. If you use any of the following pillar backends, you will lose the ability to encrypt the data: https://docs.saltstack.com/en/latest/ref/pillar/all/index.html#all-salt-pillars

but rather in specific files in a "shebang" line on as-needed basis.

Many folks use the different strategy: they only encrypt some of the pillar values. Forcing them to encrypt everything or split their pillar files seems wrong, and reduces the master performance.

@DmitryKuzmenko Are you sure that the ability to decrypt raw data was actually there? This is the code before #45781: https://github.com/saltstack/salt/pull/45781/files#diff-2c20356d759feadcae8c50c2a46596c6L303. The logic is quite straightforward - if GPG_HEADER.search(obj) is False, then the obj value is returned as is, without any attempts to decrypt it.

[DmitryKuzmenko]

@max-arnold just re-reviewed the original code and my update. Yes you're right. I've been confused by the original test that was calling _decrypt_ciphertext with raw crypted data.
So the correct fix would be to change https://github.com/saltstack/salt/pull/46542/files#diff-2c20356d759feadcae8c50c2a46596c6R303 to return cipher. And update the test accordingly.

[max-arnold]

@DmitryKuzmenko You or me? :)

[DmitryKuzmenko]

Done. =)

[DmitryKuzmenko]

Backported the fix to fluorine #50831

[max-arnold]

I can confirm that the issue is solved. Thanks!