Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent connect fallback behaviour for different authentication methods #57

Open
niksajakovljevic opened this issue Nov 10, 2020 · 2 comments

Comments

@niksajakovljevic
Copy link

niksajakovljevic commented Nov 10, 2020

When trying to establish a connection using MD5 auth method, if authentication fails due to wrong password pgconn will skip checking fallback configs. However if authentication method used when connecting is SCRAM-SHA-256 then even after failing with wrong password it will continue checking fallback configs.
Digging deeper into the code the problem seems to lay in the following code lines in pgconn.go:148:

for _, fc := range fallbackConfigs {
		pgConn, err = connect(ctx, config, fc)
		if err == nil {
			break
		} else if err, ok := err.(*PgError); ok {
			return nil, &connectError{config: config, msg: "server error", err: err}
		}
	}

Namely, connect function call will return PgError when MD5 auth fails, but will return connectError that wraps PgError for SCRAM-SHA-256. So basically condition else if err, ok := err.(*PgError); ok will not be hit.
I assume that one of the workarounds is to inspect the root cause error of connectError and return if it's PgError but not sure how that plays with other fallback scenarios.

@jackc
Copy link
Owner

jackc commented Nov 11, 2020

Can you try it with this code?

	for _, fc := range fallbackConfigs {
		pgConn, err = connect(ctx, config, fc)
		if err == nil {
			break
		}
		var pgErr *PgError
		if errors.As(err, &pgErr) {
			if _, ok := err.(*connectError); ok {
				return nil, err
			} else {
				return nil, &connectError{config: config, msg: "server error", err: err}
			}
		}
	}

I think that will resolve it, but I don't have a convenient way to test SCRAM at the moment.

@niksajakovljevic
Copy link
Author

niksajakovljevic commented Nov 12, 2020

Sure I can give that a run. Judging from the code change I'm confident that it should work. However this change might not play nicely with ValidateConnect function b/c the code comments is saying that on error it should try next fallback config and your proposed change will not allow that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants