openid-certification / oidctest

THE CERTIFICATION TEST SUITE HAS BEEN MIGRATED TO A NEW SERVICE https://www.certificatinon.openid.net
Other
49 stars 15 forks source link

test for long 'nonce' parameter #134

Open jogu opened 5 years ago

jogu commented 5 years ago

We ran into an interoperability problem where the RP was passing a 300 byte nonce and the OP only supported up to 100 bytes. The OP is OIDF certified.

By my reading of the OpenID connect specification, no limit is placed on the length of nonce, so I think it would be reasonable to add a test that has a (say) 1000 byte nonce.

jogu commented 5 years ago

For what it's worth, https://gitlab.com/fintechlabs/fapi-conformance-suite has a few tests that use 128 byte states, but it never occurred to us to test nonces that long. I think given we've now seen 300 byte nonces we should also be testing states longer than 128 bytes.

zandbelt commented 5 years ago

For the record the spec doesn't say anything about the length https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes, there's probably room for improvement there.

For interoperability purposes and to avoid hitting browser or server limitations we may add a nonce length test to the suite, or perhaps even a generic maximum parameter length check including state, request and request_uri, and/or a maximum URL length test.

The spec does mention request URI length in:
https://openid.net/specs/openid-connect-core-1_0.html#RequestUriParameter

The entire Request URI MUST NOT exceed 512 ASCII characters.

and the request URL length for a Self-Issued OpenID Provider Request:
https://openid.net/specs/openid-connect-core-1_0.html#SelfIssuedRequest

The entire URL MUST NOT exceed 2048 ASCII characters.

The URL length of the total Authorization Request across all cases should stay under 2000 characters, see:
https://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers and:
http://www.boutell.com/newfaq/misc/urllength.html

jogu commented 5 years ago

Further info: we found one of the UK CMA9 banks (Lloyds) seems to have a hard limit on the URL length of 2069 characters, above which something (probably a gateway in front of the AS) returns "Request-URI Too Long The requested URL's length exceeds the capacity limit for this server."

A fairly generic request from the OB conformance suite uses ~1500 bytes (mainly due to the size of the signed request object), so there's a relatively small margin for using longer states or nonces. (Here's an example with a 128 byte state: https://fapidev-as.authlete.net/api/authorization?request=eyJraWQiOiJyMGxCRzRYeDZVM1ZPXzNpWkRaUW8wQTJLUmdsQlEweFYyRTBaQzVtbm9jIiwidHlwIjoiSldUIiwiYWxnIjoiUFMyNTYifQ.eyJhdWQiOiJodHRwczpcL1wvZmFwaWRldi1hcy5hdXRobGV0ZS5uZXRcLyIsInNjb3BlIjoib3BlbmlkIGFjY291bnRzLW9iIiwiY2xhaW1zIjp7ImlkX3Rva2VuIjp7ImFjciI6eyJ2YWx1ZSI6InVybjpvcGVuYmFua2luZzpwc2QyOnNjYSIsImVzc2VudGlhbCI6dHJ1ZX0sIm9wZW5iYW5raW5nX2ludGVudF9pZCI6eyJ2YWx1ZSI6ImUwMTRiZjEyLTVmM2EtNDI2YS1hODM0LThjMmRlNDBmZDY2NCIsImVzc2VudGlhbCI6dHJ1ZX19fSwiaXNzIjoiMzA0MjU1NzQ3MzkiLCJyZXNwb25zZV90eXBlIjoiY29kZSBpZF90b2tlbiIsInJlZGlyZWN0X3VyaSI6Imh0dHBzOlwvXC9maW50ZWNobGFicy1mYXBpLWNvbmZvcm1hbmNlLXN1aXRlLXN0YWdpbmcuZmludGVjaGxhYnMuaW9cL3Rlc3RcL2FcL2ZpbnRlY2gtYXV0aGxldGVcL2NhbGxiYWNrP2R1bW15MT1sb3JlbSZkdW1teTI9aXBzdW0iLCJzdGF0ZSI6IndjbllkVFJnbFJHRWVrcGlXaTZGNXN1OWZKemZ3THFRMDBZelFvRDNuNHVZZ2tWQ0o3b0ZKZjZuU2lJdmhoeVl2U0xESURERjhGVUR0WGpyVExJVnpPN3B3d3ZRQVNsZ1g1TVkwd3A2UEpsb0U3OWtkZDU5WGpCVFdvMXc2YTR2IiwiZXhwIjoxNTQwMTgzNDc0LCJub25jZSI6ImV5QlZJaVN6dmsiLCJpYXQiOjE1NDAxODMxNzQsImNsaWVudF9pZCI6IjMwNDI1NTc0NzM5In0.qaCaJ7THYniir45No5Ot2Fvs419pfmlm_Sk0_MfradXH6arp57PXp2_jTFLZjUvXh-454kaV5I1aTx75Hv3C5LKGjLQV6-rQlBvAyvv9dN-1Eu35CMO0q4pVKlJeAja1Sh0oYMIElJIbJIk1C0thTEz-JfAF8TqabIsFOOc0_XHD1AJUECbIXdSB-4pvi-nc4qkZYmEdjz6wBhAhrunj8ZAZMX_9DXTdZdDFMJbybNEqu6jBozgn5FO-GYtiHo6bcbFek3k8dI_1yllD-1ER7K1QzUOxw5dH9H2DRMrzGMO3P7rf5Qq8MzM-TVou2sZvi8P3MwZjikMaoqdDWmVHmQ&client_id=30425574739&redirect_uri=https://fintechlabs-fapi-conformance-suite-staging.fintechlabs.io/test/a/fintech-authlete/callback?dummy1%3Dlorem%26dummy2%3Dipsum&scope=openid%20accounts-ob&response_type=code%20id_token )

