Skip to content

Fail Firestore client if we can't establish SSL connection #508

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

Merged
merged 13 commits into from
Jun 19, 2019
Merged
4 changes: 4 additions & 0 deletions firebase-firestore/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
# Unreleased
- [changed] Instead of failing silently, Firestore now crashes the client app
if it fails to load SSL Ciphers. To avoid these crashes, you must bundle
Conscrypt to support non-GMSCore devices on Android KitKat or JellyBean (see
https://github.com/grpc/grpc-java/blob/master/SECURITY.md#tls-on-android).

# 20.1.0
- [changed] SSL and gRPC initialization now happens on a separate thread, which
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

package com.google.firebase.firestore.remote;

import static com.google.firebase.firestore.remote.Datastore.SSL_DEPENDENCY_ERROR_MESSAGE;
import static com.google.firebase.firestore.util.Assert.hardAssert;

import androidx.annotation.Nullable;
Expand All @@ -24,6 +25,7 @@
import com.google.firebase.firestore.util.AsyncQueue.TimerId;
import com.google.firebase.firestore.util.ExponentialBackoff;
import com.google.firebase.firestore.util.Logger;
import com.google.firebase.firestore.util.Util;
import io.grpc.ClientCall;
import io.grpc.Metadata;
import io.grpc.MethodDescriptor;
Expand Down Expand Up @@ -269,6 +271,13 @@ private void close(State finalState, Status status) {
"Can't provide an error when not in an error state.");
workerQueue.verifyIsCurrentThread();

if (Datastore.isSslHandshakeError(status)) {
// The Android device is missing required SSL Ciphers. This error is non-recoverable and must
// be addressed by the app developer (see https://bit.ly/2XFpdma).
Util.crashMainThread(
new IllegalStateException(SSL_DEPENDENCY_ERROR_MESSAGE, status.getCause()));
}

// Cancel any outstanding timers (they're guaranteed not to execute).
cancelIdleCheck();
this.backoff.cancel();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.firebase.firestore.remote;

import android.content.Context;
import android.os.Build;
import com.google.android.gms.tasks.Task;
import com.google.firebase.firestore.FirebaseFirestoreException;
import com.google.firebase.firestore.auth.CredentialsProvider;
Expand All @@ -31,13 +32,15 @@
import com.google.firestore.v1.CommitResponse;
import com.google.firestore.v1.FirestoreGrpc;
import io.grpc.Status;
import java.net.ConnectException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import javax.net.ssl.SSLHandshakeException;

/**
* Datastore represents a proxy for the remote server, hiding details of the RPC layer. It:
Expand All @@ -54,6 +57,16 @@
*/
public class Datastore {

/**
* Error message to surface when Firestore fails to establish an SSL connection. A failed SSL
* connection likely indicates that the developer needs to provide an updated OpenSSL stack as
* part of their app's dependencies.
*/
static final String SSL_DEPENDENCY_ERROR_MESSAGE =
"The Firestore SDK failed to establish a secure connection. This is likely a problem with "
+ "your app, rather than with Firestore itself. See https://bit.ly/2XFpdma for "
+ "instructions on how to enable TLS on Android 4.x devices.";

/** Set of lowercase, white-listed headers for logging purposes. */
static final Set<String> WHITE_LISTED_HEADERS =
new HashSet<>(
Expand Down Expand Up @@ -208,6 +221,22 @@ public static boolean isPermanentError(Status status) {
}
}

/**
* Determine whether the given status maps to the error that GRPC-Java throws when an Android
* device is missing required SSL Ciphers.
*
* <p>This error is non-recoverable and must be addressed by the app developer.
*/
public static boolean isSslHandshakeError(Status status) {
Status.Code code = status.getCode();
Throwable t = status.getCause();

return Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP
&& code.equals(Status.Code.UNAVAILABLE)
&& (t instanceof SSLHandshakeException
|| (t instanceof ConnectException && t.getMessage().contains("EHOSTUNREACH")));
}

/**
* Determines whether the given status has an error code that represents a permanent error when
* received in response to a write operation.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public void onClose(Status status, Metadata trailers) {
if (status.isOk()) {
tcs.setResult(results);
} else {
tcs.setException(Util.exceptionFromStatus(status));
tcs.setException(exceptionFromStatus(status));
}
}
},
Expand Down Expand Up @@ -244,7 +244,7 @@ public void onClose(Status status, Metadata trailers) {
Code.INTERNAL));
}
} else {
tcs.setException(Util.exceptionFromStatus(status));
tcs.setException(exceptionFromStatus(status));
}
}
},
Expand All @@ -262,6 +262,17 @@ public void onClose(Status status, Metadata trailers) {
return tcs.getTask();
}

private FirebaseFirestoreException exceptionFromStatus(Status status) {
if (Datastore.isSslHandshakeError(status)) {
return new FirebaseFirestoreException(
Datastore.SSL_DEPENDENCY_ERROR_MESSAGE,
Code.fromValue(status.getCode().value()),
status.getCause());
}

return Util.exceptionFromStatus(status);
}

public void invalidateToken() {
credentialsProvider.invalidateToken();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

package com.google.firebase.firestore.util;

import android.os.Handler;
import android.os.Looper;
import androidx.annotation.Nullable;
import com.google.android.gms.tasks.Continuation;
import com.google.cloud.datastore.core.number.NumberComparisonHelper;
Expand Down Expand Up @@ -211,4 +213,13 @@ public static String toDebugString(ByteString bytes) {
public static String typeName(@Nullable Object obj) {
return obj == null ? "null" : obj.getClass().getName();
}

/** Raises an exception on Android's UI Thread and crashes the end user's app. */
public static void crashMainThread(RuntimeException exception) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: the exception's stacktrace is constructed when the exception is created, not when it is thrown. So the throw site(below) will not be part of the resulting stacktrace, make sure it is ok and won't cause confusion as I am not familiar enough with your codebase to make such a call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Stacktrace reflects the callsite for this function, which is either AbstractStream or FirestoreChannel. To me, those are the correct places and I prefer this over crashing from Util.

Copy link
Member

Choose a reason for hiding this comment

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

sgtm

new Handler(Looper.getMainLooper())
.post(
() -> {
throw exception;
});
}
}