diff --git a/api/v1/clusterextension_types.go b/api/v1/clusterextension_types.go index 6de62b0e12..72dab3b28b 100644 --- a/api/v1/clusterextension_types.go +++ b/api/v1/clusterextension_types.go @@ -49,12 +49,19 @@ const ( // ClusterExtensionSpec defines the desired state of ClusterExtension type ClusterExtensionSpec struct { // namespace is a reference to a Kubernetes namespace. - // This is the namespace in which the provided ServiceAccount must exist. - // It also designates the default namespace where namespace-scoped resources + // + // This designates the default namespace where namespace-scoped resources // for the extension are applied to the cluster. // Some extensions may contain namespace-scoped resources to be applied in other namespaces. // This namespace must exist. // + // + // This is also the namespace of the referenced service account. + // + // + // This is also the namespace of the referenced service account, if specified. + // + // // namespace is required, immutable, and follows the DNS label standard // as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), // start and end with an alphanumeric character, and be no longer than 63 characters @@ -69,12 +76,24 @@ type ClusterExtensionSpec struct { // serviceAccount is a reference to a ServiceAccount used to perform all interactions // with the cluster that are required to manage the extension. + // // The ServiceAccount must be configured with the necessary permissions to perform these interactions. - // The ServiceAccount must exist in the namespace referenced in the spec. + // // serviceAccount is required. + // // - // +kubebuilder:validation:Required - ServiceAccount ServiceAccountReference `json:"serviceAccount"` + // + // If serviceAccount is specified, OLM will authenticate as that service account. + // Otherwise, operator-controller will authenticate as: + // - User: "olm:clusterextension:" + // - Group: "olm:clusterextensions" + // + // The authenticated user must be configured with the necessary permissions to perform these interactions. + // + // + // + // + ServiceAccount ServiceAccountReference `json:"serviceAccount,omitzero"` // source is a required field which selects the installation source of content // for this ClusterExtension. Selection is performed by setting the sourceType. @@ -374,14 +393,12 @@ type CatalogFilter struct { UpgradeConstraintPolicy UpgradeConstraintPolicy `json:"upgradeConstraintPolicy,omitempty"` } -// ServiceAccountReference identifies the serviceAccount used fo install a ClusterExtension. +// ServiceAccountReference identifies the serviceAccount name used to manage a ClusterExtension. type ServiceAccountReference struct { // name is a required, immutable reference to the name of the ServiceAccount // to be used for installation and management of the content for the package // specified in the packageName field. // - // This ServiceAccount must exist in the installNamespace. - // // name follows the DNS subdomain standard as defined in [RFC 1123]. // It must contain only lowercase alphanumeric characters, // hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -404,7 +421,7 @@ type ServiceAccountReference struct { // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="name is immutable" // +kubebuilder:validation:XValidation:rule="self.matches(\"^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$\")",message="name must be a valid DNS1123 subdomain. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, and be no longer than 253 characters" // +kubebuilder:validation:Required - Name string `json:"name"` + Name string `json:"name,omitempty"` } // PreflightConfig holds the configuration for the preflight checks. If used, at least one preflight check must be non-nil. diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 7425c7b66b..e9c0b09576 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -36,6 +36,7 @@ import ( k8slabels "k8s.io/apimachinery/pkg/labels" k8stypes "k8s.io/apimachinery/pkg/types" apimachineryrand "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/apiserver/pkg/authentication/user" "k8s.io/client-go/discovery" "k8s.io/client-go/discovery/cached/memory" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" @@ -643,8 +644,7 @@ func setupHelm( if err != nil { return fmt.Errorf("unable to create core client: %w", err) } - tokenGetter := authentication.NewTokenGetter(coreClient, authentication.WithExpirationDuration(1*time.Hour)) - clientRestConfigMapper := action.ServiceAccountRestConfigMapper(tokenGetter) + clientRestConfigMapper := action.ServiceAccountRestConfigMapper() if features.OperatorControllerFeatureGate.Enabled(features.SyntheticPermissions) { clientRestConfigMapper = action.SyntheticUserRestConfigMapper(clientRestConfigMapper) } @@ -671,7 +671,16 @@ func setupHelm( // determine if PreAuthorizer should be enabled based on feature gate var preAuth authorization.PreAuthorizer if features.OperatorControllerFeatureGate.Enabled(features.PreflightPermissions) { - preAuth = authorization.NewRBACPreAuthorizer(mgr.GetClient()) + preAuth = authorization.NewRBACPreAuthorizer(mgr.GetClient(), func(ext *ocv1.ClusterExtension) (*user.DefaultInfo, error) { + if ext.Spec.ServiceAccount.Name == "" && features.OperatorControllerFeatureGate.Enabled(features.SyntheticPermissions) { + syntheticConfig := authentication.SyntheticImpersonationConfig(*ext) + return &user.DefaultInfo{Name: syntheticConfig.UserName, Groups: syntheticConfig.Groups}, nil + } else if ext.Spec.ServiceAccount.Name == "" || ext.Spec.Namespace == "" { + return nil, errors.New("service account name and namespace must be specified") + } + saConfig := authentication.ServiceAccountImpersonationConfig(*ext) + return &user.DefaultInfo{Name: saConfig.UserName, Groups: saConfig.Groups}, nil + }) } cm := contentmanager.NewManager(clientRestConfigMapper, mgr.GetConfig(), mgr.GetRESTMapper()) diff --git a/config/samples/olm_v1_clusterextension.yaml b/config/samples/olm_v1_clusterextension.yaml index 14c8e167e0..22f66158f2 100644 --- a/config/samples/olm_v1_clusterextension.yaml +++ b/config/samples/olm_v1_clusterextension.yaml @@ -4,11 +4,13 @@ kind: Namespace metadata: name: argocd --- -apiVersion: v1 -kind: ServiceAccount -metadata: - name: argocd-installer - namespace: argocd +# NOTE: The ServiceAccount resource is intentionally NOT created here. +# OLM v1 uses Kubernetes impersonation and does not require the ServiceAccount to exist. +# For security reasons, you should NOT create a ServiceAccount resource for the installer, +# as it would be highly privileged and could be mounted by other pods in the namespace. +# Instead, only the RBAC resources (ClusterRole, ClusterRoleBinding, Role, RoleBinding) +# are created, which reference the ServiceAccount name "argocd-installer". +# OLM will impersonate that ServiceAccount and be subject to these RBAC permissions. --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding diff --git a/docs/api-reference/olmv1-api-reference.md b/docs/api-reference/olmv1-api-reference.md index 317b46a00c..b5f2207a81 100644 --- a/docs/api-reference/olmv1-api-reference.md +++ b/docs/api-reference/olmv1-api-reference.md @@ -339,8 +339,8 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `namespace` _string_ | namespace is a reference to a Kubernetes namespace.
This is the namespace in which the provided ServiceAccount must exist.
It also designates the default namespace where namespace-scoped resources
for the extension are applied to the cluster.
Some extensions may contain namespace-scoped resources to be applied in other namespaces.
This namespace must exist.
namespace is required, immutable, and follows the DNS label standard
as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-),
start and end with an alphanumeric character, and be no longer than 63 characters
[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 63
Required: \{\}
| -| `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions
with the cluster that are required to manage the extension.
The ServiceAccount must be configured with the necessary permissions to perform these interactions.
The ServiceAccount must exist in the namespace referenced in the spec.
serviceAccount is required. | | Required: \{\}
| +| `namespace` _string_ | namespace is a reference to a Kubernetes namespace.
This designates the default namespace where namespace-scoped resources
for the extension are applied to the cluster.
Some extensions may contain namespace-scoped resources to be applied in other namespaces.
This namespace must exist.

This is also the namespace of the referenced service account.


This is also the namespace of the referenced service account, if specified.

namespace is required, immutable, and follows the DNS label standard
as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-),
start and end with an alphanumeric character, and be no longer than 63 characters
[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 63
Required: \{\}
| +| `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions
with the cluster that are required to manage the extension.

The ServiceAccount must be configured with the necessary permissions to perform these interactions.
serviceAccount is required.


If serviceAccount is specified, OLM will authenticate as that service account.
Otherwise, operator-controller will authenticate as:
- User: "olm:clusterextension:"
- Group: "olm:clusterextensions"
The authenticated user must be configured with the necessary permissions to perform these interactions.


| | | | `source` _[SourceConfig](#sourceconfig)_ | source is a required field which selects the installation source of content
for this ClusterExtension. Selection is performed by setting the sourceType.
Catalog is currently the only implemented sourceType, and setting the
sourcetype to "Catalog" requires the catalog field to also be defined.
Below is a minimal example of a source definition (in yaml):
source:
sourceType: Catalog
catalog:
packageName: example-package | | Required: \{\}
| | `install` _[ClusterExtensionInstallConfig](#clusterextensioninstallconfig)_ | install is an optional field used to configure the installation options
for the ClusterExtension such as the pre-flight check configuration. | | | | `config` _[ClusterExtensionConfig](#clusterextensionconfig)_ | config is an optional field used to specify bundle specific configuration
used to configure the bundle. Configuration is bundle specific and a bundle may provide
a configuration schema. When not specified, the default configuration of the resolved bundle will be used.
config is validated against a configuration schema provided by the resolved bundle. If the bundle does not provide
a configuration schema the final manifests will be derived on a best-effort basis. More information on how
to configure the bundle should be found in its end-user documentation.
| | | @@ -439,7 +439,7 @@ _Appears in:_ -ServiceAccountReference identifies the serviceAccount used fo install a ClusterExtension. +ServiceAccountReference identifies the serviceAccount name used to manage a ClusterExtension. @@ -448,7 +448,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `name` _string_ | name is a required, immutable reference to the name of the ServiceAccount
to be used for installation and management of the content for the package
specified in the packageName field.
This ServiceAccount must exist in the installNamespace.
name follows the DNS subdomain standard as defined in [RFC 1123].
It must contain only lowercase alphanumeric characters,
hyphens (-) or periods (.), start and end with an alphanumeric character,
and be no longer than 253 characters.
Some examples of valid values are:
- some-serviceaccount
- 123-serviceaccount
- 1-serviceaccount-2
- someserviceaccount
- some.serviceaccount
Some examples of invalid values are:
- -some-serviceaccount
- some-serviceaccount-
[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 253
Required: \{\}
| +| `name` _string_ | name is a required, immutable reference to the name of the ServiceAccount
to be used for installation and management of the content for the package
specified in the packageName field.
name follows the DNS subdomain standard as defined in [RFC 1123].
It must contain only lowercase alphanumeric characters,
hyphens (-) or periods (.), start and end with an alphanumeric character,
and be no longer than 253 characters.
Some examples of valid values are:
- some-serviceaccount
- 123-serviceaccount
- 1-serviceaccount-2
- someserviceaccount
- some.serviceaccount
Some examples of invalid values are:
- -some-serviceaccount
- some-serviceaccount-
[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 253
Required: \{\}
| #### SourceConfig diff --git a/docs/draft/howto/use-synthetic-permissions.md b/docs/draft/howto/use-synthetic-permissions.md index 9820ad2b6e..38e1cf1313 100644 --- a/docs/draft/howto/use-synthetic-permissions.md +++ b/docs/draft/howto/use-synthetic-permissions.md @@ -8,7 +8,7 @@ Synthetic user permissions enables fine-grained configuration of ClusterExtensio User can not only configure RBAC permissions governing the management across all ClusterExtensions, but also on a case-by-case basis. -### Run OLM v1with Experimental Features Enabled +### Run OLM v1 with Experimental Features Enabled ```terminal title=Enable Experimental Features in a New Kind Cluster make run-experimental @@ -20,10 +20,11 @@ kubectl rollout status -n olmv1-system deployment/operator-controller-controller ### How does it work? -When managing a ClusterExtension, OLM will assume the identity of user "olm:clusterextensions:" -and group "olm:clusterextensions" limiting Kubernetes API access scope to those defined for this user and group. These -users and group do not exist beyond being defined in Cluster/RoleBinding(s) and can only be impersonated by clients with - `impersonate` verb permissions on the `users` and `groups` resources. +When managing a ClusterExtension that does not specify a service account, OLM will assume the identity of user +"olm:clusterextension:" and group "olm:clusterextensions" limiting Kubernetes API access scope +to those defined for this user and group. These users and group do not exist beyond being defined in +Cluster/RoleBinding(s) and can only be impersonated by clients with `impersonate` verb permissions on the `users` and +`groups` resources. ### Demo @@ -50,7 +51,7 @@ subjects: name: "olm:clusterextensions" ``` -##### Scoped olm:clusterextension group + Added perms on specific extensions +##### Scoped olm:clusterextensions group + Added perms on specific extensions Give ClusterExtension management group broad permissions to manage ClusterExtensions denying potentially dangerous permissions such as being able to read cluster wide secrets: diff --git a/docs/howto/derive-service-account.md b/docs/howto/derive-service-account.md index c1c1204da2..93bad8b975 100644 --- a/docs/howto/derive-service-account.md +++ b/docs/howto/derive-service-account.md @@ -300,12 +300,20 @@ Therefore, the following permissions must be given to the installer service acco Once the installer service account required cluster-scoped and namespace-scoped permissions have been collected: 1. Create the installation namespace -2. Create the installer `ServiceAccount` -3. Create the installer `ClusterRole` -4. Create the `ClusterRoleBinding` between the installer service account and its cluster role -5. Create the installer `Role` -6. Create the `RoleBinding` between the installer service account and its role -7. Create the `ClusterExtension` +2. Create the installer `ClusterRole` +3. Create the `ClusterRoleBinding` between the installer service account and its cluster role +4. Create the installer `Role` +5. Create the `RoleBinding` between the installer service account and its role +6. Create the `ClusterExtension` + +!!! important "Do Not Create the ServiceAccount Resource" + OLM v1 uses Kubernetes impersonation and does not require the ServiceAccount to exist as an actual resource. + For security reasons, you should **NOT** create a ServiceAccount resource for the installer, + as it would be highly privileged and could be mounted by other pods in the namespace. + + Instead, only create the RBAC resources (ClusterRole, ClusterRoleBinding, Role, RoleBinding) + that reference the ServiceAccount name. OLM will impersonate that ServiceAccount and be subject + to these RBAC permissions without the ServiceAccount resource actually existing in the cluster. A manifest with the full set of resources can be found [here](https://github.com/operator-framework/operator-controller/blob/main/config/samples/olm_v1_clusterextension.yaml). diff --git a/docs/tutorials/upgrade-extension.md b/docs/tutorials/upgrade-extension.md index b8b2aa5aad..9fb31aff2c 100644 --- a/docs/tutorials/upgrade-extension.md +++ b/docs/tutorials/upgrade-extension.md @@ -33,12 +33,6 @@ saving it to a local file and then running `kubectl apply -f FILENAME`: metadata: name: argocd --- - apiVersion: v1 - kind: ServiceAccount - metadata: - name: argocd-installer - namespace: argocd - --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: @@ -376,4 +370,4 @@ kubectl get clusterextension argocd -o jsonpath-as-json="{.status.install}" After your upgrade, the contents of the `kubectl.kubernetes.io/last-applied-configuration` annotation field will differ depending on your method of upgrade. If you apply a new ClusterExtension manifest as in the first method shown, the last applied configuration will show the new version since we replaced the existing manifest. If you use the patch - method or `kubectl edit clusterextension`, then the last applied configuration will show the old version. \ No newline at end of file + method or `kubectl edit clusterextension`, then the last applied configuration will show the old version. diff --git a/hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml b/hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml index 7eb5a7082b..01bb6c52d5 100644 --- a/hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml +++ b/hack/demo/resources/synthetic-user-perms/argocd-clusterextension.yaml @@ -4,8 +4,6 @@ metadata: name: argocd-operator spec: namespace: argocd-system - serviceAccount: - name: "olm.synthetic-user" source: sourceType: Catalog catalog: diff --git a/hack/demo/synthetic-user-cluster-admin-demo-script.sh b/hack/demo/synthetic-user-cluster-admin-demo-script.sh index 4790e46e77..910e708ec7 100755 --- a/hack/demo/synthetic-user-cluster-admin-demo-script.sh +++ b/hack/demo/synthetic-user-cluster-admin-demo-script.sh @@ -20,7 +20,7 @@ bat --style=plain ${DEMO_RESOURCE_DIR}/synthetic-user-perms/cegroup-admin-bindin # apply cluster role binding kubectl apply -f ${DEMO_RESOURCE_DIR}/synthetic-user-perms/cegroup-admin-binding.yaml -# install cluster extension - for now .spec.serviceAccount = "olm.synthetic-user" +# install cluster extension without specifying .spec.serviceAccount bat --style=plain ${DEMO_RESOURCE_DIR}/synthetic-user-perms/argocd-clusterextension.yaml # apply cluster extension diff --git a/hack/tools/crd-generator/main.go b/hack/tools/crd-generator/main.go index 9687489f45..a34398fcee 100644 --- a/hack/tools/crd-generator/main.go +++ b/hack/tools/crd-generator/main.go @@ -23,6 +23,7 @@ import ( "log" "os" "regexp" + "slices" "strings" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -136,7 +137,7 @@ func runGenerator(args ...string) { if channel == StandardChannel && strings.Contains(version.Name, "alpha") { channelCrd.Spec.Versions[i].Served = false } - version.Schema.OpenAPIV3Schema.Properties = opconTweaksMap(channel, version.Schema.OpenAPIV3Schema.Properties) + version.Schema.OpenAPIV3Schema.Properties = opconTweaksMap(channel, version.Schema.OpenAPIV3Schema) } conv, err := crd.AsVersion(*channelCrd, apiextensionsv1.SchemeGroupVersion) @@ -179,10 +180,11 @@ func runGenerator(args ...string) { } } -func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps { +func opconTweaksMap(channel string, obj *apiextensionsv1.JSONSchemaProps) map[string]apiextensionsv1.JSONSchemaProps { + props := obj.Properties for name := range props { jsonProps := props[name] - p := opconTweaks(channel, name, jsonProps) + p := opconTweaks(channel, name, obj, jsonProps) if p == nil { delete(props, name) } else { @@ -194,7 +196,7 @@ func opconTweaksMap(channel string, props map[string]apiextensionsv1.JSONSchemaP // Custom Opcon API Tweaks for tags prefixed with `") { return nil @@ -210,6 +212,24 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche numExpressions := strings.Count(jsonProps.Description, validationPrefix) numValid := 0 if numExpressions > 0 { + requiredRe := regexp.MustCompile(validationPrefix + "Required>") + requiredMatches := requiredRe.FindAllStringSubmatch(jsonProps.Description, -1) + for range requiredMatches { + numValid += 1 + if !slices.Contains(parent.Required, name) { + parent.Required = append(parent.Required, name) + } + slices.Sort(parent.Required) + } + + optionalRe := regexp.MustCompile(validationPrefix + "Optional>") + optionalMatches := optionalRe.FindAllStringSubmatch(jsonProps.Description, -1) + for range optionalMatches { + numValid += 1 + parent.Required = slices.DeleteFunc(parent.Required, func(s string) bool { return s == name }) + slices.Sort(parent.Required) + } + enumRe := regexp.MustCompile(validationPrefix + "Enum=([A-Za-z;]*)>") enumMatches := enumRe.FindAllStringSubmatch(jsonProps.Description, 64) for _, enumMatch := range enumMatches { @@ -246,19 +266,34 @@ func opconTweaks(channel string, name string, jsonProps apiextensionsv1.JSONSche jsonProps.Description = formatDescription(jsonProps.Description, channel, name) if len(jsonProps.Properties) > 0 { - jsonProps.Properties = opconTweaksMap(channel, jsonProps.Properties) + jsonProps.Properties = opconTweaksMap(channel, &jsonProps) } else if jsonProps.Items != nil && jsonProps.Items.Schema != nil { - jsonProps.Items.Schema = opconTweaks(channel, name, *jsonProps.Items.Schema) + jsonProps.Items.Schema = opconTweaks(channel, name, &jsonProps, *jsonProps.Items.Schema) } return &jsonProps } func formatDescription(description string, channel string, name string) string { - startTag := "" - endTag := "" - if channel == StandardChannel && strings.Contains(description, startTag) { - regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*` + startTagStandard := "" + endTagStandard := "" + if channel == ExperimentalChannel && strings.Contains(description, startTagStandard) { + regexPattern := `\n*` + regexp.QuoteMeta(startTagStandard) + `(?s:(.*?))` + regexp.QuoteMeta(endTagStandard) + `\n*` + re := regexp.MustCompile(regexPattern) + match := re.FindStringSubmatch(description) + if len(match) != 2 { + log.Fatalf("Invalid tag for %s", name) + } + description = re.ReplaceAllString(description, "\n\n") + } else { + description = strings.ReplaceAll(description, startTagStandard, "") + description = strings.ReplaceAll(description, endTagStandard, "") + } + + startTagExperimental := "" + endTagExperimental := "" + if channel == StandardChannel && strings.Contains(description, startTagExperimental) { + regexPattern := `\n*` + regexp.QuoteMeta(startTagExperimental) + `(?s:(.*?))` + regexp.QuoteMeta(endTagExperimental) + `\n*` re := regexp.MustCompile(regexPattern) match := re.FindStringSubmatch(description) if len(match) != 2 { @@ -266,16 +301,16 @@ func formatDescription(description string, channel string, name string) string { } description = re.ReplaceAllString(description, "\n\n") } else { - description = strings.ReplaceAll(description, startTag, "") - description = strings.ReplaceAll(description, endTag, "") + description = strings.ReplaceAll(description, startTagExperimental, "") + description = strings.ReplaceAll(description, endTagExperimental, "") } // Comments within "opcon:util:excludeFromCRD" tag are not included in the generated CRD and all trailing \n operators before // and after the tags are removed and replaced with three \n operators. - startTag = "" - endTag = "" - if strings.Contains(description, startTag) { - regexPattern := `\n*` + regexp.QuoteMeta(startTag) + `(?s:(.*?))` + regexp.QuoteMeta(endTag) + `\n*` + startTagExclude := "" + endTagExclude := "" + if strings.Contains(description, startTagExclude) { + regexPattern := `\n*` + regexp.QuoteMeta(startTagExclude) + `(?s:(.*?))` + regexp.QuoteMeta(endTagExclude) + `\n*` re := regexp.MustCompile(regexPattern) match := re.FindStringSubmatch(description) if len(match) != 2 { diff --git a/helm/experimental.yaml b/helm/experimental.yaml index b14b1b3034..761837c7f1 100644 --- a/helm/experimental.yaml +++ b/helm/experimental.yaml @@ -13,6 +13,7 @@ options: - PreflightPermissions - HelmChartSupport - BoxcutterRuntime + - SyntheticPermissions disabled: - WebhookProviderOpenshiftServiceCA # List of enabled experimental features for catalogd diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml index 1038b7fdf0..614f3c3c08 100644 --- a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml +++ b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml @@ -151,12 +151,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account, if specified. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -173,9 +175,13 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. - The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. - serviceAccount is required. + + If serviceAccount is specified, OLM will authenticate as that service account. + Otherwise, operator-controller will authenticate as: + - User: "olm:clusterextension:" + - Group: "olm:clusterextensions" + + The authenticated user must be configured with the necessary permissions to perform these interactions. properties: name: description: |- @@ -183,8 +189,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -498,7 +502,6 @@ spec: has(self.catalog) : !has(self.catalog)' required: - namespace - - serviceAccount - source type: object status: diff --git a/helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml b/helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml index a0983e41f9..240a8d831c 100644 --- a/helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml +++ b/helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml @@ -112,12 +112,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -134,8 +136,9 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. + The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. + serviceAccount is required. properties: name: @@ -144,8 +147,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, diff --git a/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml b/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml index 84f221003c..0d30c77cdb 100644 --- a/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml +++ b/helm/olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml @@ -1,4 +1,4 @@ -{{- if and .Values.options.operatorController.enabled (not (has "BoxcutterRuntime" .Values.operatorConrollerFeatures)) }} +{{- if and .Values.options.operatorController.enabled (not (has "BoxcutterRuntime" .Values.options.operatorController.features.enabled)) }} apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: @@ -12,9 +12,25 @@ rules: - apiGroups: - "" resources: - - serviceaccounts/token + - serviceaccounts verbs: - - create + - impersonate + {{- if (has "SyntheticPermissions" .Values.options.operatorController.features.enabled) }} + - apiGroups: + - "" + resources: + - users + verbs: + - impersonate + - apiGroups: + - "" + resources: + - groups + resourceNames: + - olm:clusterextensions + verbs: + - impersonate + {{- end }} - apiGroups: - apiextensions.k8s.io resources: diff --git a/helm/tilt.yaml b/helm/tilt.yaml index aaed7c71fb..0f94e56703 100644 --- a/helm/tilt.yaml +++ b/helm/tilt.yaml @@ -17,6 +17,8 @@ options: - SingleOwnNamespaceInstallSupport - PreflightPermissions - HelmChartSupport + - BoxcutterRuntime + - SyntheticPermissions disabled: - WebhookProviderOpenshiftServiceCA catalogd: diff --git a/internal/operator-controller/action/restconfig.go b/internal/operator-controller/action/restconfig.go index 05e25f707d..d07e45eafb 100644 --- a/internal/operator-controller/action/restconfig.go +++ b/internal/operator-controller/action/restconfig.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" "k8s.io/client-go/transport" "sigs.k8s.io/controller-runtime/pkg/client" @@ -14,18 +13,15 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" ) -const syntheticServiceAccountName = "olm.synthetic-user" - // SyntheticUserRestConfigMapper returns an AuthConfigMapper that that impersonates synthetic users and groups for Object o. -// o is expected to be a ClusterExtension. If the service account defined in o is different from 'olm.synthetic-user', the -// defaultAuthMapper will be used +// o is expected to be a ClusterExtension. If the service account is defined in o, the defaultAuthMapper will be used. func SyntheticUserRestConfigMapper(defaultAuthMapper func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error)) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { cExt, err := validate(o, c) if err != nil { return nil, err } - if cExt.Spec.ServiceAccount.Name != syntheticServiceAccountName { + if cExt.Spec.ServiceAccount.Name != "" { return defaultAuthMapper(ctx, cExt, c) } cc := rest.CopyConfig(c) @@ -37,25 +33,19 @@ func SyntheticUserRestConfigMapper(defaultAuthMapper func(ctx context.Context, o } // ServiceAccountRestConfigMapper returns an AuthConfigMapper scoped to the service account defined in o, which is expected to -// be a ClusterExtension -func ServiceAccountRestConfigMapper(tokenGetter *authentication.TokenGetter) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { +// be a ClusterExtension. It uses impersonation, which allows authentication as a ServiceAccount without requiring the +// ServiceAccount to actually exist or having to manage tokens. +func ServiceAccountRestConfigMapper() func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { cExt, err := validate(o, c) if err != nil { return nil, err } - saConfig := rest.AnonymousClientConfig(c) - saConfig.Wrap(func(rt http.RoundTripper) http.RoundTripper { - return &authentication.TokenInjectingRoundTripper{ - Tripper: rt, - TokenGetter: tokenGetter, - Key: types.NamespacedName{ - Name: cExt.Spec.ServiceAccount.Name, - Namespace: cExt.Spec.Namespace, - }, - } + cc := rest.CopyConfig(c) + cc.Wrap(func(rt http.RoundTripper) http.RoundTripper { + return transport.NewImpersonatingRoundTripper(authentication.ServiceAccountImpersonationConfig(*cExt), rt) }) - return saConfig, nil + return cc, nil } } diff --git a/internal/operator-controller/action/restconfig_test.go b/internal/operator-controller/action/restconfig_test.go index 4c9f786719..bb676fd446 100644 --- a/internal/operator-controller/action/restconfig_test.go +++ b/internal/operator-controller/action/restconfig_test.go @@ -9,13 +9,11 @@ import ( "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/action" - "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" ) func Test_ServiceAccountRestConfigMapper(t *testing.T) { @@ -55,21 +53,21 @@ func Test_ServiceAccountRestConfigMapper(t *testing.T) { }, } { t.Run(tc.description, func(t *testing.T) { - tokenGetter := &authentication.TokenGetter{} - saMapper := action.ServiceAccountRestConfigMapper(tokenGetter) + saMapper := action.ServiceAccountRestConfigMapper() actualCfg, err := saMapper(context.Background(), tc.obj, tc.cfg) if tc.expectedError != nil { require.Nil(t, actualCfg) require.EqualError(t, err, tc.expectedError.Error()) } else { require.NoError(t, err) - transport, err := rest.TransportFor(actualCfg) - require.NoError(t, err) - require.NotNil(t, transport) - tokenInjectionRoundTripper, ok := transport.(*authentication.TokenInjectingRoundTripper) - require.True(t, ok) - require.Equal(t, tokenGetter, tokenInjectionRoundTripper.TokenGetter) - require.Equal(t, types.NamespacedName{Name: "my-service-account", Namespace: "my-namespace"}, tokenInjectionRoundTripper.Key) + require.NotNil(t, actualCfg) + + // test that the impersonation headers are appropriately injected into the request + // nolint:bodyclose + _, _ = actualCfg.WrapTransport(fakeRoundTripper(func(req *http.Request) (*http.Response, error) { + require.Equal(t, "system:serviceaccount:my-namespace:my-service-account", req.Header.Get("Impersonate-User")) + return &http.Response{}, nil + })).RoundTrip(&http.Request{}) } }) } @@ -98,22 +96,10 @@ func Test_SyntheticUserRestConfigMapper_Fails(t *testing.T) { }, } { t.Run(tc.description, func(t *testing.T) { - tokenGetter := &authentication.TokenGetter{} - saMapper := action.ServiceAccountRestConfigMapper(tokenGetter) + saMapper := action.SyntheticUserRestConfigMapper(nil) actualCfg, err := saMapper(context.Background(), tc.obj, tc.cfg) - if tc.expectedError != nil { - require.Nil(t, actualCfg) - require.EqualError(t, err, tc.expectedError.Error()) - } else { - require.NoError(t, err) - transport, err := rest.TransportFor(actualCfg) - require.NoError(t, err) - require.NotNil(t, transport) - tokenInjectionRoundTripper, ok := transport.(*authentication.TokenInjectingRoundTripper) - require.True(t, ok) - require.Equal(t, tokenGetter, tokenInjectionRoundTripper.TokenGetter) - require.Equal(t, types.NamespacedName{Name: "my-service-account", Namespace: "my-namespace"}, tokenInjectionRoundTripper.Key) - } + require.Nil(t, actualCfg) + require.EqualError(t, err, tc.expectedError.Error()) }) } } @@ -150,9 +136,6 @@ func Test_SyntheticUserRestConfigMapper_UsesSyntheticAuthMapper(t *testing.T) { Name: "my-clusterextension", }, Spec: ocv1.ClusterExtensionSpec{ - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "olm.synthetic-user", - }, Namespace: "my-namespace", }, } diff --git a/internal/operator-controller/authentication/serviceaccount.go b/internal/operator-controller/authentication/serviceaccount.go new file mode 100644 index 0000000000..cd28fbf8fd --- /dev/null +++ b/internal/operator-controller/authentication/serviceaccount.go @@ -0,0 +1,17 @@ +package authentication + +import ( + "fmt" + + "k8s.io/client-go/transport" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" +) + +// ServiceAccountImpersonationConfig returns an ImpersonationConfig for impersonating a ServiceAccount. +// This allows authentication as a ServiceAccount without requiring an actual token. +func ServiceAccountImpersonationConfig(ext ocv1.ClusterExtension) transport.ImpersonationConfig { + return transport.ImpersonationConfig{ + UserName: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name), + } +} diff --git a/internal/operator-controller/authentication/serviceaccount_test.go b/internal/operator-controller/authentication/serviceaccount_test.go new file mode 100644 index 0000000000..6087d0d4f7 --- /dev/null +++ b/internal/operator-controller/authentication/serviceaccount_test.go @@ -0,0 +1,24 @@ +package authentication_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" +) + +func TestServiceAccountImpersonationConfig(t *testing.T) { + config := authentication.ServiceAccountImpersonationConfig(ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-ext", + }, + Spec: ocv1.ClusterExtensionSpec{Namespace: "my-namespace", ServiceAccount: ocv1.ServiceAccountReference{Name: "my-service-account"}}, + }) + require.Equal(t, "system:serviceaccount:my-namespace:my-service-account", config.UserName) + require.Nil(t, config.Groups) + require.Empty(t, config.UID) + require.Empty(t, config.Extra) +} diff --git a/internal/operator-controller/authentication/tokengetter.go b/internal/operator-controller/authentication/tokengetter.go deleted file mode 100644 index 7870dc8e83..0000000000 --- a/internal/operator-controller/authentication/tokengetter.go +++ /dev/null @@ -1,128 +0,0 @@ -package authentication - -import ( - "context" - "fmt" - "sync" - "time" - - authenticationv1 "k8s.io/api/authentication/v1" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - corev1 "k8s.io/client-go/kubernetes/typed/core/v1" - "k8s.io/utils/ptr" -) - -type TokenGetter struct { - client corev1.ServiceAccountsGetter - expirationDuration time.Duration - tokens map[types.NamespacedName]*authenticationv1.TokenRequestStatus - mu sync.RWMutex -} - -type ServiceAccountNotFoundError struct { - ServiceAccountName string - ServiceAccountNamespace string - Err error -} - -func (e *ServiceAccountNotFoundError) Unwrap() error { - return e.Err -} - -// Error implements the error interface for ServiceAccountNotFoundError. -func (e *ServiceAccountNotFoundError) Error() string { - return fmt.Sprintf("service account \"%s\" not found in namespace \"%s\": unable to authenticate with the Kubernetes cluster.", e.ServiceAccountName, e.ServiceAccountNamespace) -} - -type TokenGetterOption func(*TokenGetter) - -const ( - rotationThresholdFraction = 0.1 - DefaultExpirationDuration = 5 * time.Minute -) - -// Returns a token getter that can fetch tokens given a service account. -// The token getter also caches tokens which helps reduce the number of requests to the API Server. -// In case a cached token is expiring a fresh token is created. -func NewTokenGetter(client corev1.ServiceAccountsGetter, options ...TokenGetterOption) *TokenGetter { - tokenGetter := &TokenGetter{ - client: client, - expirationDuration: DefaultExpirationDuration, - tokens: map[types.NamespacedName]*authenticationv1.TokenRequestStatus{}, - } - - for _, opt := range options { - opt(tokenGetter) - } - - return tokenGetter -} - -func WithExpirationDuration(expirationDuration time.Duration) TokenGetterOption { - return func(tg *TokenGetter) { - tg.expirationDuration = expirationDuration - } -} - -// Get returns a token from the cache if available and not expiring, otherwise creates a new token -func (t *TokenGetter) Get(ctx context.Context, key types.NamespacedName) (string, error) { - t.mu.RLock() - token, ok := t.tokens[key] - t.mu.RUnlock() - - expireTime := time.Time{} - if ok { - expireTime = token.ExpirationTimestamp.Time - } - - // Create a new token if the cached token expires within rotationThresholdFraction of expirationDuration from now - rotationThresholdAfterNow := metav1.Now().Add(time.Duration(float64(t.expirationDuration) * (rotationThresholdFraction))) - if expireTime.Before(rotationThresholdAfterNow) { - var err error - token, err = t.getToken(ctx, key) - if err != nil { - return "", err - } - t.mu.Lock() - t.tokens[key] = token - t.mu.Unlock() - } - - // Delete tokens that have expired - t.reapExpiredTokens() - - return token.Token, nil -} - -func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (*authenticationv1.TokenRequestStatus, error) { - req, err := t.client.ServiceAccounts(key.Namespace).CreateToken(ctx, - key.Name, - &authenticationv1.TokenRequest{ - Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To(int64(t.expirationDuration / time.Second))}, - }, metav1.CreateOptions{}) - if err != nil { - if errors.IsNotFound(err) { - return nil, &ServiceAccountNotFoundError{ServiceAccountName: key.Name, ServiceAccountNamespace: key.Namespace} - } - return nil, err - } - return &req.Status, nil -} - -func (t *TokenGetter) reapExpiredTokens() { - t.mu.Lock() - defer t.mu.Unlock() - for key, token := range t.tokens { - if metav1.Now().Sub(token.ExpirationTimestamp.Time) > 0 { - delete(t.tokens, key) - } - } -} - -func (t *TokenGetter) Delete(key types.NamespacedName) { - t.mu.Lock() - defer t.mu.Unlock() - delete(t.tokens, key) -} diff --git a/internal/operator-controller/authentication/tokengetter_test.go b/internal/operator-controller/authentication/tokengetter_test.go deleted file mode 100644 index 663e95f1b3..0000000000 --- a/internal/operator-controller/authentication/tokengetter_test.go +++ /dev/null @@ -1,89 +0,0 @@ -package authentication - -import ( - "context" - "fmt" - "testing" - "time" - - "github.com/stretchr/testify/assert" - authenticationv1 "k8s.io/api/authentication/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" - "k8s.io/client-go/kubernetes/fake" - ctest "k8s.io/client-go/testing" -) - -func TestTokenGetterGet(t *testing.T) { - fakeClient := fake.NewSimpleClientset() - fakeClient.PrependReactor("create", "serviceaccounts/token", - func(action ctest.Action) (bool, runtime.Object, error) { - act, ok := action.(ctest.CreateActionImpl) - if !ok { - return false, nil, nil - } - tokenRequest := act.GetObject().(*authenticationv1.TokenRequest) - var err error - if act.Name == "test-service-account-1" { - tokenRequest.Status = authenticationv1.TokenRequestStatus{ - Token: "test-token-1", - ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(DefaultExpirationDuration)), - } - } - if act.Name == "test-service-account-2" { - tokenRequest.Status = authenticationv1.TokenRequestStatus{ - Token: "test-token-2", - ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(1 * time.Second)), - } - } - if act.Name == "test-service-account-3" { - tokenRequest.Status = authenticationv1.TokenRequestStatus{ - Token: "test-token-3", - ExpirationTimestamp: metav1.NewTime(metav1.Now().Add(-10 * time.Second)), - } - } - if act.Name == "test-service-account-4" { - tokenRequest = nil - err = fmt.Errorf("error when fetching token") - } - return true, tokenRequest, err - }) - - tg := NewTokenGetter(fakeClient.CoreV1(), - WithExpirationDuration(DefaultExpirationDuration)) - - tests := []struct { - testName string - serviceAccountName string - namespace string - want string - errorMsg string - }{ - {"Testing getting token with fake client", "test-service-account-1", - "test-namespace-1", "test-token-1", "failed to get token"}, - {"Testing getting token from cache", "test-service-account-1", - "test-namespace-1", "test-token-1", "failed to get token"}, - {"Testing getting short lived token from fake client", "test-service-account-2", - "test-namespace-2", "test-token-2", "failed to get token"}, - {"Testing getting nearly expired token from cache", "test-service-account-2", - "test-namespace-2", "test-token-2", "failed to refresh token"}, - {"Testing token that expired 10 seconds ago", "test-service-account-3", - "test-namespace-3", "test-token-3", "failed to get token"}, - {"Testing error when getting token from fake client", "test-service-account-4", - "test-namespace-4", "error when fetching token", "error when fetching token"}, - {"Testing service account not found", "missing-sa", - "test-namespace-5", "", "service account \"missing-sa\" not found in namespace \"test-namespace-5\": unable to authenticate with the Kubernetes cluster."}, - } - - for _, tc := range tests { - got, err := tg.Get(context.Background(), types.NamespacedName{Namespace: tc.namespace, Name: tc.serviceAccountName}) - if err != nil { - t.Logf("%s: expected: %v, got: %v", tc.testName, tc.want, err) - assert.EqualError(t, err, tc.errorMsg, "Error message should match expected output") - } else { - t.Logf("%s: expected: %v, got: %v", tc.testName, tc.want, got) - assert.Equal(t, tc.want, got, tc.errorMsg) - } - } -} diff --git a/internal/operator-controller/authentication/tripper.go b/internal/operator-controller/authentication/tripper.go deleted file mode 100644 index 77bc1175c0..0000000000 --- a/internal/operator-controller/authentication/tripper.go +++ /dev/null @@ -1,38 +0,0 @@ -package authentication - -import ( - "fmt" - "net/http" - - "k8s.io/apimachinery/pkg/types" - utilnet "k8s.io/apimachinery/pkg/util/net" -) - -var _ http.RoundTripper = (*TokenInjectingRoundTripper)(nil) - -type TokenInjectingRoundTripper struct { - Tripper http.RoundTripper - TokenGetter *TokenGetter - Key types.NamespacedName -} - -func (tt *TokenInjectingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { - resp, err := tt.do(req) - if resp != nil && resp.StatusCode == http.StatusUnauthorized { - tt.TokenGetter.Delete(tt.Key) - resp, err = tt.do(req) - } - return resp, err -} - -func (tt *TokenInjectingRoundTripper) do(req *http.Request) (*http.Response, error) { - reqClone := utilnet.CloneRequest(req) - token, err := tt.TokenGetter.Get(reqClone.Context(), tt.Key) - if err != nil { - return nil, err - } - - // Always set the Authorization header to our retrieved token - reqClone.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) - return tt.Tripper.RoundTrip(reqClone) -} diff --git a/internal/operator-controller/authorization/rbac.go b/internal/operator-controller/authorization/rbac.go index 357268615c..7e42f3f99b 100644 --- a/internal/operator-controller/authorization/rbac.go +++ b/internal/operator-controller/authorization/rbac.go @@ -55,13 +55,17 @@ var namespacedCollectionVerbs = []string{"create"} var clusterCollectionVerbs = []string{"list", "watch"} type rbacPreAuthorizer struct { + getUserInfo userInfoMapper authorizer authorizer.Authorizer ruleResolver validation.AuthorizationRuleResolver restMapper meta.RESTMapper } -func NewRBACPreAuthorizer(cl client.Client) PreAuthorizer { +type userInfoMapper func(extension *ocv1.ClusterExtension) (*user.DefaultInfo, error) + +func NewRBACPreAuthorizer(cl client.Client, getUserInfo userInfoMapper) PreAuthorizer { return &rbacPreAuthorizer{ + getUserInfo: getUserInfo, authorizer: newRBACAuthorizer(cl), ruleResolver: newRBACRulesResolver(cl), restMapper: cl.RESTMapper(), @@ -84,7 +88,11 @@ func (a *rbacPreAuthorizer) PreAuthorize(ctx context.Context, ext *ocv1.ClusterE if err != nil { return nil, err } - manifestManager := &user.DefaultInfo{Name: fmt.Sprintf("system:serviceaccount:%s:%s", ext.Spec.Namespace, ext.Spec.ServiceAccount.Name)} + + manifestManager, err := a.getUserInfo(ext) + if err != nil { + return nil, fmt.Errorf("error getting user info mapping from clusterextension: %v", err) + } attributesRecords := dm.asAuthorizationAttributesRecordsForUser(manifestManager, ext) var preAuthEvaluationErrors []error diff --git a/internal/operator-controller/authorization/rbac_test.go b/internal/operator-controller/authorization/rbac_test.go index 8e3d47687d..bb8ac92ee4 100644 --- a/internal/operator-controller/authorization/rbac_test.go +++ b/internal/operator-controller/authorization/rbac_test.go @@ -20,6 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" ) var ( @@ -414,10 +415,14 @@ func setupFakeClient(role client.Object) client.Client { return fakeClientBuilder.Build() } +func saInfo(ext *ocv1.ClusterExtension) (*user.DefaultInfo, error) { + return &user.DefaultInfo{Name: authentication.ServiceAccountImpersonationConfig(*ext).UserName}, nil +} + func TestPreAuthorize_Success(t *testing.T) { t.Run("preauthorize succeeds with no missing rbac rules", func(t *testing.T) { fakeClient := setupFakeClient(privilegedClusterRole) - preAuth := NewRBACPreAuthorizer(fakeClient) + preAuth := NewRBACPreAuthorizer(fakeClient, saInfo) missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest)) require.NoError(t, err) require.Equal(t, []ScopedPolicyRules{}, missingRules) @@ -427,7 +432,7 @@ func TestPreAuthorize_Success(t *testing.T) { func TestPreAuthorize_MissingRBAC(t *testing.T) { t.Run("preauthorize fails and finds missing rbac rules", func(t *testing.T) { fakeClient := setupFakeClient(limitedClusterRole) - preAuth := NewRBACPreAuthorizer(fakeClient) + preAuth := NewRBACPreAuthorizer(fakeClient, saInfo) missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest)) require.NoError(t, err) require.Equal(t, expectedSingleNamespaceMissingRules, missingRules) @@ -437,7 +442,7 @@ func TestPreAuthorize_MissingRBAC(t *testing.T) { func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) { t.Run("preauthorize fails and finds missing rbac rules in multiple namespaces", func(t *testing.T) { fakeClient := setupFakeClient(limitedClusterRole) - preAuth := NewRBACPreAuthorizer(fakeClient) + preAuth := NewRBACPreAuthorizer(fakeClient, saInfo) missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifestMultiNamespace)) require.NoError(t, err) require.Equal(t, expectedMultiNamespaceMissingRules, missingRules) @@ -447,7 +452,7 @@ func TestPreAuthorizeMultiNamespace_MissingRBAC(t *testing.T) { func TestPreAuthorize_CheckEscalation(t *testing.T) { t.Run("preauthorize succeeds with no missing rbac rules", func(t *testing.T) { fakeClient := setupFakeClient(escalatingClusterRole) - preAuth := NewRBACPreAuthorizer(fakeClient) + preAuth := NewRBACPreAuthorizer(fakeClient, saInfo) missingRules, err := preAuth.PreAuthorize(context.TODO(), &exampleClusterExtension, strings.NewReader(testManifest)) require.NoError(t, err) require.Equal(t, []ScopedPolicyRules{}, missingRules) diff --git a/internal/operator-controller/controllers/clusterextension_admission_test.go b/internal/operator-controller/controllers/clusterextension_admission_test.go index 38c6c60d41..414867909e 100644 --- a/internal/operator-controller/controllers/clusterextension_admission_test.go +++ b/internal/operator-controller/controllers/clusterextension_admission_test.go @@ -349,7 +349,7 @@ func TestClusterExtensionAdmissionServiceAccount(t *testing.T) { {"dot-separated", "dotted.name", ""}, {"longest valid service account name", strings.Repeat("x", 253), ""}, {"too long service account name", strings.Repeat("x", 254), tooLongError}, - {"no service account name", "", regexMismatchError}, + {"no service account name", "", ""}, {"spaces", "spaces spaces", regexMismatchError}, {"capitalized", "Capitalized", regexMismatchError}, {"camel case", "camelCase", regexMismatchError}, diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index d8d9d8de0e..5d9f797d5a 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -49,7 +49,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/labels" @@ -218,12 +217,6 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl revisionStates, err := r.RevisionStatesGetter.GetRevisionStates(ctx, ext) if err != nil { setInstallStatus(ext, nil) - var saerr *authentication.ServiceAccountNotFoundError - if errors.As(err, &saerr) { - setInstalledStatusConditionUnknown(ext, saerr.Error()) - setStatusProgressing(ext, errors.New("installation cannot proceed due to missing ServiceAccount")) - return ctrl.Result{}, err - } setInstalledStatusConditionUnknown(ext, err.Error()) setStatusProgressing(ext, errors.New("retrying to get installed bundle")) return ctrl.Result{}, err diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index 20761aec91..3efbf5f064 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -28,7 +28,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" "github.com/operator-framework/operator-controller/internal/operator-controller/conditionsets" "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" "github.com/operator-framework/operator-controller/internal/operator-controller/finalizers" @@ -346,61 +345,6 @@ func TestClusterExtensionResolutionAndUnpackSuccessfulApplierFails(t *testing.T) require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) } -func TestClusterExtensionServiceAccountNotFound(t *testing.T) { - cl, reconciler := newClientAndReconciler(t) - reconciler.RevisionStatesGetter = &MockRevisionStatesGetter{ - Err: &authentication.ServiceAccountNotFoundError{ - ServiceAccountName: "missing-sa", - ServiceAccountNamespace: "default", - }} - - ctx := context.Background() - extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} - - t.Log("Given a cluster extension with a missing service account") - clusterExtension := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, - Spec: ocv1.ClusterExtensionSpec{ - Source: ocv1.SourceConfig{ - SourceType: "Catalog", - Catalog: &ocv1.CatalogFilter{ - PackageName: "test-package", - }, - }, - Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "missing-sa", - }, - }, - } - - require.NoError(t, cl.Create(ctx, clusterExtension)) - - t.Log("When reconciling the cluster extension") - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) - - require.Equal(t, ctrl.Result{}, res) - require.Error(t, err) - var saErr *authentication.ServiceAccountNotFoundError - require.ErrorAs(t, err, &saErr) - t.Log("By fetching updated cluster extension after reconcile") - require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) - - t.Log("By checking the status conditions") - installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) - require.NotNil(t, installedCond) - require.Equal(t, metav1.ConditionUnknown, installedCond.Status) - require.Contains(t, installedCond.Message, fmt.Sprintf("service account %q not found in namespace %q: unable to authenticate with the Kubernetes cluster.", - "missing-sa", "default")) - - progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) - require.NotNil(t, progressingCond) - require.Equal(t, metav1.ConditionTrue, progressingCond.Status) - require.Equal(t, ocv1.ReasonRetrying, progressingCond.Reason) - require.Contains(t, progressingCond.Message, "installation cannot proceed due to missing ServiceAccount") - require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) -} - func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) { cl, reconciler := newClientAndReconciler(t) reconciler.ImagePuller = &imageutil.MockPuller{ diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 6728302259..f6f86486eb 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -939,12 +939,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account, if specified. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -961,9 +963,13 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. - The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. - serviceAccount is required. + + If serviceAccount is specified, OLM will authenticate as that service account. + Otherwise, operator-controller will authenticate as: + - User: "olm:clusterextension:" + - Group: "olm:clusterextensions" + + The authenticated user must be configured with the necessary permissions to perform these interactions. properties: name: description: |- @@ -971,8 +977,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -1286,7 +1290,6 @@ spec: has(self.catalog) : !has(self.catalog)' required: - namespace - - serviceAccount - source type: object status: @@ -1554,71 +1557,6 @@ rules: - list - watch --- -# Source: olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: operator-controller-manager-role - labels: - app.kubernetes.io/name: operator-controller - app.kubernetes.io/part-of: olm - annotations: - olm.operatorframework.io/feature-set: experimental-e2e -rules: - - apiGroups: - - "" - resources: - - serviceaccounts/token - verbs: - - create - - apiGroups: - - apiextensions.k8s.io - resources: - - customresourcedefinitions - verbs: - - get - - apiGroups: - - olm.operatorframework.io - resources: - - clustercatalogs - verbs: - - get - - list - - watch - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions - verbs: - - get - - list - - patch - - update - - watch - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions/finalizers - verbs: - - update - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions/status - verbs: - - patch - - update - - apiGroups: - - rbac.authorization.k8s.io - resources: - - clusterrolebindings - - clusterroles - - rolebindings - - roles - verbs: - - list - - watch ---- # Source: olmv1/templates/rbac/clusterrolebinding-catalogd-manager-rolebinding.yml apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding @@ -2177,6 +2115,7 @@ spec: - --feature-gates=PreflightPermissions=true - --feature-gates=HelmChartSupport=true - --feature-gates=BoxcutterRuntime=true + - --feature-gates=SyntheticPermissions=true - --feature-gates=WebhookProviderOpenshiftServiceCA=false - --tls-cert=/var/certs/tls.crt - --tls-key=/var/certs/tls.key diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 199838eac5..08cf9929f0 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -904,12 +904,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account, if specified. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -926,9 +928,13 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. - The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. - serviceAccount is required. + + If serviceAccount is specified, OLM will authenticate as that service account. + Otherwise, operator-controller will authenticate as: + - User: "olm:clusterextension:" + - Group: "olm:clusterextensions" + + The authenticated user must be configured with the necessary permissions to perform these interactions. properties: name: description: |- @@ -936,8 +942,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -1251,7 +1255,6 @@ spec: has(self.catalog) : !has(self.catalog)' required: - namespace - - serviceAccount - source type: object status: @@ -1519,71 +1522,6 @@ rules: - list - watch --- -# Source: olmv1/templates/rbac/clusterrole-operator-controller-manager-role.yml -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: operator-controller-manager-role - labels: - app.kubernetes.io/name: operator-controller - app.kubernetes.io/part-of: olm - annotations: - olm.operatorframework.io/feature-set: experimental -rules: - - apiGroups: - - "" - resources: - - serviceaccounts/token - verbs: - - create - - apiGroups: - - apiextensions.k8s.io - resources: - - customresourcedefinitions - verbs: - - get - - apiGroups: - - olm.operatorframework.io - resources: - - clustercatalogs - verbs: - - get - - list - - watch - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions - verbs: - - get - - list - - patch - - update - - watch - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions/finalizers - verbs: - - update - - apiGroups: - - olm.operatorframework.io - resources: - - clusterextensions/status - verbs: - - patch - - update - - apiGroups: - - rbac.authorization.k8s.io - resources: - - clusterrolebindings - - clusterroles - - rolebindings - - roles - verbs: - - list - - watch ---- # Source: olmv1/templates/rbac/clusterrolebinding-catalogd-manager-rolebinding.yml apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding @@ -2088,6 +2026,7 @@ spec: - --feature-gates=PreflightPermissions=true - --feature-gates=HelmChartSupport=true - --feature-gates=BoxcutterRuntime=true + - --feature-gates=SyntheticPermissions=true - --feature-gates=WebhookProviderOpenshiftServiceCA=false - --tls-cert=/var/certs/tls.crt - --tls-key=/var/certs/tls.key diff --git a/manifests/standard-e2e.yaml b/manifests/standard-e2e.yaml index 5c95907841..0f81669e2d 100644 --- a/manifests/standard-e2e.yaml +++ b/manifests/standard-e2e.yaml @@ -703,12 +703,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -725,8 +727,9 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. + The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. + serviceAccount is required. properties: name: @@ -735,8 +738,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -1332,9 +1333,9 @@ rules: - apiGroups: - "" resources: - - serviceaccounts/token + - serviceaccounts verbs: - - create + - impersonate - apiGroups: - apiextensions.k8s.io resources: diff --git a/manifests/standard.yaml b/manifests/standard.yaml index 95e400c264..a5df5e3eb1 100644 --- a/manifests/standard.yaml +++ b/manifests/standard.yaml @@ -668,12 +668,14 @@ spec: namespace: description: |- namespace is a reference to a Kubernetes namespace. - This is the namespace in which the provided ServiceAccount must exist. - It also designates the default namespace where namespace-scoped resources + + This designates the default namespace where namespace-scoped resources for the extension are applied to the cluster. Some extensions may contain namespace-scoped resources to be applied in other namespaces. This namespace must exist. + This is also the namespace of the referenced service account. + namespace is required, immutable, and follows the DNS label standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-), start and end with an alphanumeric character, and be no longer than 63 characters @@ -690,8 +692,9 @@ spec: description: |- serviceAccount is a reference to a ServiceAccount used to perform all interactions with the cluster that are required to manage the extension. + The ServiceAccount must be configured with the necessary permissions to perform these interactions. - The ServiceAccount must exist in the namespace referenced in the spec. + serviceAccount is required. properties: name: @@ -700,8 +703,6 @@ spec: to be used for installation and management of the content for the package specified in the packageName field. - This ServiceAccount must exist in the installNamespace. - name follows the DNS subdomain standard as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.), start and end with an alphanumeric character, @@ -1297,9 +1298,9 @@ rules: - apiGroups: - "" resources: - - serviceaccounts/token + - serviceaccounts verbs: - - create + - impersonate - apiGroups: - apiextensions.k8s.io resources: