ID Token parsing and OpenID RP Certification #101

WilliamDenniss · 2017-03-25T21:34:11Z

ID Token Parsing

The 5 required fields are exposed as params.
Authorization code exchange exchange now validates the ID Token iss, aud, and iat claims.

OpenID RP Certification Tests

Response Type and Response Mode

rp-response_type-code

scope Request Parameter

rp-scope-userinfo-claims

nonce Request Parameter

rp-nonce-invalid

Client Authentication

rp-token_endpoint-client_secret_basic

ID Token

*AppAuth for iOS & macOS only supports alg:none validation.

UserInfo Endpoint

rp-userinfo-bad-sub-claim
rp-userinfo-bearer-header

Fixes:

#4 Support "nonce" OpenID Connect auth request parameter.
#17 Support JWT decoding and validation (conditional: alg:none based validation only).
#102 OpenID Connect RP Conformance Testing.

ve7jtb · 2017-03-25T21:51:50Z

Source/OIDAuthorizationService.m

+        return;
+      }
+
+      NSURL *issuer = tokenResponse.request.configuration.discoveryDocument.issuer;


The issuer always needs to be validated. If not doing discovery then the issuer would need to be statically along with Authorization endpoint etc.

Added the ability to manually specify issuer when not using discovery.

codecov-io · 2017-03-25T21:59:50Z

Codecov Report

Merging #101 into master will increase coverage by 6.47%.
The diff coverage is 89.34%.

@@            Coverage Diff             @@
##           master     #101      +/-   ##
==========================================
+ Coverage   66.84%   73.32%   +6.47%     
==========================================
  Files          54       57       +3     
  Lines        4217     4795     +578     
==========================================
+ Hits         2819     3516     +697     
+ Misses       1398     1279     -119

