openid-certification / oidctest

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

Claim/Scope-Tests do not support Distributed Claims #51

Closed marsangr closed 6 years ago

marsangr commented 7 years ago

Tests involving queries to the Userinfo-Endpoint and evaluating return values crash with the message

status=ERROR, message='NoneType' object is not callable

if confronted with distributed claims in the answer (s. https://openid.net/specs/openid-connect-core-1_0.html#AggregatedDistributedClaims). For instance:

https://op.certification.openid.net:60381/test_info/OP-scope-All

rohe commented 7 years ago

I'll have a look at this tomorrow.

rohe commented 7 years ago

OK, I know what the problem is. Will fix, but it will take some time before the official test site is running the fixed code.

selfissued commented 7 years ago

About this, Roland wrote "This was/is a bug in pyoidc. I have a Pull Request (PR) in the making just waiting for the one on phone_number_verified to be accepted. I don’t like having many PRs active at the same time."

zandbelt commented 7 years ago

It is this PR that we're waiting for: https://github.com/OpenIDC/pyoidc/pull/420 once it is included and pyoidc version 0.11.2(?) is released we will upgrade the production environment to OP 2.0.7 that will use that new version of pyoidc.

rohe commented 7 years ago

13 sep. 2017 kl. 11:35 skrev Hans Zandbelt notifications@github.com:

It is this PR that we're waiting for: OpenIDC/pyoidc#420

Yes.

once it is included and pyoidc version 0.11.2(?) is released we will upgrade the production environment to OP 2.0.7 that will use that new version of pyoidc.

Would be 0.11.1.0 I think.

-- Roland "Education is the path from cocky ignorance to miserable uncertainty.” - Mark Twain

zandbelt commented 7 years ago

pyoidc v0.12.0 is now deployed with OP release 2.0.7

marsangr commented 6 years ago

I would like to reopen this issue, something is not working properly, the error is a different one though and further down the workflow: The test client follows the redirection to the claims provider, but is not able to parse the answer and fails without further details (and a partial result)

Cf. the same test case https://op.certification.openid.net:60381/test_info/OP-scope-All

25.851 request {'body': None}
25.851 request_url https://identityagent.de/userinfo
25.851 request_http_args {'headers': {'Authorization': 'Bearer eyJraWQiOiJGT3Z5IiwiYWxnIjoiUlMyNTYifQ.eyJzdWIiOiJtYXJjb3MuZGVuaWMuZGUiLCJzY3AiOlsib3BlbmlkIl0sImNsbSI6WyJ3ZWJzaXRlIiwiem9uZWluZm8iLCJiaXJ0aGRhdGUiLCJlbWFpbF92ZXJpZmllZCIsImFkZHJlc3MiLCJnZW5kZXIiLCJwcm9maWxlIiwicGhvbmVfbnVtYmVyX3ZlcmlmaWVkIiwicHJlZmVycmVkX3VzZXJuYW1lIiwibG9jYWxlIiwibWlkZGxlX25hbWUiLCJnaXZlbl9uYW1lIiwicGljdHVyZSIsInVwZGF0ZWRfYXQiLCJuYW1lIiwibmlja25hbWUiLCJwaG9uZV9udW1iZXIiLCJmYW1pbHlfbmFtZSIsImVtYWlsIl0sImRhdCI6eyJyZWplY3RlZF9jbGFpbXMiOltdfSwiaXNzIjoiaHR0cHM6XC9cL2F1dGguZnJlZWRvbS1pZC5kZSIsImV4cCI6MTUxMjQ2OTI2NiwiaWF0IjoxNTEyNDY4NjY2LCJjaWQiOiJubXN6dGVxaGtuYW55In0.JLcqSHT-cmPj3hW75ag2PTYW5csnNftL7txkLy0Hi3D-z1Xc98fGO-4ZXmlUXXHiSX9zvyHExy0qyBypEyyA5MiL0tFgv23rscfN1nz3LLKhqEnp8KrbUWxiqZl5MiCVawW257mFUvaXJUiv0-MsM7WSru8RF-EgH5Ie-dmXV2hkni3fFz_c1d74s2zsipsBWEnn6eaQl2c7Dj7EEDyf6K3sSNCuGpC5SDyiox7MnYvcggwE1Unurj-3pp4aE48gET9rifSWwUr0uohYf6z3fvy7V8kIO6-mNJdQUWPY04EkoH3eJk30CSqXqychER0lLItLsdo0OmE92maCTVpmeQ'}}
26.507 http response url:https://identityagent.de/userinfo status_code:200
26.51 condition UserInfo:OP-scope-All: status=ERROR, message=
zandbelt commented 6 years ago

The error in the server logs is:

2017-12-05 10:11:07,541 requests.packages.urllib3.connectionpool:INFO Starting new HTTPS connection (1): identityagent.de
2017-12-05 10:11:08,195 requests.packages.urllib3.connectionpool:DEBUG "GET /userinfo HTTP/1.1" 200 1116
2017-12-05 10:11:08,197 oidctest.op.client:DEBUG Reponse text: 'eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJtYXJjb3MuZGVuaWMuZGUiLCJpc3MiOiJodHRwczovL2lkZW50aXR5YWdlbnQuZGUiLCJleHAiOjE1MTI0Njg5NTcsImNsYWltcyI6eyJwaG9uZV9udW1iZXIiOiI1NTUgMTIzNDUiLCJmYW1pbHlfbmFtZSI6IlNhbnogR3Jvc3NcdTAwZjNuIiwibmFtZSI6Ik1hcmNvcyBTYW56IEdyb3NzXHUwMGYzbiIsImVtYWlsX3ZlcmlmaWVkIjoic2FuekBkZW5pYy5kZSIsImdpdmVuX25hbWUiOiJNYXJjb3MiLCJlbWFpbCI6Im1hcmNvc0BkZW5pYy5kZSJ9LCJpYXQiOjE1MTI0Njg2NTcsIm5iZiI6MTUxMjQ2ODY1N30.WcLTstQA7t1aCxpZl5cxfCefFUZgGiebiXeKFWMVpJa9IPq035kbUmmGgaVWmxVFX8ygEOZCqCREjrcZW_zTq2sNmrp2DWf76B1hU_mF3r-LOmGM3YWn28cVcCKR5P8peXn7rkAKAvuRI3JqurkJOrbj9DKEgnIWlRWTGFtnqjfSUwGs72tv_I_MJ4geTMEzxvCSV7gosZe6xl7u5mjR-EMNZX-pu9S0BeJ3NuKQ09FNKLVqBqIDZ266R42Bsm7l9wOHhGwNefep6UhNVd6cenDTLFjeSJf_K1vLe0nN0EgejtbYs_vCAgETQ4SaqFCQP8JRhnCih-DosgAWjMhSafVAmc6sc44PCIwteQiyvTS7PgAvH9K_TXexwKUIFYnkGS1o-djZfm-6gu_wnInZlDBSlBtnLnLDXeVFbIP6j73UeNxAo5c6fTfuul8E29iHq6t1tdo4u7AFTon7W4U6fagJdBuEJGrkNre2vBu121ByDEzMghUcXYkbtbRNm7k0oyES0YnbOT_hRzuAsbMkhOHVwkLmzlkW1kCRS7eaqnfGT8kN2IQLcF7K0bKTI3hP-0fY9AMPk-_M9hsJTPz-Xe8Nq9SGoFSbBlflC-vL0McyW8Wm_1Y3bHLw0qLWIOB50zqNV_2yKgzlQfWoWqKLag63SkGKyZHWzRkogJBJglA'
2017-12-05 10:11:08,197 oic.oauth2.message:DEBUG Raw JSON: {'iat': 1512468657, 'claims': {'email_verified': 'sanz@denic.de', 'phone_number': '555 12345', 'given_name': 'Marcos', 'name': 'Marcos Sanz Grossón', 'email': 'marcos@denic.de', 'family_name': 'Sanz Grossón'}, 'iss': 'https://identityagent.de', 'sub': 'marcos.denic.de', 'nbf': 1512468657, 'exp': 1512468957}
2017-12-05 10:11:08,197 oic.oauth2.message:DEBUG JWS header: {'alg': 'RS256', 'typ': 'JWT'}
2017-12-05 10:11:08,197 oic.oauth2.message:ERROR Issuer "https://identityagent.de" not in keyjar
2017-12-05 10:11:08,197 oic.oauth2.message:DEBUG Found signing key.
2017-12-05 10:11:08,198 jwkest.jws:DEBUG Picking key by key type=RSA
2017-12-05 10:11:08,198 jwkest.jws:DEBUG Picking key based on alg=RS256, kid=None and use=
2017-12-05 10:11:08,198 jwkest.jws:DEBUG Picked: kid:wt25OgyR_nzG3OoQ7daa2rL6-gMnFdfRzBjhUVPu8RQ, use:sig, kty:RSA
2017-12-05 10:11:08,198 jwkest.jws:DEBUG Picked: kid:FOvy, use:sig, kty:RSA
2017-12-05 10:11:08,198 jwkest.jws:DEBUG Picked: kid:9wzy, use:sig, kty:RSA
2017-12-05 10:11:08,198 jwkest.jws:DEBUG Picked: kid:CXup, use:sig, kty:RSA
2017-12-05 10:11:08,200 otest.aus.tool:ERROR [UserInfo] ExcList: Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/otest-0.7.2-py3.5.egg/otest/aus/tool.py", line 86, in run_flow
    resp = _oper()
  File "/usr/local/lib/python3.5/dist-packages/otest-0.7.2-py3.5.egg/otest/operation.py", line 105, in __call__
    res = self.run(*args, **kwargs)
  File "/usr/local/lib/python3.5/dist-packages/oidctest-0.7.0-py3.5.egg/oidctest/op/oper.py", line 322, in run
    user_info = self.conv.entity.fetch_distributed_claims(user_info)
  File "/usr/local/lib/python3.5/dist-packages/oic-0.12.0-py3.5.egg/oic/oic/__init__.py", line 1071, in fetch_distributed_claims
    userinfo_endpoint=spec["endpoint"])
  File "/usr/local/lib/python3.5/dist-packages/oidctest-0.7.0-py3.5.egg/oidctest/op/client.py", line 104, in do_user_info_request
    sender=self.provider_info["issuer"])
  File "/usr/local/lib/python3.5/dist-packages/oic-0.12.0-py3.5.egg/oic/oauth2/message.py", line 699, in from_jwt
    _jw.verify_compact(txt, key)
  File "/usr/local/lib/python3.5/dist-packages/pyjwkest-1.4.0-py3.5.egg/jwkest/jws.py", line 517, in verify_compact
    return self.verify_compact_verbose(jws, keys, allow_none, sigalg)['msg']
  File "/usr/local/lib/python3.5/dist-packages/pyjwkest-1.4.0-py3.5.egg/jwkest/jws.py", line 588, in verify_compact_verbose
    raise BadSignature()
jwkest.BadSignature

2017-12-05 10:11:08,200 otest.aus.tool:ERROR [UserInfo] Exception:

Looks like an issue with the key?

marsangr commented 6 years ago

Signature looks good on my side.

Please remember that with distributed claims the client has an additional step to carry, namely, to fetch anew a public key: the public key of the claims provider. The client cannot use the keyset of the issuer to validate the answer of the claims provider.

If you take a look at https://identityagent.de/.well-known/openid-configuration you'll see that the next JWKS URI is https://identityagent.de/jwks.json

zandbelt commented 6 years ago

Reading up on distributed claims, it seems to be somewhat underspecified: http://openid.net/specs/openid-connect-core-1_0.html#AggregatedDistributedClaims.

The spec doesn't actually mention anything about verification of the JWT that carries the distributed claims so it doesn't automatically imply that one should be able to deduce the key with Discovery through an "iss" value as you'd do with regular OIDC. Though It is not far fetched either.

Implementations may actually rely on the TLS certificate presented on the "endpoint" for authenticity, I've seen at least one implementation doing that, though that's not explicitly specified either. (basically you'd end up with the same security model as obtaining keys from a TLS protected jwks_uri)

@selfissued @rohe any comments to that?

marsangr commented 6 years ago

For me it would be ok if the Client relies solely on the TLS certificate of the new UserInfo for auth/integrity. Sounds plausible. And those Clients who want to go the extra-mile, they could dig the new keys and verify the signatures. I don't think the test should enforce the latter. But the test shouldn't fail (and much less abort) if the JWT of the claims provider isn't signed by the original keys of the issuer (it just cannot).

zandbelt commented 6 years ago

I certainly agree with that last statement.

marsangr commented 6 years ago

Could it be possible to fix this one for the next release 2.0.11?

zandbelt commented 6 years ago

@rohe: can you comment on the signature verification issue?

From what I can see (at least from the logs), pyoidc/oidctest does not actually fetch keys for the issuer of the distributed claims.

marsangr commented 6 years ago

Happy New year to everyone! Any news on this one to begin 2018 with?

zandbelt commented 6 years ago

@rohe and @selfissued could you take look and add your view on it?

marsangr commented 6 years ago

Btw, there are other tests ("extra", not necessary for certification), that do not recognize Distributed Claims either, like OP-claims-IDToken or OP-claims-Split

zandbelt commented 6 years ago

This seems to be a spec issue: it is not clear how the key material for validating a JWT with distributed claims should be obtained.

marsangr commented 6 years ago

Could we at least agree, until the spec issue with the key origin is cleared, that the test shouldn't fail? Just don't enforce that JWT of the claims provider is signed by the original keys of the issuer. You could still check that it's a valid signature (by an unknown key).

zandbelt commented 6 years ago

The implementation of distributed claims in the test suite is incomplete and must be fixed in due time on the pyoidc level. But: the spec is not clear on how the key material is obtained and that should be addressed as well before a proper compliant implementation can be created.

I've raised the issue here: https://github.com/OpenIDC/pyoidc/issues/492

The fact that the test suite doesn't properly support distributed claims must not be a blocker for certification: support for distributed and aggregated claims is optional and the tests that you refer to don't imply/require using distributed claims. Those tests wouldn't fail if the provider returns regular claims which I think is the fastest way forward.

marsangr commented 6 years ago

Thanks for raising this at pyoidc. It looks like the fix for OpenIDC/pyoidc#492 is targeted at 0.14.0 there.

While you are right that distributed claims are an optional part of the specification, they are an integral part of our deployment. If a (mandatory) test in the certification suite asks for some claims, we can't do anything but deliver pointers to the claim providers, and thus trigger the bug.

Considering that pyoidc usually releases fast, we'll just wait for 0.14.0 to be integrated.

tpazderka commented 6 years ago

Disabled the verification of the JWT obtained from distributed claims. I hope that it solves the issue raised here. https://github.com/OpenIDC/pyoidc/pull/522

marsangr commented 6 years ago

From the description it looks like it will work. Also the future OpenIDC/pyoidc#524 matches the behavior of our system. Looking forward to testing it.

zandbelt commented 6 years ago

The disabled verification feature is now rolled out on production as part of OP release 2.0.14. @marsangr please confirm.

marsangr commented 6 years ago

Something it's still not working correctly, cf https://op.certification.openid.net:60381/test_info/OP-claims-essential

The test client follows the redirection to the claims provider, but still is not able to cope with the answer and fails without further details (and a partial result):

5.33 http response url:https://identityagent.de/userinfo status_code:200
5.346 condition UserInfo:OP-claims-essential: status=ERROR, message=
zandbelt commented 6 years ago

You're right: I see only now in the stacktrace that the do_user_info_request method is overridden in oidctest, so we'll need to set the verify=False parameter there as is done in the pyoidc base implementation. That requires a new release.

2018-05-18 13:44:35,701 otest.aus.tool:ERROR [UserInfo] ExcList: Traceback (most recent call last):
  File "/usr/local/lib/python3.5/dist-packages/otest-0.7.4-py3.5.egg/otest/aus/tool.py", line 86, in run_flow
    resp = _oper()
  File "/usr/local/lib/python3.5/dist-packages/otest-0.7.4-py3.5.egg/otest/operation.py", line 105, in __call__
    res = self.run(*args, **kwargs)
  File "/usr/local/lib/python3.5/dist-packages/oidctest-0.7.0-py3.5.egg/oidctest/op/oper.py", line 322, in run
    user_info = self.conv.entity.fetch_distributed_claims(user_info)
  File "/usr/local/lib/python3.5/dist-packages/oic-0.14.0-py3.5.egg/oic/oic/__init__.py", line 1096, in fetch_distributed_claims
    userinfo_endpoint=spec["endpoint"], verify=False)
  File "/usr/local/lib/python3.5/dist-packages/oidctest-0.7.0-py3.5.egg/oidctest/op/client.py", line 104, in do_user_info_request
    sender=self.provider_info["issuer"])
  File "/usr/local/lib/python3.5/dist-packages/oic-0.14.0-py3.5.egg/oic/oauth2/message.py", line 679, in from_jwt
    _jw.verify_compact(txt, key)
  File "/usr/local/lib/python3.5/dist-packages/pyjwkest-1.4.0-py3.5.egg/jwkest/jws.py", line 517, in verify_compact
    return self.verify_compact_verbose(jws, keys, allow_none, sigalg)['msg']
  File "/usr/local/lib/python3.5/dist-packages/pyjwkest-1.4.0-py3.5.egg/jwkest/jws.py", line 588, in verify_compact_verbose
    raise BadSignature()
jwkest.BadSignature
zandbelt commented 6 years ago

a new release 2.1.0 is now live that should fix the remaining issue; you should restart your instance to use it; please confirm (again...)

marsangr commented 6 years ago

Fixed with Release 2.1.0. Thank you very much.