Review the NAC restrictions and enforcements including second-level enforcement in the NonAdminBackup, NonAdminRestore and NonAdminBackupStorageLocation specifications

[mpryc]

Problem statement:
The current design and implementation for admin user enforcement of the spec values in NonAdminBackup and NonAdminRestore do not support second-level configuration options:

oadp-non-admin/docs/design/admin_control_over_spec.md

Line 27 in 019bb2a

- Allow admin users to set second level defaults (for example, NonAdminBackup `spec.backupSpec.labelSelector` can have a custom default value, but not just `spec.backupSpec.labelSelector.matchLabels`)

It is necessary to review the need for second-level enforcement for these objects and adjust the design and implementation accordingly if required.

As an example, the NonAdminBackupStorageLocation object requires second-level enforcement to allow the admin user to restrict users to a specific S3 region while still permitting them to use their own buckets within that region. This enforcement must be implemented through the second-level configuration in the spec. This is explained in the: #139 (comment)

[mpryc]

@shubham-pampattiwar @mateusoliveira43 @weshayutin

We should revisit restrictions and enforcement for Non Admin Objects.

While looking at our discussion around second-level enforcement of the NABSL I looked into that area and I think we have to double check if any of the values which user may set won't allow user to access data which the user should not have permissions to access. An example would be the Backup Hook which uses Resouces that are in fact allowing to use IncludedNamespaes or even going further they do allow to use PreHooks that actually allows to define any executable in a process which is owned by Velero pod not a non-admin user. This could potentially result in users performing actions with elevated privileges. I think the IncludedNamespaces are the top level filter and what is above will limit the use of resources outside of namespaces within that filter, but it would be great to go on every spec field and see which ones can do harm when overriden by the user.

Here is my suggestion which we should take action on:

Restrictions and Enforcement for Non-Admin Objects

The following objects should have well-defined validation, restriction, and enforcement mechanisms:

• NonAdminBackup
• NonAdminRestore
• NonAdminBackupStorageLocation

1. CRD Level

Context: These CRDs specs are used from Velero objects, so direct control is limited to what Velero provides.

• CRD Validation:

  • Ensures compliance with the schema definition.
  • Examples:
    • Data type validation (e.g., string, integer).
    • Structural validation (e.g., required fields).

• CRD Enforcement:

  • Enforced constraints like allowed enum values are part of the schema definition. Static Enforcement can not be modified by admin nor the controller and it's not calculated within controller runtime.
  • Example:
    • Restricting a field to specific allowed values, within spec we may not have any of those enforcement available for the user.

---

2. NAC Controller Level

Context: The controller applies additional runtime restrictions and enforcement for Non-Admin objects.

• Controller Restriction:

  • Configuration options that users (including administrators) cannot modify. Those options can be set dynamically by controller to ensure it's properly set in the resulting Velero Object.
  • Example:
    • Restricting Backup IncludedNamespace to only the user's namespace.
    • Restricting Non Admin Restore to only reference Non Admin Backup within user's namespace.
    • Restricting User to only specify and reference Non Admin BackupStorageLocation from within user's namespace.

• Controller Enforcement:

  • Similar to above Restrictions but allows administrators to override specific values. Admins could specify certain global policies that affect how the controller reconciles objects. User is not allowed to modify those options.

  • Example:

    • A PostHooks defined by the admin to execute action on every Restore taken by the user. This example is valid only if we decide that such PostHooks is not allowed by the user to define, otherwise if we decide on the controller level that such PostHook is available to the user then it will become Cluster Admin Level enforcement.

---

3. Cluster Admin Level

Context: Cluster administrators define global policies that users cannot override.

• Enforcement:
  • Admins enforce spec values that users can not override. Normally those values are available to the user. Those rules can be applied to any spec field other then NAC Controller restrictions, but also must comply with the CRD level validations and
    enforcement, so the admin can not set value which is out of CRD range.
  • Example:
    • Setting a global policy to enforce a specific region for the NonAdminBackupStorageLocation across all Non-Admin objects.

[shubham-pampattiwar]

I think we are on the right track. We just need to review the spec sheets for these objects and see which ones need more restrictions/validations and which ones don't.

[mpryc]

If I may ask to review the design change proposal:
#156

[shubham-pampattiwar]

Backup API Review:

• Admin enf: Admin enforceable
• Rest non-admin: Restricted for non-admins
• Admin non-enf: Admin non-enforceable

spec:
  <Admin enf>
  csiSnapshotTimeout: 10m
  
  <Admin enf>
  itemOperationTimeout: 4h
  
  <Admin enf, can be restricted for non-admin users>
  resourcePolicy:
    kind: configmap
    name: resource-policy-configmap
  
  <Restricted for non-admins>
  includedNamespaces:
  - '*'
  <Restricted for non-admins>
  excludedNamespaces:
  - some-namespace
  
  <Admin enf>
  includedResources:
  - '*'
  <Admin enf>
  excludedResources:
  - storageclasses.storage.k8s.io
  
  <Admin enf>
  orderedResources:
    pods: mysql/mysql-cluster-replica-0,mysql/mysql-cluster-replica-1,mysql/mysql-cluster-source-0
    persistentvolumes: pvc-87ae0832-18fd-4f40-a2a4-5ed4242680c4,pvc-63be1bb0-90f5-4629-a7db-b8ce61ee29b3
  
  <Admin enf>
  includeClusterResources: null
  
  <Admin enf>
  excludedClusterScopedResources: {}
  
  <Admin enf>
  includedClusterScopedResources: {}
  
  <Admin enf>
  excludedNamespaceScopedResources: {}
  
  <Admin enf>
  includedNamespaceScopedResources: {}
  
  \<Admin enf>
  labelSelector:
    matchLabels:
      app: velero
      component: server
  
  <Admin enf>
  # orLabelSelectors as well as labelSelector cannot co-exist, only one of them can be specified in the backup request
  orLabelSelectors:
  - matchLabels:
      app: velero
  - matchLabels:
      app: data-protection
  
  <Admin enf>
  snapshotVolumes: null
  
  <Admin enf>
  storageLocation: aws-primary
  
  <Admin enf>
  volumeSnapshotLocations:
    - aws-primary
    - gcp-primary
  <Admin enf>
  ttl: 24h0m0s
  
  <Admin enf>
  defaultVolumesToFsBackup: true
  <Admin enf>
  snapshotMoveData: true
  <Admin enf>
  datamover: velero
  <Admin enf>
  uploaderConfig:
      # ParallelFilesUpload is the number of files parallel uploads to perform when using the uploader.
      parallelFilesUpload: 10
  <Admin non-enf>
  hooks:
    # Array of hooks that are applicable to specific resources. Optional.
    resources:
      -
        # Name of the hook. Will be displayed in backup log.
        name: my-hook
        # Array of namespaces to which this hook applies. If unspecified, the hook applies to all
        # namespaces. Optional.
        includedNamespaces:
        - '*'
        # Array of namespaces to which this hook does not apply. Optional.
        excludedNamespaces:
        - some-namespace
        # Array of resources to which this hook applies. The only resource supported at this time is
        # pods.
        includedResources:
        - pods
        # Array of resources to which this hook does not apply. Optional.
        excludedResources: []
        # This hook only applies to objects matching this label selector. Optional.
        labelSelector:
          matchLabels:
            app: velero
            component: server
        # An array of hooks to run before executing custom actions. Only "exec" hooks are supported.
        pre:
          -
            # The type of hook. This must be "exec".
            exec:
              # The name of the container where the command will be executed. If unspecified, the
              # first container in the pod will be used. Optional.
              container: my-container
              # The command to execute, specified as an array. Required.
              command:
                - /bin/uname
                - -a
              # How to handle an error executing the command. Valid values are Fail and Continue.
              # Defaults to Fail. Optional.
              onError: Fail
              # How long to wait for the command to finish executing. Defaults to 30 seconds. Optional.
              timeout: 10s
        # An array of hooks to run after all custom actions and additional items have been
        # processed. Only "exec" hooks are supported.
        post:
          # Same content as pre above.

[mateusoliveira43]

@stillalearner tested scenarios with namespace mapping:

Scenario 1: (Namespace mapping , No label selectors)

Create non admin user user1
Create non admin namespace for user1 : nan1
Deploy app in nan1
Perform NAB
Perform NAR with nan1: nan2( doesnt exist )

Result: nan2 got created, restore successful,
user1 has access to nan2 & nan1 now.

Scenario 2: (Namespace mapping + label selectors)

Create non admin user user1
Create non admin namespace for user1 : nan1
Deploy app in nan1
Perform NAB with label selectors
Perform NAR with nan1: nan3( doesnt exist )

Result: nan3 got created, restore successful,
user1 has access to only nan1.

Problem

if non admin user can use namespace mapping, it may be able to create/restore to namespace its does not own