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

Allow using custom SSL certificates #8583

Merged
merged 48 commits into from
Mar 26, 2022
Merged

Allow using custom SSL certificates #8583

merged 48 commits into from
Mar 26, 2022

Conversation

HoussemNasri
Copy link
Member

@HoussemNasri HoussemNasri commented Mar 19, 2022

Fixes #8126.

Context

When a browser or server attempts to connect to a website (i.e., a web server) secured with SSL, the browser requests that the web server identifies itself by sending a copy of its SSL certificate in response then it checks to see whether it trusts the SSL certificate. If it does, it signals this to the webserver and starts an SSL encrypted session; if it doesn't the connection fails. The Java HTTP client uses a similar method.

So, how does Java trust certificates?
The JVM stores a list of trusted certificates in a truststore. Normally Java uses a system-wide truststore:

  • Java 8: $JAVA_HOME/jre/lib/security/cacerts
  • Java 11: $JAVA_HOME/lib/security/cacerts

However it is possible to use a different truststore by specifying a parameter, -Djavax.net.ssl.trustStore=/path/to/truststore, where /path/to/truststore is the absolute file path of the alternative truststore.

When connecting to a server over HTTPS, the Java client looks up the associated certificate in our truststore. If the certificate or Certificate Authorities presented by the external server is not in our truststore, we'll get an SSLHandshakeException and the connection won't be set up successfully.

Problem Description

Using an SSL proxy, the Java client requests a certificate from a server through the SSL proxy, which intercepts the request and provides its own SSL certificate, which may or may not be available in our truststore

Solution

The solution is to copy the original JDK truststore into the project and edit it, such as adding or removing certificates based on user interactions. It also involves persisting user-created certificates for the UI.

UI Changes

I added a new section to network preferences for SSL configurations, this section contains a table of user-added certificates, with basic information about each certificate, such as serial number, version, validation dates and more.

Screenshots

Screenshot 2022-03-26 140542

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@HoussemNasri
Copy link
Member Author

I tried putting the truststore file in resources, but it doesn't seem to work; the certificates don't seem to persist. I think I should put it in application data, but I'm not sure how to do that across operating systems.

@Siedlerchr
Copy link
Member

For the app data dir thing, checkout the PdfIndexer class, we use an external library which uses an external library that provides OS independent path to app data

@Siedlerchr
Copy link
Member

Alternative idea: Ask the user to store the file somewhere

@HoussemNasri
Copy link
Member Author

We can add another setting to SSL configuration to choose the truststore path; I'll think about it later once I've set up SSL configuration preference storing logic; then it'll be a simple task.

@calixtus calixtus added type: enhancement build-system status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers preferences and removed build-system labels Mar 23, 2022
this.storePath = storePath;
try {
LOGGER.info("Trust store path: {}", storePath.toAbsolutePath());
Path storeResourcePath = Path.of(TrustStoreManager.class.getResource("/ssl/truststore.jks").toURI());
Copy link
Member

Choose a reason for hiding this comment

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

This definitely needs to be checked at runime in a jpackage build.

@Siedlerchr
Copy link
Member

Codewise lgtm! Needs to be tested when run from jpackage to be sure that all resources paths can be accessed.
Can you test it with ./gradlew jpackage ?
That should generate the binaries under build/distribution

@HoussemNasri
Copy link
Member Author

I tested the installer and portable versions on windows, and they both work.

@calixtus
Copy link
Member

Please fixe the language test, so we can merge.

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this cool feature!

@Siedlerchr Siedlerchr merged commit 18913f6 into JabRef:main Mar 26, 2022
Siedlerchr added a commit that referenced this pull request Mar 27, 2022
* upstream/main: (104 commits)
  update test getPart (#8610)
  Add ControlHelper truncateString tests comments (#8612)
  Allow using custom SSL certificates (#8583)
  Fix protectedTerms not stored due to weaklistener (#8609)
  Fix changelog and version parsing (#8578)
  Creating more unit tests for NumericFieldComparatorTest (#8604)
  Fix merge entries dialog exceeding screen size (#8599)
  StringUtilTest new test for method GetPart (#8594)
  Use unkown entry type
  Add semantic scholar (#8575)
  Add research gate (#8580)
  fix import of unlinked files (#8444) (#8582)
  Missed l10n for fetcher fix (#8597)
  Fix some fetcher test (#8595)
  Bump slf4j-api from 2.0.0-alpha6 to 2.0.0-alpha7 in /buildSrc (#8589)
  Bump ikonli-materialdesign2-pack from 12.3.0 to 12.3.1 (#8591)
  Bump gittools/actions from 0.9.11 to 0.9.13 (#8587)
  Bump mockito-core from 4.3.1 to 4.4.0 (#8588)
  Bump flowless from 0.6.8 to 0.6.9 (#8590)
  Bump ikonli-javafx from 12.3.0 to 12.3.1 (#8592)
  ...

# Conflicts:
#	src/main/java/org/jabref/gui/EntryTypeViewModel.java
Siedlerchr added a commit that referenced this pull request Mar 30, 2022
* upstream/main: (150 commits)
  fix unit test
  Add check for developer's documentation
  Merge GitBook view
  Fix zbMath fetcher (#8623)
  GitBook: [#56] No subject
  Add an extra dialog to ask the user whether they want to open the saved file folder when the export the entries (#8567)
  Bump checkstyle from 10.0 to 10.1 (#8620)
  Bump peter-evans/create-pull-request from 3 to 4 (#8619)
  Bump pascalgn/automerge-action from 0.14.3 to 0.15.2 (#8618)
  Bump flexmark from 0.62.2 to 0.64.0 (#8621)
  Bump classgraph from 4.8.141 to 4.8.143 (#8622)
  update test getPart (#8610)
  Add ControlHelper truncateString tests comments (#8612)
  Allow using custom SSL certificates (#8583)
  Fix protectedTerms not stored due to weaklistener (#8609)
  Fix changelog and version parsing (#8578)
  Creating more unit tests for NumericFieldComparatorTest (#8604)
  Fix merge entries dialog exceeding screen size (#8599)
  StringUtilTest new test for method GetPart (#8594)
  Use unkown entry type
  ...

# Conflicts:
#	build.gradle
#	src/main/java/org/jabref/logic/pdf/search/retrieval/PdfSearcher.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow other certificates
3 participants