Skip to content

Accept SSL certificates by providing a URL to use cert from within a jar #318

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Accept SSL certificates by providing a URL to use cert from within a jar #318

wants to merge 2 commits into from

Conversation

isabek
Copy link
Contributor

@isabek isabek commented Aug 17, 2020 •
edited
Loading

[resolves #313]

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don't submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

Issue description

New Public APIs

Additional context

@Squiry
Copy link
Collaborator

Squiry commented Aug 18, 2020

@isabek your changes need to be covered by tests

  1. Integration test with classpath url here.
  2. Test cases for configuration parsing and mapping string values to URLs here.
  3. Test cases for possible mistakes in configuration (like file/resource doesn't exist) here.

@isabek
Copy link
Contributor Author

isabek commented Aug 22, 2020 •
edited
Loading

Hey @Squiry. I would like to make sure that I got the issue right. The following properties sslCert, sslKey, sslRootCert (PostgresqlConnectionConfiguration.Builder) should be changed from string to java.net.URL. We should validate URL inside setters.

Also, I tried to create an instance of a URL with classpath:file.txt and it fails with classpath protocol not found. As I understood when we create an instance of URL inside Spring context it sets some handlers for classpath and it creates URL without fail.

Also, how can I create an instance of a URL with classpath: prefix inside tests?

Btw I have checked Spring's org.springframework.util.ResourceUtils class. It has a method called getURL which accepts a String with classpath prefix. But inside the method, it cuts the classpath prefix and tries to lookup with the help of the class loader.

@mp911de
Copy link
Collaborator

mp911de commented Sep 7, 2020

classpath: is a Spring-specific feature hence it's not applicable in our context. Please use ClassLoader.getResource(…) to construct an URL pointing to class-path resources.

@mp911de
Copy link
Collaborator

mp911de commented Oct 15, 2020

Any update on this pull request? If not, we will close this one in a few days.

@mp911de mp911de added the status: waiting-for-feedback We need additional information before we can continue label Oct 15, 2020
@isabek
Copy link
Contributor Author

isabek commented Oct 30, 2020

@mp911de sorry. I will update it on weekends.

@isabek
Copy link
Contributor Author

isabek commented Nov 7, 2020

@mp911de I have pushed my last changes. Could you review it, please?

@davecramer
Copy link
Member

@isabek can we also update the README to reflect this option ?

mp911de
mp911de requested changes Dec 10, 2020
View reviewed changes
src/main/java/io/r2dbc/postgresql/util/Assert.java
}

ClassLoader classLoader = Assert.class.getClassLoader();
URL url = classLoader != null ? classLoader.getResource(path) : ClassLoader.getSystemResource(path);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the classloader supposed to be null?

src/main/java/io/r2dbc/postgresql/util/Assert.java
* @throws IllegalArgumentException if {@code value} is not of the required type
* @throws IllegalArgumentException if {@code value}, {@code type}, or {@code message} is {@code null}
*/
public static URL requireUrlExistsOrNull(@Nullable String path, String message) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should revert changes to the Assert class and instead, introduce rather an utility method to figure out, whether the given ConnectionFactoryOption tries to point to a real file or to a URL. Checking for a scheme would be a good approach if the configured string doesn't point to a file.

We might still run into an ambiguity on windows (c:\foo\bar.crt vs. classpath:foo.crt or classpath:/foo). Not sure how to resolve that.

@mp911de
Copy link
Collaborator

mp911de commented Dec 14, 2020

@isabek I'm happy to take this PR from here as it requires a bit of experimentation. WDYT?

@isabek
Copy link
Contributor Author

isabek commented Dec 14, 2020

@isabek I'm happy to take this PR from here as it requires a bit of experimentation. WDYT?

Sure.

@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-feedback We need additional information before we can continue labels Feb 18, 2021
@mp911de mp911de added this to the 0.8.7.RELEASE milestone Feb 18, 2021
@mp911de mp911de closed this in b8b7f19 Feb 18, 2021
mp911de pushed a commit that referenced this pull request Feb 18, 2021
@isabek @mp911de
Accept SSL certificates by providing a resource path.
97fc297 
We now try to resolve SSL certificates first from the class path and then fall back to files when providing a path as string.

[resolves #313][closes #318]

Signed-off-by: Mark Paluch <[email protected]>
@mp911de
Copy link
Collaborator

mp911de commented Feb 18, 2021

Thank you for your contribution. That's merged, polished, and backported now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mp911de mp911de mp911de requested changes

Assignees
No one assigned
Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
0.8.7.RELEASE
Development

Successfully merging this pull request may close these issues.

Accept SSL certificates by providing a URL to use cert from within a jar
4 participants
@isabek @Squiry @mp911de @davecramer