Fix JDBC Detector Bugs

[mustansir14]

Description:

This PR fixes recently surfaced bugs where false positive JDBC connection strings are being detected. Specifically the following bugs are fixed:

• Bug 1: Connection strings with no hosts or passwords are being attempted verification
• Bug 2: The user specified in the connection string is being ignored, and the default admin user is being used for verification

These are identified in the following places:

• sqlserver.go has both problems i.e hardcodes the users to sa and allows connection strings without hosts and passwords
• postgres.go and mysql.go only have bug 1 (trying to verify connection strings without hosts/passwords)

Tests are added that try to reproduce the bugs.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[zricethezav]

Suggestion (non-blocking) Feel free to take this or leave it but I'm not a fan of if err == nil. Idiomatic Go favors if err != nil for better or worse. You could even check for cfg == nil and ignore the err since we aren't doing anything with the err. Alternatively we could log the err continue attempting to construct a jdbc connection from the URI. Anyways, here's a potential change we could make to simplify the control flow. Feel free to take it, leave it, or tell me "hey that's a terrible idea" :)

diff --git a/pkg/detectors/jdbc/mysql.go b/pkg/detectors/jdbc/mysql.go
index e74e9ed9..9ca6d28d 100644
--- a/pkg/detectors/jdbc/mysql.go
+++ b/pkg/detectors/jdbc/mysql.go
@@ -59,22 +59,26 @@ func parseMySQL(ctx logContext.Context, subname string) (jdbc, error) {

        // need for hostnames that have tcp(host:port) format required by this database driver
        cfg, err := mysql.ParseDSN(strings.TrimPrefix(subname, "//"))
-       if err == nil {
-               if cfg.Addr == "" || cfg.Passwd == "" {
-                       ctx.Logger().WithName("jdbc").
-                               V(3).
-                               Info("Skipping invalid MySQL URL - no password or host found")
-                       return nil, fmt.Errorf("missing host or password in connection string")
-               }
-               return &mysqlJDBC{
-                       conn:     subname[2:],
-                       userPass: cfg.User + ":" + cfg.Passwd,
-                       host:     fmt.Sprintf("tcp(%s)", cfg.Addr),
-                       params:   "timeout=5s",
-               }, nil
+       if err != nil {
+               return parseMySQLURI(ctx, subname)
+       }
+
+       if cfg.Addr == "" || cfg.Passwd == "" {
+               ctx.Logger().WithName("jdbc").
+                       V(3).
+                       Info("Skipping invalid MySQL URL - no password or host found")
+               return nil, fmt.Errorf("missing host or password in connection string")
        }

-       // for standard URI format, which is all i've seen for JDBC
+       return &mysqlJDBC{
+               conn:     subname[2:],
+               userPass: cfg.User + ":" + cfg.Passwd,
+               host:     fmt.Sprintf("tcp(%s)", cfg.Addr),
+               params:   "timeout=5s",
+       }, nil
+}
+
+func parseMySQLURI(ctx logContext.Context, subname string) (jdbc, error) {
        u, err := url.Parse(subname)
        if err != nil {
                return nil, err
@@ -109,5 +113,4 @@ func parseMySQL(ctx logContext.Context, subname string) (jdbc, error) {
                host:     fmt.Sprintf("tcp(%s)", u.Host),
                params:   "timeout=5s",
        }, nil
-
 }

[mustansir14]

Thanks for this. I didn't really want to make any changes that weren't actually needed, but this looks like a solid change that should be done.

[zricethezav]

May want to check with Charlie or Martin that this is the appropriate logging level. In that example gist shared with you guys it V(3) without much thought 😬

[mustansir14]

Yep, checked with the team and they think level 2 would be appropriate for this. Made the change