Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consumer.backchannel_logout and handling state #853

Closed
gbip opened this issue Mar 29, 2023 · 11 comments
Closed

Consumer.backchannel_logout and handling state #853

gbip opened this issue Mar 29, 2023 · 11 comments

Comments

@gbip
Copy link
Contributor

gbip commented Mar 29, 2023

Hi there,

I am working on an integration of pyoidc with Django.

While writing a backchannel-logout view, I struggled to have the Consumer find the session ID through my session back-end.

My consumer is initialized like this :

consumer = Consumer(session_db = my_cache_backend)

Which sets Consumer.sdb to my_cache_backend.

However it seems that the implementation of Consumer.backchannel_logout uses Consumer.sso_db instead of Consumer.sdb.

This makes the consumer unable to find the session id since it's store in sdb.

I tried initializing my consumer like this :

consumer = Consumer(session_db = my_session_backend, sso_db=my_session_backend)

But then it breaks the alredy working login mechanism.

I ended up implementing backchannel-logout by setting Consumer.sso_db to Consumer.sdb just for this feature, like this :

consumer.sso_db = consumer.sdb
sid = consumer.backchannel_logout(request_args={"logout_token": body})

I found a test case that seems related : https://github.com/CZ-NIC/pyoidc/blob/f6c590cd8d5834f7e2a2d746ded934549e1fd5f8/tests/test_oic_consumer_logout.py

Could you please explain what are the use-cases for those two SessionBackend ?

@tpazderka
Copy link
Collaborator

The tests seem to suggest that everything is working... I personally do not have any experience with implementing a logout.

Could you please provide a MWE of the failing code?

@gbip
Copy link
Contributor Author

gbip commented Mar 29, 2023

Sure !

Here is a MWE of the failing code, as a unittest test case.

Since testing this logic involves first login the user in and then processing a backchannel logout request I decided to manually fill the SessionBackend with the expected values from a successful login.

I also choose to mock BackChannelLogoutRequest as I did not want to setup fake keys for signing JWT.

To run this sample, please first install jwt as it includes a captured JWT payload form a Keycloak server.

from unittest import TestCase
from unittest.mock import MagicMock, patch

from jwt import JWT

from oic.oic.consumer import Consumer
from oic.utils.authn.client import CLIENT_AUTHN_METHOD
from oic.utils.session_backend import DictSessionBackend


class Issue853TestCase(TestCase):
    @classmethod
    def setUpClass(cls) -> None:
        # Persistent session backend
        cls.session_backend = DictSessionBackend()

        # real JWT payload from a captured keycloak backchannel-logout request
        cls.body = "eyJhbGciOiJSUzI1NiIsInR5cCIgOiAiSldUIiwia2lkIiA6ICJkRVl0UnRmVHpBbXpNZGxlWU5aSWhvQnI3bFNieHBQU3Z4N3Y1eWVCUlU4In0.eyJpYXQiOjE2ODAwOTA5NDUsImp0aSI6ImIzMGFjOTEyLWRhYjctNDkzZC04MGQxLTM2NWU0NmEwOWUyNyIsImlzcyI6Imh0dHA6Ly9rZXljbG9hay5sb2NhbDo4MDgwL2F1dGgvcmVhbG1zL0RlbW8iLCJhdWQiOiJmdWxsIiwic3ViIjoiYWJlYzcwZjgtYTJhMC00YmYwLTg2NjUtYWI5Mzg3YTRkZTAyIiwidHlwIjoiTG9nb3V0Iiwic2lkIjoiNDRlNTliOTYtYTVhMy00NWEwLWI0ZWYtNzY3NzdmNGI3YzI1IiwiZXZlbnRzIjp7Imh0dHA6Ly9zY2hlbWFzLm9wZW5pZC5uZXQvZXZlbnQvYmFja2NoYW5uZWwtbG9nb3V0Ijp7fX19.D4XFSfVp8_T4TwDWLvHx--rs9-aLS8ZbZNPMNWIanoil9gc3N8UczsHJqqTQVQU7BNDQKeMVZQn47I3A1gW9_5WhQa5Si5xgKUmBTs-BoUjLo9Avr7lqAc7zOcGD4ehVLX6gv3PxAlD04snqjEBBW2PZYOZ04u0E--Ssbqd_LAha7ArbgMDG8dIBmJUHvJNMhWERX3QKw5cc3TXcY1TbZ-xDdkBf28DJ19ryXMHn0PybH927ZsDGX-2vxltDFRbIhotPXfoAbfZl8_TA84tn58zKcWKNd5aMtZ5Mu0D_SHPcNNpbYR3WzMlQZ6E0_8io5_buUBehaKZTryL30rK56w"

    def test_issue853(self):

        consumer_config = {
            # "debug": True,
            "response_type": "code",
        }

        client_config = {
            "client_id": "test",
            "client_authn_method": CLIENT_AUTHN_METHOD,
        }

        consumer = Consumer(
            consumer_config=consumer_config,
            client_config=client_config,
            session_db=self.session_backend,
        )

        with patch("oic.oic.consumer.BackChannelLogoutRequest") as mock:
            # Load jwt as python dict
            decoded = {"logout_token": JWT().decode(self.body, do_verify=False)}

            # Simulate previously made login : our session id is 0000
            real_sid = "0000"
            self.session_backend.update(
                real_sid, "smid", decoded["logout_token"]["sid"]
            )
            self.session_backend.update(real_sid, "sub", decoded["logout_token"]["sub"])
            self.session_backend.update(
                real_sid, "issuer", decoded["logout_token"]["iss"]
            )

            # Mock 'BackChannelLogoutRequest' as we don't want to check signature and stuff
            mocked_logout_request = MagicMock()
            mocked_logout_request.__getitem__.side_effect = decoded.__getitem__
            mock.return_value = mocked_logout_request

            # Uncomment the following line to have the test pass :
            # consumer.sso_db = consumer.sdb

            sid = consumer.backchannel_logout(request_args={"logout_token": self.body})

            self.assertIsNotNone(sid)
            self.assertEqual(sid, real_sid)

