IdpMetadataParser doesn't handle multiple IdP certificates

[tomilaine]

I'm facing a situation where IdP has two certificates during a transition period. In this situation IdpMetadataParser gets the other fingerprint from what is used in the response and authentication fails.

I think IdpMetadataParser should be robust and verbose regarding IdP metadata so that in these kind of situations users will know what happened and how to address the situation with any particular IdP. Also I think Settings should be able to store the relevant data for all IdPs. What do you think?

This pull request seems do the trick but has been rejected: #290
It has some extra stuff for handling the validation as well which is nice (but secondary to the parsing of the metadata). Also parsing one certificate is just a subset of parsing multiple certificates so the code could use some refactoring regarding that.

Should I make a pull request or are you already putting your hands on the reject button? 😃

[pitbulk]

Im ok with the fact of extending IdpMetadataParser in order to be able to parse more than 1 certificate.

But I dislike the change proposed on #290 where we had 2 new attributes for the settings:
idp_certs and idp_cert_fingerprints. What happened with the old idp_cert and idp_cert_fingerprint?

I did a proposal to be used on those transition periods:
#266 (comment)

[tomilaine]

IdpMetadataParser returns a settings object from parse so I guess the settings have to have some new attributes for the multiple certificates?

Thinking of alternatives...
If we reuse the same attributes and do something like return an Array for idp_cert and idp_cert_fingerprint that would potentially break stuff. That could be of course put behind an option like options[:enable_multiple_certs] so that the users would then take the Array into account. Of course ruby-saml would then have to internally handle the array values as well.

Another option would be to store them in the parser instance, which feels like the wrong place.

Any suggestions?

[pitbulk]

For this kind of complexity is why we currently does not support multi certs,.

take also in mind that is possible to have certificates related to signatures and others related to the encryption process, so at the end if we want to cover all the SAML implementation, we will need thousand of settings, and that is against the goal of keep the toolkit simple.

[tanelsuurhans]

Ideally you need to keep around two lists of certificates - one for signing and one for encryption. If the metadata contains only one certificate, used for both, its just populated into both lists. Toggling support for multiple certificates by settings and checking value types is all pretty much meaningless.

The fact that the gem not support multiple, or separate certificates for different purposes is a problem in itself. Its a basic security measure to not share keypairs for different operations.

[davelooi]

Speaking of #290.

The 2 new fields was created in order to avoid breaking changes. It would be very confusing if idp_cert returns a single cert sometimes, and an array some other time.

I have also deliberately avoided changing existing code too much, for 2 reasons:

1. If the PR was rejected, I could continue to rebase my branch and get the latest updates with minimal conflicts.
2. If the PR was accepted, handling multiple certs is the norm, and we can deprecate and eventually remove the existing code for handling single cert.

[pre]

Related issue: #346

[erlingwl]

Azure's latest practice on this, makes this issue very relevant: https://azure.microsoft.com/en-us/documentation/articles/active-directory-signing-key-rollover/

I am not sure about the best way forward, but if one modified the "idp_cert=" setter in the saml settings to make sure it stores this as an array with a single element that might help the backward compatibility of #290 ?

[pitbulk]

I will do some research and try to find a global solution for all the toolkits.

The php and python versions are able to read a cert from settings but also from files. I will implement a solution to unify toolkits and also for solve rollover.

Expect it to be released at the end of september.

[erlingwl]

Thank you!

[demonmind]

Is there any update on this issue. We need to parse a metadata file to integrate with ADFS 2.0 and the xml file has multiple certificates. We are not able to create a valid SAML Request because of this.

[pitbulk]

Sorry, this feature is not ready yet, but meanwhile, you can always parse it manually and set the right values to the settings.

[demonmind]

So get the Cert from the metadata file and set it as the following?
settings.idp_cert = 'long certificate string'

Also we are unable to communicate with the ADFS 2.0 systems. Does the gem support ADFS 2.0 integration?We have successfully integrated with other systems but this is the first time with ADFS 2.0.

[pitbulk]

Yes, using the settings.idp_cert

And yes, is compatible with ADFS 2.0 or 3.0, but with the right settings on both sides (ADFS and ruby-saml). Review the nameid format and the authn context request values.

If you have an specific doubt (not able to be resolved by the docs or previous closed issues) or you think you found a bug, please, open a new issue.

[demonmind]

Thank you.

[acontero]

@pitbulk is this feature ready yet? I'm running into the same issue with ADFS having multiple certs due to the Auto Certificate Rollover feature within ADFS.

[acontero]

Sorry just saw your comment on my other issues post. Guess I'll just monkey patch for now.

Though this does seem to continue to be a very relevant problem since ADFS does require multiple certs for the rollover feature. And I would assume we want this gem to continue to work with ADFS, no? Also, apart from ADFS, this is still relevant. As you said above:

For this kind of complexity is why we currently does not support multi certs,.

take also in mind that is possible to have certificates related to signatures and others related to the encryption process, so at the end if we want to cover all the SAML implementation, we will need thousand of settings, and that is against the goal of keep the toolkit

If multiple signing certs are present (which seems to be a reality we are encountering frequently enough) and they are used for different purposes, we have no way of filtering which one is the correct one to use. And storing multiple certs will certainly add complexity. However, I don't think we can always assume, as a rule, that the first x509 parsed will always be the correct one to use. Nevertheless, we need the correct one in order to validate. And I don't think hardcoding the string is a great solution.

So what type of solution would you accept a PR for if davelooi's was not acceptable? One in which we don't add new attributes? Perhaps one of us can offer a PR that you might approve, which will be helpful to everyone?

Thanks!

[vincentwoo]

Would love an update on this! @pitbulk, do you still plan to have a toolkit wide solution to this problem? Unfortunately, as an SP, you don't have too much control over what different IdPs hand you.

[pitbulk]

Yes, I plan to provide a solution for that, but atm, I'm working on other tasks at Onelogin :(

[vincentwoo]

Can I pay money to accelerate this feature?

[pitbulk]

@vincentwoo I started to implement the desired functionality on the php-saml because I fill more comfortable with that language.
Later I will ask for reviews of the provided solution and later port to the rest of toolkits, starting with the ruby-saml toolkit.

I think the approach will be very similar to 290 but without supporting multiple fingerprints. But also register the "use" of the cert (signing/encryption) in order to support specific x509cert for each process.

[pitbulk]

That functionality is ready on php-saml (now on test phase). Soon I will port that to this toolkit.

[pitbulk]

I started the development on ruby-saml, you can follow on that PR:
#389

[pitbulk]

The PR #389 is ready.

@vincentwoo @AngelicaS @demonmind @erlingwl @davelooi @tomilaine , do you want to review and test if it matches your expectation?

[acontero]

Thanks so much for updating this library! It's working great for me for multiple certs and for the ADFS rollover.

I also have the issue of the multiple entities within the single entities descriptor tag so will have to see if that works well too. I noticed you added support for that. I still have a monkey patch for it because the downloading of the huge metadata file itself takes so long, let alone parsing it. So it's too slow to do during a login and authentication. How are other people dealing with this? I wrote code to read the metadata, parse out only the entity I need from it, then write that to a file and then I check periodically if it needs to be updated. Has anyone else run into this? I will post this comment re: this specific issue in a more relevant issue post. But figured I'd mention it here too.

And I will soon be using Azure AD so will see if that works too with this gem. But so far so good and I'm optimistic. Thanks again for putting in the work to do this! It's SO helpful! 😃 I'm so glad to delete my monkey patches. 🙌

[pitbulk]

IdPMetadataParser should be executed to extract IdP metadata once to make the integration...not per login requirement. Your approach seems good to me.
Handle big files with multiple descriptors (like UK metadata file) is a well-known use case and there is no other way...than try to get an URL where only that IdP is published, instead trying to read it in a set.

[acontero]

Hm interesting. So in the examples in the README.md, you are expecting that users of this library populate the settings using the Idp Metadata Parser just once and then store it somewhere? How do you recommend knowing when the Idp's metadata has been updated? I didn't realize the expectation was that it just be used once and as a result, that is not how my team built our SSO flow. Currently we parse it each time the user authenticates. Only for one particular customer (with the multiple entities and huge metadata file) did we build this solution to store to a file.

[tomilaine]

I think this is more of an operations question (knowing how often the Idp certificate changes) and automating accordingly.

I would just like to add (even though it might be obvious) that whenever you store and read back such information make sure it's done in a secure way.

[acontero]

What do you mean @tomilaine? On the Idp side? You think it's best to ask the ADFS administrator how often the metadata file might change and try and check that often instead of automaticaly detecting it? For now I have been checking the etag on the large metadata file (the one with all of the entities) and if that has changed then I re-parse and store again to file overwriting the old file. Just wondering if there is another solution other people are implementing.

Yes definitely on the secure file storage. 👍