User GitOps is not able to set macOS Setup Assistant configuration

lucasmrod commented 1 year ago

Fleet version: main https://github.com/fleetdm/fleet/commit/68f2aef7cbdfb83c6431e90c6deeb381c2f8f91c (4.31.0)

As a GitOps user I want the ability to apply macOS Setup Assistant configuration via fleetctl, e.g.:

Sample:

$ cat team.yml
apiVersion: v1
kind: team
spec:
  team:
    name: Apollo
    mdm:
      macos_setup:
        macos_setup_assistant: tools/mdm/apple/dep_sample_profile.json

fleetctl apply --debug -f team.yml

🧑‍💻 Expected behavior

The configuration is applied successfully.

💥 Actual behavior

GET https://localhost:8080/api/latest/fleet/version

GET https://localhost:8080/api/latest/fleet/version 200 OK (38ms)
{
  "version": "fleet-v4.30.0-164-g1d022e76b-dirty",
  "branch": "11395-bp-filters",
  "revision": "1d022e76b6b63e15bf04b65804e961213c5015aa",
  "go_version": "go1.19",
  "build_date": "2023-04-28",
  "build_user": "roperzh"
}
GET https://localhost:8080/api/latest/fleet/config

GET https://localhost:8080/api/latest/fleet/config 403 Forbidden (11ms)
{
  "message": "forbidden",
  "errors": [
    {
      "name": "base",
      "reason": "forbidden"
    }
  ],
  "uuid": "185f39d1-019e-43a9-ab29-849613631253"
}
GET https://localhost:8080/api/latest/fleet/config

GET https://localhost:8080/api/latest/fleet/config 403 Forbidden (11ms)
{
  "message": "forbidden",
  "errors": [
    {
      "name": "base",
      "reason": "forbidden"
    }
  ],
  "uuid": "5b723ff2-db7b-4a8d-a0cc-bb495a7fae24"
}
Error: applying teams: GET /api/latest/fleet/config received status 403 forbidden: forbidden

More info

Before applying the config, fleetctl apply performs a GET /api/latest/fleet/config to check if MDM is enabled and configured. User GitOps is not authorized to read app's config.

Once fixed, add the corresponding ✅ in https://github.com/fleetdm/fleet/blob/main/docs/Using-Fleet/Permissions.md.

zayhanlon commented 1 year ago

1 pt to set up MDM 1 pt for unit tests

@georgekarrv if this becomes a higher priority item, we believe this can be handled by anyone on the MDM team as well. we are happy to take this though, just won't be top of our queue.

roperzh commented 1 year ago

@georgekarrv is out today, but I'm going to preemptively steal this one from g-cx and try to squeeze it in this sprint.

mna commented 1 year ago

Sounds like this should be a bug?

mna commented 1 year ago

@lucasmrod @roperzh I'm strongly tempted to simply allow gitops read access to the appconfig (obfuscated of course), like every fleet user. Anyone objects to do this?

EDIT: keep in mind that PATCH /config returns the updated appconfig, so technically it can already read the appconfig, it doesn't really add more access...

roperzh commented 1 year ago

@mna that sounds good to me 👍 unless @lucasmrod had another reason in mind.

lucasmrod commented 1 year ago

When adding the role we attempted the principle of least privilege:

The principle that a security architecture should be designed so that each entity is granted the minimum system resources and authorizations that the entity needs to perform its function.

(And also with the idea that this role is a "write-only" role.)

EDIT: keep in mind that PATCH /config returns the updated appconfig, so technically it can already read the appconfig, it doesn't really add more access...

Yeah, but it can get what it applied, right? Or probably can read all current fields even if it changed just one setting :)

So, am ok with adding read permissions to the config as long as product owners are on board and we would need to update the Permissions table.

PS: In the future, we should split /config into multiple endpoints/sections each with its own domain: agent settings, SSO, SMTP, MDM, etc.). Currently it's: you have access to all or nothing, we can exclude fields in responses but it's hacky (like we do for non global admins, e.g. https://github.com/fleetdm/fleet/issues/11266).

mna commented 1 year ago

@lucasmrod

probably can read all current fields even if it changed just one setting

It can if fact read everything without applying anything (with the "dry-run" option of the PATCH endpoint), so really, it doesn't add any more permission.

In the future, we should split /config into multiple endpoints/sections each with its own domain: agent settings, SSO, SMTP, MDM, etc.). Currently it's: you have access to all or nothing

Absolutely, that was the plan when we were discussing an eventual Fleet 5 with breaking changes. Not only from a security standpoint, but also from an implementation one, that app config is getting huge (and honestly, it's more than "configuration" at this point).

Given your reservations, though, I'll try another approach first that may not require changing the gitops permissions.

lucasmrod commented 1 year ago

It can if fact read everything without applying anything (with the "dry-run" option of the PATCH endpoint), so really, it doesn't add any more permission.

Given this am ok with explicitly giving it read access now :)

mna commented 1 year ago

@lucasmrod :D Yeah but actually I think it would go through read authorization before returning with the "dry-run" option, while with a actual change it wouldn't (just checked the code for those two branches). But anyway, as an attempt to keep the gitops user "write-only" not just as a theorical thing, I'll try something first (which wouldn't require much special-casing/complexity). If that doesn't work I'll resort to giving it read access.

mna commented 1 year ago

@lucasmrod @roperzh ok so this is not quite as straightforward as I hoped. Setting the macos setup assistant or the bootstrap packages for a team requires listing the teams in order to get the IDs (this ugly bit: https://github.com/fleetdm/fleet/blob/main/server/service/client.go#L427-L431). So now either approach don't work (the one I was thinking of was to ignore the fleetctl MDM check if it errors with Forbidden as anyway the actual API call will still make that check, the other approach was to give read access to appconfig to gitops but now it would need to read teams too and that's a stretch).

Soooo I think the comment in that linked code makes sense, Apply Team Specs could return the list of team IDs matched to the team names, but it's a breaking change (currently the endpoint doesn't return anything although it uses code 200). However, it is documented as a Contributor API so maybe we have more flexibility? Thoughts? /cc @lukeheath @georgekarrv

mna commented 1 year ago

@lucasmrod @roperzh and there's one more action that will cause issues in addition to listing teams, the way the bootstrap package is uploaded requires to make a read before it deletes it and uploads it: https://github.com/fleetdm/fleet/blob/main/server/service/client_mdm.go#L111-L141

I think it would make sense to add a fleetctl-specific "replace if needed" endpoint for that, a bit like what exists for macos setup assistant.

roperzh commented 1 year ago

I think it would make sense to add a fleetctl-specific "replace if needed" endpoint for that, a bit like what exists for macos setup assistant.

@mna is it possible to do that without having to upload the whole bootstrap package (~hundreds of MB) each time? the only motivation for that check is to keep the CI action duration in the ~seconds timeframe if you haven't changed the bootstrap package. This was an explicit ask

mna commented 1 year ago

@roperzh

the only motivation for that check is to keep the CI action duration in the ~seconds timeframe if you haven't changed the bootstrap package? This was an explicit ask

Gotcha, then I think we have a problem... Note that it will still download the whole bootstrap package though, and the read-delete-upload sequence is racy. But yeah if that was an explicit ask... I guess we could special-case a call to GET with a query string (e.g. ?for_update=1?) that would authorize as a write op? Thoughts, alternatives?

roperzh commented 1 year ago

Gotcha, then I think we have a problem... Note that it will still download the whole bootstrap package though, and the read-delete-upload sequence is racy. But yeah if that was an explicit ask... I guess we could special-case a call to GET with a query string (e.g. ?for_update=1?) that would authorize as a write op? Thoughts, alternatives?

@mna yeah it's a tough one :( . ~AFAIK it won't download the bootstrap package (GetBootstrapPackageMetadata returns a SHA we use to do the comparison) but agreed that's racy.~ EDIT: you meant the first download from the URL provided by the IT admin, sorry.

Maybe have a first call that gets a SHA and returns some kind of flag indicating if the bootstrap package needs to be uploaded? it's still kind of a read though... (and still racy)

roperzh commented 1 year ago

You make a good point about the download, maybe we could give your original idea a try and optimize later?

I think it would make sense to add a fleetctl-specific "replace if needed" endpoint for that, a bit like what exists for macos setup assistant.

mna commented 1 year ago

You make a good point about the download, maybe we could give your original idea a try and optimize later?

I mean, what troubles me is the hundreds of MB, I didn't know it could be that big. I think it makes sense in that case to try to move those bytes as little as possible/only when needed. The flow could be improved so that the download only happens if there's no bootstrap package already set for that team, but if one is set, we have to download it so we can compare the hashes (IIRC it was also an explicit ask that we compute the hash ourselves so the user doesn't have to provided it).

Given that and to prevent the scope getting out of control, I think I'd keep the same flow in place for now (we can optimize not downloading if possible later). The plan would be:

[x] Ignore "403 forbidden" errors in fleetctl that are done just as convenience checks before the actual API calls (so that gitops is not blocked from making the actual request);
[x] The GET bootstrap metadata authorization can be turned into a Write check ~~if the user's role is gitops (so no need for a query string, though a bit hacky yes)~~ turns out this would be tricky if the user had different roles for different teams, I'll go with the query string instead :( ;
[x] Add the response payload to the Apply Teams endpoint so that it returns a mapping of team names to team IDs, hopefully this is acceptable despite being a breaking change as this is a contributor API;
[x] Remove the call to ListTeams in fleetctl, since the Apply Teams response gives the required information;
[x] At this point the gitops user should be able to fleetctl apply any setting.

/cc @georgekarrv @lukeheath the plan as it stands ^ (this is no longer a "2" estimation-wise, more a 3-5, I'll update tomorrow if anything changes)

fleet-release commented 1 year ago

MacOS Setup fixed, Fleetctl now configures, Effortless, like breeze.

fleetdm / fleet