Strip key, secret and token from root config options on aws clients

[georgeboot]

Fixes #44977

Related to aws/aws-sdk-php#2567

[driesvints]

I'm super scared that this will break apps on older versions of the AWS PHP SDK. Is there any way we can verify this on a range of AWS PHP SDK versions?

[georgeboot]

AFAIK the credentials have always only been inside the credentials key. They were also supplied at root level, and ignored by the sdk as they were not valid keys. Until now.

So unless the SDK ever used key, secret and token at root level, I don't think this will cause issues.
If it was, we need to find out what version and do a version check in the code for it.

[SamRemis]

Hi there :)
Here from the PHP SDK- 'key' and 'secret' shouldn't be an issue, just 'token'; some new services are adding it as an authentication scheme this week, so we need to give customers some way of passing in their own. There's no valid reason that I could think of right now where we would need you to remove the 'key' and 'secret' from the base level. Shouldn't harm anything either though since those values would just be ignored right now.

[georgeboot]

@SamRemis how big is the chance that key or secret ever get added as parameters and this becomes an issue again?

[driesvints]

Thanks @SamRemis. If there's also no reason to keep them then we might be able to just accept the entire PR. Thanks @georgeboot for looking into it.

[georgeboot]

@driesvints revert last commit if you want

[georgeboot]

Any services I've not included that should be?

[SamRemis]

I'm super scared that this will break apps on older versions of the AWS PHP SDK. Is there any way we can verify this on a range of AWS PHP SDK versions?

Here in the V2 docs of the SDK you can see it as a nested array, and that's quite old. It doesn't look like it would have accepted those keys on the top level of the array. I can look into the history of it later today to confirm, but it'll take some time. If it is, then we can definitely push out something to fix this.

@SamRemis how big is the chance that key or secret ever get added as parameters and this becomes an issue again?

The only use case I can see for this is if for some reason customers start asking for it. There's no reason I can think of that either of these would become a valid base level config value, but I don't have a crystal ball.

[driesvints]

It's used here in V2 of the SDK

We only allow v3 in Laravel v9 so that should be okay.

[driesvints]

@SamRemis just found another issue with the token PR on aws php sdk. Left a comment on it: aws/aws-sdk-php#2517

[icreatestuff]

I am experiencing the same issue as described and fixed here for Laravel v9 but in v8 running:

• Laravel 8.83.27
• AWS SDK PHP 3.255.8

Does there need to be a similar patch made in L8? Thanks

[driesvints]

@icreatestuff Laravel v8 isn't maintained anymore. Best that you upgrade as soon as you can.

[Mesuva]

Would be nice to have a patch for V8.

For live production systems running serverless we're not going to constantly be upgrading our major Laravel version, but still might want to do updates to pull in security or bugfixes across libraries.

[dennisprudlo]

There's no need to upgrade constantly. Laravel 8 security fixes were supported 2-3 years. Big fixes ended half a year ago.

[mmehmet]

@georgeboot I'm experiencing this issue using the DynamoDB Cache driver - which I note when I look at the Illuminate\Support\Cache\CacheManager class I see a method newDynamodbClient() which appears to want to pass in a token value into the constructor of a new DynamoDbClient (which extends AwsClient)

    protected function newDynamodbClient(array $config)
    {
        $dynamoConfig = [
            'region' => $config['region'],
            'version' => 'latest',
            'endpoint' => $config['endpoint'] ?? null,
        ];

        if (isset($config['key'], $config['secret'])) {
            $dynamoConfig['credentials'] = Arr::only(
                $config, ['key', 'secret', 'token']
            );
        }

        return new DynamoDbClient($dynamoConfig);
    }

I could be reading this wrong, but I believe that 'token' should be removed from this array?

for ref: I'm on Laravel v9.52.10

edit: I also note if I remove 'key' and 'secret' from my dynamodb config under cache.stores.dynamodb (which is not a great solution) it works just fine

[anuragnandan]

Hey @mmehmet I tried removing key and secret from cache config for dynamodb and Im still getting the same error. Did you had to do anything more to make it work ?

[mmehmet]

Hey @mmehmet I tried removing key and secret from cache config for dynamodb and Im still getting the same error. Did you had to do anything more to make it work ?

@anuragnandan sorry I should have clarified, this was for an AWS instance - so authentication happens via IAM and credentials are unused/ignored, within the AWS-to-AWS ecosystem

if you actually do need to authenticate using credentials (eg from an instance running on a dev machine and not within AWS) then this will not work - since you do actually need a key and secret...

in that case, I would recommend building your own client, which extends the above or at least also extends an AwsClient - and just leave out the token value in your implementation:

        if (isset($config['key'], $config['secret'])) {
            $dynamoConfig['credentials'] = Arr::only(
                $config, ['key', 'secret']
            );
        }

[anuragnandan]

@mmehmet I'm having an issue running the app on a Lambda which uses AWS Role. It seems to have started when 909ea24 was merged and we upgraded our framework. I have tried removing key and secret from cache.stores.dynamodb but it didnt seem to work as you suggested.

[mmehmet]

did you only remove key and secret - as in you did not also remove token?

because the code will try to use that if it finds one

if (! empty($config['token'])) {
    $dynamoConfig['credentials']['token'] = $config['token'];
}

[anuragnandan]

Yes, I tried removing just key and secret first and then key, secret and token from config and have the same issue.

[anuragnandan]

@mmehmet Nothing seem to work on Lambda, Im applying below patch to make it work for now. Let me know if I can try anything else.

--- vendor/laravel/framework/src/Illuminate/Cache/CacheManager.php	2023-09-19 15:25:04.000000000 +0000
+++ CacheManager.php	2023-11-01 20:02:45.693625146 +0000
@@ -92,7 +92,7 @@
             return $this->callCustomCreator($config);
         }

-        $driverMethod = 'create'.ucfirst($config['driver']).'Driver';
+        $driverMethod = 'create' . ucfirst($config['driver']) . 'Driver';

         if (method_exists($this, $driverMethod)) {
             return $this->{$driverMethod}($config);
@@ -260,14 +260,11 @@

         if (! empty($config['key']) && ! empty($config['secret'])) {
             $dynamoConfig['credentials'] = Arr::only(
-                $config, ['key', 'secret']
+                $config,
+                ['key', 'secret', 'token']
             );
         }

-        if (! empty($config['token'])) {
-            $dynamoConfig['credentials']['token'] = $config['token'];
-        }
-
         return new DynamoDbClient($dynamoConfig);
     }