Nonce validation shouldn't fail when not using nonce

[andrewbaxter]

When the client doesn't provide a nonce but the provider responds with a nonce, validation currently fails.

This was already brought up in #21 and closed with a workaround but I'd like to request again that this behavior be removed. I just hit this issue with AWS Cognito with Google as an upstream.

If I understand the discussion in the other issue, this behavior

• Is not necessary
• Doesn't add safety
• Reduces reliability depending on upstream behavior
• Goes against expectation (depending on the provider, users are unlikely to have working code the first time following examples)
• The workaround is non-obvious (is it documented anywhere?)

Removing the claims.nonce().is_some() check in the None branch would allow everyone to use the same method for validation, simplifying the interface and solving all the above issues.

[kilork]

Hey, thanks for the issue. I quickly revised specs, and found nonce validation depends on flow used. If implicit flow is used - it MUST be passed as parameter and MUST be validated. In general it MUST be validated if it was passed. I will organize here all cites from specs later. Which flow are you using?

[andrewbaxter]

Thanks for being willing to give the issue a second look! I'm using authorization code flow, per the example in the readme.

If this check is protecting against misuse in the code, having a separate interface for the implicit flow that enforces these spec requirements (such as making the nonce non-optional both in auth_uri options and the validation call) would be a lot better I think.

Alternatively, maybe the nonce setting could be configured in the client so the validation does something like this?

fn validate_nonce(...) {
    if self.using_nonce {
        assert!(nonce.is_some());
    }
    if let Some(nonce) = nonce {
        match claims.nonce() {
            ...
        }
    }
}

(and also assert that the nonce is specified in auth_url? since it needs to be specified in both)

[kilork]

Yeap, reading specs is quite difficult sometimes. The task of library for some specification is to avoid misusage. This means, that if something is not allowed by specification, we should not make non-compliant behavior default, or make it easy to break things.

In the very beginning of specs for OpenID we have here following info:

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.

It tells we must verify nonce if it is present in ID token. The behavior of Authorization Server, which changes nonce without notice seems to fall into category of wrong behavior.

But it is still not quite clear, how it is possible to compare some value to something, that does not exist.

Later specification comes again to ID Token validation and nonce validation in particular:

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.

This looks already more suitable for us and answers to the question above - if value was not passed before, it is not possible to compare. This information is from Authorization Code Flow and I think my library currently works only with this flow, even technically it is possible to use other flows.

Next time nonce mentioned in scope of Implicit Flow here:

nonce REQUIRED. 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.

Now nonce must be used from the beginning. But we do not support this flow anyway.

Other mentions of nonce in Implicit Flow:

1. https://openid.net/specs/openid-connect-core-1_0.html#ImplicitIDToken

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

2. https://openid.net/specs/openid-connect-core-1_0.html#ImplicitIDTValidation

The value of the nonce Claim MUST be 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.

Starting from this point specs do not give other variants.

Seems like we can safely drop this check.