Skip to content

Commit 20b7793

Browse files
rosecodymrosecodym
authored
JDBC indeterminacy (#1507)
This PR adds an indeterminacy check to the JDBC verifiers.
1 parent 8fad5ff commit 20b7793

9 files changed

+190
-79
lines changed

‎pkg/detectors/jdbc/jdbc.go

+21-6
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,14 @@ matchLoop:
8686
}
8787
ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
8888
defer cancel()
89-
s.Verified = j.ping(ctx)
89+
pingRes := j.ping(ctx)
90+
s.Verified = pingRes.err == nil
91+
// If there's a ping error that is marked as "determinate" we throw it away. We do this because this was the
92+
// behavior before tri-state verification was introduced and preserving it allows us to gradually migrate
93+
// detectors to use tri-state verification.
94+
if pingRes.err != nil && !pingRes.determinate {
95+
s.VerificationError = pingRes.err
96+
}
9097
// TODO: specialized redaction
9198
}
9299

@@ -198,8 +205,13 @@ var supportedSubprotocols = map[string]func(string) (jdbc, error){
198205
"sqlserver": parseSqlServer,
199206
}
200207

208+
type pingResult struct {
209+
err error
210+
determinate bool
211+
}
212+
201213
type jdbc interface {
202-
ping(context.Context) bool
214+
ping(context.Context) pingResult
203215
}
204216

