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

[micro-service] Set target-issuer from disco response and decide backend by target-issuer #220

Merged
merged 4 commits into from
Jul 16, 2021

Conversation

peppelinux
Copy link
Member

@peppelinux peppelinux commented Apr 14, 2019

Hello,
I developed a microservice that can map specific SaToSa backends to specific target entity id. A configuration example can be this:

module: satosa.micro_services.custom_routing.DecideBackendByTarget
name: TargetRouter
config:
  target_mapping:
    "http://idpspid.testunical.it:8088": "spidSaml2"
    "http://strangeIDP.testunical.it:8081/saml2/metadata": "strangeSaml2"

In the debug logs if the microservice has been activated we can read

[2019-04-14 23:01:29] [DEBUG]: [urn:uuid:acedce81-9287-4700-bd96-15b7adaeab5a] Routing path: Saml2/disco
[2019-04-14 23:01:29] [DEBUG]: [urn:uuid:acedce81-9287-4700-bd96-15b7adaeab5a] Found DecideBackendByTarget (TargetRouter microservice ) redirecting Saml2 backend to spidSaml2

I needed a backend routing based on the target entity ID because I have some SAML2 IDP that only accepts highly customized authn request and metadata. An example could be SPID italian federation. Another example could be the need to use different configurations, like sign and digest algorithms, depending by target IDP.

I was looking into DecideBackendByRequester microservice but soon I realized that it was made for different goals, in it the subjects are the requester entity ID and not the target entity ID.

This PR also answers to my related issue.

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@peppelinux peppelinux force-pushed the DecideBackendByTarget branch 3 times, most recently from 03afea4 to 603c03f Compare April 9, 2021 23:57
@peppelinux peppelinux force-pushed the DecideBackendByTarget branch from 15e15da to f22d38c Compare April 11, 2021 16:40
@peppelinux peppelinux force-pushed the DecideBackendByTarget branch from 6c7f70e to d46c86f Compare July 13, 2021 13:43
@c00kiemon5ter c00kiemon5ter force-pushed the DecideBackendByTarget branch 2 times, most recently from 969a2ba to 4341437 Compare July 13, 2021 23:08
@c00kiemon5ter
Copy link
Member

c00kiemon5ter commented Jul 13, 2021

@peppelinux with the latest changes we now have two micro-services, with the test rewritten to test both services.

  • DiscoToTargetIssuer registers a handler that parses a response from the discovery service and sets the target_entity_id on the context.
  • DecideBackendByTargetIssuer sets the target_backend based on the target_entity_id and the mapping between issuers and backends

Nothing else is done, but what is necessary for these goals. The responsibilities are clear.


DiscoToTargetIssuer should run before DecideBackendByTargetIssuer.


Also, squashed and rebased on top of current master.

@c00kiemon5ter c00kiemon5ter force-pushed the DecideBackendByTarget branch from 4341437 to 612e205 Compare July 13, 2021 23:18
@c00kiemon5ter c00kiemon5ter force-pushed the DecideBackendByTarget branch from 612e205 to 4ec361c Compare July 13, 2021 23:19
@c00kiemon5ter c00kiemon5ter changed the title [Micro Service] Decide backend by target entity ID [micro-service] Set target-issuer from disco response and decide backend by target-issuer Jul 13, 2021
@peppelinux
Copy link
Member Author

I have to test It, probably tomorrow

@peppelinux
Copy link
Member Author

peppelinux commented Jul 15, 2021

@c00kiemon5ter I MUST set context.target_frontend otherwise I get the following exception when the Response came back to ACS endoint

SATOSA/src/satosa/routing.py", line 108, in frontend_routing
    frontend = self.frontends[context.target_frontend]["instance"]
KeyError: None

I made my tests, It seems to me that's quite good as it is now

When the processing of the request micro-services is finished, the
context is switched from the frontend to the backend. At that point
target_frontend is needed to set the state of the router. The router
state will be used when the processing of the response by the response
micro-services is finished, to find the appropriate frontend instance.

---

The routing state is set at the point when the switch from the frontend
(and request micro-service processing) is made towards the backend. If
the discovery response was not intercepted by the DiscoToTargetIssuer
micro-service, and instead was processed by the backend's disco-response
handler, the target_frontend would not be needed, as the routing state
would have already been set.

When the DiscoToTargetIssuer micro-service intercepts the response, the
point when the switch from the frontend to the backend happens will be
executed again. Due to leaving the proxy, going to the discovery service
and coming back to the proxy, context.target_frontend has been lost.
Only the state stored within context.state persists (through the
cookie).

---

When the request micro-services finish processing the request,
backend_routing is called, which sets the router state
(context.state['ROUTER']) to target_frontend, and returns the
appropriate backend instance based on target_backend. When the time
comes to switch from the backend to the frontend, that state is looked
up (see below).

When the response micro-services finish processing the response,
frontend_routing is called, which sets target_frontend from the router
state (context.state['ROUTER']) and returns the appropriate frontend
instance based on target_frontend.

---

Signed-off-by: Ivan Kanakarakis <[email protected]>
@c00kiemon5ter c00kiemon5ter force-pushed the DecideBackendByTarget branch from 5c8eaeb to d7e4572 Compare July 16, 2021 19:18
@c00kiemon5ter
Copy link
Member

c00kiemon5ter commented Jul 16, 2021

Hello @peppelinux, I made another update, effectively moving the responsibility to set the target_frontend to the disco micro-service. If the discovery response were not intercepted, there would be no reason to set the target_frontend on DecideBackendByTargetIssuer.

The reason we need to set target_frontend is because it is needed to set the router state when switching from the frontend (and the response micro-services processing) to the backend (on _auth_req_finish). The discovery request and response used to be handled by the backend, and at that point the router state had been set already. With the interception of the discovery response, we force some stages to run again, and those include the switch from the frontend to the backend.

Because we have navigated away from the proxy (we redirected to the discovery service) the data on context has been lost (including the target_frontend) except for the state (persisted on the cookie). Now that we need to go through the switch to the backend again - but without a target_frontend - the error occurs. To fix it, we make sure that we store the target_frontend on the cookie, and restore it once we have a response from the discovery service.

This process does not involve the selection of the backend. Thus this is the responsibility of the DiscoToTargetIssuer to handle.

Ideally, DiscoToTargetIssuer would also create the request to the discovery service. Then this whole process would become apparent that it is needed to be done that way. We will look into doing this in the future.

@c00kiemon5ter
Copy link
Member

Please, have a look and test it. Then we should merge ;)

@peppelinux
Copy link
Member Author

Ok, everything works great even with idp_hinting ... Three year passed meantime but it was worth of it, let's merge it now!

@c00kiemon5ter c00kiemon5ter merged commit 961dd99 into IdentityPython:master Jul 16, 2021
@peppelinux
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants