1. OpenID Foundation Avatar OpenID Foundation
  2. WGs
  3. connect
  4. Issues

make clear that nonce is always required for Hybrid flows

Issue #1052 closed
Hans Zandbelt created an issue

Assuming that that nonce is always required for Hybrid flows no matter where the id_token is returned from, also following: https://github.com/rohe/oidctest/issues/111#issuecomment-426450954

In section 3.3.2.11. ID Token for the Core spec, https://openid.net/specs/openid-connect-core-1_0.html#HybridIDToken, it describes the ID token in the Hybrid flow, which says

When using the Hybrid Flow, these additional requirements for the following ID Token Claims apply to an ID Token returned from the Authorization Endpoint:

Since an ID Token may also be returned from the Token Endpoint, that sentence seems to be too restrictive and the last part "returned from the Authorization Endpoint" should be removed.

FWIW: this may be a left-over from

3.3.2.12. ID Token Validation

where ID token validation is discussed for an id_token returned from the Authorization Endpoint, as opposed to:

3.3.3.7. ID Token Validation

where ID token validation is discussed for an id_token returned from the Token Endpoint.

OTOH: if section 3.3.2.11. is only about ID tokens returned from the Authorization Endpoint and it is supposed to be the counterpart of

3.3.3.6. ID Token

where validation of the contents of an ID Token returned from the Token Endpoint is discussed, then the following should be added to the latter:

Use of the nonce Claim is REQUIRED for this flow.

otherwise it is not clear that nonce is always required in Hybrid flows no matter where the ID token is returned from.

