go-mysql password sanitizing support

[aircraft-cerier]

Issue #, if available:
#131 and #160 but more general
Description of changes:
Adds a case to the stripPasswords function to cover the case for using the go-sql-driver/mysql (https://github.com/go-sql-driver/mysql#dsn-data-source-name). The current code does not support stripping the passwords from the default go-mysql DSN format:

The Data Source Name has a common format, like e.g. PEAR DB uses it, but without type-prefix (optional parts marked by squared brackets):

[username[:password]@][protocol[(address)]]/dbname[?param1=value1&...&paramN=valueN]

A DSN in its fullest form:

username:password@protocol(address)/dbname?param=value

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[bhautikpip]

Hi @aircraft-cerier,

Thanks for opening up this PR. There is one missing case in this logic. when there is not password this will strip the user. For example in this case, "user@tcp(localhost:5555)/dbname?tls=skip-verify&autocommit=true" logic in the PR will return this string "@tcp(localhost:5555)/dbname?tls=skip-verify&autocommit=true". Also, I am not sure if this logic affects other drivers.

[aircraft-cerier]

Hi @aircraft-cerier,

Thanks for opening up this PR. There is one missing case in this logic. when there is not password this will strip the user. For example in this case, "user@tcp(localhost:5555)/dbname?tls=skip-verify&autocommit=true" logic in the PR will return this string "@tcp(localhost:5555)/dbname?tls=skip-verify&autocommit=true". Also, I am not sure if this logic affects other drivers.

@bhautikpip thanks for your feedback.

Fixed to accommodate no password being included in the DSN. Also separated tests for pre Go 1.11 and Go 1.11 and later. As pre Go 1.11, the DSN for MySQL would be parsed as a URL and therefore wasn't an issue, but in Aug of 2019 the Go team made some changes to URL parsing as a security fix in 1.13 and backported the change to 1.11.13 and 1.12.8.

Specifically, the part that affects our issue is that they rejected parsing of URLs where the port includes anything other than digits. In the case of the MySQL DSN, the port is parsed as 3306), which used to be OK (as long as nothing tried to actually use that port for anything, which X-Ray doesn't). But now it's rejected and causes parsing to return an error. Thanks to @idubinskiy for helping figure this out!

golang/go#29098

[bhautikpip]

Hi @aircraft-cerier,

Thanks for your contribution. Unfortunately we might not be able to merge this PR since it modifies the logic inside stripPassword because we are going to merge custom SQL driver support PR (https://github.com/aws/aws-xray-sdk-go/pull/169/files#) so this logic might affect other drivers dsn string. Moreover, I am currently working on sanitizing strategy (#193) and we will have both the options custom sanitizer if customer wants to provide their sanitizer and default sanitizer implemented by us.

[bhautikpip]

Hi @aircraft-cerier,

Test case have failed in Travis CI 1.11 and 1.12 Go runtime.

[aircraft-cerier]

Hi @aircraft-cerier,

Test case have failed in Travis CI 1.11 and 1.12 Go runtime.

Hi @bhautikpip

The test cases are failing in Travis because the .travis.yml file is specifying to use versions "1.11" and "1.12"

This is a problem because according to travis' documentation (https://docs.travis-ci.com/user/languages/go/#specifying-a-go-version-to-use) this version specification will default to the original major version. So in this case 1.11.0 and 1.12.0 which do not include the updates to the Go version security fixes (to not parse as URLs) in 1.11.13 and 1.12.8. It passes for 1.13 because this have the security fix from the get go. So I think there are 2 options:

• update the .travis.yml to say 1.11.x and 1.12.x

• Update the tests to allow it to pass with either the DSN or URL set just for 1.11 and 1.12

Which approach should we take?

[bhautikpip]

Hi @aircraft-cerier,
Test case have failed in Travis CI 1.11 and 1.12 Go runtime.

Hi @bhautikpip

The test cases are failing in Travis because the .travis.yml file is specifying to use versions "1.11" and "1.12"

This is a problem because according to travis' documentation (https://docs.travis-ci.com/user/languages/go/#specifying-a-go-version-to-use) this version specification will default to the original major version. So in this case 1.11.0 and 1.12.0 which do not include the updates to the Go version security fixes (to not parse as URLs) in 1.11.13 and 1.12.8. It passes for 1.13 because this have the security fix from the get go. So I think there are 2 options:

• update the .travis.yml to say 1.11.x and 1.12.x
• Update the tests to allow it to pass with either the DSN or URL set just for 1.11 and 1.12

Which approach should we take?

I think let's take the approach of updating the test because our goal is to verify logic inside strippassword works and as far as we can test that behavior we should be good.

[aircraft-cerier]

Hi @aircraft-cerier,
Test case have failed in Travis CI 1.11 and 1.12 Go runtime.

Hi @bhautikpip
The test cases are failing in Travis because the .travis.yml file is specifying to use versions "1.11" and "1.12"
This is a problem because according to travis' documentation (https://docs.travis-ci.com/user/languages/go/#specifying-a-go-version-to-use) this version specification will default to the original major version. So in this case 1.11.0 and 1.12.0 which do not include the updates to the Go version security fixes (to not parse as URLs) in 1.11.13 and 1.12.8. It passes for 1.13 because this have the security fix from the get go. So I think there are 2 options:

• update the .travis.yml to say 1.11.x and 1.12.x
• Update the tests to allow it to pass with either the DSN or URL set just for 1.11 and 1.12

Which approach should we take?

I think let's take the approach of updating the test because our goal is to verify logic inside strippassword works and as far as we can test that behavior we should be good.

@bhautikpip
I updated the test cases and all checks have passed :)

[bhautikpip]

Hi @aircraft-cerier,

Thanks for the changes. Would you mind taking a look at this PR (#169) ? According to recent update on this PR we have kept xray.SQL (added a deprecated note as per your suggestion) APIs (basically function signature stays the same) but underlying implementation will change. And will also make sure that this PR adds updated '@' case inside stripPassword logic that you are trying to address and will unblock you. Basically the issue is if we are to merge this PR then we will have to keep 2 different SQL tracing implementation with different test cases for both the implementation and will add more redundant code. Moreover, the current SQL tests consists data races and this PR (#169) will be able to resolve that. Let me know if that makes sense for you guys. I am really sorry for last min suggestions.

[aircraft-cerier]

Hi @bhautikpip
my team and I looked at #169, assuming sql function is kept and the update we are trying to make here for the case "@" is also added to their PR we should be ok. Are you going to merge that PR soon? I ask because you can merge our fix and then #169 could rebase/merge master back and resolve any conflicts and still remove the old tests in favor of the new ones.

[bhautikpip]

Hi @aircraft-cerier,

Good to hear that! Yes, I am going to merge that PR soon but to unblock you guys immediately I will merge this PR and then PR (#169) can rebase.

[bhautikpip]

Looks good!