salopensource / sal-scripts

Apache License 2.0
23 stars 31 forks source link

Requests #71

Closed sheagcraig closed 4 years ago

sheagcraig commented 4 years ago

This PR pulls out the curl wrapper used currently and replaces it with python requests.

Why?

This was predicated on having mysterious curl failures that I could only resolve by dropping from http2 down to http1.1 by editing the curl command used OR using a current brew curl instead of the OS-provided one.

Instead, I figured why not just use requests? So I created a separate project to provide requests tools to let you use the macOS keychain for verifying SSL certs (pip install macsesh) and moved all of the requests between the client and Sal into a client class.

This cleans up some of the error handling and logging as a freebie.

Outstanding, I have yet to hear from anyone regarding whether they use sal-scripts with client certs for mutual TLS that relies on specifying the cert by name (i.e. curl built with secure transport) rather than providing the path to a cert somewhere on disk. I don't have a test environment for this, and it seems unlikely that anyone else does, since the existing code seems to contradict the curl man page's information re: how to use certs by name and not actually work.

If you rely on this, and you want it to keep working, please contact me! I think we have an easy way to handle this, but I want to make sure!

sheagcraig commented 4 years ago

I've done some more work on this.

Work on macsesh led to the ability to use the macOS keychain for client cert auth, just like curl does, by specifying the CN of the identity you want to use.

The sal scripts will let you use either a path or a CN; behind the scenes it decides which to try based on whether the input exists as a file path or not.

I probably need to put some checking in; if you have a self-signed certificate for your Sal server and you're specifying it as a path, but then doing cert auth and specifying that as a CN, it won't work. If ANY of the cert prefs are a path, it won't use the keychain.

It's absolutely possible to take that path, create a temporary keychain, import the cert into that keychain, and carry on, but I don't have the energy for that right now.

The docs will be updated and maybe I'll put in some error checking or asserts just to steer people away from this.

TL;DR: Either put all of your certs in the keychain or all on disk.

As part of the code reshaping, I also started teasing the Mac-specific code into a mac_utils module within the Sal python package and wrapping imports of it in exception handlers with the goal of making a Sal python package that could be used on other platforms.

This is partially selfish on my part, just so we can unify a lot of the code we use at work to checkin all of our platforms.

But longterm I think this would serve as a useful python client for doing API stuff with an easy setup.

This PR also fixes the way the preflight determines whether scripts in preflight.d are executable, following on the work by @Zolotkey to make this conform to Munki's expectations.

This PR also also fixes an incorrect logical check during the preflight that results in syncing plugin scripts not working correctly.

So yes, unfortunately, another omnibus PR from me. Sorry.

sheagcraig commented 4 years ago

Oh. I already have a warning for mixing certs on file vs. in the keychain.

sheagcraig commented 4 years ago

This fixes #70

sheagcraig commented 4 years ago

Pending review, I'd like to merge this in and cut a pre-release for people to test out, since it's changing the requests tooling to use requests rather than curl, which is definitely worth testing.

Zolotkey commented 4 years ago

After the macsesh fix, It works on both our MDM enabled mac's with a Root CA via MDM and our non MDM mac's that have the CA on disk and profile.