allow pillar to be used in vault policies

[gtmanfred]

What does this PR do?

Allows the more secure pillar object to be used in vault policies.

What issues does this PR fix or reference?

Closes
#43287

Tests written?

No

[ghost]

@gtmanfred, thanks for your PR! By analyzing the history of the files in this pull request, we identified @carlpett and @danielmotaleite to be potential reviewers.

[carlpett]

Hi,
This sadly cannot be merged, since it will cause infinite recursion when evaluated if there are also pillars values that use the module. This is the reason it is not included in the first place, a comment that for some reason seems to have disappeared when the first bits were merged.

If pillar-based targeting is considered better than pillars with vault-based values, then the PR needs to be extended with removing that functionality instead. Not sure if that is possible, though, the execution module will always be possible to use in pillar rendering, right?

[carlpett]

As per comment in main thread

[The-Loeki]

As the third guy to trip over this, I'd like to reopen the discussion regarding this:

• I'm not sure I'm following the recursive loop here; The runner calls salt.utils.minion.get_minion_data, which already fetches the pillardata from cache. That implies that regardless of whether it's used or not, the pillar would get rendered and the infinite loop would happen. Or am I getting this wrong?
• The pillar is rendered master-side/less, both of which looks to always use _use_local_config() (bypass the runner) in salt.utils.vault; that seems to prevent it from happening as well, right?

Secondly, if there truly is an infinite recursion problem, another stop I can think of is the Vault pillar's __virtual__(), something akin to:

def __virtual__():
  if '{pillar' in config:
    return (False, 'Cannot combine {pillar} policy expansion and Vault pillar') 

If that's OK with all I'll gladly hackup a PR either way

[carlpett]

It has been a year since I left the company where I initially developed this, and I also haven't been a regular saltstack user since (😞), so my memory is a bit hazy on the details.
But in general terms, the problem is that the pillars you get would be dependent on which pillars you have. So there is an obvious circular dependency there. The caches don't come into play, since they will not have the data, and thus need to be refreshed, and you are back to the circular dependency.

You might be able to resolve this by having some limit as you suggest, I'm not sure if there would be some confusing cases, or any corner cases around the less mainstream usages (salt-ssh, masterless, etc) resulting from that.

Not sure how much this helps, but that's my take.

[The-Loeki]

@carlpett thanks for your input, I'll continue the discussion in other tickets ;) long story short; apparently some older version did not do things the way it does now, so the infinite-recursion loop no longer exists at the expense of pillar-side runner/token impersonation.