SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968 #356

Alameyo · 2017-07-24T17:46:18Z

Generally trust managers, checking CRL and some minor things. Still TO-DO: accepting self signed certificates, make use of blacklist keystore and do OCSP.

akrherz · 2017-07-24T17:48:02Z

@Alameyo It would be good for your commit message to actually have the SPARK-???? nomenclature for each issue so that the Jira auto-linking works / this commit gets associated with the proper Jira issue.

Alameyo · 2017-07-24T17:51:58Z

@akrherz Ok, title was generated from commit comment and I forgot to add SPARK

Alameyo · 2017-07-27T19:21:02Z

Now also SPARK-1975 as Self Signed certificates can be accepted.

guusdk · 2017-07-27T22:03:04Z

That last commit did not have SPARK-1975 in it either. :)

You can (and should!) rewrite those commit messages. There are a couple of options, see: https://help.github.com/articles/changing-a-commit-message/

guusdk · 2017-07-28T00:02:00Z

core/src/main/java/org/jivesoftware/sparkimpl/certificates/SparkTrustManager.java

+
+            // check if certificate isn't self signed, self signed certificate still have to be in TrustStore to be
+            // accepted
+            if (chain.length > 1) {


Chain length is not a safe way to determine if the certificate in the chain is self-signed. The chain could be incomplete (it might depend on certificates that are in the truststore, without including those certificates), and chains might contain certificates that are not part of the chain at all (and are unused). You should find a better way to identify "self-signed"

Alameyo · 2017-07-29T12:50:04Z

Added support for SPARK-1977 OCSP.

guusdk · 2017-08-02T10:54:20Z

core/src/main/java/org/jivesoftware/sparkimpl/certificates/SparkTrustManager.java

+     * Return true if the certificate chain contain only one Self Signed certificate
+     */
+    private boolean isSelfSigned(X509Certificate[] chain) {
+        if(chain[0].getIssuerX500Principal().getName().equals(chain[0].getSubjectX500Principal().getName())){


Perhaps replace by: return chain[0].getIssuerX500Principal().getName().equals(chain[0].getSubjectX500Principal().getName()) (there's no need for the if/else)?

guusdk · 2017-08-02T10:58:01Z

core/src/main/java/org/jivesoftware/sparkimpl/certificates/SparkTrustManager.java

@@ -187,6 +188,17 @@ public void checkServerTrusted(X509Certificate[] chain, String authType) throws
    }

    /**
+     * Return true if the certificate chain contain only one Self Signed certificate
+     */
+    private boolean isSelfSigned(X509Certificate[] chain) {


This code assumes that the first certificate is the self-signed one, which could lead to several problems. For example, a certificate chain should, but is not always ordered (and if it is, I think it's typically the last certificate that's self-signed).

Perhaps you should consider changing the argument to this method: instead of passing a chain, you might want to pass a single certificate. That way, this method's responsibility is very well defined. If needed, you could add additional methods that check if the entire chain represents a self-signed certificate (by checking if the chain size is equal to 1, and by passing that certificate to this method).

I added check if chain length equals 1. If it is true together as well as same subject issuer requirement then certificate is self signed. This way in case when chain would be reversed there will be no problems about that.
One note: I don't worry here about standard root CA at the end of certificate path but about situation when server send only one self signed certificate for it's identification. If chain is longer than 1 then it is standard path building.

guusdk · 2017-08-02T11:00:35Z

core/src/main/java/org/jivesoftware/sparkimpl/certificates/SparkTrustManager.java

+                PKIXRevocationChecker checker = (PKIXRevocationChecker) certPathBuilder.getRevocationChecker();
+                // soft fail option mean that OCSP or CRL must pass validation, in case when OCSP cannot be
+                // validated it will validate CRL and then if both cannot be checked it will fail
+                checker.setOptions(EnumSet.of(PKIXRevocationChecker.Option.SOFT_FAIL));


Should this be configurable? Perhaps the end-user should control if SOFT_FAIL should apply.

…ally SPARK-1968

… subject name and issuer name (have to be equal).

…PKIX validator to use default one.

…and some other minor changes.

guusdk · 2017-08-04T09:58:06Z

core/src/main/java/org/jivesoftware/AccountCreationWizard.java

-
+
+        if (securityMode != ConnectionConfiguration.SecurityMode.disabled && !useOldSSL) {
+            builder.setPort(5222);


Does this take into account that the user can also set another port than 5222? If that's done, we should not hard-code to 5222.

Changed. I digged into that a bit more and while 5222 is recommended because other ports might be blocked by firewalls, there is no general prohibition against using other ports. Though so far I cannot connect without using "automatically discover host and port".

guusdk · 2017-08-04T09:58:54Z

core/src/main/java/org/jivesoftware/LoginDialog.java

@@ -277,6 +284,22 @@ protected XMPPTCPConnectionConfiguration retrieveConnectionConfiguration() {
            builder.setProxyInfo( proxyInfo );
        }

+        if (securityMode != ConnectionConfiguration.SecurityMode.disabled && !useOldSSL) {


(same as before - can you get rid of this code duplication?) Does this take into account that the user can also set another port than 5222? If that's done, we should not hard-code to 5222.

Indeed this two classes share a bunch of the similar code, tomorrow I will try to reduce some of its redundancy.

guusdk · 2017-08-04T10:06:22Z

core/src/main/java/org/jivesoftware/spark/ui/login/CertificatesManagerSettingsPanel.java

 		ResourceUtils.resButton(acceptSelfSigned, Res.getString("checkbox.accept.self.signed"));
 		ResourceUtils.resButton(checkCRL, Res.getString("checkbox.check.crl"));
 		ResourceUtils.resButton(checkOCSP, Res.getString("checkbox.check.ocsp"));
 		ResourceUtils.resButton(showCert, Res.getString("button.show.certificate"));
 		ResourceUtils.resButton(fileButton, Res.getString("label.choose.file"));
-
+


Small bug: when I open the "certificates" tab and the "accept all" option is already checked, then the others are not disabled. It works when I unselect and reselect again.

Added setUp method to SPARKSSLContext, removed acceptAll from Security tab as there is already Accept All checkbox in certificates tab.

Alameyo requested a review from guusdk July 24, 2017 17:46

Alameyo changed the title ~~SPARK-1966, 1952, 1969, 1970, 1971 and partially 1968~~ SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968 Jul 24, 2017

Alameyo changed the title ~~SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968~~ SPARK-1960, SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968 Jul 24, 2017

Alameyo force-pushed the SSLContext branch 7 times, most recently from 81e4228 to 81ba4c9 Compare July 27, 2017 13:12

Alameyo changed the title ~~SPARK-1960, SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968~~ SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968 Jul 27, 2017

Alameyo force-pushed the SSLContext branch from eb595f9 to 775b476 Compare July 27, 2017 19:49

Alameyo force-pushed the SSLContext branch from 775b476 to 358c77d Compare July 27, 2017 22:40

guusdk reviewed Jul 28, 2017

View reviewed changes

guusdk reviewed Aug 2, 2017

View reviewed changes

Alameyo added 5 commits August 3, 2017 22:47

SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and parti…

f7c09f7

…ally SPARK-1968

SPARK-1975 Self Signed certificates now will be accepted.

ee36077

Additional file path changes.

c0631d2

Now information if certificate is Self Signed is determined comparing…

a17fb9e

… subject name and issuer name (have to be equal).

SPARK-1977 Support for OCSP. Also removed Bouncy Castle provider for …

bf2570b

…PKIX validator to use default one.

Alameyo force-pushed the SSLContext branch from 675e433 to 7ff8678 Compare August 3, 2017 20:52

Changes for self signed certificate, soft fail policy, not Valid Yet …

6c4c731

…and some other minor changes.

Alameyo force-pushed the SSLContext branch from 7ff8678 to 6c4c731 Compare August 3, 2017 21:03

guusdk reviewed Aug 4, 2017

View reviewed changes

Alameyo force-pushed the SSLContext branch 2 times, most recently from c6edac8 to 034cb63 Compare August 5, 2017 18:52

Removed setting port for STARTTLS, fixed enabled on start checkboxes.

8fc3d2c

Added setUp method to SPARKSSLContext, removed acceptAll from Security tab as there is already Accept All checkbox in certificates tab.

Alameyo force-pushed the SSLContext branch from 034cb63 to 8fc3d2c Compare August 5, 2017 18:56

guusdk merged commit 29f89bd into igniterealtime:master Aug 7, 2017

Alameyo deleted the SSLContext branch December 9, 2018 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968 #356

SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968 #356

Alameyo commented Jul 24, 2017

akrherz commented Jul 24, 2017

Alameyo commented Jul 24, 2017

Alameyo commented Jul 27, 2017

guusdk commented Jul 27, 2017

guusdk Jul 28, 2017

Alameyo commented Jul 29, 2017 •

edited

Loading

guusdk Aug 2, 2017

guusdk Aug 2, 2017

Alameyo Aug 3, 2017

guusdk Aug 2, 2017

guusdk Aug 4, 2017

Alameyo Aug 4, 2017 •

edited

Loading

guusdk Aug 4, 2017

Alameyo Aug 4, 2017

guusdk Aug 4, 2017

Alameyo Aug 4, 2017



		if (securityMode != ConnectionConfiguration.SecurityMode.disabled && !useOldSSL) {
		builder.setPort(5222);

SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968 #356

SPARK-1966, SPARK-1952, SPARK-1969, SPARK-1970, SPARK-1971 and partially SPARK-1968 #356

Conversation

Alameyo commented Jul 24, 2017

akrherz commented Jul 24, 2017

Alameyo commented Jul 24, 2017

Alameyo commented Jul 27, 2017

guusdk commented Jul 27, 2017

Choose a reason for hiding this comment

Alameyo commented Jul 29, 2017 • edited Loading

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

Alameyo Aug 4, 2017 • edited Loading

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

Alameyo commented Jul 29, 2017 •

edited

Loading

Alameyo Aug 4, 2017 •

edited

Loading