Skip to content

Implement ID Token validation according to OIDC spec #385

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.

Sign up for GitHub

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
iainmcgin merged 13 commits into from
Aug 10, 2018
Merged

Implement ID Token validation according to OIDC spec #385

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
06a3d1c
fix: move google() to top of repositories list
Jul 23, 2018
ad64a89
feat: Add nonce to AuthorizationRequest
Jul 23, 2018
c73985a
feat: include nonce when serializing AuthorizationRequests
Jul 24, 2018
9f117d4
feat: parse and validate ID Token on TokenResponse
Jul 25, 2018
a8503d2
fix: bugs in ID Token validation
Jul 25, 2018
fbb274b
fix: lint errors
Jul 25, 2018
90e1ac4
test: add test for token request with nonce checking
Jul 25, 2018
063e51c
chore: minor changes to comments according to PR feedback
Aug 8, 2018
9d9cf74
fix: correct test for nonce validation, improve test for ID Token par…
Aug 8, 2018
91dc403
test: better coverage for ID Token parsing
Aug 8, 2018
169e3d6
chore: remove unused verifyNonce method
Aug 8, 2018
8c2fa9d
kaizen: move validation logic to IdToken class
Aug 9, 2018
78a5d40
Revert unintentional copyright year bump
oyvindrobertsen Aug 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ buildscript {

subprojects {
repositories {
jcenter()

@oyvindrobertsen oyvindrobertsen Jul 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This one is weird. From a fresh clone of the repo, I was unable to get the project to build (either through ./gradlew assemble or through Android Studio, both with the same error message:

* What went wrong:
Could not resolve all files for configuration ':app:debugCompileClasspath'.
> Could not find support-fragment.jar (com.android.support:support-fragment:27.1.1).
  Searched in the following locations:
https://jcenter.bintray.com/com/android/support/support-fragment/27.1.1/support-fragment-27.1.1.jar

It looks like gradle doesn't move on to the next repos in the list after not finding the package on jcenter. Moving the google repo first fixes the issue. This may be a problem with my environment, and if it is, this commit can be removed from the PR, but I wanted to include it just to potentially figure out what the issue is.

@iainmcgin iainmcgin Jul 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What OS are you building on? I've seen some build issues recently that are platform-specific; so much for Gradle hiding platform details. The build definitions for AppAuth are starting to show their age, I should really take some time to overhaul them.

@oyvindrobertsen oyvindrobertsen Aug 8, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

MacOS.

FWIW, I've had to order repositories in the same way in the application where I'm using AppAuth.

google()
jcenter()
maven { url 'https://jitpack.io' }
}
}
Expand Down
12 changes: 12 additions & 0 deletions library/java/net/openid/appauth/AuthorizationException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,18 @@ public static final class GeneralErrors {
*/
public static final AuthorizationException INVALID_REGISTRATION_RESPONSE =
generalEx(7, "Invalid registration response");

/**
* Indicates that a received ID token could not be parsed
*/
public static final AuthorizationException ID_TOKEN_PARSING_ERROR =
generalEx(8, "Unable to parse ID Token");

/**
* Indicates that a received ID token is invalid
*/
public static final AuthorizationException ID_TOKEN_VALIDATION_ERROR =
generalEx(9, "Invalid ID Token");
}

/**
Expand Down
43 changes: 43 additions & 0 deletions library/java/net/openid/appauth/AuthorizationRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,9 @@ public static final class ResponseMode {
@VisibleForTesting
static final String PARAM_STATE = "state";

@VisibleForTesting
static final String PARAM_NONCE = "nonce";

private static final Set<String> BUILT_IN_PARAMS = builtInParams(
PARAM_CLIENT_ID,
PARAM_CODE_CHALLENGE,
Expand All @@ -324,6 +327,7 @@ public static final class ResponseMode {
private static final String KEY_REDIRECT_URI = "redirectUri";
private static final String KEY_SCOPE = "scope";
private static final String KEY_STATE = "state";
private static final String KEY_NONCE = "nonce";
private static final String KEY_CODE_VERIFIER = "codeVerifier";
private static final String KEY_CODE_VERIFIER_CHALLENGE = "codeVerifierChallenge";
private static final String KEY_CODE_VERIFIER_CHALLENGE_METHOD = "codeVerifierChallengeMethod";
Expand Down Expand Up @@ -436,6 +440,19 @@ public static final class ResponseMode {
@Nullable
public final String state;

/**
* String value used to associate a Client session with an ID Token, and to mitigate replay
* attacks. The value is passed through unmodified from the Authentication Request to the ID
* Token. If this value is not explicitly set, this library will automatically add nonce and
* perform appropriate validation of the ID Token. It is recommended that the default
* implementation of this parameter be used wherever possible.
*
* @see "OpenID Connect Core 1.0, Section 3.1.2.1
* <https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1>"
*/
@Nullable
public final String nonce;

/**
* The proof key for code exchange. This is an opaque value used to associate an authorization
* request with a subsequent code exchange, in order to prevent any eavesdropping party from
Expand Down Expand Up @@ -542,6 +559,9 @@ public static final class Builder {
@Nullable
private String mState;

@Nullable
private String mNonce;

@Nullable
private String mCodeVerifier;

Expand Down Expand Up @@ -570,6 +590,7 @@ public Builder(
setResponseType(responseType);
setRedirectUri(redirectUri);
setState(AuthorizationRequest.generateRandomState());
setNonce(AuthorizationRequest.generateRandomState());
setCodeVerifier(CodeVerifierUtil.generateRandomCodeVerifier());
}

Expand Down Expand Up @@ -763,6 +784,22 @@ public Builder setState(@Nullable String state) {
return this;
}

/**
* Specifies the String value used to associate a Client session with an ID Token, and to
* mitigate replay attacks. The value is passed through unmodified from the Authentication
* Request to the ID Token. If this value is not explicitly set, this library will
* automatically add nonce and perform appropriate validation of the ID Token. It is
* recommended that the default implementation of this parameter be used wherever possible.
*
* @see "OpenID Connect Core 1.0, Section 3.1.2.1
* <https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1>"
*/
@NonNull
public Builder setNonce(@Nullable String nonce) {
mNonce = checkNullOrNotEmpty(nonce, "state cannot be empty if defined");
return this;
}

/**
* Specifies the code verifier to use for this authorization request. The default challenge
* method (typically {@link #CODE_CHALLENGE_METHOD_S256}) implemented by
Expand Down Expand Up @@ -877,6 +914,7 @@ public AuthorizationRequest build() {
mPrompt,
mScope,
mState,
mNonce,
mCodeVerifier,
mCodeVerifierChallenge,
mCodeVerifierChallengeMethod,
Expand All @@ -895,6 +933,7 @@ private AuthorizationRequest(
@Nullable String prompt,
@Nullable String scope,
@Nullable String state,
@Nullable String nonce,
@Nullable String codeVerifier,
@Nullable String codeVerifierChallenge,
@Nullable String codeVerifierChallengeMethod,
Expand All @@ -913,6 +952,7 @@ private AuthorizationRequest(
this.prompt = prompt;
this.scope = scope;
this.state = state;
this.nonce = nonce;
this.codeVerifier = codeVerifier;
this.codeVerifierChallenge = codeVerifierChallenge;
this.codeVerifierChallengeMethod = codeVerifierChallengeMethod;
Expand Down Expand Up @@ -952,6 +992,7 @@ public Uri toUri() {
UriUtil.appendQueryParameterIfNotNull(uriBuilder, PARAM_LOGIN_HINT, loginHint);
UriUtil.appendQueryParameterIfNotNull(uriBuilder, PARAM_PROMPT, prompt);
UriUtil.appendQueryParameterIfNotNull(uriBuilder, PARAM_STATE, state);
UriUtil.appendQueryParameterIfNotNull(uriBuilder, PARAM_NONCE, nonce);
UriUtil.appendQueryParameterIfNotNull(uriBuilder, PARAM_SCOPE, scope);
UriUtil.appendQueryParameterIfNotNull(uriBuilder, PARAM_RESPONSE_MODE, responseMode);

Expand Down Expand Up @@ -983,6 +1024,7 @@ public JSONObject jsonSerialize() {
JsonUtil.putIfNotNull(json, KEY_SCOPE, scope);
JsonUtil.putIfNotNull(json, KEY_PROMPT, prompt);
JsonUtil.putIfNotNull(json, KEY_STATE, state);
JsonUtil.putIfNotNull(json, KEY_NONCE, nonce);
JsonUtil.putIfNotNull(json, KEY_CODE_VERIFIER, codeVerifier);
JsonUtil.putIfNotNull(json, KEY_CODE_VERIFIER_CHALLENGE, codeVerifierChallenge);
JsonUtil.putIfNotNull(json, KEY_CODE_VERIFIER_CHALLENGE_METHOD,
Expand Down Expand Up @@ -1020,6 +1062,7 @@ public static AuthorizationRequest jsonDeserialize(@NonNull JSONObject json)
.setLoginHint(JsonUtil.getStringIfDefined(json, KEY_LOGIN_HINT))
.setPrompt(JsonUtil.getStringIfDefined(json, KEY_PROMPT))
.setState(JsonUtil.getStringIfDefined(json, KEY_STATE))
.setNonce(JsonUtil.getStringIfDefined(json, KEY_NONCE))
.setCodeVerifier(
JsonUtil.getStringIfDefined(json, KEY_CODE_VERIFIER),
JsonUtil.getStringIfDefined(json, KEY_CODE_VERIFIER_CHALLENGE),
Expand Down
1 change: 1 addition & 0 deletions library/java/net/openid/appauth/AuthorizationResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ public TokenRequest createTokenExchangeRequest(
.setCodeVerifier(request.codeVerifier)
.setAuthorizationCode(authorizationCode)
.setAdditionalParameters(additionalExchangeParameters)
.setNonce(request.nonce)
.build();
}

Expand Down
25 changes: 25 additions & 0 deletions library/java/net/openid/appauth/AuthorizationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import net.openid.appauth.AuthorizationException.RegistrationRequestErrors;
import net.openid.appauth.AuthorizationException.TokenRequestErrors;

import net.openid.appauth.IdToken.IdTokenException;
import net.openid.appauth.browser.BrowserDescriptor;
import net.openid.appauth.browser.BrowserSelector;
import net.openid.appauth.browser.CustomTabManager;
Expand Down Expand Up @@ -315,6 +316,7 @@ public void performTokenRequest(
request,
clientAuthentication,
mClientConfiguration.getConnectionBuilder(),
SystemClock.INSTANCE,
callback)
.execute();
}
Expand Down Expand Up @@ -386,20 +388,24 @@ private Intent prepareAuthorizationRequestIntent(

private static class TokenRequestTask
extends AsyncTask<Void, Void, JSONObject> {

private TokenRequest mRequest;
private ClientAuthentication mClientAuthentication;
private final ConnectionBuilder mConnectionBuilder;
private TokenResponseCallback mCallback;
private Clock mClock;

private AuthorizationException mException;

TokenRequestTask(TokenRequest request,
@NonNull ClientAuthentication clientAuthentication,
@NonNull ConnectionBuilder connectionBuilder,
Clock clock,
TokenResponseCallback callback) {
mRequest = request;
mClientAuthentication = clientAuthentication;
mConnectionBuilder = connectionBuilder;
mClock = clock;
mCallback = callback;
}

Expand Down Expand Up @@ -495,6 +501,25 @@ protected void onPostExecute(JSONObject json) {
return;
}

if (response.idToken != null) {
IdToken idToken;

@iainmcgin iainmcgin Aug 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you move all of this validation logic in a utility method within the IdToken class? I think that will help to keep all of the parsing and validation logic in one place.

try {
idToken = IdToken.from(response.idToken);
} catch (IdTokenException | JSONException ex) {
mCallback.onTokenRequestCompleted(null,
AuthorizationException.fromTemplate(
GeneralErrors.ID_TOKEN_PARSING_ERROR,
ex));
return;
}

try {
idToken.validate(mRequest, mClock);
} catch (AuthorizationException ex) {
mCallback.onTokenRequestCompleted(null, ex);
return;
}
}
Logger.debug("Token exchange with %s completed",
mRequest.configuration.tokenEndpoint);
mCallback.onTokenRequestCompleted(response, null);
Expand Down
Loading