Skip to content

Commit

Permalink
[10.x] Do not add token to AWS credentials without validating it first (
Browse files Browse the repository at this point in the history 
#48297)

* Remove token from config options when making a new DynamoDB client - including this creates an error with the `AwsClient` (which `DynamoDbClient` extends) in recent versions of the AWS PHP SDK:

```Invalid configuration value provided for "token"...```

* Do not add `token` value to the `credentials` array element _unless it was already present_ within the config.

Adding a blank `token` value into this array element - simply because two other values (`key` and `secret`) happened to be found within the config - can break the `AwsClient`/`S3Client` being built by these managers.

It is also cleaner to have a separate check for this `token` value - rather than assume that because you found two other values, you may as well go ahead and add this third value into the mix too, without having validated it first.

---------

Co-authored-by: Michael Mehmet <>
  • Loading branch information
@mmehmet
mmehmet authored Sep 5, 2023
1 parent abe8656 commit 909ea24
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 2 deletions.
6 changes: 5 additions & 1 deletion src/Illuminate/Cache/CacheManager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,10 +260,14 @@ protected function newDynamodbClient(array $config)

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

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

return new DynamoDbClient($dynamoConfig);
}

Expand Down
6 changes: 5 additions & 1 deletion src/Illuminate/Filesystem/FilesystemManager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,11 @@ protected function formatS3Config(array $config)
$config += ['version' => 'latest'];

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

if (! empty($config['token'])) {

This comment has been minimized.

Sign in to view

Copy link
@robchett

robchett Jun 21, 2024

@mmehmet
This can create a config of just ['token' => '...'] which is not valid AFAIK
We have a case when brefphp/laravel-bridge is setting the token based on AWS Lambda environment, but no key/secret is present. This worked fine in a Laravel 9 env.
I'll unset the bref-bridge set config for now but is it worth opening a PR to make sure the credentials have the required data?

This comment has been minimized.

Sign in to view

Copy link
@mmehmet

mmehmet Jun 25, 2024

Author Contributor

@robchett I'm not sure that's correct

if would actually create a config of ['credentials' => ['token' => '...']] (in fact, the token part of the array is removed below) – which I believe is valid and should work, even (particularly) if no secret/key is present, which I believe is the point of the token

I actually created this PR after having issues with brefphp/laravel-bridge – but in the specific case where there is no secret/key and there is also no token

if this is in fact not working for you, because you do have a token, I would be interested to know more about why it is now failing – particularly if it used to work for you in Laravel 9

(I assume you upgraded from 9 to 10? AFAIK the config array should have the same structure between the two?)

This comment has been minimized.

Sign in to view

Copy link
@robchett

robchett Jun 25, 2024 •
edited
Loading

The error we were getting was from vendor/aws/aws-sdk-php/src/ClientResolver.php _apply_credentials occurred after upgrading from Laravel 9 to 10

Credentials must be an instance of 'Aws\Credentials\CredentialsInterface, an associative array that contains "key", "secret", and an optional "token" key-value pairs, a credentials provider function, or false.

Before this change that method was not called as no secret/key credentials are present in config (it'll use the Lambda IAM Role instead)

Based on the contents of _apply_credentials it seems that token should only be provided if secret and key are present

Perhaps this solution would be better?

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

This comment has been minimized.

Sign in to view

Copy link
@mmehmet

mmehmet Jun 26, 2024

Author Contributor

@robchett that sounds perfectly reasonable

I would suggest the same solution should also be applied to:
src/Illuminate/Cache/CacheManager.php
$config['credentials']['token'] = $config['token'];
}

return Arr::except($config, ['token']);
Expand Down

0 comments on commit 909ea24

Please sign in to comment.