Fix AWS CLI access using AWS OIDC integration

[marcoandredinis]

Fixes #57831

changelog: Fix AWS CLI access using AWS OIDC integration

[r0mant]

I think that trying to determine whether to generate credentials or not based on the returned error is very awkward and error-prone (and not very Go idiomatic) - which is why I suspect we got this regression in the first place.

Can this be done by doing explicit checks on the integration type and request instead of doing this opportunistically based on the error?

[marcoandredinis]

Integration type is explicitly checked.
The request is not checked because it is un-changed from regular AWS App access, the client does not explicitly ask for client side credentials.

I can add other sentinel errors, and check all of them here.

	// Generate AWS client side credentials if the user is trying to access an AWS App with an AWS Roles Anywhere Integration.
	awsCredentialProcessCredentials, err := generateAWSClientSideCredentials(ctx, a, req, notAfter)
	switch {
	case errors.Is(err, errNotAWSAccessApp) || errors.Is(err, errAWSAppWithoutIntegration) || errors.Is(err, errProxiedAWSApp):
	case err != nil:
		return nil, trace.Wrap(err)
	}

I'm not convinced by this, it adds a list of errors which can become an oversight later on.
On 2nd thought, maybe this is not what you meant.

I don't think moving the logic from the function to here is a good approach as it would add quite a lot of code to an already large method.

[GavinFrazar]

i'm not strongly opposed to leaving it as-is, but you could refactor this to just return nil in cases where you would return the errAppWithoutAWSClientSideCredentials?

It seems like the graceful error handling behavior just ignores the error anyway and allowing for empty credentials is the expected behavior, so return "", nil makes sense to me.

I'm not convinced the error handling scheme had anything to do with the bug though. We just forgot to check for the AWS OIDC subkind, and for other subkinds it makes sense to return an error e.g. it should be an error to request an --aws-role with an Azure OIDC integration subkind.

[GavinFrazar]

imo we should have an exported list of integration subkinds and then table test against that.
That way if a subkind is added in the future that supports aws client side credentials, the tests will force us to update the switch statement to account for it

[marcoandredinis]

so return "", nil makes sense to me.

(when I first implemented, it was like that before code review 😅)

As for adding all the integration subkinds to table test them, maybe that's a good idea.
For now, their features barely intercept, so that's why it wasn't an issue in the past.

[GavinFrazar]

i'm not strongly opposed to leaving it as-is, but you could refactor this to just return nil in cases where you would return the errAppWithoutAWSClientSideCredentials?

It seems like the graceful error handling behavior just ignores the error anyway and allowing for empty credentials is the expected behavior, so return "", nil makes sense to me.

I'm not convinced the error handling scheme had anything to do with the bug though. We just forgot to check for the AWS OIDC subkind, and for other subkinds it makes sense to return an error e.g. it should be an error to request an --aws-role with an Azure OIDC integration subkind.

[backport-bot-workflows]

@marcoandredinis See the table below for backport results.

Branch	Result
branch/v18	Create PR