Salt: Grains insecure for targeting sensitive data

[rcallphin]

Description of Issue/Question

This is in regards to the new Hashicorp Vault module introduced in 2017.7.1

https://docs.saltstack.com/en/latest/ref/modules/all/salt.modules.vault.html

Unless I am missing something the example:
"If a template contains a grain which evaluates to a list, it will be expanded into multiple policies. For example, given the template saltstack/by-role/{grains[roles]}, and a minion having these grains:
The minion will have the policies saltstack/by-role/web and saltstack/by-role/database."

Is not necessarily a secure way to target your secrets as a compromised minion could simply change its role in order to retrieve secrets it should not be able to access. I think there should be a warning next to this example that grains are controlled by minions and therefore insecure.

I think that a more appropriate support for targeting in this module would be pillar data. Is that a possibility? Can we get that added?

[gtmanfred]

That is a great idea!

I have implemented it in #43288

It will be in the next major release of salt, or you can use the salt://_runners dynamic-modules dir to make it available in 2017.7.

Thanks,
Daniel

[rcallphin]

@gtmanfred Thanks for the quick response. That PR would be a great addition. Any chance this could be considered for a dot release for 2017.7? I'd rather not use dynamic-modules dir

In my mind only having support for grains is a "bug" in the module. The documentation suggests using saltstack/by-role/{grains[roles]} to target secret data within Vault which opens up a security hole with a minion able to assume any role it wants. Currently the only way for me to securely target using grains is ID, but that forces me to have a policy in Vault per minion with no way to group polices. Pillar support seems like a minimum to be able to securely use this module at scale.

[gtmanfred]

It implements new functionality, so it is a Feature imho

[carlpett]

Hi @rcallphin,
As I explain in comments on the PR, the reason that this is not already a part of the functionality is that it causes infinite recursion when pillars have values taken from Vault. I could not figure out a way to get around this when implementing the original module, but maybe there are ways.

[carlpett]

But please note that I completely agree agree that it is sub-optimal. However, then implementation that was merged previous to mine simply exposed the entire vault to every minion without any checks at all.

[gtmanfred]

That makes sense, the only way we could get around it would be to be able to strip the sdb + vault stuff from the pillars, which isn't going to be easy.

I have closed that PR. I am going to leave this as a feature request.

Thanks @carlpett

[carlpett]

@rcallphin To think a bit differently, could we store the targeting within Vault itself somehow?

[rcallphin]

@carlpett Good call on the PR, that makes a lot of sense.

I was just thinking through how we could move the targeting to Vault. In the 0.8.0 release (https://www.hashicorp.com/blog/vault-0-8/#entities-and-multi-factor-authentication), Hashicorp introduced this concept of Vault being aware of identities and entities that can then be scoped to specific policies. I know that there is a lot more coming around this in upcoming releases. Maybe that is a direction worth looking into.

[damon-atkins]

I am not across how the keys section of minion work (salt-key). Why not put secure grains inside the key which is (I assume) signed. This data within the key can be used for secure targeting, because it was accepted as part of signing the key.

[carlpett]

@damon-atkins Hm, how do you mean inside the key? It seems like an interesting idea. I already use the key for signing token requests to the master (since the publish interface does not expose which minion sent the command), so perhaps this is not so far away.

[damon-atkins]

e.g. made up example.

$ salt-key
Accepted Keys:
win10test
   role = 'AD'
linux7test
  role = 'web'
  app = 'middleware'  # basically key/value pairs
Denied Keys:
Unaccepted Keys:
Rejected Keys:

As the master has a copy of the it accepted/signed.

The general process is a product initiates the build e.g. salt-cloud daemon, Right Scale etc -> Salt Client with extra details in the key send a request to the master -> master then checks with the product which initialise the build see if the extra details are correct -> key is accepted with the extra values.

[carlpett]

That seems very useful! Never seen this before. What are the requirements on the key generation to stick those pairs in there?

[damon-atkins]

If Salt is using X.509 Certificates it should be simple to do. For example

Master Signs X.509 Certificate, and sends it to the client to use for future authentication. Every time the client authenticates its also sending the "secure grains/tags" within X.509 which can be trusted as the Master checks if its signed correctly. The certificate tells Salt Master the name of the system and extra value/pairs which is independent of current DNS, IP address etc.

Changing the client X.509 certificate would invalidate it, and the "secure grains/tags" can not be changed, with out the master accepting a new key.

[carlpett]

Ah, so your example is not something that already exists? In that case I misunderstood :)

[damon-atkins]

Its a made up example. Have no idea on the details of how the existing authentication works and if it can be changed to support adding secure value/pairs

[carlpett]

The idea was very interesting, so I spent a bit of time exploring it. However, salt uses rsa keys, not x509 certificates to sign communications. As far as I'm aware, there is no way to embed metadata within rsa keypairs :( (Also, it wasn't all that simple in x509 certs either, the most reasonable field to use, subjectDirectoryAttributes, isn't supported by OpenSSL as far as I can tell).

So there would need to be an additional bundle of data that the master accepts as part of the key-acceptance phase, or similar.

[evelineraine]

Hi @carlpett,
Sorry if I misunderstand how it all works, but really, how is this infinite recursion a thing?
When pillars are rendered on the master, it's access to the vault is restricted only by the policies already applied to the vault:auth:token. So nothing under vault:policies has to be processed until a minion attempts to access the vault, at which point all pillar processing should be finished.
Am I missing something?

[carlpett]

Hi @fillarios,
The flow is a bit different from your description. The token under vault:auth:token on the master is not used to render any pillars, it is used to request tokens that used for rendering the pillars.

Imagine I have a pillar file that looks like this:
```foo:
bar: {{ salt['vault'].read_secret('secret/foo/bar') }}

And a salt-vault config like this:

vault:
[...]
policies:
- saltstack/by-role/{pillar[roles]}
```

Now, say we want to do something to a minion M, which requires access to the pillars (such as a state run). It will start to render the pillars, and encounter foo, which calls into vault to read a secret. When this happens, the master will try to generate a token for this minion. To do so, it needs to expand the policies that are to be used. It then asks for the minion's pillars, and then start rendering them, and finding foo... And so on.

[evelineraine]

Yes, but as far as I know pillars are rendered on the master.
You could try to put this in a pillar sls file and see what value a minion gets:

foo: {{ salt.cmd.run('hostname') }}

For all my minions foo is set to the hostname of my master.

[carlpett]

Yes, pillars are rendered on the master, but they are rendered for a specific minion. salt['vault'].read_secret will take which minion is rendered for into account (in fact, if the master did not know which minion it was rendering for, all the minions would have access to the same secrets).

[evelineraine]

Yes, I understand why modules.vault.read_secret has to account for which minion it is called from, when called from a minion to make sure it's access to the vault is properly policed.
But, when called from the master (like when rendering pillars), I imagine it is possible to just use the token provided in the master config to read from the vault and ignore the policies defined for minions.
This way when pillars are rendered, no policy templates are expanded, and so an infinite recursion would not happen if pillars are used in vault:policies.

Does that make sense? :)

[mgriffin13]

@carlpett I think the secure grain functionality you're looking for would be solved by #31109. Of course, that feature request has been on the books for a while.

As a mitigation to the non-secure grains, what we've done is created a random pre-shared key. Then, we define the grain on the minion

grains:
  psks:
    - <long psk>
    - <second psk for additional secrets>

The vault.conf on the salt-master:

vault:
    [...]
    policies:
        - saltstack/psks/{grains[psks]}

This is not perfect, but it does make it substantially harder for a compromised minion or developer to just assume any role and read the secrets.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[mchugh19]

Still a valid feature request

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

[mchugh19]

Seriously, this is still a valid feature request

[stale[bot]]

Thank you for updating this issue. It is no longer marked as stale.

[driskell]

Yes, pillars are rendered on the master, but they are rendered for a specific minion. salt['vault'].read_secret will take which minion is rendered for into account (in fact, if the master did not know which minion it was rendering for, all the minions would have access to the same secrets).

Sorry to raise old things. I somewhat hit this issue currently. I was expecting the vault calls from pillar to be trusted (since it's running on the master and contains secrets anyway) but then discovered it is impersonating the minion. Is there a reason this is needed? Is it not the case that the master will always know what minion it is rendering for (at least in pillar context) since otherwise how does it know which pillar to render? It would never reach the vault calls.

Is the least resistant path here to allow (perhaps via option) pillar calls to vault to be run using the saltstack/master policy and thus allow the controlling of access to be controlled via pillar. All other calls anywhere else which are not the master rendering pillar can properly be restricted.

I'd use ext_pillar vault but it doesn't do auto-generation and I'm currently looking at cluster formation where first server generates the secret. Plenty of other ways I guess - just found this to be unexpected since everything we learn about pillar is - it runs on the master - but with sdb vault - it runs on the master but as the minion which is fairly confusing at first.