@tpazderka
Copy link
Collaborator

OK, not sure what to make out of this...

The split between sdb and sso_db seems to be a deliberate decision made by @rohe during the implementation of logout support. The sso_db is only updated when the client received an id_token response (as is mentioned in the specs. So the separation probably makes sence, as there should be no session without the id_token?

What flow are you using in your consumer? Could you verify that the logout is working correctly if you use id_token?

@gbip
Copy link
Contributor Author

gbip commented Mar 29, 2023

Ok, I think I might get what is going on. I start from a fresh Consumer when processing a back-channel logout request.

I could try to restore an existing consumer, however to call Consumer.restore I need the session ID.

Maybe I should process myself the JWT first, call Consumer.restore, then call Consumer.backchannel_logout. I'll try that now.

@gbip gbip changed the title Consumer.backchannel_logout and sso_sdb Consumer.backchannel_logout and handling state Mar 29, 2023
@gbip
Copy link
Contributor Author

gbip commented Mar 29, 2023

I am not sure how one is supposed to handle Logout JWT from an Authorization server using this library.

On one side, in an authorization code flow, the Consumer instance is saved using the value of state from an Authorization Response as the key.

On the other side, when I receive a Logout JWT I need to find back the consumer instance since the session details where stored in sso_db.

Howevere I don't have any to find back the state value used to store the session.

I could rely on the fact that my Keycloak server sends a session_state in the Authorization Response and store the mapping between state and session_state.
Since the Logout JWT might have sid attribute I could use it to find back the state value and thus the consumer instance.

But it seems that this behaviour is in the grey zone of the spec.

@tpazderka
Copy link
Collaborator

Well, the LogoutToken must contain one of the sub or sid. So that should be your way how to figure out the session?

The SessionDB has get_by_sub method so you should be able to do that.

@gbip
Copy link
Contributor Author

gbip commented Mar 30, 2023

Well, if we receive a Logout JWT with sid we are screwed because it won't match the key used to save the Consumer instance.
I'm kind of reluctant to adding more state to our integration (such as to keep the mapping between state <-> sid), as keeping this kind of mapping between domain-level identifier is very prone to bugs.

However for a Logout JWT with sub I agree with you, the following algorithm should work :

  1. Decode JWT using a third party lib, and extract the sub value
  2. Fetch the key used to save the Consumer instance, using get_by_sub and call Consumer.restore to have an initialized sso_db
  3. Pass the undecoded JWT string to Consumer.backchannel_logout

Would you be open to a PR that uses session_state from the Authorization Response as the Consumer key in the SessionDB (if available) ? It would allow to use the following for implementation :

JWT Logout field present Description state was used as the Consumer ID session_state was used as the Consumer ID
sub The Authorization server want to kill all session use get_by_sub to fetch Consumer ID use get_by_sub to fetch Consumer ID
sid The Authorization server wants to kill one session Can not happen by the spec : > When the OP supports session management, it MUST also return the Session State as an additional session_state parameter in the Authentication Response Spec Call Consumer.restore with the sid

@gbip gbip closed this as completed Mar 30, 2023
@gbip gbip reopened this Mar 30, 2023
@tpazderka
Copy link
Collaborator

Hm, not sure if I completely follow...
But it looks to me, that it should be sufficient to store session_state instead of sid in here?

If that is what you were suggesting, than please go ahead. If not, than I might need more info :)

@gbip
Copy link
Contributor Author

gbip commented Apr 3, 2023

Hm, not sure if I completely follow... But it looks to me, that it should be sufficient to store session_state instead of sid in here?

If that is what you were suggesting, than please go ahead. If not, than I might need more info :)

Not really, I was thinking about patching this so that the client is either saved using session_state (new behaviour) or state (current behaviour) depending on the Authorization Server response.

I'll start a quick MR to show you.

EDIT : Here is a draft PR that shows what I'd like to do.

@tpazderka
Copy link
Collaborator

OK, sounds reasonable. Please move the kwarg to the end as not to change the order for existing uses.

@tpazderka
Copy link
Collaborator

Closed via #854.

gbip added a commit to makinacorpus/django_pyoidc that referenced this issue Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants