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

OP-Req-max_age=1 registers twice #179

Open zandbelt opened 5 years ago

zandbelt commented 5 years ago

It appears that OP-Req-max_age=1 differs slightly from OP-Req-max_age=1000 in that it registers twice:

---> Registration",
    "AsyncAuthn",
    {
      "AccessToken": {
        "conditional_execution":{
          "return_type": ["CIT","CI","C","CT"]
        }
      }
    },
    "Note",
    {
      "Webfinger": {
        "set_webfinger_resource": null
      }
    },
    {
      "Discovery": {
        "set_discovery_issuer": null
      }
    },
--->    "Registration",
    {
      "AsyncAuthn": {
        "set_response_where": null,
        "set_request_args": {
          "max_age": 1
        }
      }
    },
    {
      "AccessToken": {
        "conditional_execution":{
          "return_type": ["CIT","CI","C","CT"]
        }
      }
    }

whereas OP-Req-max_age=1000 only registers once:

---> "Registration",
    {
      "AsyncAuthn": {
        "set_response_where": null
      }
    },
    {
      "AccessToken": {
        "conditional_execution":{
          "return_type": ["CIT","CI","C","CT"]
        }
      }
    },
    {
      "AsyncAuthn": {
        "set_response_where": null,
        "set_request_args": {
          "max_age": 10000
        }
      }
    },
    {
      "AccessToken": {
        "conditional_execution":{
          "return_type": ["CIT","CI","C","CT"]
        }
      }
    }

apparently the difference triggers a bug in the test suite when using encrypted id_tokens .

My hypothesis is that the problem be avoided by not registering twice (as sort of a workaround for a bug when using double registration).

@rohe I'd like to know where the difference comes from and if we can/should get rid of it.

rohe commented 5 years ago

The difference is expressed in the test specification. Compare test_tool/cp/test_op/flows/OP-Req-max_age=1.json with test_tool/cp/test_op/flows/OP-Req-max_age=10000.json. So you can easily change the later to not have 2 registrations.

zandbelt commented 5 years ago

ok, but the question was more about why there are 2 registrations in one and 1 in the other.

rohe commented 5 years ago

OK, so it's a matter of what we're emulating here. In the first case (max_age=1) it's getting an authorisation request from a second RP. In the second case we only have one RP involved. It really shouldn't make any difference for the outcome of the tests. Since the tests is about how long a go the last authentication of a specific user was made.

zandbelt commented 5 years ago

that confirms my understanding and I expect that using a single RP for both will make the problem at hand (with the one that uses 2) go away...

rohe commented 5 years ago

So what was the problem with encrypted ID Tokens ?

zandbelt commented 5 years ago

that the 2nd encrypted id_token seems to be encrypted with the client secret of the 1st (at least that's the hypothesis since that's the diff between the max age 1 and max age 1000 tests)

panva commented 5 years ago

eh? max_age=1 doesn't deal with encryption. Even if /the dynamic registration had a policy and returned encryption properties/, it doesn't send an id_token_hint in the requests.

panva commented 5 years ago

I checked the pdf of the tester, the tool does not ask for encryption and their dynamic registration response doesn't signal id_token_encrypted_response_alg and/or _enc in the client metadata, so for one, the tool should err if it encounters an encrypted id token.

That being said the first token endpoint response gets decrypted okay, second one isn't probably because of the first cached client secret-derived encryption key (A128KW).

rohe commented 5 years ago

OK, I can see the problem. Can fix. Will not happen today :-)