New Annotation for Constraint Templates' which require SyncData

[apeabody]

I propose (and can submit the PR) for adding a new Constraint Templates annotation for the 4 Constraint Templates which require SyncData:

• K8sPodDisruptionBudget
• K8sStorageClass
• K8sUniqueIngressHost
• K8sUniqueServiceSelector

Here is an example of what that would look like for K8sStorageClass:

  annotations:
    metadata.gatekeeper.sh/requiresSyncData: |-
      [
         {
            "group":"storage.k8s.io",
            "version":"v1",
            "kind":"StorageClass"
         }
      ]

For Constraint Templates that don't require SyncData, it should simply be omitted.

[maxsmythe]

Makes sense, we'd probably need some test to enforce that this label was properly applied. I'm not sure we can check the accuracy of the contents (since that would require parsing Rego), but we should at least be able to determine whether referential data is needed.

[apeabody]

Even fully parsing the Rego that might not be feasible for all templates, however we can at least check for the presence of data.inventory.

[ritazh]

@apeabody are you thinking the new annotation is just used for validation or it will also be used to sync the data?

[apeabody]

Hi @ritazh My thought is initially for validation, but longer term a sufficient source from which to potentially generate the required sync data configuration - similar to how some (but not all) of the Constraint Templates which rely on data.inventory have a sync.yaml file.

[maxsmythe]

I would push back on using constraint templates as a source-of-truth for which data to sync for security reasons. Otherwise, the ability to write a constraint template becomes equivalent to the ability to read any resource on the cluster (e.g. secrets).

[ekitson]

Can we enumerate the practical security concerns? Is it just Secrets? What about other well-known types?

Obviously someone could stick sensitive data in anything (ConfigMap, Deployment env var), but Secrets are the only type that come to mind that are explicitly intended for sensitive content.

[maxsmythe]

Secrets are a pretty big one. Right now the RBAC is arranged such that the ability to author CTs != the ability to read any data in the cluster.

Config Maps could also be sensitive. The contents of non-owned namespaces if running in a multi-tenant cluster (particularly if we ever develop namespace-scoped ConstraintTemplates). Contents of RBAC could be used to figure out which users/groups are the most valuable to compromise.

TBH I don't think it's possible to enumerate the security concerns. Since K8s is extensible and changing over time, we cannot know if something that is safe today or for a vanilla install is dangerous for a specific cluster or K8s release. The best we can do is avoid blurring the boundaries set by other security systems (in this case K8s RBAC), so users can continue to depend on their efficacy.

FWIW I don't think this hinders people building something more single-touch on top of this model. For instance, if someone (or some controller) took on the role of both CT author and G8r admin, they would be free to both create CTs and add syncs and it would be on them to avoid authoring CTs that allow sensitive data leaks. Crucially, such a blended role can exist next to lower-privileged roles that can only write CTs (though these CTs would have access to all synced data, they could not add more syncs).

[ekitson]

Thanks Max! Everything above makes sense. So I think Andrew's proposal has value for validation use cases alone. The aspirational idea of generating the syncs needs some detailed consideration around security, but I don't think it needs to block this initial proposal?

[maxsmythe]

Correct, not a blocker for the general idea of adding an annotation.

To re-emphasize, the idea of generating syncs is not per-se bad or a security risk if it happens at the right level (which is, IMO, a level above G8r).

[apeabody]

Thanks everyone - Per the discussion I've opened #251 which adds the annotation to the 4 template which require syncData (use data.inventory), and adds a simple require-sync test for the presence of both the new annotation and the existing sync.yaml file.