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

top file targeting: key length limit causing issues in large environments #52323

Closed
Paulo-Nunes opened this issue Mar 26, 2019 · 11 comments
Closed
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged stale
Milestone

Comments

@Paulo-Nunes
Copy link
Contributor

Description of Issue/Question

It seems that YAML has a key limit of 1024 characters:(https://yaml.org/spec/1.1/#simple%20key/)
as noted in the salt doc: (https://docs.saltstack.com/en/latest/topics/troubleshooting/yaml_idiosyncrasies.html#keys-limited-to-1024-characters)

Pillar top should not use grain targeting for security reasons. I use list of hostnames, but when there are hundreds of minions this limit of 1024 characters is quickly met.

What is the "Salt" way of handling hundreds of minions in pillar when the key length would exceed 1024? Could this be done when globing is not an option and IP ranges are not sequential?

Setup

(Please provide relevant configs and/or SLS files (Be sure to remove sensitive info).)

This is an example of the pillar top line causing an issue:

'[email protected], ... , minion0100.domain.com':
    - match: compound
    - pillar/file/sls

Steps to Reproduce Issue

(Include debug logs if possible and relevant.)

2019-03-26 11:51:23,083 [salt.pillar      :458 ][ERROR   ][3946] Pillar rendering failed for minion minion0100.domain.com:
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/salt/pillar/__init__.py", line 450, in get_tops
    _pillar_rend=True,
  File "/usr/lib/python2.7/site-packages/salt/template.py", line 95, in compile_template
    ret = render(input_data, saltenv, sls, **render_kwargs)
  File "/usr/lib/python2.7/site-packages/salt/renderers/yaml.py", line 58, in render
    raise SaltRenderError(err_type, line_num, exc.problem_mark.buffer)
SaltRenderError: could not found expected ':'; line 100

Versions Report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

Salt Version:
           Salt: 2017.7.7
 
Dependency Versions:
           cffi: 1.6.0
       cherrypy: Not Installed
       dateutil: 1.5
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: 0.26.5
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: Not Installed
      pycparser: 2.18
       pycrypto: 2.6.1
   pycryptodome: 3.6.1
         pygit2: 0.26.3
         Python: 2.7.5 (default, May 31 2018, 09:41:32)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4
 
System Versions:
           dist: redhat 7.5 Maipo
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-862.14.4.el7.x86_64
         system: Linux
        version: Red Hat Enterprise Linux Server 7.5 Maipo
@dmurphy18
Copy link
Contributor

@Paulo-Nunes Can you offer an explanation as to why you state:

Pillar top should not use grain targeting for security reasons

Given that this is considered a feature of Salt especially in targeting different platforms with globbing, for example:

'G@os_family:Redhat and G@os:Amazon and not G@osmajorrelease:2':
- auto_setup.amazon

'G@os_family:Redhat and G@os:Amazon and G@osmajorrelease:2':
- auto_setup.amazon2

'G@os_family:Redhat and G@osmajorrelease:7 and not G@os:Amazon':
- auto_setup.redhat7

'G@os_family:Redhat and G@osmajorrelease:6 and not G@os:Amazon':
- auto_setup.redhat6

'G@os_family:Debian and G@osmajorrelease:9 and not G@osfullname:Raspbian':
- auto_setup.debian9

'G@os_family:Debian and G@osmajorrelease:8 and not G@osfullname:Raspbian':
- auto_setup.debian8

'G@osfullname:Raspbian and G@osmajorrelease:9 and G@os_family:Debian':
- auto_setup.raspbian9

'G@osfullname:Raspbian and G@osmajorrelease:8 and G@os_family:Debian':
- auto_setup.raspbian8

@Paulo-Nunes
Copy link
Contributor Author

Paulo-Nunes commented Mar 28, 2019

Grain targeting in state top is fine as long as there is no sensitive data.
Pillar data on the other hand often times has sensitive data for the eyes of the minion that is targeted only.
Note I am referring to the state top file vs the pillar top file.

As an example, lets say I have a grain foo and use that grain on minions that receive the bar.sls pillar file.
Then I could:

'foo':
  - match: grain
  - bar

Within that pillar file there could be sensitive information or things we don't want other minions to be privy to. Even if data is gpg encrypted the minion should have the ability to decrypt the pillar if memory serves me. But even if not, sometimes just the structure of the pillar could be information an attacker could use.
If an attacker gets access to a minion, what is stopping them from adding the foo grain to their minion?
Am I wrong in my understanding of this? The attacker would be able to add the grain and receive the pillar data, correct?

By using minion ID we prevent that method of attack. Or is the minion ID easily spoof-able as well?

I am a fan of using grains in a compound with and. So that would be something like:

'G@foo and minion0001.domain.com':
  - match: compound
  - bar

This adds some redundancy and readability as well as helping to make sure all minions are grained appropriately, but not by itself.

Since I raised this question I have thought of 2 ways that I might try to work around this, but neither is great. One would require upgrading to the newest version, but there is an open issue that was affecting the master/minions in 2018.3 that required a downgrade and I haven't seen anything to suggest it has been resolved in 2019.2 (#49863)

Update
Here is saltstack documentation saying not to use grains for pillar targeting:
https://docs.saltstack.com/en/latest/faq.html#is-targeting-using-grain-data-secure

@dmurphy18
Copy link
Contributor

@Paulo-Nunes The item you refer to in #49863 was entirely something differed, it was an issue in the version of pycryptodomex v3.4 which was corrected in v3.6, in that they were handling armored files incorrectly w.r.t to RSA being included. The other part of that issue was that RHEL 7.2 shipped with openssl v1.0.1 and not v1.0.2k as in CentOS 7.2, RHEL 7.5 how ships with v1.0.2, hence had to ship a version of openssl v1.0.2k for those used using RHEL 7.2.

Mitigation would be to use M2Crypto instead of pycryptodomex since it has become an active project again in the last couple of years.

If an attacker gets access to a minion, then in a way all bets are off, since as root on a minion who knows what they could introduce and do: ship all keys to be broken, etc.

However Salt does encrypt communication between the master and the minion pillar information, and the master is unlikely to be affected by anything on the minion, however the minion if compromised could trick the master into allowing packages to be installed on to it which may not be correct, for example: Debian system claiming to be Redhat.

So yes, there could be issues using Pillar top and grain targeting, and the 'id' would be harder to spoof. but currently that is how they are used.

To navigate around the 1K limit on yaml, your could load the keys etc as JSON which would not have a limit, since essentially the yaml is parsed into JSON and the result loaded. Googling should bring up examples where users have done similar.

If the suggestion of using JSON to circumvent YAML 1K limits is sufficient, please consider closing this issue.

@Paulo-Nunes
Copy link
Contributor Author

My aim was to find the Salt approved way of dealing with this limitation. If using JSON is the approved method then that should be sufficient.

Could you explain/provide a simple example of this? I am having a hard time understanding how this would work. Are you saying to use the JSON renderer instead of the YAML default renderer on the entire top file? When you say "load the keys etc as JSON" do you mean to use load_json? Hard to google when I am not sure what I am looking for.

For the sake of others, the 2 methods I was thinking of are: jinja loops and nodegroups.

In 2019.2 nodegroups can be used as N@ in the top files. I could define them on the master and use them in the top. I am not a fan of this method because edits require a master restart and it removes the list of minions from the states stored in github.

Jinja loops would be another way to do this, but it would make the top file a bit messy.

{% logic to loop over all minions %}
'L@{{ 1024_or_less_characters_of_minions }}':
  - match: list
  - state/to/apply
{% endloop %}

This would render to something like:

'[email protected], ... ,minion0046.domain.com':
  - match: list
  - state/to/apply

'[email protected], ... ,minion0092.domain.com':
  - match: list
  - state/to/apply

'[email protected], ... ,minion0138.domain.com':
  - match: list
  - state/to/apply

... etc

In this example I have all my minions defined in another file as YAML that I read into the top and render using jinja into the key:

{%- from 'some_file' import minions with context -%}

'L@{{ minions.role | join(',') }}':
  - match: list
  - state/to/apply

The above however does not work with many minions

@garethgreenaway
Copy link
Contributor

@Paulo-Nunes You correct, for targeting pillar values we do not recommend using Grains because the are stored on the minion and can be easily be overridden. For targeting the number of minions you're looking to target, nodegroups are definitely a good option, but you could also look at adding some regular expressions in as well. https://docs.saltstack.com/en/latest/topics/targeting/globbing.html#regular-expressions

@garethgreenaway garethgreenaway added this to the Blocked milestone Mar 29, 2019
@garethgreenaway garethgreenaway added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Mar 29, 2019
@Paulo-Nunes
Copy link
Contributor Author

@garethgreenaway The problem with regex (and the same reason I mention no globbing) is because I am not necessarily aware of what the server IDs will be ahead of time.
With hundreds/thousands of minions where minions are being added or removed on a daily basis automation is necessary to maintain sanity and readability in the top file.

Yes nodegroups are an option I gave some thought to, but it has some drawbacks. One is it forces me to upgrade salt to be able to use it. Another is that the nodegroup file is stored on the master instead of the git filesystem where the states live. Another still is the requirement of restarting the master on edit.

Regex would work if we could guarantee consistent naming, but we can't. I find regex to be difficult for people to understand unless they are very familiar with it.

The ideal solution would be:
-Secure
-preferably unbound from extra constraints (like having to restart the master)
-easily maintainable/readable
-doesn't affect the readability of other components (like the top) [I worry JSON would make the top harder to read which is why I asked for an example to see what that would look like]

@mattp-
Copy link
Contributor

mattp- commented Apr 1, 2019

@Paulo-Nunes a top file is passed through rendering pipeline like any other file, just add a shebang and you can generate any other type of data, whether that is python dict, json, etc.

@Paulo-Nunes
Copy link
Contributor Author

@mattp- But can we have: #!jinja|yaml|json? In other words, can we have mixed content, YAML and JSON in the same top file? The documentation I read does not make it seem like that can happen since both yaml and json return a python data structure and both accept string.

'minion1'
  - state1

{"minion2": [state2]}
# not sure what the json syntax would be here, but the intention is that it is the JSON equivalent of the above and below

'minion3'
  - state3

Would I have to write the entire top in JSON format? That is not ideal because it is much harder to read and maintain.

Ideally I would like to keep the YAML formatting as much as possible because of consistency and readability/maintainability. I don't like the idea of changing the format of my files because the size of the environment reaches some threshold.

@mattp-
Copy link
Contributor

mattp- commented Apr 2, 2019

yes, you'd need to rewrite it for json. that said what you are actually trying to do could be served by node groups defined in your master conf. that way you could stay in yaml and avoid this problem.

@Paulo-Nunes
Copy link
Contributor Author

Unfortunately it seems like using node groups is high on the list of viable options. It has its downsides as I noted earlier. The two worst negatives for me is the need to restart the salt master for new node groups to work, and the upgrade to new salt( tried it and ran into some breaking behavior that needs to be resolved).

To automate things I am generating a list of minions grouped by roles, writing that to a file, and using jinja to load those into the top. In essence I am creating node groups, but they render out to yaml and that is what causes the issue (hitting the character limit).

If there is no great way of doing this I might have to settle for node groups.
Definitely don't want to give up YAML, there is almost no documentation on how things should look except for in YAML.
The jinja loop method seems like it could get a bit messy with the logic required.
A script to convert a list of strings to a regex expression I think would be too intensive an operation.

A little disappointed salt cant handle many minions using the same standards/best practices it uses for few minions.

Something that would be cool to have would be a way to tag a portion of the top to use a separate renderer:

@yaml
'minion1':
  - state1

@json
{"minion2": [state2]}

they would both render out to python data structure and then be merged into one

or have a way to add a target property that would be the actual target instead:

'Descriptive target group name':
  - match: target
  - target: '[email protected], ... , minion0100.domain.com'
  - state1

@stale
Copy link

stale bot commented Jan 8, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged stale
Projects
None yet
Development

No branches or pull requests

4 participants