Skip to content

fix: PgConnectOptions docs #3809

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

Merged
merged 1 commit into from
Apr 14, 2025
Merged

fix: PgConnectOptions docs #3809

merged 1 commit into from
Apr 14, 2025

Conversation

mbj
Copy link
Contributor

@mbj mbj commented Mar 30, 2025

Note I'm PRing this to document the current behavior, I'm not sure if it was the intention of the original author to support MORE behavior than libpq offers.

If I look at pub fn ssl_client_key(self, key: impl AsRef<Path>) -> PgConnectOptions there is no way to "set" the inline version of CertificateInput, it only accepts the file version. This potentially should be lifted for symmetry with the environment variables.

This may not be the intention? But also I seen no reason to artificially constrain sqlx-postgres to libpq behavior, in lots of scenarios (single file lambda binary) its much better to be able to set the certificate inline.

Regardless of what the desired final behavior is: This PR just documents the status quo, even if we wish to change it in the future.

Is this a breaking change?

no

@mbj mbj force-pushed the fix/docs branch 3 times, most recently from f430a00 to 8c4b663 Compare March 30, 2025 03:27
@abonander
Copy link
Collaborator

I'm going to merge this so you get the attribution, but the release is going to have pretty significant wording changes since this could be blended better with the rest of the doc comment.

@abonander abonander merged commit 1548785 into launchbadge:main Apr 14, 2025
81 checks passed
@abonander
Copy link
Collaborator

As it turns out, the handling of PGSSLKEY is a bit buggy and it won't actually accept a private key directly from the environment variable. That's because it only looks for -----BEGIN CERTIFICATE----- and -----END CERTIFICATE----- which wouldn't appear in a PEM encoded private key file.

I'm not sure whether to fix that or leave it as-is.

abonander added a commit that referenced this pull request Apr 14, 2025
* chore: prepare 0.8.4 release

* fix(postgres): send `limit: 0` for all `Execute` messages

fixes #3673

* fix: let `CertificateInput::from` infer any PEM-encoded document

#3809 (comment)

* doc: improve documentation of `PgConnectOptions`

fixes #3740

* chore: update CHANGELOG from PR
@mbj
Copy link
Contributor Author

mbj commented Apr 18, 2025

As it turns out, the handling of PGSSLKEY is a bit buggy and it won't actually accept a private key directly from the environment variable. That's because it only looks for -----BEGIN CERTIFICATE----- and -----END CERTIFICATE----- which wouldn't appear in a PEM encoded private key file.

I'm not sure whether to fix that or leave it as-is.

If inlining key materials via the environment is a deliberate feature and not just a feature by accident than IMO:

I think the internal representation of certs and keys has to be split to fix this. And IMO it should be fixed. We need dedicated types so we can run dedicated inference logic. Basically we'd need to add an CertificateKey type with a dedicated CertificateKey::from that would than snoop for ----BEGIN PRIVATE KEY----- and friends.

On the other hand if its not a deliberate feature and just a feature by accident: Inference logic should NOT exist and the PG* environment variables should only support whats in libpq and if we added extra logic to environment varibles these should be in SQLXPG prefixed ones.

I personally prefer that PG* env variables behave exactly as in libpq, and any supersetting should be done explicitly in SQLXPG* or similar.

@mbj mbj deleted the fix/docs branch April 18, 2025 02:49
@tisonkun
Copy link
Contributor

As it turns out, the handling of PGSSLKEY is a bit buggy and it won't actually accept a private key directly from the environment variable. That's because it only looks for -----BEGIN CERTIFICATE----- and -----END CERTIFICATE----- which wouldn't appear in a PEM encoded private key file.

I'm not sure whether to fix that or leave it as-is.

From PostgreSQL's docs it seems that all these options only accept the param as filepath. Also the documented "system" option doesn't seem to be handled in sqlx-postgres.

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

Successfully merging this pull request may close these issues.

3 participants