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

new: add microservice to expose info about IdP from SAML metadata as attributes #448

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vladimir-mencl-eresearch
Copy link
Contributor

Primary goal was to expose registration_authority for services connected to eduGAIN.

Generalised to expose all possibly useful info available from MDStore.

Notable decisions:

  • while registration_authority is included in registration_info data structure, exposing it directly should make it easier for applications to consume and process the value.
  • to expose scopes, it was necessary to revert the regex_compile step done when regexp="true" (as Pattern objects are not seriazable)
  • tuple objects are not serializable and had to be converted to a list
  • mod_auth_openidc does not accept lists of complex types as claim value, assuming this might also apply to other RP implementations, wrap the listt a dict to match other attributes.

Please let me know what you think @c00kiemon5ter - happy to add tests once I get the overall OK.

All Submissions:

  • 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?

…attributes

Primary goal was to expose registration_authority for services connected to eduGAIN.

Generalised to expose all possibly useful info available from MDStore.

Notable decisions:
* while registration_authority is included in registration_info data structure,
  exposing it directly should make it easier for applications to consume and process the value.
* to expose scopes, it was necessary to revert the regex_compile step done when regexp="true"
  (as Pattern objects are not seriazable)
* tuple objects are not serializable and had to be converted to a list
* mod_auth_openidc does not accept lists of complex types as claim value, assuming this might
  also apply to other RP implementations, wrap the listt a dict to match other attributes.
@vladimir-mencl-eresearch
Copy link
Contributor Author

@c00kiemon5ter , just wondering if I could get your thoughts on this - at least whether you see this extension as useful in principle (and find the approach suitable). If so, I'd add tests and finalise the PR.

@c00kiemon5ter
Copy link
Member

Thank you for being patient @vladimir-mencl-eresearch

I don't think we should follow this approach. Mixing attributes with metadata is confusing and can lead to false assumptions and issues. Additionally, the information is about the metadata of the issuer; there is no place for the metadata of the service, which is also important for certain decisions.


Instead, I think that we should follow what was done with the metainfo plugin here. The idea is that a new slot is set on the InternalData instance which holds the metadata of the requester and of the issuer that are involved in this flow separately (with some conventions for OIDC entities).

In addition to the above, I liked the notation that was set on #438 to signify the language of a value, without using a hierarchy (<attr>#{language}).

The metainfo plugin needs some further work and some considerations to be taken into account. For example

  • the is_oidc_client function should change as the convention that is checked there does not generally hold.
  • the keys that are retrieved from the Context object should be verified, as there have been changes on the backends and frontends
  • the metadata values that are retrieved need to be filtered; the metadata property, being part of the InternalData, will be serialized as part of the cookie that holds the state. If it is not properly managed, as some values can be very large, the cookie will exceed the allowed limits and will be invalidated.

Ideally, I'd like to push this responsibility to the backends and frontends. Those modules would prepare the metadata and pass it on to satosa-core to be exposed to the plugins. That would mean that we need an API to call into these modules and an internal format for the metadata structure.

@vladimir-mencl-eresearch
Copy link
Contributor Author

Hi @c00kiemon5ter ,

Thanks for the response and review - very much appreciated. This is also why I sent the PR in at an early stage to get your opinion, to allow me to shape it properly.

I'm trying to get what approach you'd prefer instead.

Looking at the metainfo plugin, it is to a degree similar to what this PR does - a ResponseMicroService that extracts useful bits out of the metadata and stores it somewhere. It differs in where it gets stored - my approach puts it into attributes, metainfo puts it into a separate part of the internal state. (And it so far misses RegistrationAuthority which is critical in my use case).

The part I don't get so far is how would get used further on in the attribute state. What I'm aiming to do is to expose the info (in particular RegistrationAuthority) to OIDC clients - so that when the SATOSA SAML SP is connected to eduGAIN, they can tell which federation the user came from (or can at least distinguish between local federation and an eduGAIN user).

There could be a separate microservice that would map the metadata state to attributes ... but that would be getting rather close to my original PR. This approach would have the advantage that it gets only mapped to attributes when explicitly requested so - but the same could be said about my original PR. Well, the split approach would allow to use the metadata info without polluting the attributes.

What are your thoughts - should I be heading this way? Separate service like metainfo (or reusing it directly) to store the metadata info in internal state, and separate service for copying the internal state into attributes?

Please let me know.

Cheers,
Vlad

@c00kiemon5ter
Copy link
Member

And it so far misses RegistrationAuthority which is critical in my use case

it should be part of the registration info

The part I don't get so far is how would get used further on in the attribute state.

either you create a new micro-service an map the metadata values that you want to attributes/claims, or this is offloaded to the frontend that will pick up this information from the internal data and use it as needed.
For the case you describe, the possible communication channels for this information is

  • the id-token
  • the access-token (if it is a JWT)
  • the userinfo endpoint response
  • the introspection endpoint response
    How this information is communicated depends also on the semantics you are giving it - ie, if it is used to allow access to resources then it is part of an authorization policy, thus part of a jwt access-token or the introspection response, etc.

What are your thoughts - should I be heading this way? Separate service like metainfo (or reusing it directly) to store the metadata info in internal state, and separate service for copying the internal state into attributes?

Yes, I think they are different concerns and should be separate.

Further down the road, if the metadata is provided by the backends and frontends and is part of the InternalData object before the processing of the micro-services/plugins, then the micro-service that maps metadata to attributes/claims should keep working unaffected.

@vladimir-mencl-eresearch
Copy link
Contributor Author

Hi @c00kiemon5ter ,

Thanks for the patient discussion!

This all started with one of our members asking the question:

So, if you connect our service (via SATOSA OIDC/SAML proxy) into eduGAIN, how do we tell whether the user is from our own federation or remote?

Which very much makes sense to me - and I've been looking for ways to expose this information (registrationAuthority) to them. (And sorry, I missed registrationInfo was covered by the metainfo plugin - can see it now).

Given the OIDC client/RP is a Keycloak instance and the code evaluating the user information will be further down, I thought of additional attributes as the best way forward.

I took inspiration from Shibboleth SP's MetadataAttributeExtractor which does exactly this: extracts values from metadata of the IdP issuing the SAML response and adds them to the set of attributes received. The attribute names can all be assigned into a separate "namespace" by prefixing the attribute name with a value set in metadataAttributePrefix.

One option for tweaking this PR would be to prefix all attribute names with a prefix indicating these come from the metadata (changing it in the example config).

When you suggest to expose this information in id-token / access-token / userinfo endpoint, in what form did you mean that? Other part of the structure then the list of attributes? Anything specific?

Thanks again for the patience in this discussion.

Cheers,
Vlad

@c00kiemon5ter
Copy link
Member

When you suggest to expose this information in id-token / access-token / userinfo endpoint, in what form did you mean that? Other part of the structure then the list of attributes? Anything specific?

As a claim in the token, or a claim in the userinfo response (or, a claim in the introspection response). There is no other structure there than the claims.
Which of those mechanism you are going to use depends on the semantics given by the registration-authority information:

  • what will the services do knowing whether the user came from within the federation?
  • can the same user come both from inside and outside the federation? (ie, through a community) (is it considered the same user?)

Try to understand if the knowledge of this property gives the user access to more resources (thus, it is authorization information / access-token and introspection), or it is an unstable property of the current flow (thus, it is session information / id-token), or if it is part of the user identity (userinfo).

To be clear, I don't think that what you are trying to do is wrong. What I do not like is mixing the metadata extraction process, with the metadata-to-attribute mapping process. Especially if the metadata extraction process is going to move outside of the micro-services.
The metadata itself can be used for other purposes too. Thus, it makes sense to have a plugin that makes them available for processing, and have a separate plugin to map them in the attributes struct, and have a separate plugin to evaluate whether certain attributes should be dropped because of some signal in the metadata, etc.

@vladimir-mencl-eresearch
Copy link
Contributor Author

Thanks @c00kiemon5ter .

You are overseeing the overall architecture of SATOSA - so thanks for keeping an eye on that.

I agree the approach with separate services for extracting metadata info and for mapping it to attributes is a better choice.

To get this to usable state, I understand I'd have to write the second half as new code. As for the first half, do you know what the position/state of the metainfo plugin and the swamid-satosa code/repo is?

Should I import the code from there (crediting original authors ... so you :-) - or should it stay in that repo? Is that repo actively maintained / would accept PRs and publish releases ?

I expect it would look odd to have the part mapping the internal state metadata info to attributes in SATOSA but the metainfo plgun populating this state only in the separate swamid-SATOSA repo...

Thanks a lot for this discussion - I hope we can converge to a workable plan.

Cheers,
Vlad

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