205217
func newJDBC(conn string) (jdbc, error) {
@@ -220,13 +232,16 @@ func newJDBC(conn string) (jdbc, error) {
220232
return parser(subname)
221233
}
222234

223-
func ping(ctx context.Context, driverName string, candidateConns ...string) bool {
235+
func ping(ctx context.Context, driverName string, isDeterminate func(error) bool, candidateConns ...string) pingResult {
236+
var indeterminateErrors []error
224237
for _, c := range candidateConns {
225-
if err := pingErr(ctx, driverName, c); err == nil {
226-
return true
238+
err := pingErr(ctx, driverName, c)
239+
if err == nil || isDeterminate(err) {
240+
return pingResult{err, true}
227241
}
242+
indeterminateErrors = append(indeterminateErrors, err)
228243
}
229-
return false
244+
return pingResult{errors.Join(indeterminateErrors...), false}
230245
}
231246

232247
func pingErr(ctx context.Context, driverName, conn string) error {

‎pkg/detectors/jdbc/mysql.go

+19-4
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,8 @@ package jdbc
33
import (
44
"context"
55
"errors"
6+
"github.com/go-sql-driver/mysql"
67
"strings"
7-
8-
_ "github.com/go-sql-driver/mysql"
98
)
109

1110
type mysqlJDBC struct {
@@ -16,8 +15,8 @@ type mysqlJDBC struct {
1615
params string
1716
}
1817

19-
func (s *mysqlJDBC) ping(ctx context.Context) bool {
20-
return ping(ctx, "mysql",
18+
func (s *mysqlJDBC) ping(ctx context.Context) pingResult {
19+
return ping(ctx, "mysql", isMySQLErrorDeterminate,
2120
s.conn,
2221
buildMySQLConnectionString(s.host, s.database, s.userPass, s.params),
2322
buildMySQLConnectionString(s.host, "", s.userPass, s.params))
@@ -34,6 +33,22 @@ func buildMySQLConnectionString(host, database, userPass, params string) string
3433
return conn
3534
}
3635

36+
func isMySQLErrorDeterminate(err error) bool {
37+
// MySQL error numbers from https://dev.mysql.com/doc/mysql-errors/8.0/en/server-error-reference.html
38+
if mySQLErr, isMySQLErr := err.(*mysql.MySQLError); isMySQLErr {
39+
switch mySQLErr.Number {
40+
case 1044:
41+
// User access denied to a particular database
42+
return false // "Indeterminate" so that other connection variations will be tried
43+
case 1045:
44+
// User access denied
45+
return true
46+
}
47+
}
48+
49+
return false
50+
}
51+
3752
func parseMySQL(subname string) (jdbc, error) {
3853
// expected form: [subprotocol:]//[user:password@]HOST[/DB][?key=val[&key=val]]
3954
hostAndDB, params, _ := strings.Cut(subname, "?")

‎pkg/detectors/jdbc/mysql_integration_test.go

+28-19
Original file line numberDiff line numberDiff line change
@@ -21,45 +21,54 @@ const (
2121
)
2222

2323
func TestMySQL(t *testing.T) {
24+
type result struct {
25+
parseErr bool
26+
pingOk bool
27+
pingDeterminate bool
28+
}
2429
tests := []struct {
25-
input string
26-
wantErr bool
27-
wantPing bool
30+
input string
31+
want result
2832
}{
2933
{
30-
input: "",
31-
wantErr: true,
34+
input: "",
35+
want: result{parseErr: true},
3236
},
3337
{
34-
input: "//" + mysqlUser + ":" + mysqlPass + "@tcp(127.0.0.1:3306)/" + mysqlDatabase,
35-
wantPing: true,
38+
input: "//" + mysqlUser + ":" + mysqlPass + "@tcp(127.0.0.1:3306)/" + mysqlDatabase,
39+
want: result{pingOk: true, pingDeterminate: true},
3640
},
3741
{
38-
input: "//wrongUser:wrongPass@tcp(127.0.0.1:3306)/" + mysqlDatabase,
39-
wantPing: false,
42+
input: "//wrongUser:wrongPass@tcp(127.0.0.1:3306)/" + mysqlDatabase,
43+
want: result{pingOk: false, pingDeterminate: true},
4044
},
4145
{
42-
input: "//" + mysqlUser + ":wrongPass@tcp(127.0.0.1:3306)/" + mysqlDatabase,
43-
wantPing: false,
46+
input: "//" + mysqlUser + ":wrongPass@tcp(127.0.0.1:3306)/" + mysqlDatabase,
47+
want: result{pingOk: false, pingDeterminate: true},
4448
},
4549
{
46-
input: "//" + mysqlUser + ":" + mysqlPass + "@tcp(127.0.0.1:3306)/",
47-
wantPing: true,
50+
input: "//" + mysqlUser + ":" + mysqlPass + "@tcp(127.0.0.1:3306)/",
51+
want: result{pingOk: true, pingDeterminate: true},
4852
},
4953
{
50-
input: "//" + mysqlUser + ":" + mysqlPass + "@tcp(127.0.0.1:3306)/wrongDB",
51-
wantPing: true,
54+
input: "//" + mysqlUser + ":" + mysqlPass + "@tcp(127.0.0.1:3306)/wrongDB",
55+
want: result{pingOk: true, pingDeterminate: true},
5256
},
5357
}
5458
for _, tt := range tests {
5559
t.Run(tt.input, func(t *testing.T) {
5660
j, err := parseMySQL(tt.input)
57-
if tt.wantErr {
58-
assert.Error(t, err)
61+
62+
if err != nil {
63+
got := result{parseErr: true}
64+
assert.Equal(t, tt.want, got)
5965
return
6066
}
61-
assert.NoError(t, err)
62-
assert.Equal(t, tt.wantPing, j.ping(context.Background()))
67+
68+
pr := j.ping(context.Background())
69+
70+
got := result{pingOk: pr.err == nil, pingDeterminate: pr.determinate}
71+
assert.Equal(t, tt.want, got)
6372
})
6473
}
6574
}

‎pkg/detectors/jdbc/postgres.go

+30-7
Original file line numberDiff line numberDiff line change
@@ -4,22 +4,45 @@ import (
44
"context"
55
"errors"
66
"fmt"
7+
"github.com/lib/pq"
78
"strings"
8-
9-
_ "github.com/lib/pq"
109
)
1110

1211
type postgresJDBC struct {
1312
conn string
1413
params map[string]string
1514
}
1615

17-
func (s *postgresJDBC) ping(ctx context.Context) bool {
18-
return ping(ctx, "postgres",
19-
s.conn,
20-
"postgres://"+s.conn,
16+
func (s *postgresJDBC) ping(ctx context.Context) pingResult {
17+
// It is crucial that we try to build a connection string ourselves before using the one we found. This is because
18+
// if the found connection string doesn't include a username, the driver will attempt to connect using the current
19+
// user's name, which will fail in a way that looks like a determinate failure, thus terminating the waterfall. In
20+
// contrast, when we build a connection string ourselves, if there's no username, we try 'postgres' instead, which
21+
// actually has a chance of working.
22+
return ping(ctx, "postgres", isPostgresErrorDeterminate,
2123
buildPostgresConnectionString(s.params, true),
22-
buildPostgresConnectionString(s.params, false))
24+
buildPostgresConnectionString(s.params, false),
25+
s.conn,
26+
"postgres://"+s.conn)
27+
}
28+
29+
func isPostgresErrorDeterminate(err error) bool {
30+
// Postgres codes from https://www.postgresql.org/docs/current/errcodes-appendix.html
31+
if pqErr, isPostgresError := err.(*pq.Error); isPostgresError {
32+
switch pqErr.Code {
33+
case "28P01":
34+
// Invalid username/password
35+
return true
36+
case "3D000":
37+
// Unknown database
38+
return false // "Indeterminate" so that other connection variations will be tried
39+
case "3F000":
40+
// Unknown schema
41+
return false // "Indeterminate" so that other connection variations will be tried
42+
}
43+
}
44+
45+
return false
2346
}
2447

2548
func joinKeyValues(m map[string]string, sep string) string {

‎pkg/detectors/jdbc/postgres_integration_test.go

+34-21
Original file line numberDiff line numberDiff line change
@@ -20,49 +20,62 @@ const (
2020
)
2121

2222
func TestPostgres(t *testing.T) {
23+
type result struct {
24+
parseErr bool
25+
pingOk bool
26+
pingDeterminate bool
27+
}
2328
tests := []struct {
24-
input string
25-
wantErr bool
26-
wantPing bool
29+
input string
30+
want result
2731
}{
2832
{
29-
input: "//localhost:5432/foo?sslmode=disable&password=" + postgresPass,
30-
wantPing: true,
33+
input: "//localhost:5432/foo?sslmode=disable&password=" + postgresPass,
34+
want: result{pingOk: true, pingDeterminate: true},
35+
},
36+
{
37+
input: "//localhost:5432/foo?sslmode=disable&user=" + postgresUser + "&password=" + postgresPass,
38+
want: result{pingOk: true, pingDeterminate: true},
3139
},
3240
{
33-
input: "//localhost:5432/foo?sslmode=disable&user=" + postgresUser + "&password=" + postgresPass,
34-
wantPing: true,
41+
input: "//localhost/foo?sslmode=disable&port=5432&password=" + postgresPass,
42+
want: result{pingOk: true, pingDeterminate: true},
3543
},
3644
{
37-
input: "//localhost/foo?sslmode=disable&port=5432&password=" + postgresPass,
38-
wantPing: true,
45+
input: "//localhost:5432/foo?password=" + postgresPass,
46+
want: result{pingOk: false, pingDeterminate: false},
3947
},
4048
{
41-
input: "//localhost:5432/foo?password=" + postgresPass,
42-
wantPing: false,
49+
input: "//localhost:5432/foo?sslmode=disable&password=foo",
50+
want: result{pingOk: false, pingDeterminate: true},
4351
},
4452
{
45-
input: "//localhost:5432/foo?sslmode=disable&password=foo",
46-
wantPing: false,
53+
input: "//localhost:5432/foo?sslmode=disable&user=foo&password=" + postgresPass,
54+
want: result{pingOk: false, pingDeterminate: true},
4755
},
4856
{
49-
input: "//localhost:5432/foo?sslmode=disable&user=foo&password=" + postgresPass,
50-
wantPing: false,
57+
input: "//badhost:5432/foo?sslmode=disable&user=foo&password=" + postgresPass,
58+
want: result{pingOk: false, pingDeterminate: false},
5159
},
5260
{
53-
input: "invalid",
54-
wantErr: true,
61+
input: "invalid",
62+
want: result{parseErr: true},
5563
},
5664
}
5765
for _, tt := range tests {
5866
t.Run(tt.input, func(t *testing.T) {
5967
j, err := parsePostgres(tt.input)
60-
if tt.wantErr {
61-
assert.Error(t, err)
68+
69+
if err != nil {
70+
got := result{parseErr: true}
71+
assert.Equal(t, tt.want, got)
6272
return
6373
}
64-
assert.NoError(t, err)
65-
assert.Equal(t, tt.wantPing, j.ping(context.Background()))
74+
75+
pr := j.ping(context.Background())
76+
77+
got := result{pingOk: pr.err == nil, pingDeterminate: pr.determinate}
78+
assert.Equal(t, tt.want, got)
6679
})
6780
}
6881
}

‎pkg/detectors/jdbc/sqlite.go

+9-3
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,18 @@ type sqliteJDBC struct {
1414
testing bool
1515
}
1616

17-
func (s *sqliteJDBC) ping(ctx context.Context) bool {
17+
var cannotVerifySqliteError error = errors.New("sqlite credentials cannot be verified")
18+
19+
func (s *sqliteJDBC) ping(ctx context.Context) pingResult {
1820
if !s.testing {
1921
// sqlite is not a networked database, so we cannot verify
20-
return false
22+
return pingResult{cannotVerifySqliteError, true}
2123
}
22-
return ping(ctx, "sqlite3", s.filename)
24+
return ping(ctx, "sqlite3", isSqliteErrorDeterminate, s.filename)
25+
}
26+
27+
func isSqliteErrorDeterminate(err error) bool {
28+
return true
2329
}
2430

2531
func parseSqlite(subname string) (jdbc, error) {

‎pkg/detectors/jdbc/sqlite_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func TestParseSqlite(t *testing.T) {
3939
assert.Error(t, err)
4040
} else {
4141
assert.NoError(t, err)
42-
assert.True(t, j.ping(context.Background()))
42+
assert.True(t, j.ping(context.Background()).err == nil)
4343
}
4444
})
4545
}

‎pkg/detectors/jdbc/sqlserver.go

+18-5
Original file line numberDiff line numberDiff line change
@@ -3,23 +3,36 @@ package jdbc
33
import (
44
"context"
55
"errors"
6+
mssql "github.com/denisenkom/go-mssqldb"
67
"strings"
7-
8-
_ "github.com/denisenkom/go-mssqldb"
98
)
109

1110
type sqlServerJDBC struct {
1211
conn string
1312
params map[string]string
1413
}
1514

16-
func (s *sqlServerJDBC) ping(ctx context.Context) bool {
17-
return ping(ctx, "mssql",
18-
s.conn,
15+
func (s *sqlServerJDBC) ping(ctx context.Context) pingResult {
16+
return ping(ctx, "mssql", isSqlServerErrorDeterminate,
1917
joinKeyValues(s.params, ";"),
18+
s.conn,
2019
"sqlserver://"+s.conn)
2120
}
2221

22+
func isSqlServerErrorDeterminate(err error) bool {
23+
// Error numbers from https://learn.microsoft.com/en-us/sql/relational-databases/errors-events/database-engine-events-and-errors?view=sql-server-ver16
24+
if mssqlError, isMssqlError := err.(mssql.Error); isMssqlError {
25+
switch mssqlError.Number {
26+
case 18456:
27+
// Login failed
28+
// This is a determinate failure iff we tried to use a real user
29+
return mssqlError.Message != "login error: Login failed for user ''."
30+
}
31+
}
32+
33+
return false
34+
}
35+
2336
func parseSqlServer(subname string) (jdbc, error) {
2437
if !strings.HasPrefix(subname, "//") {
2538
return nil, errors.New("expected connection to start with //")

0 commit comments

Comments
 (0)