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

cli: passwords with special characters cannot be passed in URL #35998

Closed
joshimhoff opened this issue Mar 20, 2019 · 3 comments · Fixed by #65460
Closed

cli: passwords with special characters cannot be passed in URL #35998

joshimhoff opened this issue Mar 20, 2019 · 3 comments · Fixed by #65460
Labels
A-authentication Pertains to authn subsystems A-cc-enablement Pertains to current CC production issues or short-term projects A-cli-client CLI commands that pertain to using SQL features A-cli-flags Pertains to CLI flag handling common to all CLI commands C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. docs-done docs-known-limitation S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.

Comments

@joshimhoff
Copy link
Collaborator

Describe the problem

@vilterp @mberhault @nstewart

The managed console has a model that shows a cockroach command that lets the user connect securely to their cluster. Here is an example command, with a fake password:

cockroach sql --url 'postgres://josh:password>@aws-us-east-1.laughing-ladybug.cockroachlabs.cloud:26257/defaultdb?sslmode=verify-full&sslrootcert=/Users/josh/Downloads/laughing-ladybug-ca.crt'

If your password has special characters in it (e.g. ">"), the cmd fails. I used a password manager to generate my password, and so it had five or so different special characters in it. The error message looks like this:

Error: invalid argument "postgres://josh:password>@aws-us-east-1.laughing-ladybug.cockroachlabs.cloud:26257/defaultdb?sslmode=verify-full&sslrootcert=/Users/josh/Downloads/laughing-ladybug-ca.crt" for "--url" flag: parse postgres://josh:password>@aws-us-east-1.laughing-ladybug.cockroachlabs.cloud:26257/defaultdb?sslmode=verify-full&sslrootcert=/Users/josh/Downloads/laughing-ladybug-ca.crt: net/url: invalid userinfo
Failed running "sql"

It works with psql tho:

psql 'postgres://josh:password>@aws-us-east-1.laughing-ladybug.cockroachlabs.cloud:26257/defaultdb?sslmode=verify-full&sslrootcert=/Users/josh/Downloads/laughing-ladybug-ca.crt'

cockroach works if you don't put the password in the URL; cockroach then prompts you for a password and there seems to be no issue even with special characters.

On the managed side, we are gonna work around this issue by not including the password in the cockroach command we display to users. Perhaps we should fix the broader issue tho.

Nate found the following issue on stackoverflow:

https://stackoverflow.com/questions/48671938/go-url-parsestring-fails-with-certain-user-names-or-passwords

The issue looks relevant. Could we call url.QueryEscape()?

To Reproduce

Connect to cluster with cockroach command with special character such as ">" in password part of connection URL. e.g.:

cockroach sql --url 'postgres://josh:password>@aws-us-east-1.laughing-ladybug.cockroachlabs.cloud:26257/defaultdb?sslmode=verify-full&sslrootcert=/Users/josh/Downloads/laughing-ladybug-ca.crt'

Expected behavior

Ideally this would work without requiring user to escape the password. At the very least the error message could be clearer.

Environment:

$ cockroach version
Build Tag: v2.1.5
Build Time: 2019/02/19 16:35:28
Distribution: CCL
Platform: darwin amd64 (x86_64-apple-darwin18.2.0)
Go Version: go1.11.5
C Compiler: 4.2.1 Compatible Apple LLVM 10.0.0 (clang-1000.11.45.5)
Build SHA-1: 1634c6b
Build Type: release

@awoods187 awoods187 added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Apr 22, 2019
@awoods187 awoods187 added this to Triage in Bulk IO via automation Apr 22, 2019
@rolandcrosby rolandcrosby removed this from Triage in Bulk IO Jul 8, 2019
@knz knz added this to To do in DB Server & Security via automation Apr 24, 2020
@knz knz changed the title cockroach command doesn't escape passwords in connection strings the way that psql does cli: passwords with special characters cannot be passed in URL Apr 24, 2020
@knz knz moved this from To do to Legacy debt in DB Server & Security Apr 24, 2020
@knz knz added the S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. label Jun 15, 2020
@knz knz added A-cli-client CLI commands that pertain to using SQL features A-cli-flags Pertains to CLI flag handling common to all CLI commands and removed A-cli labels Mar 20, 2021
@knz
Copy link
Contributor

knz commented Mar 20, 2021

the issue for this is in the go URL parser. We can't just blindly call an unescape function; this would make the behavior break for folk who were using % in their passwords previously.

@craig craig bot closed this as completed in edc8333 Jun 8, 2021
DB Server & Security automation moved this from Tech debt to Done 21.2 Jun 8, 2021
@knz knz added A-authentication Pertains to authn subsystems A-cc-enablement Pertains to current CC production issues or short-term projects labels Jul 29, 2021
@u007
Copy link

u007 commented Nov 23, 2021

i encounter similar issue, ive to url encode my password

@knz
Copy link
Contributor

knz commented Nov 23, 2021

we also now support the password as a query argument at the end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-authentication Pertains to authn subsystems A-cc-enablement Pertains to current CC production issues or short-term projects A-cli-client CLI commands that pertain to using SQL features A-cli-flags Pertains to CLI flag handling common to all CLI commands C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. docs-done docs-known-limitation S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption.
Projects
DB Server & Security
  
Done 21.2
Development

Successfully merging a pull request may close this issue.

6 participants