Prevent Segfault when connecting to non-existent DB with Scram Authentication #730
Conversation
There was a problem hiding this comment.
Code looks good. I left some comments on the tests.
Also CI is failing on Windows. I'm not sure why, but various tests are being skipped on windows. I think we could do the same for the offending test, which seems to be: test_no_database_md5_auth_success
Adding this line at the top of the test should skip it on windows:
case `uname` in MINGW*) return 77;; esac
PS. Alpine is also failing, but that's expected because of: #722
test/test.sh
Outdated
| } | ||
|
|
||
| test_no_database_md5_auth_success() { | ||
| # Testing what happens on successful SCRAM auth connection to non-existent DB |
There was a problem hiding this comment.
| # Testing what happens on successful SCRAM auth connection to non-existent DB | |
| # Testing what happens on successful MD5 auth connection to non-existent DB |
There was a problem hiding this comment.
Good catch, fun times with lazy and copy and pasting
| # Testing what happens on successful SCRAM auth connection to non-existent DB | ||
| # Segfaults have been seen after mock authentication was put in place | ||
|
|
||
| admin "set auth_type='md5'" |
There was a problem hiding this comment.
After testing, it seems that md5 was used here on purpose to trigger the bug. Using scram-256 as auth_type doesn't crash. Would be good to add this detail to the comment above.
There was a problem hiding this comment.
I reran the tests using the current master branch of pgbouncer and saw the segfault occur when auth_type was set to scram or md5 with a scram PW. I've added more detail about the source of the bug to the tests though
| # Testing what happens on successful SCRAM auth connection to non-existent DB | ||
| # Segfaults have been seen after mock authentication was put in place | ||
|
|
||
| admin "set auth_type='md5'" |
There was a problem hiding this comment.
This test didn't fail before. I agree that it makes sense for completeness, but maybe swap the auth_type here too (so use scram-256 instead of md5). Or add all 4 different combinations:
- scramuser1 + md5
- scramuser1 + scram-256
- muser1 + md5
- muser1 + scram-256
There was a problem hiding this comment.
Good suggestion. Added all combinations here except for muser1 + scram since that failed authentication (as its supposed to).
|
Thanks @JelteF for the review! I missed some conditions when I set up those tests so hopefully things will no longer hang on the Windows tests. |
…tication (pgbouncer#730) It was possible to get segfaults when using scram authentication while connecting to non-existent databases. Looks like the issue arises when attempting to save the Scram authentication keys. This PR adds a check that the DB isn't fake before attempting to save the credentials Co-authored-by: Ben Sabath <mbsabath@Bens-MacBook-Pro.local>
We were seeing segfaults occur when using scram authentication to connect to non-existent databases. Looks like the issue arises when attempting to save the Scram authentication keys. This PR adds a check that the DB isn't fake before attempting to save the credentials