Resolves #24699, Support ES2 and ECS instance providers for S3 buckets

[Imajie]

Switch out specifically called out env and instanceProvider CredentialProvider for the defaultProvider, this adds support for running nextcloud in ECS instances and using the task role to provide permissions for S3 access.

New order for fallback is defined in the AWS docs

It first attempts to load credentials from environment variables, then from an .ini file (an .aws/credentials file first, followed by an .aws/config file), and then from an instance profile (EcsCredentials first, followed by Ec2 metadata).

[Imajie]

I changed it to use defaultProvider so if new methods were added by AWS it would presumably automatically pick them up, but if it's preferred I can update to specifically call out the order instead so there's more control of what's used.

[cuppett]

I changed it to use defaultProvider so if new methods were added by AWS it would presumably automatically pick them up, but if it's preferred I can update to specifically call out the order instead so there's more control of what's used.

It's definitely the way to go if we don't have reservations specifically about the user $HOME directory method being "in there".

[cuppett]

When we made the first change, I read the doc reference as saying "instance profile contains 2 paths, ECS then instance profile". Looking deeper at the code, it appears not the case; they are separate and discrete.

However, this also has a side effect of picking up the user's $HOME directory. For whatever user ID the Nextcloud instance is running as, before ECS and instance profile, it would interrogate $HOME/.aws/credentials. Originally, I didn't want to introduce that wrinkle. It seemed too implicit & nuanced for shared hosting accounts, or folks running the docker container on a random machine under a random UID. Do we want to pick that up as well? I'm not sure....

If we do not want the $HOME directory ini-file behavior, changing the code to something similar would have the desired effect to add the ECS case to the existing ENV and instance profile:

$provider = CredentialProvider::memoize(
    CredentialProvider::chain(
        $this->paramCredentialProvider(),
        CredentialProvider::env(),
        CredentialProvider::ecsCredentials(),
        CredentialProvider::instanceProfile()
    )
);

It may make sense not to inject it unless it passes the same condition in the defaultProvider factory:

!empty(getenv(EcsCredentialProvider::ENV_URI))

Happy to comment/review more if folks have thoughts or comments on this.

[cuppett]

In reviewing the code, we can actually exclude the ini files with a simpler change:

$provider = CredentialProvider::memoize(
    CredentialProvider::chain(
        $this->paramCredentialProvider(),
        CredentialProvider::defaultProvider(['use_aws_shared_config_files' => false])
    )
);

(Double check my syntax.)

This should accomplish what we both were after, picking up any/all desirable future methods (including the current one), but not the $HOME directory ones.

[Imajie]

I saw that option too, but it looks like a little further down it loads the same file from $HOME as a process credential provider, so it doesn't seem like that would fully exclude $HOME from being included

[cuppett]

I saw that option too, but it looks like a little further down it loads the same file from $HOME as a process credential provider, so it doesn't seem like that would fully exclude $HOME from being included

I'm not sure the $HOME is an issue. I'm just not familiar enough with all the different deployment models. If not a big issue, we can just have/take the original commit.

If we go further down this route (trying to avoid the $HOME directory), we should make the function look a bit more like defaultProvider and build up the explicit chain, adding param to the beginning, but including all the others (in order) and excluding those two ini/process calls. We'll want to pick up the WebIdentity one. Similar to the ECS method, that's the one which can use the AWS IAM pod identity work on EKS/OpenShift.

[cuppett]

I've submitted a change to the aws-sdk-php. It seems awkward to re-use the same files for this other shell method without considering that config variable. Also, I'm rusty, but in Java, I didn't see the same sequence or a discrete process provider. Let's give it a minute to see what they say:

aws/aws-sdk-php#2172

[cuppett]

Also, I'm rusty, but in Java, I didn't see the same sequence or a discrete process provider.

I found it.

https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-core/src/main/java/com/amazonaws/auth/DefaultAWSCredentialsProviderChain.java#L46-L52

The profile types are all lumped together:

https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-core/src/main/java/com/amazonaws/auth/profile/ProfilesConfigFile.java#L207-L216

And then, there's a toggle between ECS and instance profile "either/or" versus "both":

https://github.com/aws/aws-sdk-java/blob/master/aws-java-sdk-core/src/main/java/com/amazonaws/auth/EC2ContainerCredentialsProviderWrapper.java#L58-L64

My first AWS SDK PR could take a minute, but I wouldn't mind chasing it with a couple more to make it behave more like the Java SDK. There's also some missing functionality for ECS full URI or an Authorization header in the PHP SDK.

Until then, we can do the most complete thing here with something like this:

$provider = CredentialProvider::memoize(
	CredentialProvider::chain(
		$this->paramCredentialProvider(),
		CredentialProvider::env(),
		CredentialProvider::assumeRoleWithWebIdentityCredentialProvider(),
		!empty(getenv(EcsCredentialProvider::ENV_URI))
			? CredentialProvider::ecsCredentials()
			: CredentialProvider::instanceProfile()
	)
);

[cuppett]

I've submitted a change to the aws-sdk-php. It seems awkward to re-use the same files for this other shell method without considering that config variable. Also, I'm rusty, but in Java, I didn't see the same sequence or a discrete process provider. Let's give it a minute to see what they say:

aws/aws-sdk-php#2172

I've added commits which will allow this work:

CredentialProvider::defaultProvider(['use_aws_shared_config_files' => false])

But until (if) that merges and then we have a bump in 3rdparty to pick it up, we'll need the full thing:

$provider = CredentialProvider::memoize(
	CredentialProvider::chain(
		$this->paramCredentialProvider(),
		CredentialProvider::env(),
		CredentialProvider::assumeRoleWithWebIdentityCredentialProvider(),
		!empty(getenv(EcsCredentialProvider::ENV_URI))
			? CredentialProvider::ecsCredentials()
			: CredentialProvider::instanceProfile()
	)
);

[cuppett]

Looks good, just double check the syntax (can't tell with the markup, but looks like a comma missing).

[cuppett]

Looks good. With any luck, next time we visit this file, we'll be able to use the simpler provider with the config option. Thanks!

[Imajie]

You're welcome!

Looks like the test failures are all marked with "Unreliable test" is there a way to rerun that stage of the build?

[cuppett]

AWS SDK change has merged. Once we bump to 3.171.1, we can switch over to:

CredentialProvider::defaultProvider(['use_aws_shared_config_files' => false])

I'm guessing in the NC22 release.

[cuppett]

@kesselb @rullzer Am I able to merge this or something else going on? I was thinking somebody would snarf this up in one of the betas.

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[rullzer]

/backport to stable21