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-BackChannel-RpInitLogout error - Not for me! #224

Open travisspencer opened 4 years ago

travisspencer commented 4 years ago

In the test OP-BackChannel-RpInitLogout, I am getting the fault Not for me!. I think this is due to configuration error, but I can't figure out what the right configuration is.

This test, as you probably know better than me, runs the code flow (or the like), and then does a logout. The logout has a post-logout redirect URI which the OP under test should redirect to when logout is done. After doing so, the test prints an error if it didn't get its back-channel logout call.

The post-logout redirect URI is configured in the policy of my OP for the tool's static client used by this test. Specifically, it's https://op-test:60007/logout. Also, the back-channel logout URI is also configured. It is https://op-test:60007/backchannel_logout?entity_id=client-one. I was using https://op-test:60007/backchannel_logout because of info I found on the OIDF web site. That led to a server 500 error in the test tool though with a stack trace:

[30/Apr/2020:17:59:01] HTTP
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/CherryPy-8.9.1-py3.6.egg/cherrypy/_cprequest.py", line 670, in respond
    response.body = self.handler()
  File "/usr/local/lib/python3.6/dist-packages/CherryPy-8.9.1-py3.6.egg/cherrypy/lib/encoding.py", line 220, in __call__
    self.body = self.oldhandler(*args, **kwargs)
  File "/usr/local/lib/python3.6/dist-packages/CherryPy-8.9.1-py3.6.egg/cherrypy/_cpdispatch.py", line 60, in __call__
    return self.callable(*self.args, **self.kwargs)
  File "/usr/local/lib/python3.6/dist-packages/oidctest-0.9.1-py3.6.egg/oidctest/optt/__init__.py", line 405, in backchannel_logout
    if kwargs['entity_id'] != self.tester.conv.entity.entity_id:
KeyError: 'entity_id'
172.22.0.1 - - [30/Apr/2020:17:59:01] "POST /backchannel_logout HTTP/1.1" 500 823 "" "Apache-HttpClient/4.5.9 (Java/1.8.0_242)"

That's why I added entity_id to the query string. What should this be though? I suppose it should be the client ID, but this isn't working. It seems to fail here in the Python code:

def backchannel_logout(self, **kwargs):
    logger.debug('Back channel logout: {}'.format(kwargs))
    self.sh['conv'].events.store(EV_HTTP_REQUEST, kwargs)
    if cherrypy.request.process_request_body is True:
        _request = as_unicode(cherrypy.request.body.read())
        if _request:
            logger.info('back_channel logout request: {}'.format(_request))
            if kwargs['entity_id'] != self.tester.conv.entity.entity_id:
                self.sh['conv'].events.store(EV_FAULT, "Not for me!")
                logger.debug('Not for me')
                self.opresult()   

What is self.tester.conv.entity.entity_id though. I also tried https://op-test:60007 but that wasn't it. Any clue?

BTW, I am running 10c6d9349a4fb60d9956650e457574533666c4af tools/oidctest (v1.2.5-1-g10c6d93) of the test tool.

zandbelt commented 4 years ago

@rohe can you advise? the entity_id seems to be dynamically generated but I'm not sure that can be used in a static setup

travisspencer commented 4 years ago

I can update the static client's config at the start of the test -- that's no problem. I just need to know what the value should be.

rohe commented 4 years ago

It is indeed generate dynamically. Which as you say means it won't work in a static setup. The value is a random string so as things stands now you can't add it to the configuration. I have to change things in the code to make that possible.

travisspencer commented 4 years ago

We can pull the random string out of the Docker container as our test starts, and then push this into our OP. We do this kinda thing to get the public key used by the static client to sign JWTs for authentication. We can't do this, obviously, in the certification environment. In the case of JWT authentication, that's OK because it's an extra test, so we run that in our lab but not during certification. For this one, however, it's a mandatory logout test, so we'll need some way to configure this in the test spec setup. In the meantime, is it possible to grab the random value from the Docker container somehow?

For reference, this is what we do in the JWT test, OP-ClientAuth-PrivateJWT:

docker cp $containerId:/usr/local/src/oidf/oidc_op/keys/$filename /tmp/keys/

Then, we can use the keys, sig.key.pub, from the local machine's /tmp/keys dir and delete it after test execution.

Could we do something similar here till there's a config setting or other workaround for certification environment testing?

rohe commented 4 years ago

Well, the entity_id is jus kept in memory so there is no easy access. Unless you would add some lines in the code that would print it to a file. It would be after this line https://github.com/openid-certification/oidctest/blob/0448e1b71fc3a238173e99f5840de75b3e41eab3/src/oidctest/op/func.py#L811

Like this:

 with open('workfile') as f:
     f.write(str(entity_id))
travisspencer commented 4 years ago

Even after doing that, the entity IDs don't match; something like whitespace or encoding, I'm guessing. Gave up trying to debug it as I couldn't get logging or print to a file or attaching from PyCharm to work in my Dockerized setup 😞 So, I've disabled static clients for this test for now.

Ultimately, I think the fix is for the test tool to use the static client's ID for this, and not require the OP to register a back-channel logout URL with an entity_id query string argument.

rohe commented 4 years ago

Sorry about that. I'll look at this first thing next week.

rohe commented 4 years ago

So, I could make the entity_id == client_id if a client_id exists. I've seen some strange static client_ids being use so we would have to url encode it, but that's not a biggie.

rohe commented 4 years ago

The way the test tool is written the entity_id must be unique over the lifetime of a test instance. The client_id doesn't seem to be. client_id + rp_id is probably a better choice.

travisspencer commented 4 years ago

Then, the OP has to be configured with something extra? What is RP ID? What would my OP have to do to send something that the test tool accepts if this tact is used?

rohe commented 4 years ago

Sorry, my confusion. What I meant was the issuer ID. But need to think a bit more and do some testing.

rohe commented 4 years ago

@travisspencer When you say static do you mean that the client does neither provider info discovery nor dynamic client registration ?

rohe commented 4 years ago

So what about this. This is a thought experiment.

You choose to construct a new test instance.

On the first page you fill in Issuer and Tag, leave out Webfinger, Dynamic Provider Information Discovery and Dynamic Client registration. You chose response type and click create

On the second page you change back from False to True. You may do other things too but let's leave it at that for the time being.

Now, you chose Save&Start and you will get to a page that let's you continue on to the test instance. Now, instead you go back to the Test instance management page, choses List and finds the instance you just created. Clicks on it and gets to a page where you can see the status of the test instance and you can chose to Restart, Stop, Reconfigure or Delete the instance. If you chose Reconfigure and on the page you get to scrolls down a bit you should now be able to see the backchannel_logout_uri.

It's a bit awkward but would this work for you ?

travisspencer commented 4 years ago

Ya, "static" is a bit of jargon we use around our shop for "not dynamically registered" (static being the opposite of dynamic). In the tests, we always enable OP info discovery and WebFinger, but I don't think they are used for static clients. It's less differences between the dynamic tests anyway, so we do it even if not needed.

Your idea could be done manually during certification (which is good), but setup of each of the test combo (we have 12 ATM) takes a bit of time already. Extending this would not be preferred. ATM, on our CI server (which is pretty beefy), the tests take 3 hours and 43 minutes. (This long time is largely due to setup issues -- TLS errors as the test instances start up which causes pauses and retries, setting up SSO sessions to avoid numerous logins, etc.).

The other issue is around automation. We've been able to make things work, but it isn't easy. Part of the problem is just that there's always issues with Selenium. What you're propossing doesn't sound very hard though since the reconfigure page is always the same -- https://op-test:60000/action/$ID?action=configure. It's a small bit of work, I imagine.

If the only way to solve this is to do this bit of work and increase our test time, then so be it. We feel that testing static clients is important because so many people use this technique. Just saying, it isn't ideal for these couple of reasons.

rohe commented 4 years ago

OK, I'll think a bit more :-)

Yeah, Selenium is a bitch.

As with redirect_uris which follow a well-known format I guess we should be able to use the same pattern for front/backchannel_logout_uri. I just have to figure out why I didn't do it that way to start with. :-/

If it can be done that way you would still not see it until you looked a second time at the configuration. But since it would always follow the same pattern you wouldn't have to look.

travisspencer commented 4 years ago

In the test setup, I have this essentially: updateBackChannelLogoutOpConfigForClient("MyGoodClient", "${testStartPage}backchannel_logout")

where testStartPage is the certification test list page (e.g., https://op-test:60001/). The issuer is always the same and I have the tag as well. So, I should have all that's needed to add something to the query string of the above if required.

Would that do? Can I simply update that to include ?entity_id=???? with the issuer and tag, like

updateBackChannelLogoutOpConfigForClient("MyGoodClient", "${testStartPage}backchannel_logout?entity_id=${Constants.ISSUER}-${tag}")

travisspencer commented 4 years ago

We have also learned that this is an issue in the OP-FrontChannel-RpInitLogout as well. If we set the client's front-channel logout URL to https://op-test:60001/frontchannel_logout?entity_id=foo, then we get the Not for me error. Without this though, we get a 500 from the RP's iframe.

For now, we have disabled that test for static clients as well.

rohe commented 4 years ago

I think that we just will get rid of entity_id. Working on it.