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

RedShift doesn't support ssl_renegotiation_limit #321

Closed
jacobmarble opened this issue Sep 9, 2017 · 3 comments
Closed

RedShift doesn't support ssl_renegotiation_limit #321

jacobmarble opened this issue Sep 9, 2017 · 3 comments

Comments

@jacobmarble
Copy link

(Originally mentioned in #282)

pgx can't connect to a RedShift cluster when the cluster is configured to require SSL. The patch below illustrates a workaround, not a fix; there should be a more thoughtful way to approach this problem.

As RedShift inevitably continues to diverge from Postgres, it might make sense to add a new query parameter to the data source name parser. For example, redshift=true. This could be translated to a boolean field of the ConnConfig struct, made available for future RedShift-specific workarounds. If that sounds like a good idea, I'll be happy to write a PR.

RedShift have so far not committed to a fix, but at least someone there is aware of the problem.
https://forums.aws.amazon.com/thread.jspa?threadID=229990

$ git diff
diff --git a/conn.go b/conn.go
index f549e03..d0b4f4b 100644
--- a/conn.go
+++ b/conn.go
@@ -321,7 +321,7 @@ func (c *Conn) connect(config ConnConfig, network, address string, tlsConfig *tl
        // Go does not support (https://github.com/golang/go/issues/5742)
        // PostgreSQL recommends disabling (http://www.postgresql.org/docs/9.4/static/runtime-config-connection.html#GUC-SSL-RENEGOTIATION-LIMIT)
        if tlsConfig != nil {
-               startupMsg.Parameters["ssl_renegotiation_limit"] = "0"
+               //startupMsg.Parameters["ssl_renegotiation_limit"] = "0"
        }
@jackc
Copy link
Owner

jackc commented Sep 9, 2017

I'd prefer to avoid needing named modes for "almost PostgreSQL" servers. It's not the end of the world, but if possible I'd prefer to expose settings that can configure the connection to be compatible with various servers. See also #320 for more discussion of connecting to "almost PostgreSQL" servers.

In RedShift's case there are 2 issues. The first is the inability to connect due to the ssl_renegotiation_limit parameter. But with that removed, the first time RedShift sends a renegotiate request the connection will die. When I last looked into this that was at 512 MB of transit.

At the time Go did not support renegotiation at all. But now Go at least has partial opt-in support. I don't know if it is sufficient. If not, there's nothing we can do. But if it is, then there is a clean solution. The only reason we send ssl_renegotiation_limit is to avoid mysterious connection errors when connecting to servers that use renegotiation. But PostgreSQL defaults to 0 for all supported versions, and removed the setting for 9.6.

So I guess all we need to do is remove the default startup parameter message. Anyone who really needs it can easily add it back with ConnConfig.RuntimeParams. Then the only thing a user connecting to RedShift would need to do is configure ConnConfig.TLSConfig appropriately for renegotiation. I'd rather not make renegotiate a default as I think it weakens security slightly.

If we go this route it might be worth adding documentation next to the current SSL explanation regarding renegotiate and RedShift.

tejasmanohar added a commit to tejasmanohar/pgx that referenced this issue Oct 30, 2018
This addresses jackc#321 with the
fix @jackc proposed there. Redshift users that need to connect
w/ SSL currently fork the library to delete this parameter, e.g.

segmentio@8e0028d

And, that's annoying to keep up-to-date :)
@tejasmanohar
Copy link
Contributor

So I guess all we need to do is remove the default startup parameter message. Anyone who really needs it can easily add it back with ConnConfig.RuntimeParams.

Sent a PR with your proposal :) #476

tejasmanohar added a commit to tejasmanohar/pgx that referenced this issue Oct 30, 2018
This addresses jackc#321 with the
fix @jackc proposed there. Redshift users that need to connect
w/ SSL currently fork the library to delete this parameter, e.g.

segmentio@8e0028d

And, that's annoying to keep up-to-date :)
tejasmanohar added a commit to tejasmanohar/pgx that referenced this issue Oct 30, 2018
This addresses jackc#321 with the
fix @jackc proposed there. Redshift users that need to connect
w/ SSL currently fork the library to delete this parameter, e.g.

segmentio@8e0028d

And, that's annoying to keep up-to-date :)
tejasmanohar added a commit to tejasmanohar/pgx that referenced this issue Oct 30, 2018
This addresses jackc#321 with the
fix @jackc proposed there. Redshift users that need to connect
w/ SSL currently fork the library to delete this parameter, e.g.

segmentio@8e0028d

And, that's annoying to keep up-to-date :)
@jackc
Copy link
Owner

jackc commented Nov 3, 2018

Fixed in PR #476.

@jackc jackc closed this as completed Nov 3, 2018
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

3 participants