Skip to content

Commit

Permalink
Move TLS Required check at the end of connect()
Browse files Browse the repository at this point in the history
It was a *very* bad idea to perform the SecurityMode.Required check in
the connection's reader thread and not at the end of
AbstractXMPPConnectin's connect(). :/

This behavior dates back to 8e75091

Fixes SMACK-739
  • Loading branch information
Flowdalic committed Nov 12, 2016
1 parent 013f4d6 commit a9d5cd4
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.jivesoftware.smack.SmackException.NoResponseException;
import org.jivesoftware.smack.SmackException.NotConnectedException;
import org.jivesoftware.smack.SmackException.ResourceBindingNotOfferedException;
import org.jivesoftware.smack.SmackException.SecurityRequiredByClientException;
import org.jivesoftware.smack.SmackException.SecurityRequiredException;
import org.jivesoftware.smack.XMPPException.StreamErrorException;
import org.jivesoftware.smack.XMPPException.XMPPErrorException;
Expand Down Expand Up @@ -373,6 +374,13 @@ public synchronized AbstractXMPPConnection connect() throws SmackException, IOEx
// Wait with SASL auth until the SASL mechanisms have been received
saslFeatureReceived.checkIfSuccessOrWaitOrThrow();

// If TLS is required but the server doesn't offer it, disconnect
// from the server and throw an error. First check if we've already negotiated TLS
// and are secure, however (features get parsed a second time after TLS is established).
if (!isSecureConnection() && getConfiguration().getSecurityMode() == SecurityMode.required) {
throw new SecurityRequiredByClientException();
}

// Make note of the fact that we're now connected.
connected = true;
callConnectionConnectedListener();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,7 @@
import org.jivesoftware.smack.SmackException.NoResponseException;
import org.jivesoftware.smack.SmackException.NotConnectedException;
import org.jivesoftware.smack.SmackException.ConnectionException;
import org.jivesoftware.smack.SmackException.SecurityRequiredByClientException;
import org.jivesoftware.smack.SmackException.SecurityRequiredByServerException;
import org.jivesoftware.smack.SmackException.SecurityRequiredException;
import org.jivesoftware.smack.SynchronizationPoint;
import org.jivesoftware.smack.XMPPException.StreamErrorException;
import org.jivesoftware.smack.XMPPConnection;
Expand Down Expand Up @@ -917,7 +915,7 @@ protected void setWriter(Writer writer) {
}

@Override
protected void afterFeaturesReceived() throws SecurityRequiredException, NotConnectedException, InterruptedException {
protected void afterFeaturesReceived() throws NotConnectedException, InterruptedException {
StartTls startTlsFeature = getFeature(StartTls.ELEMENT, StartTls.NAMESPACE);
if (startTlsFeature != null) {
if (startTlsFeature.required() && config.getSecurityMode() == SecurityMode.disabled) {
Expand All @@ -929,13 +927,6 @@ protected void afterFeaturesReceived() throws SecurityRequiredException, NotConn
sendNonza(new StartTls());
}
}
// If TLS is required but the server doesn't offer it, disconnect
// from the server and throw an error. First check if we've already negotiated TLS
// and are secure, however (features get parsed a second time after TLS is established).
if (!isSecureConnection() && startTlsFeature == null
&& getConfiguration().getSecurityMode() == SecurityMode.required) {
throw new SecurityRequiredByClientException();
}

if (getSASLAuthentication().authenticationSuccessful()) {
// If we have received features after the SASL has been successfully completed, then we
Expand Down

1 comment on commit a9d5cd4

@konradkierus
Copy link

Choose a reason for hiding this comment

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

@Flowdalic The issue SMACK-739 isn't listed here: https://issues.igniterealtime.org/projects/SMACK/issues/
Can you add the issue to the list or just explain what issue fixes this commit?

Please sign in to comment.