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

windows: support special characters in the password for FSx #3669

Merged
merged 1 commit into from
May 19, 2023

Conversation

rawahars
Copy link
Contributor

@rawahars rawahars commented May 3, 2023

Summary

Presently, When a password contains characters that contain escape sequences such as a quotation mark PowerShell fails the command. Reference: #3270

To mitigate the same and support handling of special characters, we are making the change to use single quoted strings. This means that the password is passed-on to Powershell as it is without any substitutions. Reference: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-5.1#single-quoted-strings

Implementation details

Instead of using double quotes, we are using single quotes which means that Powershell would be passed-on a value without any substitution.

Testing

Created an ECS instance with the custom agent. We used a password which had many special characters.
Tested that the task with FSx volume was running successfully on the instance,

New tests cover the changes:
Yes

Description for the changelog

windows: support special characters in the password for FSx

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@rawahars rawahars requested a review from a team as a code owner May 3, 2023 12:16
@rawahars rawahars requested a review from a team May 3, 2023 12:16
arun-annamalai
arun-annamalai previously approved these changes May 3, 2023
singholt
singholt previously approved these changes May 3, 2023
@rawahars
Copy link
Contributor Author

rawahars commented May 3, 2023

Please do not merge yet. I realised that these changes work for special chars but not for ". Let me look into that more to figure out the fix.

@rawahars rawahars dismissed stale reviews from singholt and arun-annamalai via 56bebd5 May 4, 2023 19:54
@rawahars rawahars force-pushed the dev branch 4 times, most recently from b553bdc to 332235f Compare May 9, 2023 20:00
Presently, When a password contains characters that contain escape sequences such as a quotation mark PowerShell fails the command. Reference: aws#3270

To mitigate the same and support handling of special characters, we are making the change to use single quoted strings. This means that the password is passed-on to Powershell as it is without any substitutions. Reference:
https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules?view=powershell-5.1#single-quoted-strings
@rawahars
Copy link
Contributor Author

rawahars commented May 10, 2023

This PR is ready for review now.

We have added the support for special characters including ", \, and '.

However, when using Secrets Manager, the password cannot have any escape characters, more specifically the JSON representation cannot have any escape characters. For example- Microsoft"1234 is not a valid supported password when using ASM. The reason for this limitation is that ASM stores these key-pairs as a JSON string and therefore, the JSON representation which it stores it in would be Microsoft\"1234 which is the escaped string. Now, ASM escapes the entire string again before sending it in the response. When we unmarshall the same in agent, we are still left with the original escapes in the password. This leads to incorrect password while authentication.
For example-
Consider that we are storing password Microsoft"1234 and user Admin in ASM. The JSON representation for the same is {"username":"Admin","password":"Microsoft\"1234"}". This is what is stored in ASM. When we call GetSecretValue API, ASM constructs a string of the JSON where it would escape the sequence again- "{\"username\":\"Admin\",\"password\":\"Microsoft\\\\"1234\"}". This is returned in the API response. Now during our unmarshalling, we would get password as Microsoft\"1234 which is not the same as the actual password.

This unfortunately is the limitation of Secrets Manager and therefore, any username or password which has escape characters in json representation cannot be used here.

Above is not exactly true. This becomes a bug when we use AWSCLI to set the secret in Secret Manager. However, when we use Console or SDK to set the secret, we can use escaped characters as well.

So when we use Console/SDK, we can directly set the password. Let's assume we are storing password Microsoft"1234 and user Admin in ASM. It will be stored in ASM as {"username":"Admin","password":"Microsoft\"1234"} in plaintext.
This is returned appropriately to the agent and then it can unmarshall it in the correct password.

However, when we use CLI, we need to create the JSON string which leads to double escaping. For example, in above example, we will have to enter "{\"username\":\"Admin\",\"password\":\"Microsoft\\\\"1234\"}" as the input string. This therefore, gets stored as {"username":"Admin","password":"Microsoft\\\"1234"} in plaintext in ASM. Then we get an incorrect value in agent (Microsoft\"1234) leading to authentication failure on the node.

@singholt @jterry75 @arun-annamalai

@arun-annamalai
Copy link
Contributor

Can we work with doc writers to state this limitation somewhere on public docs so customers dont have future issues with this?

@arun-annamalai arun-annamalai self-requested a review May 11, 2023 01:45
@jterry75
Copy link
Contributor

jterry75 commented May 11, 2023

Hang on dont merge this

@rawahars
Copy link
Contributor Author

@arun-annamalai Our fix works for all the special characters including ". I encountered a corner case when using CLI for storing the password. I have updated the comment above to reflect the same.

Copy link
Contributor

@YashdalfTheGray YashdalfTheGray left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for making this change!

@arun-annamalai arun-annamalai merged commit 75c5d33 into aws:dev May 19, 2023
@ubhattacharjya ubhattacharjya mentioned this pull request May 24, 2023
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.

ECS agent unable to mount a fsx window volume that contains a quotation mark in password
6 participants