From c6e32b087328b3a014c6c7d812015112d8421497 Mon Sep 17 00:00:00 2001 From: Alexandru Gologan Date: Mon, 1 Mar 2021 23:58:25 +0200 Subject: [PATCH 1/2] Add ability to skip issuer https check and token nonce verification --- .../openid/appauth/AppAuthConfiguration.java | 45 +++++++++++++++++- .../openid/appauth/AuthorizationService.java | 19 ++++++-- library/java/net/openid/appauth/IdToken.java | 13 ++++- .../net/openid/appauth/IdTokenTest.java | 47 +++++++++++++++++++ 4 files changed, 117 insertions(+), 7 deletions(-) diff --git a/library/java/net/openid/appauth/AppAuthConfiguration.java b/library/java/net/openid/appauth/AppAuthConfiguration.java index a0255cfc..c379eabe 100644 --- a/library/java/net/openid/appauth/AppAuthConfiguration.java +++ b/library/java/net/openid/appauth/AppAuthConfiguration.java @@ -40,11 +40,19 @@ public class AppAuthConfiguration { @NonNull private final ConnectionBuilder mConnectionBuilder; + private final boolean mSkipIssuerHttpsCheck; + + private final boolean mSkipNonceVerification; + private AppAuthConfiguration( @NonNull BrowserMatcher browserMatcher, - @NonNull ConnectionBuilder connectionBuilder) { + @NonNull ConnectionBuilder connectionBuilder, + Boolean skipIssuerHttpsCheck, + Boolean skipNonceVerification) { mBrowserMatcher = browserMatcher; mConnectionBuilder = connectionBuilder; + mSkipIssuerHttpsCheck = skipIssuerHttpsCheck; + mSkipNonceVerification = skipNonceVerification; } /** @@ -64,6 +72,16 @@ public ConnectionBuilder getConnectionBuilder() { return mConnectionBuilder; } + /** + * Disables https validation for the issuer identifier. + */ + public boolean getSkipIssuerHttpsCheck() { return mSkipIssuerHttpsCheck; } + + /** + * Disables nonce verification for value sent in the Authentication Request. + */ + public boolean getSkipNonceVerification() { return mSkipNonceVerification; } + /** * Creates {@link AppAuthConfiguration} instances. */ @@ -71,6 +89,8 @@ public static class Builder { private BrowserMatcher mBrowserMatcher = AnyBrowserMatcher.INSTANCE; private ConnectionBuilder mConnectionBuilder = DefaultConnectionBuilder.INSTANCE; + private boolean mSkipIssuerHttpsCheck; + private boolean mSkipNonceVerification; /** * Specify the browser matcher to use, which controls the browsers that can be used @@ -94,12 +114,33 @@ public Builder setConnectionBuilder(@NonNull ConnectionBuilder connectionBuilder return this; } + /** + * Disables https validation for the issuer identifier. + */ + public Builder setSkipIssuerHttpsCheck(Boolean skipIssuerHttpsCheck) { + mSkipIssuerHttpsCheck = skipIssuerHttpsCheck; + return this; + } + + /** + * Disables nonce verification for value sent in the Authentication Request. + */ + public Builder setSkipNonceVerification(Boolean skipNonceVerification) { + mSkipNonceVerification = skipNonceVerification; + return this; + } + /** * Creates the instance from the configured properties. */ @NonNull public AppAuthConfiguration build() { - return new AppAuthConfiguration(mBrowserMatcher, mConnectionBuilder); + return new AppAuthConfiguration( + mBrowserMatcher, + mConnectionBuilder, + mSkipIssuerHttpsCheck, + mSkipNonceVerification + ); } diff --git a/library/java/net/openid/appauth/AuthorizationService.java b/library/java/net/openid/appauth/AuthorizationService.java index 0b78d0d2..49b2e3dc 100644 --- a/library/java/net/openid/appauth/AuthorizationService.java +++ b/library/java/net/openid/appauth/AuthorizationService.java @@ -489,7 +489,9 @@ public void performTokenRequest( clientAuthentication, mClientConfiguration.getConnectionBuilder(), SystemClock.INSTANCE, - callback) + callback, + mClientConfiguration.getSkipIssuerHttpsCheck(), + mClientConfiguration.getSkipNonceVerification()) .execute(); } @@ -567,6 +569,8 @@ private static class TokenRequestTask private final ConnectionBuilder mConnectionBuilder; private TokenResponseCallback mCallback; private Clock mClock; + private boolean mSkipIssuerHttpsCheck; + private boolean mSkipNonceVerification; private AuthorizationException mException; @@ -574,12 +578,16 @@ private static class TokenRequestTask @NonNull ClientAuthentication clientAuthentication, @NonNull ConnectionBuilder connectionBuilder, Clock clock, - TokenResponseCallback callback) { + TokenResponseCallback callback, + Boolean skipIssuerHttpsCheck, + Boolean skipNonceVerification) { mRequest = request; mClientAuthentication = clientAuthentication; mConnectionBuilder = connectionBuilder; mClock = clock; mCallback = callback; + mSkipIssuerHttpsCheck = skipIssuerHttpsCheck; + mSkipNonceVerification = skipNonceVerification; } @Override @@ -687,7 +695,12 @@ protected void onPostExecute(JSONObject json) { } try { - idToken.validate(mRequest, mClock); + idToken.validate( + mRequest, + mClock, + mSkipIssuerHttpsCheck, + mSkipNonceVerification + ); } catch (AuthorizationException ex) { mCallback.onTokenRequestCompleted(null, ex); return; diff --git a/library/java/net/openid/appauth/IdToken.java b/library/java/net/openid/appauth/IdToken.java index 302bcc3c..0c6f1301 100644 --- a/library/java/net/openid/appauth/IdToken.java +++ b/library/java/net/openid/appauth/IdToken.java @@ -19,6 +19,7 @@ import android.util.Base64; import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.annotation.VisibleForTesting; import net.openid.appauth.AuthorizationException.GeneralErrors; import org.json.JSONException; @@ -109,7 +110,15 @@ static IdToken from(String token) throws JSONException, IdTokenException { ); } + @VisibleForTesting void validate(@NonNull TokenRequest tokenRequest, Clock clock) throws AuthorizationException { + validate(tokenRequest, clock, false, false); + } + + void validate(@NonNull TokenRequest tokenRequest, + Clock clock, + boolean skipIssuerHttpsCheck, + boolean skipNonceVerification) throws AuthorizationException { // OpenID Connect Core Section 3.1.3.7. rule #1 // Not enforced: AppAuth does not support JWT encryption. @@ -129,7 +138,7 @@ void validate(@NonNull TokenRequest tokenRequest, Clock clock) throws Authorizat // components. Uri issuerUri = Uri.parse(this.issuer); - if (!issuerUri.getScheme().equals("https")) { + if (!skipIssuerHttpsCheck && !issuerUri.getScheme().equals("https")) { throw AuthorizationException.fromTemplate(GeneralErrors.ID_TOKEN_VALIDATION_ERROR, new IdTokenException("Issuer must be an https URL")); } @@ -189,7 +198,7 @@ void validate(@NonNull TokenRequest tokenRequest, Clock clock) throws Authorizat // OpenID Connect Core Section 3.1.3.7. rule #11 // Validates the nonce. String expectedNonce = tokenRequest.nonce; - if (!TextUtils.equals(this.nonce, expectedNonce)) { + if (!skipNonceVerification && !TextUtils.equals(this.nonce, expectedNonce)) { throw AuthorizationException.fromTemplate(GeneralErrors.ID_TOKEN_VALIDATION_ERROR, new IdTokenException("Nonce mismatch")); } diff --git a/library/javatests/net/openid/appauth/IdTokenTest.java b/library/javatests/net/openid/appauth/IdTokenTest.java index 60c6a98e..c3753704 100644 --- a/library/javatests/net/openid/appauth/IdTokenTest.java +++ b/library/javatests/net/openid/appauth/IdTokenTest.java @@ -202,6 +202,36 @@ public void testValidate_shouldFailOnNonHttpsIssuer() idToken.validate(tokenRequest, clock); } + @Test + public void testValidate_shouldSkipNonHttpsIssuer() + throws AuthorizationException, JSONException, MissingArgumentException { + Long nowInSeconds = SystemClock.INSTANCE.getCurrentTimeMillis() / 1000; + Long tenMinutesInSeconds = (long) (10 * 60); + IdToken idToken = new IdToken( + "http://other.issuer", + TEST_SUBJECT, + Collections.singletonList(TEST_CLIENT_ID), + nowInSeconds + tenMinutesInSeconds, + nowInSeconds, + TEST_NONCE + ); + + String serviceDocJsonWithOtherIssuer = getDiscoveryDocJsonWithIssuer("http://other.issuer"); + AuthorizationServiceDiscovery discoveryDoc = new AuthorizationServiceDiscovery( + new JSONObject(serviceDocJsonWithOtherIssuer)); + AuthorizationServiceConfiguration serviceConfiguration = + new AuthorizationServiceConfiguration(discoveryDoc); + TokenRequest tokenRequest = new TokenRequest.Builder(serviceConfiguration, TEST_CLIENT_ID) + .setAuthorizationCode(TEST_AUTH_CODE) + .setCodeVerifier(TEST_CODE_VERIFIER) + .setGrantType(GrantTypeValues.AUTHORIZATION_CODE) + .setRedirectUri(TEST_APP_REDIRECT_URI) + .setNonce(TEST_NONCE) + .build(); + Clock clock = SystemClock.INSTANCE; + idToken.validate(tokenRequest, clock, true, false); + } + @Test(expected = AuthorizationException.class) public void testValidate_shouldFailOnIssuerMissingHost() throws AuthorizationException, JSONException, MissingArgumentException { @@ -359,6 +389,23 @@ public void testValidate_shouldFailOnNonceMismatch() throws AuthorizationExcepti idToken.validate(tokenRequest, clock); } + @Test + public void testValidate_shouldSkipNonceMismatch() throws AuthorizationException { + Long nowInSeconds = SystemClock.INSTANCE.getCurrentTimeMillis() / 1000; + Long tenMinutesInSeconds = (long) (10 * 60); + IdToken idToken = new IdToken( + TEST_ISSUER, + TEST_SUBJECT, + Collections.singletonList(TEST_CLIENT_ID), + nowInSeconds + tenMinutesInSeconds, + nowInSeconds, + "some_other_nonce" + ); + TokenRequest tokenRequest = getAuthCodeExchangeRequestWithNonce(); + Clock clock = SystemClock.INSTANCE; + idToken.validate(tokenRequest, clock, false, true); + } + private static String base64UrlNoPaddingEncode(byte[] data) { return Base64.encodeToString(data, Base64.URL_SAFE | Base64.NO_PADDING | Base64.NO_WRAP); } From 9c2502965846fbac866375bfc0c83be8d8be89e2 Mon Sep 17 00:00:00 2001 From: Alexandru Gologan Date: Wed, 3 Mar 2021 20:41:00 +0200 Subject: [PATCH 2/2] Update documentation --- .../openid/appauth/AppAuthConfiguration.java | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/library/java/net/openid/appauth/AppAuthConfiguration.java b/library/java/net/openid/appauth/AppAuthConfiguration.java index c379eabe..2a065e1f 100644 --- a/library/java/net/openid/appauth/AppAuthConfiguration.java +++ b/library/java/net/openid/appauth/AppAuthConfiguration.java @@ -73,12 +73,18 @@ public ConnectionBuilder getConnectionBuilder() { } /** - * Disables https validation for the issuer identifier. + * Returns true if issuer https validation is disabled, otherwise + * false. + * + * @see Builder#setSkipIssuerHttpsCheck(Boolean) */ public boolean getSkipIssuerHttpsCheck() { return mSkipIssuerHttpsCheck; } /** - * Disables nonce verification for value sent in the Authentication Request. + * Returns true if nonce verification on response is disabled, otherwise + * false. + * + * @see Builder#setSkipNonceVerification(Boolean) */ public boolean getSkipNonceVerification() { return mSkipNonceVerification; } @@ -116,6 +122,9 @@ public Builder setConnectionBuilder(@NonNull ConnectionBuilder connectionBuilder /** * Disables https validation for the issuer identifier. + * + *

NOTE: Disabling issuer https validation implies the app is running against an + * insecure environment. Enabling this option is only recommended for testing purposes. */ public Builder setSkipIssuerHttpsCheck(Boolean skipIssuerHttpsCheck) { mSkipIssuerHttpsCheck = skipIssuerHttpsCheck; @@ -124,6 +133,13 @@ public Builder setSkipIssuerHttpsCheck(Boolean skipIssuerHttpsCheck) { /** * Disables nonce verification for value sent in the Authentication Request. + * + *

NOTE: Some Authorization Servers do not return the requested nonce value thus failing + * ID token validation. Please consider raising an issue with your Identity Provider and + * disabling this option once it is fixed. + * + *

Alternatively, you may avoid sending a nonce by passing null to + * {@link AuthorizationRequest.Builder#setNonce} */ public Builder setSkipNonceVerification(Boolean skipNonceVerification) { mSkipNonceVerification = skipNonceVerification;