Adding client_acl functionality to rest_cherrypy API.

[TheScriptSage]

Hey Saltstack folks!

We have a business need for allowing certain users to access the API from certain hosts. Because of this the normal whitelist IP wouldn't work, so I stole the idea from master and made a client_acl inside of the rest_cherrypy configuration. It looks similar to the other one as well:

rest_cherrypy:
  port: 8888
  client_acl:
    user1:
      - 1.1.1.1
    user2:
      - 1.1.1.2

This has been tested and working using the following inputs:

• Valid user and valid IP
• Valid user and invalid IP
• Invalid user and valid IP
• Invalid user and invalid IP

All seem to work. This also should work without the value being there, so nobody who is using the API should have to change anything, it's just a new feature they can get.

If there's a better place to put this, please let me know, but this was pretty easy to implement here.

Thanks.

[jfindlay]

this needs to be .. versionadded:: Boron.

[jfindlay]

Thanks, @TheScriptSage.

[TheScriptSage]

I put the code in the wrong spot for my pull request, so I've fixed it. Additionally I've fixed the version added.

Sorry about that, thanks for picking it up.

[jacksontj]

I'm not sure we want to call it client_acl as that name is already used for something else: http://docs.saltstack.com/en/latest/ref/clientacl.html Also, we should definitely have some tests around the feature :) In addition, does this scoping support groups?

[TheScriptSage]

@jacksontj - I believe since the options are specific to the rest_cherrypy (inside of the configuration for it), I think the name is OK inside of the rest_cherrypy section, and it lets people know it's similar to the old one. I can change it to something like "api_acl" if needed.

As for the groups, no, right now it just checks the user, I'm going to look into the groups as well but for now this works for users (which is needed for the business need we have). If I have some more time I'll look into adding group support but I have this tested and working for users.

As for the actual tests, I'll look into those as well but I'm not entirely familiar with salt's testing so I suspect that would take longer.

Thanks for the feedback.

[TheScriptSage]

Talked to @jacksontj in person, will rename it once the tests have all passed and I know that I don't have any more problems.

[TheScriptSage]

@jfindlay - I am not sure what to do about this. This looks like an ubuntu test and I don't have any way to really test it. My assumption is that this is related to the if statement where it assigns creds and it's likely that it's picking up a dict that doesn't have username, can you provide some insight here?

Thanks.

[TheScriptSage]

@jfindlay - Nevermind, I see what the problem is. The creds dict being passed for the test doesn't have a valid username key. I'll wrap the username assignment with .get().

[TheScriptSage]

I think these test failures are unrelated, I'll make my change for it to be "api_acl" either tonight or tomorrow and then update this again. Thanks.

[jfindlay]

@TheScriptSage, yes those test failures are unrelated. @jacksontj, these test tcp transport unit test failures have been happening consistently on the CentOS 7 pull requests. I looked at them a little today but am unsure what to do. Do you have any ideas? Thanks.

[jacksontj]

@jfindlay I noticed that too-- although I can't reproduce it locally at all. Can we spin a VM or something that can repro the issue?

[jfindlay]

@jacksontj, I'll try to do that, thanks.

[TheScriptSage]

@jfindlay - I put in my changes from client_acl to api_acl, but the tests are still failing, this one isn't from me either (out of memory, etc). Do you want to still merge this in? Also I don't know the oldest tag to apply this to, so if you have any tips on how to figure that out that would be great.

Thanks!

[TheScriptSage]

@jfindlay - I've updated this pull request again to check if the request is being proxied as well, which should help when people want to go through a main proxy but still only be allowed from certain hosts.

Once these tests pass/fail I think that's all from me for this pull request. Thanks.

[jfindlay]

@TheScriptSage, if there are any test failures, I should be able to know if they are unrelated. Thanks for your work.

[TheScriptSage]

I think there might be something wrong with some jenkins nodes or something those errors are odd. Anyway, I'll leave this to you. Thanks again.

[jacksontj]

Using the X-Forwarded-For header has a couple problems:

• First off, its not a single ip-- its the chain of proxies and the client for this request (https://en.wikipedia.org/wiki/X-Forwarded-For)
• Second off, this header can't really be trusted... since its just a header (meaning anyone can set it to whatever they want).

I'd suggest not supporting proxies, unless its needed as it can be a bit difficult on the API side to determine what the actual client is. If support is needed, then it should be configurable (on/off-- and ideally to let the user pick a header-- since its common for people's setups to have a specific header which is sanitized).

[TheScriptSage]

I'll add a configuration for it. Good idea. Will get another commit up sometime tonight or tomorrow.

[TheScriptSage]

I'm still working on this, my weekend got away from me and I'll update this shortly. Thanks.

[TheScriptSage]

I'll change it to fail closed due to the '*' stuff I added. Good point.

As for the group stuff, the groups would be harder since I'd have to process their request, pass their request to their eauth of choice (and make sure it has a groups() function), and then parse that. Not sure if that would fit well here, but I can look into it.

Thanks for the feedback. Will get to work on this shortly.

[TheScriptSage]

So, I've been thinking about this, and after consulting with some other folks internally, I'm going to scratch the proxy stuff entirely. It's too insecure and I can't think of a fast and efficient way to do this. So, for now I'm going to make sure it passes for just doing IP whitelisting for users that are trying to auth through the API.

Added some sphinx docs and stuff as well so that things are more 'clean'. Will watch for the results. Thanks for the back and forth @jacksontj, @jfindlay.

[TheScriptSage]

@jfindlay Is there an easy way to kick off the tests again? I don't think I am responsible for these failures.
21:36:01 * ERROR: Failed to run install_centos_git_deps()!!!

I can always push an empty commit, not sure if there's another option.

Thanks.

[TheScriptSage]

Pushed an empty commit and still saw the same error. Gonna leave this for you guys to look at, since I'm not sure this is me. Thanks.

22:18:02 * DEBUG: Installed git version: 1.9.1
22:18:02 * WARN: The git revision being installed does not match a Salt version tag. Shallow cloning disabled
22:18:02 Cloning into 'salt'...
22:18:17 Connection to 23.253.78.240 closed.
22:18:17 Error: There was a profile error: Command "ssh -t -t -oStrictHostKeyChecking=no -oUserKnownHostsFile=/dev/null -oControlPath=none -p 22 root@23.253.78.240 '/tmp/.saltcloud-4ebf54c5-c47f-4871-8dfb-7e297aa235e5/deploy.sh -c /tmp/.saltcloud-4ebf54c5-c47f-4871-8dfb-7e297aa235e5 -ZD git 2014.7.0'" failed. Exit code: 1
22:18:17 * DEBUG: Checking out 2014.7.0
22:18:17 error: pathspec '2014.7.0' did not match any file(s) known to git.
22:18:17 * ERROR: Failed to run install_ubuntu_git_deps()!!!
22:18:17 * DEBUG: Cleaning up the Salt Temporary Git Repository

[TheScriptSage]

Oh, looks like the tests finally went further, I see the same problem that I had fixed before and accidentally reverted out, I'll get it fixed again. Thanks for looking at the tests if someone did.

[TheScriptSage]

Hey again, I think this failure isn't related to me. Mind looking? @jfindlay

[jfindlay]

@TheScriptSage, yes, that is unrelated.

[cachedout]

Finally got this one in. :] Thanks for your patience @TheScriptSage