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 parsing of special comment characters in configuration #766

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

Handle parsing of special comment characters in configuration #766

wants to merge 1 commit into from

Conversation

ckluy31
Copy link
Contributor

@ckluy31 ckluy31 commented Oct 2, 2024 •
edited by chrnorm
Loading

What changed?

In order to fix issue #739, the ini client is loaded with the following options:

  • SpaceBeforeInlineComment : Only interprets the inline comment if there is a space preceding the # character
  • UnescapeValueCommentSymbols : Prevents the # from being automatically escaped (i.e. #) as part of the output

Why?

Users were unable to use URLs that included the # character, which was typically used for inline comments. A thorough description of the issue can be seen here #739.

Fixes #739.

How did you test it?

Using the dummy configuration with # characters in git registry https://github.com/common-fate/granted-registry/blob/main/config2:

# this is a dummy config 
[profile demo-main_dummy]
granted_sso_start_url = https://d-976708da7d.awsapps.com/start\#0000000000042
granted_sso_region = ap-southeast-2
granted_sso_account_id = 632700053629
granted_sso_role_name = AWSAdministratorAccess
region = ap-southeast-2
session_name = {{ .Required.SessionName }}
credential_process = granted credential-process --profile {{ .Profile }} --url https://internal.prod.granted.run

We then generate the registries in .aws/config using dgranted registry add -name test12 -url https://github.com/common-fate/granted-registry/ -prefix-duplicate-profiles

The output observed in .aws/config is:

...

[profile test12.demo-main_dummy]
granted_sso_start_url  = `https://d-976708da7d.awsapps.com/start#0000000000042`
granted_sso_region     = ap-southeast-2
granted_sso_account_id = 632700053629
granted_sso_role_name  = AWSAdministratorAccess
region                 = ap-southeast-2
session_name           =
credential_process     = granted credential-process --profile test12.demo-main_dummy --url https://internal.prod.granted.run

[granted_registry_end test12]

Furthermore, when creating a new profile registry from your local AWS config, this is the generated granted.yml:

[profile test12.demo-main_dummy]
granted_sso_start_url  = `https://d-976708da7d.awsapps.com/start#0000000000042`
granted_sso_region     = ap-southeast-2
granted_sso_account_id = 632700053629
granted_sso_role_name  = AWSAdministratorAccess
region                 = ap-southeast-2
session_name           =
credential_process     = granted credential-process --profile test12.demo-main_dummy --url https://internal.prod.granted.run

Potential risks

Previous configurations that do not have a space before the inline comment will not have it recognised as an inline comment. Instead, it will be recognised as part of the value.

Is patch release candidate?

Link to relevant docs PRs

@chrnorm
Copy link
Contributor

chrnorm commented Oct 4, 2024

I tested this locally, I think this might cause us problems because backticks are not supported by the AWS CLI's config parser.

For example, the following configuration is invalid:

[profile example-backtick]
sso_start_url  = `https://d-12345abcdef.awsapps.com/start`
sso_region     = ap-southeast-2
sso_account_id = 123456789012
sso_role_name  = AWSAdministratorAccess

With the following error:

❯ assume

? Please select the profile you would like to assume: example-backtick
[!] error retrieving IAM Identity Center token from secure storage: The specified item could not be found in the keyring
[✘] operation error SSO OIDC: StartDeviceAuthorization, https response error StatusCode: 400, RequestID: 6bd61cdf-4aa7-419c-8fdd-786a96fd12dc, InvalidRequestException:

Essentially this is caused by a difference between the current ini package we use, and the Python ConfigParser package used in the AWS CLI. We might want to look at https://github.com/bigkevmcd/go-configparser as an alternative library

@chrnorm
Copy link
Contributor

chrnorm commented Oct 7, 2024

I realised we actually swapped away from the configparser library in this PR: #297

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mangled AWS config output when certain characters are present in values
2 participants
@ckluy31 @chrnorm