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

postgres: Fix connection string #92

Merged
merged 1 commit into from
Nov 18, 2022

Conversation

dave-tucker
Copy link
Contributor

@dave-tucker dave-tucker commented Nov 18, 2022

Usernames, Passwords and Database Names could in theory contain special characters - Secure passwords are highly likely to. With the current code, a password with special characters will result in the following error:

$ docker run -d -e POSTGRES_USER="ad&min" -e POSTGRES_DB="ip@m" -e POSTGRES_PASSWORD="i.am<password>" -p 5432:5432 postgres
$ ./bin/server pg --password="i.am<password>" --host=localhost --user="ad&min" --dbname="ip@m"

2022/11/18 00:35:01 Error in cli: unable to connect to database:parse "postgres://ad&min:i.am<password>@localhost:5432/ip@m?sslmode=disable": net/url: invalid userinfo

This is because the current connection string is in URI format - to make this work those fields should be URL escaped.
A more compact approach is to use the connection string format shown in the sqlx example
With this patch applied the connection will proceed successfully.

Signed-off-by: Dave Tucker dave@dtucker.co.uk

@dave-tucker dave-tucker requested a review from a team as a code owner November 18, 2022 00:42
Usernames, Passwords and Database Names could in theory contain special
characters - Secure passwords are highly likely to.
With the current code, a password with special characters will result in
the following error:

```console
$ docker run -d -e POSTGRES_USER="ad&min" -e POSTGRES_DB="ip@m" -e POSTGRES_PASSWORD="i.am<password>" -p 5432:5432 postgres
$ ./bin/server pg --password="i.am<password>" --host=localhost --user="ad&min" --dbname="ip@m"

2022/11/18 00:35:01 Error in cli: unable to connect to database:parse "postgres://ad&min:i.am<password>@localhost:5432/ip@m?sslmode=disable": net/url: invalid userinfo
```

With this patch applied the connection will proceed successfully.

Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
@dave-tucker dave-tucker changed the title postgres: URL Encode Fields postgres: Fix connection string Nov 18, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #92 (e7f4dac) into master (f12556b) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   70.72%   70.75%   +0.02%     
==========================================
  Files          10       10              
  Lines        1117     1118       +1     
==========================================
+ Hits          790      791       +1     
  Misses        207      207              
  Partials      120      120              
Impacted Files Coverage Δ
postgres.go 73.33% <100.00%> (+1.90%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@majst01
Copy link
Contributor

majst01 commented Nov 18, 2022

Thanks for this finding !

@majst01 majst01 merged commit c865256 into metal-stack:master Nov 18, 2022
@ralfonso
Copy link
Contributor

ralfonso commented Feb 15, 2023

This change results in failed connections when one of the parameters is empty. For my use case, I connect without using a password, which results in the first word after password= being interpreted as the password. i.e.

host=host user=user password= dbname=mydb port=port sslmode=disable

Is understood as:

host: host
user: user
password: dbname=mydb
port: port
sslmode: disable

According to the libpq docs here, the way to pass empty arguments (or arguments with spaces) is to use single quotes and escape each value.

Update: I've discovered that CockroachDB does not support the form with single quoted param values. Perhaps reverting to the URI format (and encoding the values) should be considered here.

Update 2: I've opened a PR to do just that #102

ralfonso added a commit to ralfonso/go-ipam that referenced this pull request Feb 16, 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.

None yet

4 participants