selfissued commented 5 years ago

It's probably worth filing an issue for the Connect spec itself at https://bitbucket.org/openid/connect/issues talking about the possible need to have practical limits for the entire request.

jogu commented 5 years ago

Thanks Mike. I've raised https://bitbucket.org/openid/connect/issues/1055/limits-on-overall-url-length

sakimura commented 5 years ago

And exactly to cater this kind of cases, we have something called request_uri in OpenID Connect and forthcoming OAuth JAR.

zandbelt commented 5 years ago

@sakimura well not exactly: although in principle (and by design) those can be used to mitigate request URL length issues, there are actually no boundaries specified there either so it is still up in the air. Even the JAR draft only implicitly addresses this by restricting the length of only one parameter and mentioning the maximum URL length for a particular browser. More restrictive text is needed.

jogu commented 5 years ago

I believe there is a boundary in https://openid.net/specs/openid-connect-core-1_0.html#RequestUriParameter :

The entire Request URI MUST NOT exceed 512 ASCII characters.

(I'd say the bigger issue is that request_uri isn't supported in many real world deployments :-) )

zandbelt commented 5 years ago

It is not clear whether this is about the request_uri parameter value lenght or about the total authorization request length. For the latter it should allow 2000.

sakimura commented 5 years ago

This is on request_uri parameter value. There is no limit the specification to the length of each parameter in the specification. Some of them can be pretty big depending on the nature of the claim. When you are not using request_uri, there is an implicit length limit due to the (old) browser limitations. Some browsers has/had 2046 or so bytes limitation on the entire URI length. That browser was pretty common, so it was a practice at the time to factor that into in designing the parameters values.

On Fri, Nov 2, 2018 at 3:34 AM Hans Zandbelt notifications@github.com wrote:

It is not clear whether this is about the request_uri parameter value lenght or about the total authorization request length. For the latter it should allow 2000.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openid-certification/oidctest/issues/134#issuecomment-435138646, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJSN4ZNyIDU6M2WWH6dyaZKKO-bn-W3ks5uqz6agaJpZM4XHyL3 .

-- Nat Sakimura (=nat) Chairman, OpenID Foundation http://nat.sakimura.org/ @_nat_en

zandbelt commented 5 years ago

So yes, there's a limit on the request_uri value but when this value is sent to the OP it may still result in an authorization request value that is truncated by browsers since it may contain state/nonce or may just overrun because the authorization endpoint URL is 1800 characters long itself...

panva commented 5 years ago

I don’t think we should be testing for something not in the specs and at most it should be giving recommendations, not impose limits on these parameters. I believe we can agree there are more pressing topics to spend time on in this area, e.g. actually giving OP/RP assurances that request object is used and no downgrade on the auth request can happen or #92 actually testing the OPs are using the params from request objects.

zandbelt commented 5 years ago

I disagree: firstly the spec should be extended, secondly the test suite is not just about spec compliance, it also for increasing interoperability and this is a real problem, albeit with somewhat lower priority indeed.

panva commented 5 years ago

Once the spec is updated with new requirements or recommendations, sure. Agreed on the prio.

zandbelt commented 5 years ago

that's not what I suggested ;-)

panva commented 5 years ago

Imposing limits is primarily the spec’s job, the suite then complements in that regard. I agree with the interoperability argument but at most to a level where it takes optionals from the spec and makes them mandatory for a given profile.

jogu commented 5 years ago

I'm not sure we should use "the spec doesn't state a limit" as a reason not to test things; the reductio ad absurdum outcome of that is that we should update the test suite to only use single character state and nonces.

