Skip to content

Commit 7cab545

Browse files
velitasalivelitasali
committed
encrpytion: Warn both parties on SSL errors
Previously, we were trying to warn the server when there is an SSL error, indicating that a client is pretending to be someone else. The problem is that it is hard to accuretely to detect which side is on the wrong using the exception types because different JVM implementations throw different types of exceptions for the same type of error. In the end, I decided to warn both parties. because it is easier and should lead to 'less' confusion. Credentials should not change by default, so I don't think any of the warning systems will be in use that prominently anyway.
1 parent 3df41ca commit 7cab545

7 files changed

Lines changed: 123 additions & 32 deletions

File tree

‎pom.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222

2323
<groupId>org.monora.uprotocol</groupId>
2424
<artifactId>core</artifactId>
25-
<version>0.1.0-ALPHA33</version>
25+
<version>0.1.0-ALPHA34</version>
2626
<name>UProtocol</name>
2727
<description>Java implementation of uprotocol, an open specification for p2p content exchange</description>
2828
<url>https://github.com/uprotocol</url>

‎src/main/java/org/monora/uprotocol/core/CommunicationBridge.java‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@
3030
import org.monora.uprotocol.core.protocol.Client;
3131
import org.monora.uprotocol.core.protocol.ClientAddress;
3232
import org.monora.uprotocol.core.protocol.ConnectionFactory;
33-
import org.monora.uprotocol.core.protocol.communication.CommunicationException;
33+
import org.monora.uprotocol.core.protocol.communication.CredentialsException;
3434
import org.monora.uprotocol.core.protocol.communication.ProtocolException;
3535
import org.monora.uprotocol.core.protocol.communication.SecurityException;
3636
import org.monora.uprotocol.core.protocol.communication.client.BlockedRemoteClientException;
@@ -39,6 +39,7 @@
3939
import org.monora.uprotocol.core.transfer.TransferItem;
4040
import org.monora.uprotocol.core.transfer.Transfers;
4141

42+
import javax.net.ssl.SSLException;
4243
import javax.net.ssl.SSLSocket;
4344
import javax.net.ssl.SSLSocketFactory;
4445
import java.io.Closeable;
@@ -141,7 +142,7 @@ public void closeSafely() throws IOException
141142
static void convertToSSL(@NotNull ConnectionFactory connectionFactory,
142143
@NotNull PersistenceProvider persistenceProvider,
143144
@NotNull ActiveConnection activeConnection, @NotNull Client client, boolean isClient)
144-
throws IOException, CommunicationException, CertificateException
145+
throws IOException, ProtocolException, CertificateException
145146
{
146147
Socket socket = activeConnection.getSocket();
147148
SSLSocketFactory sslSocketFactory = persistenceProvider.getSSLContextFor(client).getSocketFactory();
@@ -188,8 +189,15 @@ static void convertToSSL(@NotNull ConnectionFactory connectionFactory,
188189

189190
try {
190191
sslSocket.startHandshake();
192+
} catch (SSLException e) {
193+
boolean firstTime = !persistenceProvider.hasRequestForInvalidationOfCredentials(client.getClientUid());
194+
if (firstTime) {
195+
persistenceProvider.saveRequestForInvalidationOfCredentials(client.getClientUid());
196+
}
197+
198+
throw new CredentialsException(client, e, firstTime);
191199
} catch (Exception e) {
192-
throw new SecurityException(client, e);
200+
throw new ProtocolException(e);
193201
}
194202
}
195203

@@ -586,8 +594,16 @@ public CommunicationBridge connect() throws IOException, JSONException, Protocol
586594
persistenceProvider.persist(clientAddress);
587595

588596
Responses.checkError(client, jsonObject);
589-
convertToSSL(connectionFactory, persistenceProvider, activeConnection, client, true);
590597

598+
try {
599+
convertToSSL(connectionFactory, persistenceProvider, activeConnection, client, true);
600+
} catch (SecurityException e) {
601+
if (!persistenceProvider.hasRequestForInvalidationOfCredentials(client.getClientUid())) {
602+
persistenceProvider.saveRequestForInvalidationOfCredentials(client.getClientUid());
603+
}
604+
605+
throw e;
606+
}
591607
return new CommunicationBridge(persistenceProvider, activeConnection, client, clientAddress);
592608
}
593609

‎src/main/java/org/monora/uprotocol/core/TransportSession.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.monora.uprotocol.core.protocol.ClientAddress;
1414
import org.monora.uprotocol.core.protocol.ConnectionFactory;
1515
import org.monora.uprotocol.core.protocol.communication.ContentException;
16+
import org.monora.uprotocol.core.protocol.communication.CredentialsException;
1617
import org.monora.uprotocol.core.protocol.communication.ProtocolException;
1718
import org.monora.uprotocol.core.protocol.communication.SecurityException;
1819
import org.monora.uprotocol.core.spec.v1.Config;
@@ -100,11 +101,12 @@ public void onConnected(@NotNull ActiveConnection activeConnection)
100101

101102
handleRequest(new CommunicationBridge(persistenceProvider, activeConnection, client, clientAddress),
102103
client, clientAddress, hasPin, request);
103-
} catch (SecurityException e) {
104-
if (!persistenceProvider.hasRequestForInvalidationOfCredentials(e.client.getClientUid())) {
105-
persistenceProvider.saveRequestForInvalidationOfCredentials(e.client.getClientUid());
104+
} catch (CredentialsException e) {
105+
if (e.firstTime) {
106106
transportSeat.notifyClientCredentialsChanged(e.client);
107107
}
108+
} catch (SecurityException e) {
109+
getLogger().log(Level.INFO, "Security error occurred: " + e.toString());
108110
} catch (ClosedException e) {
109111
getLogger().log(Level.INFO, "Closed successfully by " + (e.remoteRequested ? "remote" : "you"));
110112
} catch (CancelledException e) {

‎src/main/java/org/monora/uprotocol/core/protocol/communication/CredentialsException.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
package org.monora.uprotocol.core.protocol.communication;
2+
3+
import org.jetbrains.annotations.NotNull;
4+
import org.monora.uprotocol.core.persistence.PersistenceProvider;
5+
import org.monora.uprotocol.core.protocol.Client;
6+
7+
/**
8+
* Thrown when an SSL-related error occurs and is considered a result of invalid credentials.
9+
*/
10+
public class CredentialsException extends SecurityException
11+
{
12+
/**
13+
* Will be true if {@link PersistenceProvider#hasRequestForInvalidationOfCredentials(String)} returned false and
14+
* {@link PersistenceProvider#saveRequestForInvalidationOfCredentials(String)} was invoked.
15+
*/
16+
public final boolean firstTime;
17+
18+
/**
19+
* Create a new instance.
20+
*
21+
* @param client With which the error occurred.
22+
* @param cause Of the issue.
23+
* @param firstTime Will be true if {@link PersistenceProvider#hasRequestForInvalidationOfCredentials(String)}
24+
* returned false and {@link PersistenceProvider#saveRequestForInvalidationOfCredentials(String)}
25+
* was invoked.
26+
*/
27+
public CredentialsException(@NotNull Client client, @NotNull Throwable cause, boolean firstTime)
28+
{
29+
super(client, cause);
30+
this.firstTime = firstTime;
31+
}
32+
}

‎src/main/java/org/monora/uprotocol/core/protocol/communication/SecurityException.java‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,15 @@
11
package org.monora.uprotocol.core.protocol.communication;
22

33
import org.jetbrains.annotations.NotNull;
4-
import org.jetbrains.annotations.Nullable;
54
import org.monora.uprotocol.core.protocol.Client;
65

76
/**
87
* Thrown when an SSL-related error occurs.
9-
* <p>
10-
* The cause reflects what exactly went wrong.
118
*/
129
public class SecurityException extends CommunicationException
1310
{
14-
public SecurityException(@NotNull Client client, @Nullable Throwable cause)
11+
public SecurityException(@NotNull Client client, @NotNull Throwable cause)
1512
{
1613
super(client, cause);
17-
18-
if (cause == null)
19-
throw new NullPointerException("The cause cannot be null.");
2014
}
2115
}

‎src/test/java/org/monora/uprotocol/RequestTest.java‎

Lines changed: 54 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@
88
import org.monora.uprotocol.core.CommunicationBridge;
99
import org.monora.uprotocol.core.protocol.Client;
1010
import org.monora.uprotocol.core.protocol.communication.ContentException;
11+
import org.monora.uprotocol.core.protocol.communication.CredentialsException;
1112
import org.monora.uprotocol.core.protocol.communication.ProtocolException;
12-
import org.monora.uprotocol.core.protocol.communication.SecurityException;
1313
import org.monora.uprotocol.core.protocol.communication.client.BlockedRemoteClientException;
1414
import org.monora.uprotocol.core.protocol.communication.client.DifferentRemoteClientException;
1515
import org.monora.uprotocol.core.protocol.communication.client.UnauthorizedClientException;
@@ -19,7 +19,6 @@
1919
import org.monora.uprotocol.variant.holder.TransferHolder;
2020
import org.monora.uprotocol.variant.test.DefaultTestBase;
2121

22-
import javax.net.ssl.SSLHandshakeException;
2322
import java.io.IOException;
2423
import java.security.cert.CertificateException;
2524
import java.util.ArrayList;
@@ -163,8 +162,8 @@ public void remoteNotifiesAsNotFoundWhenGroupNotExists() throws ProtocolExceptio
163162
}
164163
}
165164

