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.

Sign up for GitHub

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

Jump to bottom

Handle sso session names with quotes/spaces #2895

Merged
merged 4 commits into from
Aug 9, 2023
Merged

Handle sso session names with quotes/spaces #2895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a5903e
Handle sso session names with quotes/spaces
alextwoods Aug 9, 2023
dc05315
Add changelog
alextwoods Aug 9, 2023
ef5d075
Minor cleanups
alextwoods Aug 9, 2023
733322a
Fix tests
alextwoods Aug 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions gems/aws-sdk-core/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
Unreleased Changes
------------------

* Issue - Add support for sso-session names with whitespace configured by the CLI `aws sso configure` command (#2895).

3.180.2 (2023-08-07)
------------------

Expand Down
38 changes: 21 additions & 17 deletions gems/aws-sdk-core/lib/aws-sdk-core/shared_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,12 +359,8 @@ def sso_credentials_from_profile(cfg, profile)
!(prof_config.keys & SSO_CREDENTIAL_PROFILE_KEYS).empty?

if sso_session_name = prof_config['sso_session']
sso_session = cfg["sso-session #{sso_session_name}"]
unless sso_session
raise ArgumentError,
"sso-session #{sso_session_name} must be defined in the config file. " \
"Referenced by profile #{profile}"
end
sso_session = sso_session(cfg, profile, sso_session_name)

sso_region = sso_session['sso_region']
sso_start_url = sso_session['sso_start_url']

Expand All @@ -389,7 +385,7 @@ def sso_credentials_from_profile(cfg, profile)
sso_role_name: prof_config['sso_role_name'],
sso_session: prof_config['sso_session'],
sso_region: sso_region,
sso_start_url: prof_config['sso_start_url']
sso_start_url: sso_start_url
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change should be safe since the non-legacy flow does NOT use the sso_start_url, see: https://github.com/aws/aws-sdk-ruby/blob/version-3/gems/aws-sdk-core/lib/aws-sdk-core/sso_credentials.rb#L82

)
end
end
Expand All @@ -402,16 +398,7 @@ def sso_token_from_profile(cfg, profile)
!(prof_config.keys & SSO_TOKEN_PROFILE_KEYS).empty?

sso_session_name = prof_config['sso_session']
sso_session = cfg["sso-session #{sso_session_name}"]
unless sso_session
raise ArgumentError,
"sso-session #{sso_session_name} must be defined in the config file." \
"Referenced by profile #{profile}"
end

unless sso_session['sso_region']
raise ArgumentError, "sso-session #{sso_session_name} missing required parameter: sso_region"
end
sso_session = sso_session(cfg, profile, sso_session_name)

SSOTokenProvider.new(
sso_session: sso_session_name,
Expand Down Expand Up @@ -469,5 +456,22 @@ def determine_profile(options)
ret ||= 'default'
ret
end

def sso_session(cfg, profile, sso_session_name)
# aws sso-configure may add quotes around sso session names with whitespace
sso_session = cfg["sso-session #{sso_session_name}"] || cfg["sso-session '#{sso_session_name}'"]

unless sso_session
raise ArgumentError,
"sso-session #{sso_session_name} must be defined in the config file. " \
"Referenced by profile #{profile}"
end

unless sso_session['sso_region']
Copy link
Contributor

Choose a reason for hiding this comment

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

sso_credentials_from_profile is not validating sso_region existing - SSO_CREDENTIAL_PROFILE_KEYS does not require sso_region. There is a branch that checks if it exists on profile and compares it to sso_region if present. I think this refactor is maybe breaking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SSOCredentials will fail if sso_region is unset, so this moves the failure case up to here (with hopefully slightly. more info).

raise ArgumentError, "sso-session #{sso_session_name} missing required parameter: sso_region"
end

sso_session
end
end
end
27 changes: 25 additions & 2 deletions gems/aws-sdk-core/spec/aws/credential_resolution_chain_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ module Aws

it 'prefers sso credentials over assume role' do
expect(SSOCredentials).to receive(:new).with(
sso_start_url: nil,
sso_start_url: 'START_URL',
sso_region: 'us-east-1',
sso_account_id: 'SSO_ACCOUNT_ID',
sso_role_name: 'SSO_ROLE_NAME',
Expand Down Expand Up @@ -161,6 +161,29 @@ module Aws
).to eq('SSO_AKID')
end

it 'loads SSO credentials from when the session name has quotes' do
expect(SSOCredentials).to receive(:new).with(
sso_start_url: 'START_URL',
sso_region: 'us-east-1',
sso_account_id: 'SSO_ACCOUNT_ID',
sso_role_name: 'SSO_ROLE_NAME',
sso_session: 'sso test session'
).and_return(
double(
'creds',
set?: true,
credentials: double(access_key_id: 'SSO_AKID')
)
)
client = ApiHelper.sample_rest_xml::Client.new(
profile: 'sso_creds_session_with_quotes',
token_provider: nil
)
expect(
client.config.credentials.credentials.access_key_id
).to eq('SSO_AKID')
end

it 'raises when attempting to load an incomplete SSO Profile' do
expect do
ApiHelper.sample_rest_xml::Client.new(
Expand Down Expand Up @@ -366,7 +389,7 @@ module Aws

it 'supports :source_profile from sso credentials' do
expect(SSOCredentials).to receive(:new).with(
sso_start_url: nil,
sso_start_url: 'START_URL',
sso_region: 'us-east-1',
sso_account_id: 'SSO_ACCOUNT_ID',
sso_role_name: 'SSO_ROLE_NAME',
Expand Down
3 changes: 3 additions & 0 deletions gems/aws-sdk-core/spec/aws/ini_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ module Aws
[sso-session dev]
sso_region = us-east-1

[sso-session 'profile with spaces']
sso_region = us-east-1

[services test-services]
s3 =
endpoint_url = https://localhost:8000
Expand Down
10 changes: 10 additions & 0 deletions gems/aws-sdk-core/spec/fixtures/credentials/mock_shared_config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,16 @@ sso_account_id = 123456789012
source_profile = sso_creds
role_arn = arn:aws:iam::123456789012:role/bar

[profile sso_creds_session_with_quotes]
sso_account_id = SSO_ACCOUNT_ID
sso_role_name = SSO_ROLE_NAME
sso_session = sso test session
region = us-west-1

[sso-session 'sso test session']
sso_region = us-east-1
sso_start_url = START_URL

[profile sts_regional]
aws_access_key_id = AKID
aws_secret_access_key = SECRET
Expand Down
Loading