I would actually read the spec the other way around currently - the lack of a limit means that clients are justified in using long states and nonces, and if an OP rejects them (or fails to replay them correctly in the id_token) because of their length then it is not compliant.

Either way, I think it is important to at least test using state & nonce values of lengths that are commonly used in real world RPs.

travisspencer commented 5 years ago

I'll chime in here as an OP implementer. It's very hard when the test suite adds cases for things that aren't in the spec. We're reading the standard, and coding to that. Once we get things to a good state, we certify the implementation by running the tests. When some of those fail for reasons not defined by the spec, it's confusing, unexpected and causes delays. I understand that this is done for the sake of interoperability, but isn't that the entire point of having a standard? If additional requirements are imposed by the tests, what happens is that the certification test suite effectively becomes in unwritten profile of OpenID Connect.

jogu commented 5 years ago

@travisspencer Thanks for adding your thoughts!

Do you know if your OP allows unlimited length state & nonce values as the current spec seems to require?

I'd also be interested to know if you would be up for standing up a permanent cloud hosted version of your OP that new conformance suite tests could be tested against, hopefully automatically?

(I do recognise the need to clarify the spec when it's not clear hence why, at Mike's suggestion, I opened a bug against the openid spec, https://bitbucket.org/openid/connect/issues/1055/limits-on-overall-url-length .)

zandbelt commented 5 years ago

we all agree that adding URL length limitations improves interoperability, so there's no doubt that this is a useful addition; and yes, if by all means it should be backported to the spec

remember: the test suite is already doing more than in the spec, on purpose and with good reason; this is not a spec compliance thingy, it is a "real world" thingy

if your .implementation creates URLs that exceed 2000 bytes, you may have certified but you'll be in trouble down the road; that is no reason to stop adding new tests that improve good behavior and interop

panva commented 5 years ago

the test suite is already doing more than in the spec

For instance? Other than optional single redirect uri which is allowed per oauth (but not oidc, there its stricly required) i cant recall anything else

travisspencer commented 5 years ago

remember: the test suite is already doing more than in the spec, on purpose and with good reason; this is not a spec compliance thingy, it is a "real world" thingy

This is one of the challenges with conformance and something the tool should not do IMHO. It would be fine if those things were all written out in a "conformance profile". The compliance to that would be a lot easier because there would be a doc to read in addition to the tests.

@jogu does the spec require that? Haven't noticed that, and it's not something we test for. So, unsure. If we don't though, we support by-ref request objects, so it's easy to work around.

zandbelt commented 5 years ago

the test suite is already doing more than in the spec

For instance?

requiring support for client_secret_post is one such thing

At the end of the day this the difference is in ticking a box or actually caring about interop. I believe the latter is the only thing that counts, the former is marketing.

panva commented 5 years ago

Requiring post is in the category i described, it takes an optional and makes it mandatory for a profile’s sake. But it is already part of the spec. It’s not an entirely new auth method, e.g. requirement such as imposing length limits would be.

The suite’s purpose is not to impose arbitrary requirements. Once there’s an errata draft and a language behind a consensus there’s no argument.

Edit: Sorry for closing, was a misclick.

zandbelt commented 5 years ago

"arbitrary requirements" shows bias; this is a useful test; I get it that implementers of a particular implementation merely want to tick a box, we should be looking to stimulate interop across many implementations/browsers and not just care about our own

panva commented 5 years ago

I’m glad there’s a discussion going, but it would be are arbitrary until there’s a consensus from the WG.

zandbelt commented 5 years ago

I consider that an arbitrary comment ;-)

zandbelt commented 5 years ago

also: the current implementation of the test suite already imposes an actual arbitrary limit on URL lengths today: the Python CherryPy server will barf on certain URL lengths because any implementation has its own limits for that; we just don't know what it is, and that is arbitrary today; it would be good to create an explicit, sane, implementation independent limit

jogu commented 5 years ago

@jogu does the spec require that? Haven't noticed that, and it's not something we test for. So, unsure. If we don't though, we support by-ref request objects, so it's easy to work around.