166-
@Test(expected = SecurityException.class)
167-
public void failsWithKeyMismatchTest() throws IOException, InterruptedException, ProtocolException,
165+
@Test(expected = CredentialsException.class)
166+
public void failsWithCredentialsMismatchTest() throws IOException, InterruptedException, ProtocolException,
168167
CertificateException
169168
{
170169
primarySession.start();
@@ -223,7 +222,7 @@ public void connectsAfterKeyMismatchWithRightKey() throws IOException, ProtocolE
223222

224223
try (CommunicationBridge bridge = openConnection(secondaryPersistence, clientAddress)) {
225224
bridge.requestAcquaintance();
226-
} catch (SecurityException ignored) {
225+
} catch (CredentialsException ignored) {
227226
}
228227

229228
primaryPersistence.restoreSecrets();
@@ -249,12 +248,9 @@ public void acceptNewKeysTest() throws IOException, InterruptedException, Protoc
249248

250249
try (CommunicationBridge bridge = openConnection(secondaryPersistence, clientAddress)) {
251250
bridge.requestAcquaintance();
252-
} catch (SecurityException e) {
253-
if (e.getCause() instanceof SSLHandshakeException) {
254-
e.client.setClientCertificate(null);
255-
secondaryPersistence.persist(e.client, true);
256-
} else
257-
throw e;
251+
} catch (CredentialsException e) {
252+
e.client.setClientCertificate(null);
253+
secondaryPersistence.persist(e.client, true);
258254
}
259255

260256
try (CommunicationBridge bridge = openConnection(secondaryPersistence, clientAddress)) {
@@ -468,10 +464,56 @@ public void throwsDifferentClientExceptionOnUidMismatch() throws IOException, In
468464

469465
primarySession.start();
470466

471-
try(CommunicationBridge bridge = builder.connect()) {
467+
try (CommunicationBridge bridge = builder.connect()) {
472468
Assert.fail("The above statement should have failed");
473469
} finally {
474470
primarySession.stop();
475471
}
476472
}
473+
474+
@Test
475+
public void warnsBothPartiesOnInvalidServerCredentials() throws IOException, InterruptedException,
476+
ProtocolException, CertificateException
477+
{
478+
primarySession.start();
479+
480+
try (CommunicationBridge bridge = openConnection(secondaryPersistence, clientAddress)) {
481+
bridge.requestAcquaintance();
482+
}
483+
484+
primaryPersistence.regenerateSecrets();
485+
486+
try (CommunicationBridge bridge = openConnection(secondaryPersistence, clientAddress)) {
487+
Assert.fail("Should have not reached here.");
488+
} catch (Exception e) {
489+
Assert.assertTrue("Should result in the right error", e instanceof CredentialsException);
490+
} finally {
491+
primarySession.stop();
492+
}
493+
494+
Assert.assertTrue("Both parties should be warned", primaryPersistence.gotInvalidationRequest());
495+
}
496+
497+
@Test
498+
public void warnsBothPartiesOnInvalidClientCredentials() throws IOException, InterruptedException,
499+
ProtocolException, CertificateException
500+
{
501+
primarySession.start();
502+
503+
try (CommunicationBridge bridge = openConnection(secondaryPersistence, clientAddress)) {
504+
bridge.requestAcquaintance();
505+
}
506+
507+
secondaryPersistence.regenerateSecrets();
508+
509+
try (CommunicationBridge bridge = openConnection(secondaryPersistence, clientAddress)) {
510+
Assert.fail("Should have not reached here");
511+
} catch (Exception e) {
512+
Assert.assertTrue("Should result in the right error", e instanceof CredentialsException);
513+
} finally {
514+
primarySession.stop();
515+
}
516+
517+
Assert.assertTrue("Both parties should be warned", primaryPersistence.gotInvalidationRequest());
518+
}
477519
}

‎src/test/java/org/monora/uprotocol/variant/persistence/BasePersistenceProvider.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,8 @@ public abstract class BasePersistenceProvider implements PersistenceProvider
6161
private @Nullable KeyPair keyPairBackup;
6262
private @Nullable X509Certificate certificateBackup;
6363

64+
private boolean gotInvalidationRequest = false;
65+
6466
private int networkPin;
6567

6668
public BasePersistenceProvider()
@@ -108,7 +110,13 @@ public BasePersistenceProvider()
108110
}
109111
}
110112

111-
public boolean hasPicture(@NotNull Client client) {
113+
public boolean gotInvalidationRequest()
114+
{
115+
return gotInvalidationRequest;
116+
}
117+
118+
public boolean hasPicture(@NotNull Client client)
119+
{
112120
return pictureList.containsKey(client.getClientUid());
113121
}
114122

@@ -406,10 +414,7 @@ public void revokeNetworkPin()
406414
@Override
407415
public void saveRequestForInvalidationOfCredentials(@NotNull String clientUid)
408416
{
409-
if (hasRequestForInvalidationOfCredentials(clientUid)) {
410-
return;
411-
}
412-
417+
gotInvalidationRequest = true;
413418
synchronized (invalidationRequestList) {
414419
invalidationRequestList.add(clientUid);
415420
}

0 commit comments

Comments
 (0)