Update ADR12 with notes for a third option

malexmave · malexmave · commit c7ffe40d3871 · 2022-08-05T14:12:49.000+02:00
Signed-off-by: Max Maass &lt;max.maass@iteratec.com&gt;
diff --git a/docs/adr/adr_0012.md b/docs/adr/adr_0012.md
@@ -118,17 +118,6 @@ This fixed namespace mode has been added to the `ClusterScanCompletionHook` but
 
 Can lead to conflicts between teams if a scan of "team 1" fails because of an error in the `ClusterScanCompletionHook` managed by a "central cluster team". Members of "team 1" will likely not even have access to the namespace the failing hooks run in and won't be able to debug / fix these issues by themselves.
 
-#### Advantages of this Approach
-
-TBD
-
-#### Disadvantages of this Approach
-
-TBD
-
-#### Security Considerations
-
-TBD
 
 ### Proposal 2: Distinct Cluster-wide and Namespace-local Resources
 
@@ -139,10 +128,12 @@ This alternative proposal uses the same cluster-wide CRDs mentioned above, but a
 The `ClusterScan` is almost identical to the `Scan` type, however, it includes an additional field called `executionNamespace` that controls in which namespace it is scheduled. The operator will schedule it in to the namespace, or throw an error if the namespace does not exist or the operator cannot schedule into it for any reason. They will only trigger a `ClusterScanCompletionHook`, only respect `ClusterCascadingRule`, and in all other ways be kept separate from non-Cluster-resources, with one major exception: access to namespace-specific ConfigMaps and Secrets. Here, this access is desireable, as it allows teams to customize the behavior of cluster-managed scans to their own situation (e.g., provide a cluster-wide ZAP scan with a namespace-specific authentication configuration for the microservice). The same consideration from the other proposal apply for ensuring that the secrets and configMaps are available.
 
 **OPEN QUESTION:** From a threat perspective (what happens if an unauthorized user can create cluster-wide scans and schedule them into other teams' namespaces?), it may be desireable to create a specific label or annotation for secrets and configmaps that has to be present for them to be accessible to cluster-wide scans, and to hide other such resources from them for security reasons, as a defense-in-depth measure against incorrectly configured clusters (we really need to threat model this feature). Is this something that is feasible with a k8s operator?
+=> This is possible, either in the operator, or using a validating webhook (the former being preferred).
 
 These CRDs will likely not be used a lot by human operators, but they are helpful for use with the autodiscovery feature, which can use them to schedule scans into namespaces without interfering with any scans, hooks or other features the owners of the namespace are using. Pods scheduled from scans like this should also receive a distinct label or annotation from regular scans in that namespace, to allow for fine-grained network policies (e.g., "Cluster scans are allowed to access this specific server that caches nuclei templates for them, but regular scans are not").
 
 **OPEN QUESTION:** Can people with rights on their own namespace, but not other namespaces, see ClusterScans that are scheduled in their namespace? Should they be able to?
+=> This should be blocked.
 
 #### `ClusterScanType`
 
@@ -160,23 +151,6 @@ Other than being cluster scoped, `ClusterCascadingRule`s are identical to the ex
 
 `ClusterScanCompletionHook` behave mostly like their namespaced counterpart: when the parser for a `ClusterScan` completes, the operator fetches the list of `ClusterScanCompletionHook`(respecting the hookSelector of the `ClusterScan`), and processes them the same way it would regular `ScanCompletionHook`s (prioritization etc.). The concept of a "fixed namespace mode" from the other proposal can be maintained, if desireable.
 
-#### Advantages of this Approach
-
-This approach has a number of advantages over the other:
-
-- A stricter separation between cluster-wide and namespace-scoped resources reduces the likelihood of confusion behavior ("Why is this scan scheduling if I don't have the ScanType installed?")
-- This separation also improves security (I cannot trivially create a cluster-wide hook that siphons off scan data from other namespaces)
-- The concept of limiting access to secrets and ConfigMaps by default, and requiring a special label to be set to allow access for a `ClusterScan` makes data exfiltration from other namespaces more difficult in a scenario where unauthorized people manage to introduce a `ClusterScan` (e.g., because the cluster is misconfigured or there is a bug in the SCB). This significantly reduces the blast radius of such an attack.
-- In general, it is designed to allow cluster-wide scans to operate with minimal disruption of the rest of the cluster, e.g., in a scenario where there is both a cluster-wide autodiscovery and a namespace-specific team-managed scan infrastructure.
-
-
-#### Disadvantages of this Approach
-
-TBD
-
-#### Security Considerations
-
-TBD
 
 ### Changes to Existing Helm Charts
 
@@ -188,8 +162,22 @@ The following (not so) edge cases should be considered:
 1. Scanners and Hooks with namespaced resources (config maps, e.g. amass, zap-advanced...) should also contain the `clusterWide` parameter, when installed with it the installation should fail with guidance and links to a (non scanner specific) doc site which details on how a namespaced ScanType can be adjusted to as a ClusterScanType, e.g. on how to set up a init container to compensate the missing ConfigMaps, see [Issues with ConfigMap / Secret Mounts](#issues-with-configmap--secret-mounts).
 2. When a hook is installed in `clusterWide` mode and the `executionNamespace` being set (should be exposed as a value in all hook helm charts), the install should fail if the helm install namespace != `executionNamespace`. This should ensure that all namespaced resources (ConfigMaps, Secrets) are present in the `executionNamespace.`
 
+
+--- TODO: Put proposal 3 here - like proposal 2, but control execution based on an attribute in the Scan CRD (default to namespace-only, switch to cluster-only on demand)
+--- TODO: Also include a more detailed description of the planned RBAC implementation
+
 ## Decision
-TBD
+We use Proposal 3, because:
+- The strict separation in Proposal 2 makes the system hard debug (pods spawn randomly in your namespace and you don't know why).
+- RBAC becomes more difficult in Proposal 2
+- Currently, jobs have a child relationship with the underlying scan. This would have to be broken in proposal 2 (as the child relationship only works within a namespace), leading to more work for the cleanup. Prop 1 and 3 also have this problem, but a lot less so (only for scan completion hooks with fixed namespace mode).
+- Proposal 1 can also lead to hard-to-understand failure modes if unexpected ScanTypes are used (why can the teams own ZAP installation break the global ZAP installation of autodiscovery?)
+- Proposal 3 allows better visibility and debugging on failing scans
+
+### Security Considerations
+
+- RBAC needs to be non-broken :) (we assign write rights on global types only to admins by default). Being able to change who can modify cluster-wide resources is equivalent to full read and execute access to the entire cluster.
+- `Cluster*` resources can be inspected by anyone, don't put secrets in there (if you need to keep a secret for a hook safe, use the target namespace mode of the cluster scan hook)
 
 ## Consequences
 
