Narrow Postgres detector to only look for URIs

[rosecodym]

Description:

This PR re-enables the Postgres detector and modifies it to only look for connection string URIs. If we're happy that it's not generating a bunch of false hits we can later add separate-credential support.

Checklist:

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

[ahrav]

Thanks for reworking this ❤️

[ahrav]

nit: If we construct params after parts, we can pre-alloc the map. it'll make it go SOOOO much faster :)

params := make(map[string]string, len(parts))

[ahrav]

nit: A switch is a lot easier to read here imo. We can also capture the final return false, nil in the default block:

switch {
	case err == nil:
		return true, nil
	case strings.Contains(err.Error(), "password authentication failed"):
		return false, nil
	case errors.Is(err, pq.ErrSSLNotSupported) && params[pg_sslmode] == "":
		// If the sslmode is unset, then either it was unset in the candidate secret, or we've intentionally unset it
		// because it was specified as 'allow' or 'prefer', neither of which pq supports. In all of these cases, non-SSL
		// connections are acceptable, so now we try a connection without SSL.
		params[pg_sslmode] = pg_sslmode_disable
		defer delete(params, pg_sslmode) // We want to return with the original params map intact (for ExtraData)
		return verifyPostgres(params)
	case isErrorDatabaseNotFound(err, params[pg_dbname]):
		return true, nil // If we know this, we were able to authenticate
	default:
		return false, nil
	}

[ahrav]

I think this is unused.

[ahrav]

same with inactivePort. I know you didn't touch these, but figured while i'm here i'd call it out.

[ahrav]

nifty!

[0x1]

nit: probably should be "no port" instead of "wrong port"?

[rosecodym]

oh, this is poorly explained. when no port is specified the detector defaults to 5432, but the test server is running on 5434, so the root cause is a bad port. but the test is exercising both the port fallback logic and the bad port logic. do you think it's worth clarifying this in a followup pr?

[0x1]

nice, thanks for cleaning this up!

[rosecodym]

the failing test was caused by a new gitlab project - i'll fix in a follow-up pr

[rgmz]

This won't find things like "postgresql+asyncpg://postgres:secret@localhost/testdb". Not sure what the + syntax is called.
https://github.com/agronholm/apscheduler/blob/b4ceea0ed300545a27bb8dbbbfb382a46d8ea90f/examples/web/asgi_starlette.py#L59

[rosecodym]

Hm, I was just going off the official connection URI docs, and I don't see that mentioned anywhere. Do you have any references you can point me to?

[rgmz]

It appears to be a Python-specific quirk:

dialect+driver://username:password@host:port/database

https://www.tutorialspoint.com/sqlalchemy/sqlalchemy_dialects.htm

[rosecodym]

Ah, ok. For this pass we're aiming for correctness over completeness, because the previous implementation was yielding a ton of false positives. The eventual plan is to add other types of Postgres secrets (non-URI connection strings, at the very least), so we can add SQLAlchemy-extended URIs to that list.