libssh2: Print user with verbose flag #16430
Conversation
This change: - Breaks out the existing print out of the LIBSSH2_DEBUG compile-time flag - Adds (single) quotation marks around the string to better expose the actual value - Adds a NULL print, mirroring other verbose prints in libssh2 Why was this done? I was trying out the `sftp` option in `curl`, and found myself hitting an issue where I was not able to get curl to tell me which username it was using to connect to a host. With this change, the `User: ` line is printed with `-v`, just like other SSH verbose prints. Instead of using the pattern used with *SSH MD5 public key*, where a ternary is used to print `NULL` on NULL values, it is using a different branch to add quotes around the string value. The quotes around the string value are used to better expose to the user an empty string value, compared to "no-value".
| infof(data, "User: '%s'", conn->user); | ||
| } | ||
| else { | ||
| infof(data, "User: NULL"); |
There was a problem hiding this comment.
Would it make sense to do the same for Password:, for consistency?
There was a problem hiding this comment.
Good question, this slots in with the two final "further thought" points. I'll leave this to the maintainers to make the call.
Though the reason I did not do it is I considered the latter a purely debug print (i.e. compile-time flag).
There was a problem hiding this comment.
It'd be nice to keep this consistent IMO.
| @@ -3167,10 +3167,13 @@ static CURLcode ssh_connect(struct Curl_easy *data, bool *done) | |||
|
|
|||
| sshc = &conn->proto.sshc; | |||
|
|
|||
| #ifdef CURL_LIBSSH2_DEBUG | |||
There was a problem hiding this comment.
IMO this would better be kept under the DEBUG flag.
Username is sensitive information and should not be
dumped into the log.
There was a problem hiding this comment.
I don't intend to question your opinion on this, but I'm wondering if I missed that there is precedent around this. (To be fair, I did not try to find documentation about what is considered sensitive and not to be logged.)
My quick and biased first thought seems to indicate it's at least not sensitive for HTTP.
~ $ curl -v 'http://obviouslyusername:obviouslypassword@example.com/' 2>&1 | grep obviously
* Server auth using Basic with user 'obviouslyusername'
~ $ curl -v --user obviouslyusername:obviouslypassword 'http://@example.com/' 2>&1 | grep obviously
* Server auth using Basic with user 'obviouslyusername'
There was a problem hiding this comment.
It's been added in 2003. It was a different environment. That username was sent over the wire unencrypted anyway. Nowadays I think it's not prudent to dump credentials into a log.
There was a problem hiding this comment.
I think user name is still fine, and since curl already shows the user name in the log for other protocols it seems inconsistent to deny it here. It is useful for debugging.
|
Analysis of PR #16430 at 4697b40c: Test http/test_09_push.py::TestPush::test_09_02_h2_push failed, which has NOT been flaky recently, so there could be a real issue in this PR. Generated by Testclutch |
|
Thanks! |
Hi! 👋
A small quality-of-life improvement, I think.
Do tell if I'm missing something here, and there were any reason not to print out the
conn->uservalue that ends-up used for the connection.This change:
Why was this done?
I was trying out the
sftpoption incurl, and found myself hitting an issue where I was not able to get curl to tell me which username it was using to connect to a host.With this change, the
User:line is printed with-v, just like other SSH verbose prints.Instead of using the pattern used with SSH MD5 public key, where a ternary is used to print
NULLon NULL values, it is using a different branch to add quotes around the string value.The quotes around the string value are used to better expose to the user an empty string value, compared to "no-value".
Further thoughts:
SSH, to formSSH User: 'samuel'?NULLprint? Is it even a possibility thatNULLus present at this location? (I presumed so, from the existing check.)