Feature/urlparser improve3 pr2 #2646

MarekUniq · 2022-10-20T19:12:49Z

1. Compatibility with libpq is improved regarding URL syntax and resolving url values

1.1. `user` and `password` are supported

Complete syntax:
jdbc:postgresql://[[user][:password]@][host1[:port1][,host2[:port2]][,...]][/database][?property1=value1[&property2=value2][&...]]

URL may include:

user (Optional) is the user to connect. Defaults to operating system username.
password (Optional) is the password to connect. No default value.

1.2. property `passfile` is supported

Example:

Properties props = new Properties();
PGProperty.PASSFILE.set(props, "/mydir/mypass");
Connection conn = DriverManager.getConnection(url, props);

1.3. Code is more clear regarding Properties override rules (source and result are logged at FINE logging level)

There are multiple sources for connection properties. If same property is specified in multiple sources then highest priority source is used. Priority list is here:

1) URL arguments (values after `?` mark)
2) URL values (values before `?` mark)
3) Properties given to `DriverManager.getConnection()`
4) values provided by `service` (from resource `.pg_service.conf`)
5) values in Java System Properties
6) values in Operating System environment
7) values from driverconfig file(s) (`org/postgresql/driverconfig.properties`)
8) global defaults (`dbname`, `host`, `pgpass`, `port`, `user`)

1.4. References

Related to: #2260 #2278 #2398 #2393 #2395 #2424 #2569 #2641 #2644

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

Does your submission pass tests?
Does ./gradlew autostyleCheck checkstyleAll pass ?
Have you added your new test classes to an existing test suite in alphabetical order?

Changes to Existing Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully run tests with your changes locally?

gitguardian · 2022-10-20T19:13:38Z

⚠️ GitGuardian has uncovered 13 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	Secret	Commit	Filename
-	PostgreSQL Credentials	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	PostgreSQL Credentials	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/resources/pg_service/pgservicefileProps.conf	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	Generic Database Assignment	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/resources/pg_service/pgservicefileProps.conf	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret
-	Generic Password	`464f9b9`	pgjdbc/src/test/java/org/postgresql/jdbcurlresolver/JdbcUrlResolverTest.java	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

davecramer · 2022-10-20T20:45:34Z

pgjdbc/src/main/java/org/postgresql/jdbcurlresolver/PgPassParser.java

-    if (resourceName == null) {
-      return null;
-    }
-    //
    String result = null;


If we are looking to provide psql compatibility we should probably check the permissions of the file to ensure they are 0600 on *nix systems.

Good observation. Permission check is added: https://github.com/pgjdbc/pgjdbc/compare/581edc3..736f2cb

MarekUniq · 2023-01-06T16:18:20Z

@davecramer
Hi! What are merge plans? Should I resolve conflicts now or at a later time?

davecramer · 2023-01-06T16:23:29Z

@MarekUniq currently my plans are 1) merge in the issue with longs #2710 will be reopened and merged. A release with this fix will be released.
Then we plan on merging in #2635 and releasing as a new major version.

I would hold off rebasing until after the above happens.

Thanks for your patience

MarekUniq · 2023-06-06T13:43:06Z

@davecramer
Hi! The following pull requests are merged: #2710 #2635.
Any plans for this one?

MarekUniq · 2024-01-16T19:41:54Z

@davecramer Hi! What are merge plans?

androa · 2024-04-18T06:50:50Z

Is there any progress on merging this?

davecramer · 2024-04-18T13:09:28Z

I'm generally in favour of this PR. It will need to be rebased.

sehrope · 2024-04-18T13:29:33Z

pgjdbc/src/test/java/org/postgresql/test/jdbc2/DriverTest.java

  }

-  private void verifyUrl(Driver drv, String url, String hosts, String ports, String dbName)
+  private void verifyUrl(Driver drv, String url, String hosts, String ports, String dbName, String user, @Nullable String pass)


How about adding an overload for the old private verifyUrl(...) to call the new one?

private void verifyUrl(Driver drv, String url, String hosts, String ports, String dbName) { verifyUrl(drv, url, hosts, ports, dbName, "osUser", null); }

That way the majority of the above test cases would not duplicate that info and it would better showcase that none of the expected behavior for those tests have changed.

duplicate info removed: c7413f7

MarekUniq · 2024-05-12T19:49:31Z

@davecramer: I'm generally in favour of this PR. It will need to be rebased.
Rebased

vlsi · 2024-05-12T19:52:37Z

@MarekUniq , the change reads +19,305 −44,408. Please make sure you include only the needed changes. There's no way we merge something that touches 44 thousand lines.

MarekUniq · 2024-05-12T21:17:11Z

@vlsi - I agree that GitHub displayed this PR in a strange way. I saw it too. (was it a glitch or my mistake, I do not know)

But I noticed there were 5 new commits in master branch and I rebased branch feature/urlparser-improve3-pr2 again.
the change reads are back to normal now:
+2,031 −909

vlsi · 2024-05-13T07:38:40Z

@MarekUniq , please rebase commits so they appear "on top of the master branch". The project uses linear history rather than merge commits.

vlsi · 2024-05-13T07:47:26Z

README.md

+| Property                      | Type |                 Default                 | Description                                                                                                                                                                                                                                                                                                                                     |
+|-------------------------------| -- |:---------------------------------------:|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------    |
+| user                          | String |       `operating system username`       | The database user on whose behalf the connection is being made.                                                                                                                                                                                                                                                                               |
+| password                      | String |                  null                   | The database user's password.                                                                                                                                                                                                                                                                                                                 |


Why the full table appears to be changed?
Can we please avoid re-formatting the table with every commit that touches it?

Please just change the lines you want to change. Keep the reset intact.

Original spaces restored with commit: 0eac4e3

vlsi · 2024-05-13T07:48:24Z

README.md

+| pgjdbc.config.cleanup.thread.ttl | long | 30000   |               | The driver has an internal cleanup thread which monitors and cleans up unclosed connections. This property sets the duration (in milliseconds) the cleanup thread will keep running if there is nothing to clean up. |
+
+#### Operating System Environment variables
+The following environment variables can be set.


What does "env vars can be set" mean? What does it give to the end users?

it is about improving compatibility with libpq

What does "env vars can be set" mean?

# An example of setting environment variables: export PGUSER=my_user export PGPASSWORD=my_password

What does it give to the end users?

# then jdbc and libpq-based programs can be launched identically: java ... MainJdbcBasedProgram ./main_libpq_based_program

What I mean is that it would be better to phrase the features in terms of their usability.
The fact that environment variable can be set does not provide much to the end users.

It might be better to have something like "Some of the settings could be passed from environment variables as follows:..."

Is it good enough: 3db232a

vlsi · 2024-05-13T07:49:49Z

pgjdbc/src/main/java/org/postgresql/PGProperty.java

+   * @deprecated Use property HOST instead
   */
+  @Deprecated
  PG_HOST(
      "PGHOST",


What happens to the code that still uses PGHOST and PGPORT properties? Will it continue to work? Where's the corresponding test case?

vlsi · 2024-05-13T07:57:18Z

pgjdbc/src/main/java/org/postgresql/jdbcurlresolver/PgPassParser.java

+          case OTHERS_READ:
+          case OTHERS_WRITE:
+          case OTHERS_EXECUTE:
+            LOGGER.log(Level.WARNING, "password file [{0}] has group or world access; permissions should be u=rw (0600) or less", new Object[]{fileName});


I suggest we log the current value along with the desired one

current value is logged now: a81210d

vlsi · 2024-05-13T08:00:01Z

pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java

@@ -1465,10 +1465,10 @@ public void setProperty(PGProperty property, @Nullable String value) {
      return;
    }
    switch (property) {
-      case PG_HOST:
+      case HOST:


This is a backward incompatible change. Old code might still call setProperty(PGProperty.PG_HOST, ...) and it should continue to work as expected.

Thank you for finding!
Added fix and test case in commit: 4b29449

vlsi · 2024-05-15T06:29:21Z

pgjdbc/src/main/java/org/postgresql/util/PGPropertyUtil.java

-/**
- * routines to support PG properties
- */
-public class PGPropertyUtil {


Class removal is a backward-incompatible change

In broad terms, you are correct. However, this class was intended for internal use only. It is not a JDBC driver service provided for external clients.

I can restore this class (and optionally mar methods as deprecated).
What are your expectations here?

The class was public, so people might be using it. We must maintain backward compatibility

class is restored and marked as deprecated

vlsi · 2024-05-15T06:30:15Z

The key question I have here is how do we test and ensure backward compatibility?

MarekUniq · 2024-05-15T18:57:08Z

@MarekUniq , please rebase commits so they appear "on top of the master branch". The project uses linear history rather than merge commits.

@vlsi I think I can't change the order of commits anymore.
However, in case this PR will be merged, there will be a "squash merge" and then there will be only one commit in the master branch, which will appear as the last commit in the master branch. Isn't it?

vlsi · 2024-05-15T19:14:34Z

What do you mean by "can't change the order of commits"? Could you please squash at your side?

MarekUniq · 2024-05-22T08:21:36Z

What do you mean by "can't change the order of commits"? Could you please squash at your side?

@vlsi Is this the result you were expecting? I did the following:

git merge --squash ...
git push --force ...

davecramer reviewed Oct 20, 2022

View reviewed changes

davecramer mentioned this pull request Jul 27, 2023

Support the postgres:// and username:password libpq URI syntax #2863

Open

sehrope reviewed Apr 18, 2024

View reviewed changes

vlsi reviewed May 13, 2024

View reviewed changes

vlsi reviewed May 15, 2024

View reviewed changes

apply diff/patch on top of master (history is lost)

464f9b9

MarekUniq force-pushed the feature/urlparser-improve3-pr2 branch from 24c5eba to 464f9b9 Compare May 22, 2024 08:08

MarekUniq force-pushed the feature/urlparser-improve3-pr2 branch 2 times, most recently from d73adc9 to 464f9b9 Compare May 22, 2024 18:36

Feature/urlparser improve3 pr2 #2646

Are you sure you want to change the base?

Feature/urlparser improve3 pr2 #2646

Conversation

MarekUniq commented Oct 20, 2022

1. Compatibility with libpq is improved regarding URL syntax and resolving url values

1.1. user and password are supported

1.2. property passfile is supported

1.3. Code is more clear regarding Properties override rules (source and result are logged at FINE logging level)

1.4. References

All Submissions:

New Feature Submissions:

Changes to Existing Features:

gitguardian bot commented Oct 20, 2022 • edited

⚠️ GitGuardian has uncovered 13 secrets following the scan of your pull request.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarekUniq commented Jan 6, 2023

davecramer commented Jan 6, 2023

MarekUniq commented Jun 6, 2023

MarekUniq commented Jan 16, 2024

androa commented Apr 18, 2024

davecramer commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarekUniq commented May 12, 2024

vlsi commented May 12, 2024

MarekUniq commented May 12, 2024

vlsi commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlsi May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlsi May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlsi commented May 15, 2024

MarekUniq commented May 15, 2024

vlsi commented May 15, 2024

MarekUniq commented May 22, 2024

1.1. `user` and `password` are supported

1.2. property `passfile` is supported

gitguardian bot commented Oct 20, 2022 •

edited

vlsi May 13, 2024 •

edited

vlsi May 13, 2024 •

edited