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

Mongo doesnt allow periods in usernames #11872

Merged
merged 6 commits into from
Jun 24, 2021
Merged

Mongo doesnt allow periods in usernames #11872

merged 6 commits into from
Jun 24, 2021

Conversation

mr-miles
Copy link
Contributor

We found that the default username template can create invalid usernames which aren't accepted by mongo. Replacing the dots with an allowed character fixes it.

@mr-miles mr-miles requested a review from a team June 15, 2021 21:52
@vercel vercel bot temporarily deployed to Preview – vault June 15, 2021 21:52 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 15, 2021 21:52 Inactive
@vishalnayak
Copy link
Member

@mr-miles The changes looks good to me. Would you be able to add a test to this effect?

@kalafut
Copy link
Contributor

kalafut commented Jun 16, 2021

@pcman312 Any issues with this change? I also wonder if there are other special characters to avoid. Should it be listed in CHANGES?

@mr-miles
Copy link
Contributor Author

Will attempt to add a test.

SO says the one other restriction on keys is that they can't start with a dollar
https://stackoverflow.com/questions/7975185/mongodb-query-over-a-hash-with-special-chars-in-keys/7976235#7976235

so I think the default template is safe

Copy link
Contributor

@pcman312 pcman312 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Can you also update the API docs to reflect the updated default template?
https://github.com/hashicorp/vault/blob/master/website/content/api-docs/secret/databases/mongodb.mdx#parameters

Thanks!

@@ -21,7 +21,7 @@ import (
const (
mongoDBTypeName = "mongodb"

defaultUserNameTemplate = `{{ printf "v-%s-%s-%s-%s" (.DisplayName | truncate 15) (.RoleName | truncate 15) (random 20) (unix_time) | truncate 100 }}`
defaultUserNameTemplate = `{{ printf "v-%s-%s-%s-%s" (.DisplayName | replace "." "-" | truncate 15) (.RoleName | replace "." "-" | truncate 15) (random 20) (unix_time) | truncate 100 }}`
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good! What do you think about moving the replace to the end so it only needs to be done once instead of for each of the two fields?

{{ printf "v-%s-%s-%s-%s" (.DisplayName | truncate 15) (.RoleName | truncate 15) (random 20) (unix_time) | replace "." "-" | truncate 100 }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Docs and template updated

@pcman312 pcman312 self-assigned this Jun 17, 2021
Update template in docs
@vercel vercel bot temporarily deployed to Preview – vault June 18, 2021 08:44 Inactive
@vercel vercel bot temporarily deployed to Preview – vault June 18, 2021 08:49 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 18, 2021 08:52 Inactive
@mr-miles
Copy link
Contributor Author

Test added also

@kalafut
Copy link
Contributor

kalafut commented Jun 18, 2021

Thanks for the updates. Can you please add a changelog file to this PR? The /changelog folder has examples.

@vercel vercel bot temporarily deployed to Preview – vault June 18, 2021 15:35 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook June 18, 2021 15:35 Inactive
@mr-miles
Copy link
Contributor Author

done (and hopefully correctly!)

@vishalnayak
Copy link
Member

Thanks for submitting this!

@vishalnayak vishalnayak merged commit 160c409 into hashicorp:main Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants