aws: IAM pod identity

[sjenning]

@derekwaynecarr

[sjenning]

/cc @marun

[sjenning]

/assign @derekwaynecarr

[sjenning]

/cc @smarterclayton

[derekwaynecarr]

I want to work out tomorrow if we should require a unique provider per cluster and if the user could provide the provider as a day-1 option rather than we create it.

[abhinavdahiya]

also this needs to pick a domain ??

[sjenning]

you mean an subdirectory under enhancements? i'm open to suggestions 👍

[dgoodwin]

I'm working on a CCO enhancement as well, my best guess was security.

[abhinavdahiya]

So this means that a pod can have only ONE identity attached right?

cloud-credential-operator currently supports dual identity.

• an admin entity that is used to create new roles/users etc.
• an reader entity that is only allowed to read roles/users to make sure things stay as-is

a lot of the operators today require create/update/delete permissions do that they can create dynamic objects. But some of the operators can be modified to have dual identities.

• ADMIN_WRITE identity that can be used to create the objects.
• RESTRICTED_WRITE identity that is restricted to only the objects it created.
  that could allow users to remove/disable the ADMIN_WRITE permissions as day-2 operation.. making the security stance even stronger.

if using the POD identity based on webhook and environment disallows us to ever move in that direction, I think we should maybe not use it and stick to secrets as multiple secrets can be mounted compared to single AWS env variable.

[sjenning]

yes, this would currently not be suitable for use by the CCO itself. since the pod can only run with one serviceaccount, this mechanism is one-to-one IAM role to pod.

[joelddiaz]

would/could this be solved by having the operator have the ADMIN_WRITE and the operand have RESTRICTED_WRITE (assuming a clean separation between operator and operand)?

i suppose this might also imply that the operator is also creating a new CredentialsRequest so that the perms for the operand only grant perms to the actually created objects...

[sjenning]

i think we should punt on the multi-cred use case for now since serviceaccount-to-pod is 1:1 vs secret-to-pod is N:1

[sjenning]

adding review request from remaining CCO approvers
/cc @abutcher @csrwng @dgoodwin @twiest

[dgoodwin]

Do we plan to update the existing CredRequests to use this new mechanism on AWS? And if so, how can we upgrade without triggering something bad here. These CredRequests are delivered via the release manifest and I believe that means the CVO will update them to their new state. Is there a way we can make this manual and opt-in?

In doing so, the operator permissions would then become fixed and need manual intervention in the event an upgraded operator needed new permissions.

[sjenning]

No we are not planning to update existing CredentialRequests at this time.

For this stage, I recommended we disallow CR changes where the spec changes from a secretRef to a serviceAccountRef or visa versa.

There doesn't seem to be a way to mark CRD fields as immutable atm kubernetes/kubernetes#65973

We'll probably just overwrite the change in the controller is someone tries to do that.

[wking]

We'll probably just overwrite the change in the controller is someone tries to do that.

Do we have an admission webhook for this? What happens if someone tries to push a change while the cred minter is, for some reason, out of action? The new cred minter comes up and... searches for existing Secrets to decide if it needs to overwrite a serviceAccountRef?

[sjenning]

@deads2k @derekwaynecarr @joelddiaz @dgoodwin @abhinavdahiya

Ok so I've encountered an issue implementing the API change to the CredentialsRequest CRD

Currently, this is the spec
https://github.com/openshift/cloud-credential-operator/blob/master/pkg/apis/cloudcredential/v1/credentialsrequest_types.go#L49-L56

// CredentialsRequestSpec defines the desired state of CredentialsRequest
type CredentialsRequestSpec struct {
	// SecretRef points to the secret where the credentials should be stored once generated.
	SecretRef corev1.ObjectReference `json:"secretRef,omitempty"`

	// ProviderSpec contains the cloud provider specific credentials specification.
	ProviderSpec *runtime.RawExtension `json:"providerSpec,omitempty"`
}

The issue is two fold depending on what solution are you trying to implement.

1. The SecretRef is not nullable because it isn't a pointer. So there is not a good way to tell that the user didn't set it (if their intent is to use the serviceaccount injection method)
2. We could just use the corev1.ObjectReference for both the secret and serviceaccount based injecting and multiplex on the SecretRef.Kind (treating no Kind as Secret for backward compat). However, the name of the field indicates that the object type is Secret even if the actual type is more flexible than that. Though use of type ObjectReference is discouraged

I've thought of 3 solutions and I'm looking for feedback:

1. Just reuse the SecretRef to hold the serivceaccount reference and multiplex in the code based on SecretRef.Kind. This is the simpliest of the solutions as it doesn't require any change to the CRD at all. However, it is messy to have a field name SecretRef that you set the Kind to ServiceAccount.
2. Add a ServiceAccountRef *corev1.ObjectReference field to the CredentialsRequestSpec and multiplex based on if SecretRef.Namespace is set. ServiceAccountRef needs to be a pointer since it will not be present in previously created CRs
3. Possibly the most correct, but also most disruptive way, switch to a union discriminator and deprecate the SecretRef field. Something like this (suggested by @deads2k)

// CredentialsRequestSpec defines the desired state of CredentialsRequest
type CredentialsRequestSpec struct {
	// SecretRef points to the secret where the credentials should be stored once generated.
	SecretRef *corev1.ObjectReference `json:"secretRef,omitempty"`

	StorageDestination *StorageDestination

	// ProviderSpec contains the cloud provider specific credentials specification.
	ProviderSpec *runtime.RawExtension `json:"providerSpec,omitempty"`
}

type StorageDestination struct {
	StorageType    string
	Secret         *SecretStorageDestination
	ServiceAccount *ServiceAccountStorageDestination
}

type SecretStorageDestination struct {
	Namespace string
	Name      string
}

type ServiceAccountStorageDestination struct {
	Namespace string
	Name      string
}

I would apreciate any feedback on these methods or there might be a far superior way that I'm overlooking.

[dgoodwin]

3 feels the best to me, going from a mandatory field to optional shouldn't cause any harm or break API compat, and I'm not particularly worried about converting that field to a pointer, though you would need to update all usage of it to add in the other implementation. We do not typically use discriminator fields for this kind of thing but it sounds like good practice. I would suggest some kind of name with "Type", maybe "CredentialsType". I don't think we have webhook validation today so this will probably need to be validated at runtime and reflected in a condition?

[sjenning]

There is going to need to be some intermediate work either way. The core operator and the actuators currently operate directly on the secret. I'm going to need to create an abstraction so we aren't if-else'ing throughout the whole codebase.

[joelddiaz]

What is the advantage of the StorageDestination approach? Is it that the StorageType tells you which of the pointers contains the actual data vs searching through the object for a non-nil value?

Is converting the SecretRef to a pointer and adding the ServiceAccountRef as a pointer not an option, or is the StorageDestination basically the solution you offered covering that?

FWIW, I'm partial to #3 (or basically any option which can take us to a place where the SecretRef field becomes optional).

[deads2k]

What is the advantage of the StorageDestination approach? Is it that the StorageType tells you which of the pointers contains the actual data vs searching through the object for a non-nil value?

Yes. UnionDiscriminators allow older clients to know they don't understand a newer format. Imagine adding, "nextCoolThingSethWants". Old clients won't even see the new fields, but they will see the discriminator.

Is converting the SecretRef to a pointer and adding the ServiceAccountRef as a pointer not an option, or is the StorageDestination basically the solution you offered covering that?

You cannot change a field to a pointer because old clients won't be able to deserialize nil and the deserialization will fail, preventing any list operation from working.

[sjenning]

Latest push contains significant revisions:

• breaking the enhancement into phases
  • phase 1: EKS end user experience parity
  • phase 2: integration with CredentialsRequest CR workflow

We also resolved to writing a new controller that will create and reconcile an OIDC discovery endpoint in a public S3 bucket. This is a significant amount of work entering the scope (configuration for enable/disable this functionality, new IAM perms required, etc).

As such, the work on openshift/cloud-credential-operator#187 will be put on hold, and work will begin on the OIDC discovery controller.

[dgoodwin]

Is the door open to seamlessly transition to ServiceAccountDiscovery later once it's out of alpha?

[sjenning]

I think so. We just have to detect if the URL in the Authentication CR is empty or our default and switch it to the ServiceAccountDiscovery URL. If it is not our default (the customer set it purposefully), we would do nothing. However, it was decided to do the s3 bucket because ServiceAccountDiscovery won't work for private clusters.

[dgoodwin]

CCO does run pretty early on the bootstrap node which may help here.

[wking]

But having in-cluster logic that switches on "am I a fresh install after $INSTALL_APPROACH_PIVOT?" seems pretty complicated.

[wking]

Why can't we expire and rotate user IDs and keys?

[sjenning]

we can, but the expiration is not a direct property of the userID/key, where it is for the token i.e. the token user can discover the token's expiration.

[joelddiaz]

What happens when a Pod is defined that references the not-yet-created Service Account (CCO hasn't finished creating the role/ServiceAccount)?

[sjenning]

the webhook does not inject the projected volume. the annotated SA must exist before the pod is admitted for this to work. This is a known limitation of the upstream implementation i.e. that a pod can be admitted before the service account that runs it exists.

[joelddiaz]

Will the first release of CCO (and all future ones) with support for pod-identiy be responsible for performing this modification of the Authentication object (so that all 4.5+ clusters are in pod-identity-capable-mode)?

[gregsheremeta]

Getting requests for STS to be in 4.8. What is the status of this RFE?

[derekwaynecarr]

can make the requested tweaks in follow-up.

/approve
/lgtm

[derekwaynecarr]

can you update which of these items got completed since we are retro merging?

[derekwaynecarr]

nit: Secrets

[derekwaynecarr]

clarify that this is how we are rolling out now (i see issuer specified)

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, sjenning

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [derekwaynecarr,sjenning]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[dgoodwin]

@sjenning we've talked about only having done phase 1 but this Phase 2 is actually complete isn't it?

There's really a phase 3 of transitioning a full OCP install and all components to use this functionality, is that right?

[wking]

Looks like everything after this is template boilerplate. Maybe a followup PR where you drop the sections? Or explicitly say "We do not need a test plan for this enhancement" and so forth for each section?