Skip to content

Be able to handle multiple signing certs provided by AzureAD IdP#101

Merged
pitbulk merged 2 commits intoSAML-Toolkits:masterfrom
inflexiontecnologia:master
Jan 8, 2018
Merged

Be able to handle multiple signing certs provided by AzureAD IdP#101
pitbulk merged 2 commits intoSAML-Toolkits:masterfrom
inflexiontecnologia:master

Conversation

@anderson-sillos
Copy link
Copy Markdown

Allow to set multiple fingerprints comma separated in onelogin.saml2.idp.certfingerprint config parameter.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.004%) to 95.071% when pulling e8356e1 on inflexiontecnologia:master into b8a7c92 on onelogin:master.

@anderson-sillos
Copy link
Copy Markdown
Author

@pitbulk & @miszobi, it takes something else for this approach to be merged? Or would not this viable? Thanks

@pitbulk
Copy link
Copy Markdown
Contributor

pitbulk commented Jan 3, 2018

Rather than support just fingerprints the desired solution will be to support multiple certs for sign and encrypt as implemented here. But had no time to implement it on java-saml yet.

@anderson-sillos
Copy link
Copy Markdown
Author

thanks @pitbulk, then I´ll provide the solution you described, OK?

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 4, 2018

Coverage Status

Coverage increased (+0.004%) to 95.198% when pulling 535d302 on inflexiontecnologia:master into ea3e51a on onelogin:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 4, 2018

Coverage Status

Coverage increased (+0.004%) to 95.198% when pulling 20778d8 on inflexiontecnologia:master into ea3e51a on onelogin:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 5, 2018

Coverage Status

Coverage decreased (-0.06%) to 95.131% when pulling 37cb12f on inflexiontecnologia:master into ea3e51a on onelogin:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 5, 2018

Coverage Status

Coverage decreased (-0.06%) to 95.131% when pulling 9caf8a6 on inflexiontecnologia:master into ea3e51a on onelogin:master.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 5, 2018

Coverage Status

Coverage increased (+0.06%) to 95.256% when pulling 5021d69 on inflexiontecnologia:master into ea3e51a on onelogin:master.

@anderson-sillos
Copy link
Copy Markdown
Author

Hi @pitbulk, could you review this implementation?

@pitbulk
Copy link
Copy Markdown
Contributor

pitbulk commented Jan 8, 2018

The implementation seems to work, but I see you only covered
onelogin.saml2.idp.x509certMulti for signing.

Maybe we can extend it and use:
onelogin.saml2.idp.x509certMulti.signing.0 =
onelogin.saml2.idp.x509certMulti.signing.1 =
onelogin.saml2.idp.x509certMulti.signing.2 =

and
onelogin.saml2.idp.x509certMulti.encryption =

Or in the documentation explain that onelogin.saml2.idp.x509certMulti covers only certs for validate signature, if admin want to set a different value for encrypt, should be placed on the onelogin.saml2.idp.x509cert parameter.

@anderson-sillos
Copy link
Copy Markdown
Author

Yes, because always will be used just one certificate for encrypting, so it was maintained the original x509cert for this. And for signing validation will be used both x509cert and x509certMulti.

I fixed the documentation.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 8, 2018

Coverage Status

Coverage increased (+0.06%) to 95.256% when pulling 0f53b87 on inflexiontecnologia:master into 6c320d0 on onelogin:master.

@pitbulk pitbulk merged commit fa1df60 into SAML-Toolkits:master Jan 8, 2018
@pitbulk
Copy link
Copy Markdown
Contributor

pitbulk commented Jan 8, 2018

Thanks for this contribution, I plan to implement an IdPMetadataParser like the one implemented on php-saml and then release a new version of java-saml.

Best regards.

@anderson-sillos
Copy link
Copy Markdown
Author

@pitbulk, how could I help you?

@pitbulk
Copy link
Copy Markdown
Contributor

pitbulk commented Jan 8, 2018

Basically the work to be done is to translate PHP code of the metadata parser in Java and adapt the IdP info injection, so will be able to inject into the settings builder

@anderson-sillos
Copy link
Copy Markdown
Author

OK, I'll work on this and send a PR

@pitbulk
Copy link
Copy Markdown
Contributor

pitbulk commented Jan 8, 2018

great!

@pitbulk
Copy link
Copy Markdown
Contributor

pitbulk commented Jan 17, 2018

I created #140, @anderson-sillos please review it.

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.

3 participants