Improve OIDC compliance #265

lbalmaceda · 2019-11-28T19:28:05Z

Changes

This update improves the SDK support for OpenID Connect. In particular, it modifies the sign-in verification phase by substituting backchannel based checks with id_token validation where possible.

Testing

This PR contains some unit tests and integration tests to prove the logic is in place for the different authentication scenarios. Additional unit tests on a separate PR will complement the token verification logic.

This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not

Checklist

I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors

lbalmaceda · 2019-11-29T20:25:30Z

auth0/src/test/resources/jwks_rsa.json

@@ -1,16 +0,0 @@
-{


renamed to rsa_jwks.json

lbalmaceda · 2019-11-29T20:25:53Z

auth0/src/test/java/com/auth0/android/util/MockAuthCallback.java

+
+import java.util.concurrent.Callable;
+
+public class MockAuthCallback implements AuthCallback {


belongs to the test folder. Helps assert async results

lbalmaceda · 2019-11-29T20:26:17Z

auth0/src/test/java/com/auth0/android/util/AuthCallbackMatcher.java

+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
+
+public class AuthCallbackMatcher extends BaseMatcher<AuthCallback> {


belongs to the test folder. Helps assert async results

lbalmaceda · 2019-11-29T20:26:51Z

auth0/src/test/java/com/auth0/android/request/internal/JwksGsonTest.java

-            "343064304897328574182258821353161480771090757577522755330350956839942443917109665602066633259326057104246834759269337437620283600011255" +
-            "336891753201992873507788546321379785324808697815139356191481700515096717744643081207558743005545844515461091232322385145532862505800570" +
-            "17053908908888974416937916517307412534155165744508160026990874221314025481541880285443289277406354687335857437573080004037";
+    private static final String EXPECTED_RSA_MODULUS = "2327856100899355911632462598898247024131242" +


updated to match the new RSA keys for the test resources

lbalmaceda · 2019-11-29T20:27:26Z

auth0/src/test/java/com/auth0/android/provider/OAuthManagerTest.java

-        assertThat(merged.getType(), is(codeCredentials.getType()));
-        assertThat(merged.getRefreshToken(), is(codeCredentials.getRefreshToken()));
-        assertThat(merged.getExpiresIn(), is(codeCredentials.getExpiresIn()));
+        Credentials frontChannelCredentials = new Credentials("urlId", "urlAccess", "urlType", null, expiresAt, "urlScope");


basically changed the logic so the preferred credentials are:

id token from the front channel, when available

remaining things from the back channel

lbalmaceda · 2019-11-29T21:46:26Z

auth0/src/main/java/com/auth0/android/provider/OAuthManager.java

@@ -128,40 +136,115 @@ boolean resume(AuthorizeResult result) {
        try {
            assertNoError(values.get(KEY_ERROR), values.get(KEY_ERROR_DESCRIPTION));
            assertValidState(parameters.get(KEY_STATE), values.get(KEY_STATE));
-            if (parameters.containsKey(KEY_RESPONSE_TYPE) && parameters.get(KEY_RESPONSE_TYPE).contains(RESPONSE_TYPE_ID_TOKEN)) {


nonce check moved into ID token verification logic

lbalmaceda · 2019-11-29T21:47:39Z

auth0/src/main/java/com/auth0/android/provider/OAuthManager.java

+        final Credentials frontChannelCredentials = new Credentials(values.get(KEY_ID_TOKEN), values.get(KEY_ACCESS_TOKEN), values.get(KEY_TOKEN_TYPE), values.get(KEY_REFRESH_TOKEN), expiresAt, values.get(KEY_SCOPE));
+        boolean frontChannelIdTokenExpected = parameters.containsKey(KEY_RESPONSE_TYPE) && parameters.get(KEY_RESPONSE_TYPE).contains(RESPONSE_TYPE_ID_TOKEN);
+
+        if (frontChannelIdTokenExpected) {


always verify the front-channel ID token first, if present. Also, prefer this one over the one received on the back channel (if there is one)

auth0/src/main/java/com/auth0/android/provider/SignatureVerifier.java

lbalmaceda · 2019-11-29T21:57:10Z

auth0/src/main/java/com/auth0/android/provider/SymmetricSignatureVerifier.java

+/**
+ * Token signature verifier for HS256 algorithms.
+ */
+class SymmetricSignatureVerifier extends SignatureVerifier {


Kept for similarity across implementations. But could remove this class if the main class is declared non-abstract and already implements "doing nothing". Asymmetric could subclass it and implement the new behavior, like today.

I would suggest omitting it. Creates a bit of confusion having it (IMHO)

@damieng what's your take here? I know for .net you've added a NoSignatureVerifier that extends/implements SignatureVerifier. Should we keep this but rename it as yours, or remove it?

lbalmaceda · 2019-11-29T21:58:37Z

auth0/src/test/java/com/auth0/android/provider/OAuthManagerTest.java

-
-    @Test
-    public void shouldHaveValidNonce() {
-        OAuthManager.assertValidNonce("1234567890", "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJub25jZSI6IjEyMzQ1Njc4OTAifQ.oUb6xFIEPJQrFbel_Js4SaOwpFfM_kxHxI7xDOHgghk");


This static method was removed. But the scenario is covered in WebAuthProviderTest

…ivate

joshcanhelp

A few questions/comments.

My main concern with this PR is all the extra work done in the callback. Is all that necessary to add ID token validation? Particularly when we really should be pushing developers towards response_type=code here. Feels like a lot of work for situations that we should, at some point, disable.

auth0/src/test/resources/rsa_private.pem

auth0/src/test/resources/rsa_jwks.json

auth0/src/main/java/com/auth0/android/provider/AsymmetricSignatureVerifier.java

auth0/src/main/java/com/auth0/android/provider/IdTokenVerifier.java

auth0/src/test/java/com/auth0/android/util/MockAuthCallback.java

auth0/src/main/java/com/auth0/android/provider/SignatureVerifier.java

joshcanhelp · 2019-12-03T00:40:58Z

auth0/src/main/java/com/auth0/android/provider/SymmetricSignatureVerifier.java

+/**
+ * Token signature verifier for HS256 algorithms.
+ */
+class SymmetricSignatureVerifier extends SignatureVerifier {


I would suggest omitting it. Creates a bit of confusion having it (IMHO)

auth0/src/main/java/com/auth0/android/provider/WebAuthProvider.java

joshcanhelp

🎉

lbalmaceda added CH: Security large Large review labels Nov 28, 2019

lbalmaceda modified the milestone: v1-Next Nov 28, 2019

lbalmaceda force-pushed the oidc-compliance branch from 429671d to 24729be Compare November 29, 2019 20:24

lbalmaceda marked this pull request as ready for review November 29, 2019 20:24

lbalmaceda requested a review from a team November 29, 2019 20:24

lbalmaceda force-pushed the oidc-compliance branch from a7418c4 to eb66f48 Compare November 29, 2019 21:44

lbalmaceda commented Nov 29, 2019

View reviewed changes

joshcanhelp requested review from joshcanhelp and removed request for a team December 2, 2019 19:48

lbalmaceda added 10 commits December 2, 2019 16:49

added base validation logic

8c07a97

send nonce for code flows, fix existing tests

193f297

fix test cases inconsistencies of mixing response types

4f701b8

prefer front-channel id token when available

1181b9f

test that ID token validation happens

1632a07

add max_age and leeway parameter handling

9ad9aac

fail if the algorithm is not supported

7c9f8a4

refactor move jwt classes into provider package, make them package pr…

898e4cd

…ivate

refactor id token verifier classes for brevity

46c5853

simplify flag

7afe4b7

lbalmaceda force-pushed the oidc-compliance branch from 890a52e to 7afe4b7 Compare December 2, 2019 19:49

joshcanhelp reviewed Dec 3, 2019

View reviewed changes

chore PR comments

8931616

lbalmaceda requested a review from joshcanhelp December 3, 2019 18:11

joshcanhelp previously approved these changes Dec 3, 2019

View reviewed changes

extract class to separate file and getters/setters

2cdcf22

lbalmaceda dismissed joshcanhelp’s stale review via 2cdcf22 December 4, 2019 21:25

davidpatrick approved these changes Dec 4, 2019

View reviewed changes

lbalmaceda merged commit df07ce2 into master Dec 5, 2019

lbalmaceda deleted the oidc-compliance branch December 5, 2019 14:16

lbalmaceda added this to the v1-Next milestone Dec 5, 2019

lbalmaceda mentioned this pull request Dec 5, 2019

Additional tests for OIDC compliance #270

Merged

6 tasks

lbalmaceda modified the milestones: v1-Next, 1.20.0 Dec 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve OIDC compliance #265

Improve OIDC compliance #265

lbalmaceda commented Nov 28, 2019 •

edited

Loading

lbalmaceda Nov 29, 2019

lbalmaceda Nov 29, 2019

lbalmaceda Nov 29, 2019

lbalmaceda Nov 29, 2019

lbalmaceda Nov 29, 2019

lbalmaceda Nov 29, 2019

lbalmaceda Nov 29, 2019

lbalmaceda Nov 29, 2019

joshcanhelp Dec 3, 2019

lbalmaceda Dec 3, 2019

lbalmaceda Nov 29, 2019

joshcanhelp left a comment

joshcanhelp Dec 3, 2019

joshcanhelp left a comment


		import java.util.concurrent.Callable;

		public class MockAuthCallback implements AuthCallback {

Improve OIDC compliance #265

Improve OIDC compliance #265

Conversation

lbalmaceda commented Nov 28, 2019 • edited Loading

Changes

Testing

Checklist

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

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

Choose a reason for hiding this comment

joshcanhelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshcanhelp left a comment

Choose a reason for hiding this comment

lbalmaceda commented Nov 28, 2019 •

edited

Loading