Add canonical PostgreSQL client parameter sslmode
#202
Conversation
| if use_ssl: | ||
| # TODO: Switch to the canonical default `sslmode=prefer` later. | ||
| sslmode = kwargs.pop("sslmode", "disable") | ||
| if use_ssl or sslmode in ["allow", "prefer", "require", "verify-ca", "verify-full"]: |
There was a problem hiding this comment.
If ssl is explicit disabled, sslmode shouldn't have any effect. At least for bwc imho.
| if use_ssl or sslmode in ["allow", "prefer", "require", "verify-ca", "verify-full"]: | |
| if use_ssl and sslmode in ["allow", "prefer", "require", "verify-ca", "verify-full"]: |
There was a problem hiding this comment.
The patch is intended to set the stage to transition to PostgreSQL conventions, so sslmode is intended to be handled in parallel / as an alternative to the ssl option parameter, so we can phase it out in the future.
There was a problem hiding this comment.
If that is the intention, then we should deprecate ssl in the docs/changes, and clearly state that sslmode has higher precedence and overrides ssl.
There was a problem hiding this comment.
Yes, documentation needs to be added. I will reflect that correspondingly, but did not intend to deprecate the ssl option just yet, maybe just give an outlook about it.
If you think it is the right choice to officially deprecate it, but still keep it just for bwc reasons, sure I can do it timely. I am trying to find out about your opinion on this. Thanks!
There was a problem hiding this comment.
At least this should be documented as it may not behave as expected.
There was a problem hiding this comment.
imho, it wouldn't make sense to keep 2 different mechanisms in the future, or do you see some value in that?
There was a problem hiding this comment.
I agree. Let's just keep it for bwc purposes until we may remove the previous one, possibly paired with a 1.0.0 release?
| servers = ["https://" + server for server in servers] | ||
| if sslmode == "require": | ||
| kwargs["verify_ssl_cert"] = False |
There was a problem hiding this comment.
This looks odd as it disables cert verification even that the user did not intend to do that.
If one need to do that, one can set the arg explicitly, or what do I miss?
There was a problem hiding this comment.
Currently, verify_ssl_cert can not be defined by the user on behalf of the SQLAlchemy connection string. It's the intention of the patch to unlock that, and make connectivity options adhere to PostgreSQL conventions.
There was a problem hiding this comment.
Ok, but this will disable cert verification for everyone who uses require. This sounds not like it is what we really want.
There was a problem hiding this comment.
We think sslmode=require is the right option to strictly use an SSL connection, but turn off host name validation on it.
The PostgreSQL docs possibly uses uncommon jargon there, that's why it is sometimes difficult to understand, but of course we may also be wrong on this detail. 1
Please validate and clarify.
Footnotes
-
We will try to use a better jargon on our docs, but still adhere to the option values PostgreSQL clients are using here. ↩
There was a problem hiding this comment.
Ah ok, I understood it wrongly, thanks for the clarification.
There was a problem hiding this comment.
Why would we want to disable hostname validation with sslmode=require ?
There was a problem hiding this comment.
Hi. It's for the same reasons PostgreSQL and other network clients talking SSL offer the same option in one way or another.
The specific need is originating from cloud operations when connecting to CrateDB/SSL from internal spots within an K8s cluster. @WalBeh can possibly explain it better.
This implements `sslmode=prefer` to connect to SSL-enabled CrateDB instances without verifying the host name.
| @@ -1,6 +1,11 @@ | |||
| # Changelog | |||
|
|
|||
| ## Unreleased | |||
| - Added canonical [PostgreSQL client parameter `sslmode`], implementing | |||
| `sslmode=prefer` to connect to SSL-enabled CrateDB instances without | |||
There was a problem hiding this comment.
| `sslmode=prefer` to connect to SSL-enabled CrateDB instances without | |
| `sslmode=require` to connect to SSL-enabled CrateDB instances without |
| return self.dbapi.connect(servers=servers, **kwargs) | ||
| return self.dbapi.connect(**kwargs) |
There was a problem hiding this comment.
When evaluating the patch downstream using sqlalchemy-cratedb==0.42.0.dev2, we found that this connection string syntax, omitting the host name, is not supported yet: crate://?sslmode=require.
Currently, it bails out like this:
TypeError: Connection.__init__() got an unexpected keyword argument 'sslmode'The reason is, as you can observe above, that two different code paths are used here.
Introduction
We need the capability to connect to CrateDB using SSL without verifying the host name.
We also would like to standardize connection parameters to be more like PostgreSQL.
About
This implements
sslmode=requireto connect to SSL-enabled CrateDB instances without verifying the host name.sslmodeoption #197Preview
uv pip install 'sqlalchemy-cratedb @ https://github.com/crate/sqlalchemy-cratedb/archive/refs/heads/sslmode.zip'NB: The environment where you are installing this into needs to have Git installed first, see jwodder/versioningit#106.
Backlog
No dedicated software tests yet, which would validate this change thoroughly. This miniature rig can be used to validate it manually.
NB: Please also make yourself heard on this patch and/or acknowledge even when not assigned as reviewers. I am just not doing it because it is not appropriate to slot in more than two of us. /cc @surister, @kneth