simplesamlphp/saml2

eIDAS compatibility: samlp:Extensions with RequestedAttributes and SPType

smarek opened this issue · 13 comments

smarek commented

As of specifications for eIDAS eID Profile from EC: https://ec.europa.eu/cefdigital/wiki/display/CEFDIGITAL/eIDAS+eID+Profile

Within AuthnRequest there should be samlp:Extensions included, with eidas:SPType attribute (ServicePoviderType) and eidas:RequestedAttributes with each attribute SeP requests from IdP in SSO process.

Example XSD for said data, if not taken from linked documentation (eIDAS message format since version 1.0) can be seen here: https://github.com/france-connect/eidas-identity-provider/blob/master/EIDAS-SAMLEngine/src/main/resources/eidas/saml_eidas_extension.xsd

I've worked on implementing the SeP with your saml2 library, because whole SimpleSamlPHP is too heavy for my SeP expectations, and I've concluded with library, that extends saml2 with said data types and process to include them in AuthRequest

You can see that here: https://github.com/otevrenamesta/simplesamlphp-saml2-eidas and specifically https://github.com/otevrenamesta/simplesamlphp-saml2-eidas/blob/master/src/SamlpExtensions.php which takes SAML2\AuthnRequest or DOMElement as basis, and allows to configure RequestedAttributes and correct values for SPType

Would it be possible, that eIDAS data types would be included within your SAML2 library directly?

thijskh commented

Hi, I think we’d accept adding such an extension element. But please submit your request as a pull request. We can then see concretely what we’d think of the changes. Thanks.

smarek commented

Thank you, i'm not sure how to provide that, because I don't really understand the structure of your codebase.

Since https://github.com/simplesamlphp/saml2/blob/master/src/SAML2/XML/md/RequestedAttribute.php is using md: namespace exclusively, I'm probably need to create whole new namespace EIDAS as a folder and constant NS_EIDAS, and somehow update SAML2\XML\samlp\Extensions to support new implementation probably?
Also RequestedAttributes/RequestedAttribute are easily confused with for example SAML2\XML\md\RequestedAttribute, should I use prefix naming such as EidasRequestedAttribute or similar?

I'd be grateful for some starting points

thijskh commented

Sorry, maybe I misunderstood the wish.

The ability to add custom extensions, as defined in the saml spec, to an AuthnRequest already exists in the library. So I think you can already do this in your code without needing to change the saml2 lib itself (with getExtensions/setExtensions). Not sure if you're using that currently. Are those working for your use case? They should given that this is part of the saml spec so saml2 should implement it.

Then there is the second question of whether profile specific things like an eidas namespace should be in the saml2 lib. I'm doubtful about that. The purpose is to be a generic saml2 library and the saml2 spec allows specifically to add extensions in any format and schema to a message. So you can create only the desired extension element yourself and add that to your authnrequest. So I think you should be able to achieve what you want without requiring saml2 to know about anything eidas specific. Correct?

smarek commented

  1. I explored this option, but I did not find a way to implement it correct, from AuthnRequest addExtension expects XML\samlp\Extensions which is only helper class with addList/getList functions, and setExtensions expects array of the same type, so I guess there is missing comment about library supporting custom extensions, if provided by subclassing Chunk class?

  2. Yes, if your library is intended as general-purpose only, I can see your doubt, however eIDAS (due to European Union/Commission) is large user base, growing each day in whole EU, and I do also see the opportunity in supporting that segment by default.

smarek commented

I've tried to prepare for integration/compatibility, now there are EidasRequestedAttributes, EidasRequestedAttribute and EidasSPType chunks in https://github.com/otevrenamesta/simplesamlphp-saml2-eidas/tree/master/src/Chunks

However I'd still appreciate your guidance, on how you expect the extensions to be integrated. I'm unable to find any existing integration.

tvdijen commented

Hi @smarek !
We are currently working on a rewrite / new major version of this library and I think it will make your life much easier.. It's still a work in progress, but I hope you can see where we're going..

To add the eidas-extension you would have to:

@jaimeperez have I missed something?

smarek commented

Hi @tvdijen thank you, that's certainly great news. I haven't been watching your work closely, but I'll do now to update the compatibility. Do I understand corectly, the new version you mention, is tag v4.x ?

Also implementation instructions clear to me, please just clarify, if you want me to provide PR to provide eIDAS functionality as built-in to saml2 library, or if you want me to publish compatible library, that would user install along saml2 library.

tvdijen commented

We are actually working on v5.x already, which is only in master and some of the last open PRs..

We'll get back on your second question.. We're leaning towards an external repository that can seamlessly integrated with this saml2 library, but we may have to do some work to support that.

smarek commented

cool, let me know, i'm ready to do updates of existing eIDAS implementation to support v5.x and than we can work on seamlessness of inter-working.

jaimeperez commented

Hi @smarek!

I think Tim gave you a pretty complete overview of what would be needed with our new API. It should also be easier for you to implement, since most of the functionality will be already there for you to use automatically, just by extending our classes and/or implementing interfaces. However, there's still some instability since this is a work in progress, and some design decisions might change.

We are trying to achieve a couple of goals with this rewrite:

  • Get our objects to be immutable, meaning once they are created, you can be 100% sure they are in a correct state (e.g. not missing any mandatory attributes, etc).

  • Get our classes, interfaces and so on to properly reflect the hierarchy defined in the SAML standard. This should help users (and developers like you) understand how to use the API, since whatever is defined in the standard should have an equivalence in the code.

I'd recommend you to have a look at the PRs we have currently open to grasp a sense of where we are going. We'll probably need to fix a few things in order to support your extension without us having to explicitly add it to the code base (probably by declaring an interface that you would need to implement in your extension), but in general I think it should be quite simple.

As far as I understand, this is just a new extension providing a couple of new elements, right?

smarek commented

Hi @jaimeperez!

very cool, immutability is really appreciated.

Yes, the extensions is in sense of SAML specification, just adding new XML namespace(s) and introducing new classes reflecting available elements.

Second part of the extension is simplifying the developer use-cases by explicitly setting certificates/private keys, verifying responses validity and integrity, and simplifying integration with IdP using provided metadata configuration (in eIDAS there are more things hard-wired in contract between SeP and IdP, than in general SAML, where IdP metadata can extensively configure SeP capabilities and/or communication procedures), see ie. https://github.com/otevrenamesta/simplesamlphp-saml2-eidas/blob/master/src/OMSAML2.php#L298 and https://github.com/otevrenamesta/simplesamlphp-saml2-eidas/blob/master/src/OMSAML2.php#L111

jaimeperez commented

Hi @smarek!

I've been taking a look at what would be needed in order to support your use case, and summarised my thoughts in #211. That should also serve as a roadmap, if we all agree that's a sensible way to proceed.

smarek commented

Closing this ticket in favor of #211