Yes, my current reading of the spec is that clients are entitled to use nonce & state of whatever length they like (with the only limit being any imposed by the resource owner's web browser).

I think I may have a different background to others - I've spent a not insignificant time over the past year acting as one of the arbitrators when OPs & RPs disagree over what the OpenID Connect specs mean in an organisation that essentially has delegated powers (from the government) to force the OPs comply with the specs (ie. the UK OpenBanking implementation entity). We can expect to see more of this as profiles like FAPI get adopted by government mandated API programs (e.g. Australia). This can often mean extrapolating or inferring from available information - for example in this case the fact that the openid specs do explicitly impose limits in some places (for example on sub "MUST NOT exceed 255 ASCII characters in length") and the oauth2 RFC explicitly mentions that accesstoken/clientid/authcode lengths are OP defined) and explicitly stating that state should be exactly replayed, would be taken to mean that a limit was not imposed on state - unless there was other evidence to the contrary.

(In that context the existence of a workaround would also not be accepted as a reason not to comply with the spec, though might be accepted as an interim measure.)

travisspencer commented 5 years ago

@jogu https://github.com/jogu does the spec require that? Haven't noticed that,

Yes, my current reading of the spec is that clients are entitled to use nonce & state of whatever length they like... the oauth2 RFC explicitly.. stating that state should be exactly replayed, would be taken to mean that a limit was not imposed on state

You have to keep in mind when making these interpretations that the standards are a stack -- OpenID Connect builds on OAuth which builds on HTTP. RFCs 7230 and 7231, the HTTP spec, addresses this:

HTTP does not have specific length limitations for many of its protocol elements because the lengths that might be appropriate will vary widely, depending on the deployment context and purpose of the implementation.... A server that receives a request-target longer than any URI it wishes to parse MUST respond with a 414 (URI Too Long) status code.

So, if an RP send my OP a nonce or state that I think is too long, and my OP responds with a 414, I'm in compliance with all relevant specs.

As a product developer, these specs are my compass. If the test tool imposes unwritten limits like this, it creates an unspecified profile that is harder to conform to. For this reason, I really hope this test isn't added to the suite...at least until there is matching wording in an OpenID Connect core errata or other profile.

zandbelt commented 5 years ago

@travisspencer thanks for providing more background; I agree that introducing a test like this actually creates an unwritten profile which is not something I like; the alternative is to leave things up in the air and break randomly; I like that even less.

Also: your concern about backwards incompatible changes is understandable: we have not been very good in the past about maintaining a Changelog and doing proper versioning. We've gotten better in that area, but feel free to suggest more improvements.

Certification is a public record against a specific version of the test suite at a specific point in time. Nothing changes that historical record, but things will change and break over time. Regular re-certification is a good thing IMHO and yes, it does require an effort (on both sides).

travisspencer commented 5 years ago

If you guys add a test for this, please make sure to accept a 414 as passing in addition to a successful authorization flow back to the client. If you do that, I think all will be happy.

zandbelt commented 5 years ago

Indeed I would not want the suite to enforce a minimum length that that OPs needs to accept. That seems one step to far to me. I would like to make sure that RPs (and OPs for that matter) don't generated URLs that exceed about 2000 characters. But also that's up for debate in the group. Thanks for your input, that is very helpful to steer the discussion.

sakimura commented 5 years ago

For nonce, there is an implicit minimum length limit as the spec states: Sufficient entropy MUST be present in the noncevalues used to prevent attackers from guessing values. For implementation notes, see Section 15.5.2 https://openid.net/specs/openid-connect-core-1_0.html#NonceNotes. The problem then us how to test it. The implementation guidance suggests to use a string that includes the hash of cryptographic random.

On Mon, 5 Nov 2018, 19:16 Hans Zandbelt <notifications@github.com wrote:

Indeed I would not want the suite to enforce a minimum length that that OPs needs to accept. That seems one step to far to me. I would like to make sure that RPs (and OPs for that matter) don't generated URLs that exceed about 2000 characters. But also that's up for debate in the group. Thanks for your input, that is very helpful to steer the discussion.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openid-certification/oidctest/issues/134#issuecomment-435977936, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJSN-idSVbhUdswwLlobh6HAH6d8_jQks5usIBjgaJpZM4XHyL3 .

selfissued commented 5 years ago

Replying to @travisspencer 's question about the requirements for OpenID Connect certification, yes, the certification process definitely and deliberately imposes additional requirements for certified implementations beyond those in the specifications. These were the result of explicit OpenID Connect working group decisions that are recorded in the certification profile definitions. The latest version of this document is at http://openid.net/wordpress-content/uploads/2018/06/OpenID-Connect-Conformance-Profiles.pdf.

This shouldn't come as a surprise. Most of the additional requirements are to require support for things that are optional in the specs or to make SHOULDs into MUSTs. For example, support for the UserInfo Endpoint is optional in OpenID Connect Core. It's required to achieve certification. These additional requirements created by the working group are intended to promote interoperation among implementations.

zandbelt commented 5 years ago

On the certification team call in December we agreed that for this case issuing a warning about running over a maximum total URL length would be most appropriate (such checks are not part of the conformance profile doc anyhow), possibly we could add warnings about individual parameter lengths later.