Impacted Files	Coverage Δ
Source/OIDAuthorizationService.h	`0% <ø> (ø)`	⬆️
Source/OIDTokenResponse.h	`100% <ø> (ø)`	⬆️
Source/OIDAuthState.m	`46.99% <0%> (+3.73%)`	⬆️
Source/OIDAuthorizationRequest.h	`100% <100%> (ø)`	⬆️
Source/OIDServiceConfiguration.h	`100% <100%> (+25%)`	⬆️
UnitTests/OIDAuthorizationRequestTests.m	`100% <100%> (ø)`	⬆️
Source/OIDAuthorizationRequest.m	`86.04% <33.33%> (-7.34%)`	⬇️
Source/OIDServiceConfiguration.m	`78.4% <72.72%> (-14.02%)`	⬇️
Source/OIDIDToken.h	`75% <75%> (ø)`
Source/OIDAuthorizationService.m	`62.22% <80.39%> (+45.18%)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c16ebbe...e30ea9d. Read the comment docs.

zboralski · 2017-07-04T11:16:01Z

any update?

WilliamDenniss · 2018-01-05T20:35:54Z

Updated the documentation & code comments for the ID Token features. Should be good to go now.

Here is a good summary (taken from the code) of the validation that is performed. Basically we are following the OpenID Connect specification for verification of ID Tokens on the code flow, with the important note that we are exercising the option to rely only on the TLS validation of the direct connection between the token endpoint and client, and are not performing JWT signature verification (users of AppAuth are welcome to add this themselves, should they wish).

// If an ID Token is included in the response, validates the ID Token following the rules
// in OpenID Connect Core Section 3.1.3.7 for features that AppAuth directly supports
// (which excludes rules #1, #4, #5, #7, #8, #12, and #13). Regarding rule #6, ID Tokens
// received by this class are received via direct communication between the Client and the Token
// Endpoint, thus we are exercising the option to rely only on the TLS validation. AppAuth
// has a zero dependencies policy, and verifying the JWT signature would add a dependency.
// Users of the library are welcome to perform the JWT signature verification themselves should
// they wish.

jakeholland · 2018-02-13T01:44:21Z

@WilliamDenniss - Need anything else for this? I have a project in flight this would be great for.

zboralski · 2018-03-13T19:21:26Z

Any progress on this? Do you need any help?

zboralski · 2018-03-14T12:07:36Z

@WilliamDenniss @StevenEWright What's the status of this pull request?

StevenEWright · 2018-03-14T16:56:29Z

@zboralski thank you for the ping. Ugh. Looks like I really dropped the ball on this. I’m sorry. I will look tonight.

StevenEWright

General Question... where do we stand on Swift support? Have you had a chance to test the new APIs with Swift? Do they look OK?

StevenEWright · 2018-03-14T17:14:11Z

Source/OIDAuthState.m

@@ -125,7 +125,8 @@ @implementation OIDAuthState
                               OIDTokenRequest *tokenExchangeRequest =
                                   [authorizationResponse tokenExchangeRequest];
                               [OIDAuthorizationService
-                                   performTokenRequest:tokenExchangeRequest
+                                             performTokenRequest:tokenExchangeRequest
+                                   originalAuthorizationResponse:authorizationResponse
                                              callback:^(OIDTokenResponse *_Nullable tokenResponse,


How picky are we about colon alignment and style issues these days? I've seen a lot of people adopt clang-format, and I've been using it myself. It can be configured to mostly mirror our style guide. Nice to not have to worry about this stuff as much.

You'll be pleased to know we're still picky. I took another stab at it, this one is kind of hard to fit in, PTAL. If you have any guidance please let me know.

StevenEWright · 2018-03-14T17:17:00Z

Source/OIDAuthorizationRequest.h

@@ -46,6 +46,7 @@ extern NSString *const OIDOAuthorizationRequestCodeChallengeMethodS256;
  NSString *_scope;
  NSURL *_redirectURL;
  NSString *_state;
+  NSString *_nonce;


Looks like this is supposed to be a nullable field. Shouldn't it be:

NSString *_Nullable _nonce;

?

But I realize we don't have nullability attributes on the other property-backing ivars either. Am surprised this hasn't raised warnings / etc. Obviously I am not suggesting we deal with or "fix" this issue here. Mostly a question more so than a comment.

Hmm, at least the property itself is declared as nullable. I hope to remove this code in the near future, once 32bit is gone.

StevenEWright · 2018-03-14T17:30:32Z

Source/OIDAuthorizationService.m

+      NSTimeInterval expiresAtDifference = [idToken.expiresAt timeIntervalSinceNow];
+      if (expiresAtDifference < 0) {
+        NSError *invalidIDToken =
+        [OIDErrorUtilities errorWithCode:OIDErrorCodeIDTokenFailedValidationError


StevenEWright · 2018-03-14T17:35:39Z

Source/OIDAuthorizationService.m

+
+      // OpenID Connect Core Section 3.1.3.7. rule #10
+      // Validates that the issued at time is not more than +/- 5 minutes on the current time.
+      NSTimeInterval issuedAtDifference = [idToken.issuedAt timeIntervalSinceNow];


Just curious - I know we have other date/time related validations which take the local clock into account, but this seems like it may be the most specific. Is that true? I don't know why, on an iOS device, someone would have clock skew on the order of minutes - especially considering iPhones in particular should have fairly constant network connectivity, and should be getting their clocks updated regularly - but hypothetically if someone's clock were off by a couple minutes, would you expect this to have a chance of failing for that reason where-as it wouldn't before? (Reiterating it's just a question for my own edification, not that I see it as a problem/issue.)

I don't think too many people set the time manually, I agree – but perhaps some do. I'm more concerned about server clock skew.

I don't really know what const to use here. The docs say we should "reject tokens that were issued too far away from the current time", but don't specify what "too far away" is, so I plucked 5 minutes out of the air, since it seemed short, but also long enough to account for multiple-minutes of clock skew on both sides.

StevenEWright · 2018-03-14T17:41:03Z

Source/OIDIDToken.h

+    @remarks iat
+    @see http://openid.net/specs/openid-connect-core-1_0.html#IDToken
+ */
+@property(nonatomic, readonly) NSDate *issuedAt;


There seem to exist cases in which this field is optional? Should it be nullable here?

When a max_age request is made or when auth_time is requested as an Essential Claim, then this Claim is REQUIRED; otherwise, its inclusion is OPTIONAL.

Appears to be required to me:

iat
REQUIRED. Time at which the JWT was issued. Its value is a JSON number representing the number of seconds from 1970-01-01T0:0:0Z as measured in UTC until the date/time.

I think you're quoting from auth_time?

StevenEWright · 2018-03-14T18:43:31Z

Source/OIDIDToken.m

+          if ([value isKindOfClass:[NSArray class]]) {
+            return value;
+          }
+          return @[value];


In this case it's expected value is a type of string, right? You've been so diligent about checking types... do we want to check that as well?

StevenEWright · 2018-03-14T18:44:22Z

Source/OIDIDToken.m

+            return value;
+          }
+          NSNumber *valueAsNumber = (NSNumber *)value;
+          return [NSDate dateWithTimeIntervalSince1970:[valueAsNumber longLongValue]];


dot notation for properties? (longLongValue below as well)

StevenEWright · 2018-03-14T18:47:39Z

Source/OIDServiceConfiguration.h

@@ -48,6 +49,10 @@ typedef void (^OIDServiceConfigurationCreated)
 */
 @property(nonatomic, readonly) NSURL *tokenEndpoint;

+/*! @brief The token exchange and refresh endpoint URI.
+ */
+@property(nonatomic, readonly) NSURL *issuer;


Should this be nullable?

@property(nonatomic, readonly, nullable) NSURL *issuer;

StevenEWright · 2018-03-14T18:48:44Z

Source/OIDServiceConfiguration.h

+    @param tokenEndpoint The token exchange and refresh endpoint URI.
+    @param issuer The OpenID Connect issuer.
+ */
+- (instancetype)initWithAuthorizationEndpoint:(NSURL *)authorizationEndpoint


Should we mark init as unavailable in the header?

I think we do already? - (instancetype)init NS_UNAVAILABLE;

StevenEWright · 2018-03-14T18:55:27Z

Source/OIDServiceConfiguration.m

 @synthesize registrationEndpoint = _registrationEndpoint;
 @synthesize discoveryDocument = _discoveryDocument;

 - (instancetype)init
    OID_UNAVAILABLE_USE_INITIALIZER(@selector(
        initWithAuthorizationEndpoint:
-                       tokenEndpoint:
-                registrationEndpoint:)
+                       tokenEndpoint:)
    );

 - (instancetype)initWithAuthorizationEndpoint:(NSURL *)authorizationEndpoint


Looks like this is the actual designated initializer? Should we mark it as such in the header?

So this method is not actually exposed in the header at all.

We're expecting people to either specify the endpoints manually, OR use a discovery doc – but not both. Do you think we should revise this?

ghost · 2018-03-27T19:33:32Z

Source/OIDAuthorizationService.m

+      // OpenID Connect Core Section 3.1.3.7. rule #11
+      // Validates the nonce.
+      NSString *nonce = authorizationResponse.request.nonce;
+      if (nonce && ![idToken.nonce isEqual:nonce]) {


Not sure if you are already aware, but there is a problem here. A nonce isn't sent with the call and then it expects one here.

If the authorization request doesn't set nonce, then this check will be skipped (if (nonce &&… should achieve that). Are you sure there's a problem?

I have a project using this library with an IdentityServer4 server. The problem is that authorizationResponse.request.nonce is a valid nonce and idToken.nonce is null.

OpenID Connect Core Section 2 states:

If present in the Authentication Request, Authorization Servers MUST include a nonce Claim in the ID Token with the Claim Value being the nonce value sent in the Authentication Request.

I believe we are implementing this correctly in AppAuth , is there a bug in IdentityServer4?

I have opened a bug with IdentityServer4 here: IdentityServer/IdentityServer4#2180

@WilliamDenniss
Section 12.1
Does not state anything about needing a nonce to verify a Refresh Request. Not sure AppAuth is really doing the right thing here.

Ah, I didn't realize you were referring the refresh token flow.

Section 12.1 is a bit ambiguous for nonce, but yeah I don't think the intention was for nonce to be required in the ID Token for the token response during refreshes. I sent a note to the Connect mailing list regarding the possible ambiguity. Google doesn't return the nonce in refreshed ID Tokens either.

I've fixed this in the PR. Thanks very much for engaging and reporting this problem!

StevenEWright · 2018-04-28T08:47:43Z

LGTM

ShaneQi · 2018-05-23T22:00:59Z

Our identify server requires mobile clients to send nonce. I verified that this branch will work for our case. Are we merging this PR in the near future?

Thanks!

– New OIDIDToken object to parse ID Tokens. – Token exchange now validates the ID Token pursuant to OpenID Connect Core Section 3.1.3.7. (see code comments for details).

WilliamDenniss force-pushed the certification branch 2 times, most recently from 100149e to 9802336 Compare March 25, 2017 21:40

ve7jtb reviewed Mar 25, 2017

View reviewed changes

WilliamDenniss force-pushed the certification branch from 9802336 to 3c73785 Compare March 25, 2017 21:52

WilliamDenniss force-pushed the certification branch 2 times, most recently from 196d034 to beb4b67 Compare March 26, 2017 15:32

WilliamDenniss force-pushed the certification branch from 05f6853 to c19f32e Compare April 14, 2017 23:37

WilliamDenniss force-pushed the certification branch from c19f32e to 5a3fc72 Compare April 30, 2017 20:37

WilliamDenniss added the openid-cla: yes label Apr 30, 2017

WilliamDenniss changed the title ~~ID Token parsing.~~ ID Token parsing and OpenID RP Certification Apr 30, 2017

WilliamDenniss force-pushed the certification branch 2 times, most recently from 5569652 to 44e75a0 Compare May 21, 2017 23:45

WilliamDenniss force-pushed the certification branch 2 times, most recently from 313cd4a to 21ddaa5 Compare October 12, 2017 21:58

WilliamDenniss force-pushed the certification branch 6 times, most recently from 36b9235 to cc96234 Compare January 5, 2018 20:28

StevenEWright self-assigned this Jan 9, 2018

WilliamDenniss added the enhancement label Jan 23, 2018

WilliamDenniss force-pushed the certification branch 3 times, most recently from 2f708cd to 3f6f6d5 Compare January 25, 2018 23:10

WilliamDenniss mentioned this pull request Feb 20, 2018

IdToken validation openid/AppAuth-Android#311

Open

StevenEWright reviewed Mar 14, 2018

View reviewed changes

ghost reviewed Mar 27, 2018

View reviewed changes

WilliamDenniss force-pushed the certification branch 3 times, most recently from 463b66a to f445320 Compare April 20, 2018 23:14

WilliamDenniss force-pushed the certification branch 2 times, most recently from e0b3a12 to c1db1d6 Compare June 1, 2018 21:00

WilliamDenniss added 3 commits June 1, 2018 16:50

Add nonce to AuthorizationRequest

11e680b

Implement ID Token parsing and validation

89b5473

– New OIDIDToken object to parse ID Tokens. – Token exchange now validates the ID Token pursuant to OpenID Connect Core Section 3.1.3.7. (see code comments for details).

Implement OpenID Connect RP Conformance Tests for Code Flow

e30ea9d

WilliamDenniss force-pushed the certification branch from c1db1d6 to e30ea9d Compare June 1, 2018 23:51

WilliamDenniss merged commit 96b5b8b into openid:master Jun 2, 2018

WilliamDenniss deleted the certification branch July 13, 2018 00:09

KarlBusse mentioned this pull request Oct 18, 2018

New release request + OpenID RP Certification? openid/AppAuth-Android#422

Closed

ID Token parsing and OpenID RP Certification #101

ID Token parsing and OpenID RP Certification #101

Conversation

WilliamDenniss commented Mar 25, 2017 • edited Loading

ID Token Parsing

OpenID RP Certification Tests

Response Type and Response Mode

scope Request Parameter

nonce Request Parameter

Client Authentication

ID Token

UserInfo Endpoint

Fixes:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 25, 2017 • edited Loading

Codecov Report

zboralski commented Jul 4, 2017

WilliamDenniss commented Jan 5, 2018

jakeholland commented Feb 13, 2018

zboralski commented Mar 13, 2018

zboralski commented Mar 14, 2018

StevenEWright commented Mar 14, 2018

StevenEWright left a 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

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

ghost Mar 27, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Mar 27, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 2, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

WilliamDenniss Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

StevenEWright commented Apr 28, 2018

ShaneQi commented May 23, 2018

WilliamDenniss commented Mar 25, 2017 •

edited

Loading

codecov-io commented Mar 25, 2017 •

edited

Loading

ghost Mar 27, 2018 •

edited by ghost

Loading

ghost Mar 27, 2018 •

edited by ghost

Loading

ghost Apr 2, 2018 •

edited by ghost

Loading

WilliamDenniss Apr 20, 2018 •

edited

Loading