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

idp_multi_cert nil if only multiple signing certs in metadata in 1.4.3 #412

Closed
bheeshmar opened this issue Aug 5, 2017 · 8 comments
Closed

Comments

@bheeshmar
Copy link

bheeshmar commented Aug 5, 2017

I have a metadata file from an Azure AD instance which has 3 "signing" certs but no "encryption" certs. When I parse this with the IdpMetadataParser I get the first signing cert in the idp_cert field, but a nil in idp_cert_multi instead of the 3 certs I expect.

I believe the root cause is the certificates.size == 1 || condition in #merge_certificates_into, which I think can be safely eliminated.

Alternatively, perhaps initializing certs = {signing: [], encryption: []} in #certificates would also help (and properly return symbolized keys).

@pitbulk
Copy link
Collaborator

pitbulk commented Aug 7, 2017

@bheeshmar
Are you using master with recent changes?
#410

@bheeshmar
Copy link
Author

@pitbulk No, I'm running 1.4.3 and using omniauth-saml. I worked around the symbolized keys problem for now, but the bigger issue is the lack of idp_cert_multi support when there are only signing KeyDescriptors.

@pitbulk
Copy link
Collaborator

pitbulk commented Aug 14, 2017

I will do a new release soon and review idp_cert_multi support

@bheeshmar
Copy link
Author

Thanks! Looking forward to it and thanks for all the hard work!

@thiago-sydow
Copy link

Having this issue too, glad to see that the fix is not far away to be released. Thanks. 👍

@bheeshmar
Copy link
Author

bheeshmar commented Aug 30, 2017

One more issue around idp_cert_multi support: Response#validate_response_state only checks idp_cert and idp_cert_fingerprint from settings - it doesn't consider idp_cert_multi which when set, does not set either of those fields.

Update: I see this is addressed in #402 . Looking forward to a release with these fixes! :)

@pitbulk
Copy link
Collaborator

pitbulk commented Aug 31, 2017

I will release it today.

@pitbulk
Copy link
Collaborator

pitbulk commented Aug 31, 2017

Released 1.5.0

@pitbulk pitbulk closed this as completed Aug 31, 2017
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

No branches or pull requests

3 participants