Comments (25)

  1. Brian Campbell

    I firmly believe that that assumption ("that that nonce is always required for Hybrid flows no matter where the id_token is returned from") is not correct.

    Things would have been simpler, if Connect had just made nonce mandatory for all authentication requests. But that's not the case.

    The nonce parameter and claim are used to protect against replay of an ID token. Direct replay or injection of an ID token is only possible when returned from the Authorization Endpoint because that flows through the browser via some sort of redirection. When an ID token is returned from the Token Endpoint, it's over a direct HTTPS connection between client and server and there's no opportunity for replay or injection of an ID token. There is opportunity there for replay or injection of the authorization code but there are other protections for that including one time use of the code and redirect_uri checking on the access token request. That was the logic underlying nonce being required for ID tokens when returned from the Authorization Endpoint and optional for ID tokens when returned from the Token Endpoint (and in turn when it is required on the authentication request). That's why nonce is required for OIDC implicit flow (ID token returned from the Authorization Endpoint) and optional for OIDC code flow (ID token returned from the Token Endpoint). Consistent with that, for hybrid it depends on the response_type with code id_token or code id_token token requiring nonce while it's optional for code token.

    Both Connect Core 3.3.2.11 and 3.3.2.12 are about the hybrid flow where ID tokens when returned from the Authorization Endpoint. Which is what the text "apply to an ID Token returned from the Authorization Endpoint" and "the contents of an ID Token returned from the Authorization Endpoint" says. Whereas 3.3.3.6 and 3.3.3.7 are about ID tokens when returned from the Token Endpoint.

    Changing Core so that nonce is always required for Hybrid flows no matter where the id_token is returned from would be a breaking change, which really isn't okay for an errata.

    Per spec, our AS/OP implementation only requires nonce on authentication requests that have a response_type that contains id_token and would result in an ID token returned from the Authorization Endpoint. It's worked that way, per spec, since 2013. And it was OpenID Certified as a Hybrid OP (and other profiles) in in 2015.

    https://github.com/rohe/oidctest/issues/111#issuecomment-426450954 is indicative of an issue with the test suite. Which, if not fixed, puts us in the very bad position of having to introduce a breaking change to product in-order to re-certify.

  2. Michael Jones

    Brian - it's not an assumption that nonce is required for all Hybrid response types. There's clear normative text requiring that.

    Section 3.3.2.11. ID Token (in the Hybrid Flow section 3.3) at https://openid.net/specs/openid-connect-core-1_0.html#HybridIDToken says:

    The contents of the ID Token are as described in Section 2. When using the Hybrid Flow, these additional requirements for the following ID Token Claims apply to an ID Token returned from the Authorization Endpoint:

    nonce Use of the nonce Claim is REQUIRED for this flow.

    This has been explicitly discussed by the working group in the past. 3.3.2.11 is clear that the request must contain a nonce and the nonce must be present in the issued ID Token for all hybrid flows. The fact that the ID Token is returned from the Token Endpoint doesn't invalidate this requirement.

    In fact, this is parallel to the requirement in the code flow that if a nonce is present in the request, a nonce value MUST be present in the issued ID Token (which is also returned from the Token Endpoint). This is in 3.1.3.7, which says:

    If a nonce value was sent in the Authentication Request, a nonce Claim MUST be present and its value checked to verify that it is the same value as the one that was sent in the Authentication Request. The Client SHOULD check the nonce value for replay attacks. The precise method for detecting replay attacks is Client specific.

    Section 2 (ID Token) also makes it clear that a requested nonce must be acted upon:

    nonce 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 present in the ID Token, Clients MUST verify that the nonce Claim Value is equal to the value of the nonce parameter sent in the Authentication Request. 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. Authorization Servers SHOULD perform no other processing on nonce values used. The nonce value is a case sensitive string.

    With respect to breaking changes, the PingFederate Summer 2015 Release passed all these tests. Apparently a change somehow made it into more recent code that broke spec compliance in this area. Many product groups run regression tests against the conformance suite during development. If Ping had been doing that, this would have been caught at the time the breaking change was made to the code base that previously passed all the tests. I'm not telling you how to run your engineering processes, but many other development teams have found this to be useful.

  4. Hans Zandbelt reporter

    I still think that the sentence "additional requirements for the following ID Token Claims apply to an ID Token returned from the Authorization Endpoint:" is very confusing and should get rid of the part that says "returned from the Authorization Endpoint:".

    Just to clarify for quick readers: this discussion is not about what to do when a nonce is present, it is about when to reject when a nonce is not present.

  5. Michael Jones

    I believe that this is a duplicate of https://bitbucket.org/openid/connect/issues/972/nonce-requirement-in-hybrid-auth-request - which was already discussed by the working group and resolved. I'll leave it open for now so it can be discussed at an upcoming call (or possible at the workshop later this month) but given this issue has already been discussed by the working group and a resolution agreed to, I believe that after discussion, the default action should be to close this issue as a duplicate.

  6. Hans Zandbelt reporter

    Once more, MHO it is not a duplicate since the text is (still) not clear: it should get rid of the "returned from the Authorization Endpoint:" part.

  7. Michael Jones

    Providing more context, issue #972 was fixed on July 29, 2015 with the commit https://bitbucket.org/openid/connect/commits/0eed40d6c278. This applied the fix agreed to by the working group. You can also see that John Bradley wrote at the time that this was not a breaking change.

    You can see this fix in context in the latest Core draft at https://openid.net/specs/openid-connect-core-1_0-24.html#HybridAuthRequest (which is currently equivalent to the editor's draft at https://openid.bitbucket.io/connect/openid-connect-core-1_0.html#HybridAuthRequest).

    Hans, Section 3.3 on Hybrid Authentication contains two ID Token subsections 3.3.2.11 and 3.3.3.6, respectively about ID Tokens returned from the Authentication Endpoint (if any) and ID Tokens returned from the Token Endpoint. So the right fix is not to delete the "returned from the Authorization Endpoint" clause in 3.3.2.11.

    The fix I'd recommend is to add a note to 3.3.2.11 reminding readers that an ID Token is not returned from the Authentication Endpoint for the code+token and response_type and therefore that subsection (and 3.3.2.12 about ID Token Validation) only applies to the code+id_token and code+id_token+token response types for the Hybrid Flow. If we want to be truly pedantic, we can also add a note to 3.3.3.6 saying that the ID Token rules there and in 3.3.3.7 apply to all Hybrid response types.

    Those reminders should eliminate any remaining possibility of confusion. I'll assign this bug to myself and add it to the errata milestone. Reviews of the proposed fix are welcomed.

    In retrospect, while we were trying to be concise when writing the Authentication sections, and therefore a lot of subsections referenced others and said "do what it says there", it seems that being a bit more verbose in these cases could have eliminated potential confusion. We should consider that lesson when reviewing normative text in the future.

  9. Hans Zandbelt reporter

    By intent it was a breaking change, by syntax it still did not have the desired effect because of the lingering "returned from the Authorization Endpoint:"

    The former is enough to sharpen the spec, but moreover: if one needs 5 paragraphs to explain that "always add a nonce in Hybrid flows" is obvious from the spec it probably makes sense to add something like "always add a nonce in Hybrid flows" to that very spec.

  11. Michael Jones

    We don't need 5 paragraphs. The existing fix that's been in place since July 2015 where the nonce requirement is made explicit in https://openid.bitbucket.io/connect/openid-connect-core-1_0.html#HybridAuthRequest does the trick without any additional text being added. We can add more if people still think confusion is possible.

    I think there's enough record in the tracker now to facilitate working group discussion. Therefore, I'm going to stop replying to these comments in real time because there's other work I also have to get done today - some of it for OpenID. ;-)

  12. Brian Campbell

    Brian - it's not an assumption that nonce is required for all Hybrid response types. There's clear normative text requiring that.

    Section 3.3.2.11. ID Token (in the Hybrid Flow section 3.3) at https://openid.net/specs/openid-connect-core-1_0.html#HybridIDToken says:

    The contents of the ID Token are as described in Section 2. When using the Hybrid Flow, these additional requirements for the following ID Token Claims apply to an ID Token returned from the Authorization Endpoint:

    nonce Use of the nonce Claim is REQUIRED for this flow.

    In that very text you cite, the requirement for nonce is clearly qualified to apply to an ID Token returned from the Authorization Endpoint. That's the case both by the text right before the list item with nonce as well as the section in which it appears - 3.3.2. and subsections are about the Authorization Endpoint in the hybrid flow.

    Honestly, when looking at this within the spec as whole, I don't see how you can reach the interpretation that you've landed on. But you have and I'll have to concede you that interpretation. And we disagree. That disagreement and the confusion and disagreement by others in this ticket and the several related items should more than demonstrate that there are two interpretations that can reasonably be reached from the spec as written. Changing the spec via errata to the more restrictive of the two interpretations is unquestionably a breaking change to implementations built against the more tolerant interpretation. While on the other hand, changing the spec via errata to the more tolerant of the two interpretations doesn't break anything. Furthermore, because nonce is intended to protect ID tokens returned in the front channel, there's no necessary reason to go with the restrictive interpretation.

    With respect to breaking changes, the PingFederate Summer 2015 Release passed all these tests. Apparently a change somehow made it into more recent code that broke spec compliance in this area.

    As Hans points out, the breaking change was the new test being introduced to the test suite in 2017. The PingFederate behavior with respect to this hasn't changed since I wrote it in 2013 - it has always allowed an authentication request with a response_type of code token and no nonce. Which again underscores the fact that requiring nonce for code token is indeed a breaking change.

  13. Michael Jones

    Brian, I hear you but I still believe the spec was and is clear. The definition of "nonce" in Section 2 says:

    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.

    That applies to all response types.

    Also, 3.3.3.6 https://openid.net/specs/openid-connect-core-1_0.html#HybridIDToken2 - which is about ID Tokens returned from the Token Endpoint says that its validation steps are the same as those in 3.3.2.11 - where nonce processing is explicitly called out.

    If it were only the text in subsections of 3.3 that specify the nonce requirement, I'd agree that that different people could reasonably interpret it differently, but the text in Section 2 is completely unambiguous.

    Finally, I'm surprised that you're surprised that nonce processing is required for ID Tokens returned from the Token Endpoint. Your code supports that for response_type=code, as evidenced by passing the test OP-nonce-code (ID Token has nonce when requested for code flow). I'm surprised that you special-cased code+token to not do the same for the ID Token returned from the Token Endpoint by that flow, when your code works for the same situation with response_type=code.

    Anyway, I'm sure this is now a topic for a future call. Again, there was a working group decision on this very topic in 2015 when it was raised in issue #972. I still believe that the 2015 decision was the correct decision and that this issue is a duplicate (albeit, pointing out that the text should probably be further clarified to prevent future confusions).

  14. Brian Campbell

    Indeed a topic for a future call. My availability for the next call(s) is a bit uncertain however due to family obligations. So that future may have to be a bit out in the future.

    One point of clarification, however, as your last message suggests that we might have some miscommunication here (impossible I know!). Our code correctly handles the nonce when it's present in the request - the value is passed through unmodified from the Authentication Request to the ID Token. That's true for all requests including those with response_type=code+token. That's not the issue. The issue at hand is that we don't reject a response_type=code+token request that doesn't have a nonce parameter.

  15. Hans Zandbelt reporter

    Agree with Brian, there's no discussion on what to do when a nonce is present., which is what the text @mbj refers to is about. Repeating myself:

    this discussion is not about what to do when a nonce is present, it is about when to reject when a nonce is not present.

  16. Brian Campbell

    Repeating Hans:

    "this discussion is not about what to do when a nonce is present, it is about when to reject when a nonce is not present."

    In the Core section 3.3.2.1 defines the Authentication Request for the hybrid flow. That section defers to Authentication Requests as defined in Section 3.1.2.1 (excepting parameters defined right in that section) where nonce is OPTIONAL. The only requirement for nonce in the request has to be inferred back from the claim being required in the resulting ID token. And, as previously stated, that requirement is qualified as only applying to ID tokens returned from the Authorization Endpoint. The logical net of this is that nonce is required for requests with response types that contain id_token and optional otherwise.

    As such, I propose that the following text be added to section 3.3.2.1 as a list item below response_type.

    nonce

    • REQUIRED if the response_type of the request is code id_token or code id_token token and OPTIONAL when the response_type of the request is code token. It is a 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. Sufficient entropy MUST be present in the nonce values used to prevent attackers from guessing values. For implementation notes, see Section 15.5.2.
  17. Filip Skokan

    If this is up for re-debate since the Errata set 2 isn't published yet, I agree with Brian, nonce presence in a hybrid authentication request shouldn't be REQUIRED for code token, only code id_token and code id_token token

  19. gffletch

    +1 for Brian's proposal

    Here is my understanding of what the spec defines...

    1. If the nonce is in the request, then it MUST be in the returned id_tokens (regardless of front-channel or /token endpoint).

    2. Nonce MUST be specified for any response_type that returns an id_token in the front-channel ("code id_token", "code id_token token")

    3. Nonce is NOT required in the request for response_type of "code"

    The situation in question then, is response_type="code token". In this case an id_token is NOT returned via the front-channel and therefore I believe that nonce is NOT required for this response_type.

  20. Hans Zandbelt reporter

    @gffletch thanks for jumping in; however I would like to point out that both Brian's and your arguments should actually be applied against https://bitbucket.org/openid/connect/issues/972/nonce-requirement-in-hybrid-auth-request which should be re-opened IMHO.

    The outcome of #972 was that nonce should be required for all hybrid flows, which apparently does not have consensus (include me in disagreeing...).

    This ticket is about the work that was done to reflect the outcome of #972 in the spec: it is (still) not clear that nonce is always required.

    However, if #972 is re-opened and gets a different outcome, this ticket may be discarded.

  21. Brian Campbell

    That's a fair procedural point @zandbelt. This ticket just happens to be where the discussion is taking place because it's new and open and closely related while #972 seems to have slipped past unnoticed by a number of people. #972 should be re-opened and then resolved (with the different and correct outcome) by applying the proposed text from my previous comment rather than the overarching and breaking change introduced by 58ba615e7f43. This ticket can then be marked as "invalid" and closed with no action on the spec documents. And following from that, https://github.com/rohe/oidctest/issues/111 should be re-opened and resolved (probably) with the change you suggest in the 5th comment https://github.com/rohe/oidctest/issues/111#issuecomment-423645478

  22. Brian Campbell

    Just noting that this was discussed on the 29-Oct-18 call with some consensus there that was generally leaning towards what I'd proposed in the previous comment, which was going to be written up for review before proceeding. From the call notes:

      #1052 make clear that nonce is always required for Hybrid flows
      Mike and Brian spoke to this
      A nonce request parameter may not be strictly necessary for the code+token response type
           Both agreed that reasonable people could interpret this differently for code+token
      Brian made the case that we shouldn't introduce breaking changes via errata
      Mike pointed out that this change would probably require removing this particular test from the certification suite for code+token
      Mike will write up this possibility in the issue for review
  23. Brian Campbell

    Issue #972 has been reopened and pull request #4 submitted with proposed minor text updates to properly address the issue.

    This issue #1052 should be marked as "invalid" and/or "closed" with no action on the core document.

    @zandbelt and/or @Rohe and/or @ ? I'd request that https://github.com/rohe/oidctest/issues/111 (and maybe also https://github.com/openid-certification/oidctest/issues/14) be reopened and resolved to properly reflect the expected outcome here and in #972. As @mbj said in the 29-Oct-18 call, the change would probably require removing this particular test from the certification suite for code+token. Also, however, so as to allow for historical implementations based on the prior erroneous conclusions, the should probably not be a certification test that expects specific behavior based on the presence or lack-thereof of nonce in code+token (other than checking that when nonce is provided in the authn request, it is included in the resulting ID token).

  24. Hans Zandbelt reporter

    As suggested in the Jan 7 call meeting notes: I agree that this ticket can be closed (technically not because it is a duplicate but because the text that this ticket is about will be obsoleted by re-addressing #972)

  26. Log in to comment
Assignee
Type
enhancement
Priority
minor
Status
closed
Component
Core
Milestone
Errata
Version
–
Votes
0
Watchers
0
Jira: the preferred issue tracker for Bitbucket. Join the team!