fleetdm / fleet

Open-source platform for IT, security, and infrastructure teams. (Linux, macOS, Chrome, Windows, cloud, data center)
https://fleetdm.com
Other
2.99k stars 414 forks source link

API: Validate that secrets are >= 32 characters #19904

Open RachelElysia opened 3 months ago

RachelElysia commented 3 months ago

Fleet version: <!-- Copy this from the "My account" page in the Fleet UI, or run fleetctl --version -->

Web browser and operating system:


💥  Actual behavior

Bug 1 The API (fleetctl) allows to create an enroll secret that is <32 characters in length

Screenshot 2024-06-20 at 12 14 09 PM

~~Bug 2 Gitops users can APPLY enroll secrets including deleting previous enroll secrets (because they can apply a config that overrides a secret) However, gitops users cannot GET/view enroll secrets Is this a bug?~~ No

🧑‍💻  Steps to reproduce

Bug 1

  1. Log into fleetctl as a admin/gitops admin
  2. Create a config.yml file to have a secret that is too short
  3. Use fleetctl gitops -f config.yml OR Use fleetctl apply -f config.yml
  4. Notice the secrets are overridden and can include a short secret

Bug 2

  1. Log into fleetctl as a gitops admin
  2. Be able to apply/override enroll secrets
  3. Not have a command to view secrets

🕯️ More info (optional)

N/A

RachelElysia commented 3 months ago

@getvictor saw these bugs with me so I'm removing the :reproduce label

Added :product to help confirm intended gitops behavior for both these bugs and decide if these are actually bugs

noahtalerman commented 3 months ago
  1. The API (fleetctl) allows to create an enroll secret that is <32 characters in length

  2. gitops users cannot GET/view enroll secrets

Hey @RachelElysia, thanks for erring towards bugs!

For (1), if I'm understanding correctly, the UI rejects secrets < 32 characters while the API/GitOps doesn't.

I think we should make this consistent w/ Fleet's way of doing validation: at the API level.

I think up to @rachaelshaw (API design DRI) if this is a bug or feature request. Rachael, I'm assigning you this bug to make the call.

noahtalerman commented 3 months ago

For (2) I think this is the expected behavior but I can't find it on the manage access doc page.

@getvictor when you get the chance, can you please confirm that this is the expected behavior and open a PR to the manage access docs page? Thanks!

Why I think it's the expected behavior: GitOps users don't have read access to most items because they're used for the best practice GitOps workflow which only needs write access. As a IT admin using the GitOps workflow, I'll have another account to log into the Fleet UI to read items.

getvictor commented 3 months ago

@noahtalerman For (1), adding validation may break existing users. We would need to communicate the change.

For (2), gitops can view the team secrets (because they are part of the team config), but not the global secrets. Should we just let gitops view both types of secrets for consistency?

I don't think this issue is a bug. It should be prioritized along with other stories.

rachaelshaw commented 3 months ago

@noahtalerman @RachelElysia I'll add this to Feature Fest; I'd consider this a feature request since there are other places we have guardrails in the UI that aren't present in the API, and I think this is worth more consideration re: @getvictor's point about it potentially being a breaking change.

Basically, where I stand with adding API validations in minor versions is: if something would be broken in Fleet anyway without those validations, then they're worth adding to existing endpoints even if it could potentially break some API workflows (because it just makes an existing problem clearer earlier on.) If it's a validation that's potentially a breaking change and things still work without it, it should probably wait until a major version bump unless there's a really good reason.

I think if we only validate >= 32 characters when the secrets are created it shouldn't break things for the way we'd expect most people to use this API, but also we never know exactly how these endpoints are being used, so probably falls into the second category. Just realized both the API endpoints for adding secrets replace the entire list, so adding validation would be breaking for anyone that currently has a too-short enroll secret in use.