Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix source_profile handling #2519

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tchernomax
Copy link

"$profiles" keys are prefix with "profile ", but source_profile attribute isn't so the SDK never found the source_profile.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

how to reproduce

~/.aws/config

[profile source]
…

[profile role]
role_arn = arn:aws:iam::…:role/…
source_profile = source

test.php

<?php

require 'vendor/autoload.php';
use Aws\Sts\StsClient;

$client = new StsClient([
        'region' => 'eu-west-1',
        'version' => '2011-06-15'
]);

$result = $client->getCallerIdentity([]);

print($result);
export AWS_PROFILE=role
php test.php

result: fail
expected result: ok

But if you use this ~/.aws/config (which is ill formatted according to the doc), it works:

[profile source]
…

[profile role]
role_arn = arn:aws:iam::…:role/…
source_profile = profile source

note

follow #2516 , which is already closed

@tchernomax
Copy link
Author

ping @stobrien89 (follow up on #2516)

Copy link
Member

@stobrien89 stobrien89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tchernomax,

Thanks for the follow-up. Good catch- this change makes sense to me. We'd be glad to accept this, but will need the failing test to be adjusted to account for the new (correct) behavior. You're welcome to do this. Otherwise, we should be able to carve out some time to address this next week. Thanks!

@tchernomax
Copy link
Author

@stobrien89

We'd be glad to accept this, but will need the failing test to be adjusted to account for the new (correct) behavior.

In fact I realize the same code handle the ~/.aws/credentials file, which has other rules: https://docs.aws.amazon.com/sdk-for-php/v3/developer-guide/guide_credentials_profiles.html#assume-role-with-profile

I changed my commit adding check to see if the file is ~/.aws/config or ~/.aws/credentials and force-pushed it.

I think the tests should be ok now (I didn't run them locally), can you relaunch the CI ?

Thanks

@stobrien89 stobrien89 closed this Nov 16, 2023
@stobrien89 stobrien89 reopened this Nov 16, 2023
@stobrien89 stobrien89 closed this May 21, 2024
@stobrien89 stobrien89 reopened this May 21, 2024
# prefix with 'profile '
if ($filename == (self::getHomeDir() . '/.aws/config') &&
$sourceProfileName != 'default') {
$sourceProfileName = 'profile ' . $sourceProfileName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you please be able to add a check here for whether profile has been already appended to the profile. By doing this we keep backward compatibility for customers that are already adding the profile to profile names.

@@ -705,6 +705,12 @@ private static function loadRoleProfile(
$sourceProfileName = "";
if (!empty($roleProfile['source_profile'])) {
$sourceProfileName = $roleProfile['source_profile'];
# in ~/.aws/config all the named profile (except 'default') are
# prefix with 'profile '
if ($filename == (self::getHomeDir() . '/.aws/config') &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please remove the condition $filename == (self::getHomeDir() . '/.aws/config') since I do no think is needed. You can leave the other condition $sourceProfileName != 'default'.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants