Feedback for pac4j-saml

[Ben McCann]

Hi,

I was looking at pac4j-saml. It seems really complicated to what I've seen elsewhere. Was hoping that I could get some pointers on why things are done this way or maybe that we could make some improvements to simplify things a bit.

Keystore required?

pac4j-saml requires a keystore, which is something not needed by any other implementation I have seen. I was wondering why it is needed since it makes it more complicated to use.

Here's a couple other libraries I've seen

OneLogin Java - https://github.com/onelogin/java-saml

Parameters: assertionConsumerServiceUrl, issuer, idpSsoTargetUrl, certificate

Okta Java Toolkit - https://support.okta.com/entries/24880412-Okta-SAML-Toolkit-for-Java

Parameters: entityID, IdP metadata

Neither require a keystore, which makes me think it shouldn't be necessary.

Putting data back into identity provider?

I'm not sure why there's a step that talks about importing metadata into the identity provider. This is not typically necessary in my experience. Is this an optional step with pac4j?

// generate pac4j SAML2 Service Provider metadata to import on Identity Provider side

String spMetadata = client.printClientMetadata();

Here's an video showing how to setup Google with Okta. In SAML language, Okta is an Identity Provider and Google is a Service Provider. You can see that it doesn't require putting any info from Google into Okta. It only requires putting info from Okta into Google

Interaction with filesystem required?

I'm not a big fan of the fact that the API force interaction with the file system. Providing the metadata as a String seems much more flexible. Then I can read it from the file system or any other location I may wish to retrieve it from.

// Configure a file containing the Identity Provider (IDP) metadata.

// It is the IDP's responsibility to make its metadata freely accessible.

client.setIdpMetadataPath("testshib-providers.xml");

Thanks,

Ben

[Michaël REMOND]

Hello Ben,

First let me thank you for your feedback as we are happy to enhance our library.

Here are my answers:

Keystore required?

The keystore is required primarily to digitally sign the authentication request and possibly to decrypt attributes send by the IDP. You might think that it is not always mandatory but this way the configuration the pac4j client is more simple since you don't have to manage all possible scenarios: the client manages out of the box the most advanced use case. 

Putting data back into identity provider?

This is required in order to establish out of bounds trust between the two parties and exchange some configuration url like the Assertion Consumer Url. In your example, I'm not sure how the whole thing works but I guess it's because Google is a well-known trusted party and have standards end-points for all identity providers.

Interaction with filesystem required?

Not really (for example Open SAML offers a Metadata Provider that periodically downloads the metadata from the IDP public url) but I found it is the most basic and simple tradeoff for loading the IDP metadatas. If it is a String it will be hard-coded in your source code and you have to properly escape the XML content (not a big deal though). This way your multiples IDP metadata can be stored in a configuration folder and are easy to update.

Any suggestions and PR are welcome,

Best regards,

Michaël

--
You received this message because you are subscribed to the Google Groups "pac4j-users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to pac4j-users...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Ben McCann]

Thanks Michaël. I really appreciate your feedback as I'm not well versed in the security aspects of SAML. I wrote a SAML plugin for Jenkins recently and want to make sure I'm using it securely, so I appreciate learning more about it from you. I may update my implementation of that plugin to use pac4j SAML to ensure I have a more secure and reviewed implementation, which is one reason I'm interested in this.

Under what scenarios do you need to sign the authentication request? Could we make the keystore optional such that we only sign the request if the keystore is provided? This makes the configuration of the pac4j client simpler if you don't need the signing as setting up the keystore is one of the hardest parts of the configuration.

Regarding putting data back into identity provider, I'm not sure I understand why it is required. Would it only be needed if you're not using SSL? If you're using SSL, then can the SP just send the Assertion Consumer Url to the IdP as part of the request?

I was suggesting to make the IdP metadata a String so that it could be loaded from a database or other source as well. E.g. in my Jenkins SAML plugin, Jenkins stores all configuration data in a single configuration file. It is already setup to store and escape the data there and storing in a separate file would be much more difficult for me in that case. I like that it can be read from a file as well, but was thinking that ideally we could accept input from a String or a file. Is there anything in the file which needs to be stored securely?

Thanks,

Ben

--
about.me/benmccann

[Ben McCann]

Btw, I submitted a pull request to allow reading the IdP metadata from a string or a file. Very interested in your thoughts on these matters.

Thanks,

Ben

--
about.me/benmccann

[Michaël REMOND]

Hello Ben,

Thank you very much for your PR; Jérôme (the project owner) will handle it.

Here are my answers:

The security of the SAML authentication depends on the security level the two parties want to reach. For example the digital signature of the authn request can be mandatory for one particular SP (see the AuthnRequestsSigned="true" attribute in the SP metadata) but it won't be required by others. In fact SAML is a technical (and large) framework to address the problem of exchanging authentication claims in a trusty manner, what "trusty" means here depends of the criticality of your business case. 

So I agree that for some basic configuration, the keystore could be optional. In order to reflect the nature of this behavior, I think we must add a "noKeystore" flag which would explicitly disabled the security features (authn request signature and no decryption support). However I think we must test carefully because I feel this could break in OpenSAML or on some IDP.

When you import the SP metadata on the IDP, it allows the IDP to recognize a trusty SP, sending the SAML assertion to the correct endpoints (no phishing) and send the identity information (user attributes) this SP expects. SSL is not enough to address these needs.

Regards,

Michaël

[Ben McCann]

Thanks Michaël. Btw, for reference, I found a great description of this on serverfault in case anyone else is wondering

--
You received this message because you are subscribed to a topic in the Google Groups "pac4j-users" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/pac4j-users/MwF_M5eFh2E/unsubscribe.
To unsubscribe from this group and all its topics, send an email to pac4j-users...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
about.me/benmccann