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.

Sign up for GitHub

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

Jump to bottom

Support ADFS encrypted key #405

Merged
merged 1 commit into from
Aug 14, 2017
Merged

Support ADFS encrypted key #405

merged 1 commit into from
Aug 14, 2017

Conversation

joshwetzel
Copy link
Contributor

@joshwetzel joshwetzel commented Jun 23, 2017 •
edited
Loading

ADFS does not use the ds namespace on their KeyInfo node.

@pitbulk
Copy link
Collaborator

pitbulk commented Jun 28, 2017

Thanks for sharing that code, I will review it soon.

@joshwetzel
Copy link
Contributor Author

@pitbulk have you had a chance to review this?

@pitbulk
Copy link
Collaborator

pitbulk commented Jul 31, 2017 •
edited
Loading

Yes I reviewed it and I'm not a fan of ignore namespaces.
In php-saml I did a workaround to fix an issue when the decrypted data has issue with namespace.

Trying to figure out something here. Maybe, meanwhile can you temp change your certificates, and provide an encoded SAMLResponse encryped with non production keys (and provide me by mail the necessary data to debug)?

@joshwetzel
Copy link
Contributor Author

joshwetzel commented Aug 1, 2017 •
edited
Loading

Perhaps something not so heavy handed? I've removed the dynamic namespace.

@joshwetzel joshwetzel changed the title Support dynamic encryption namespace Support ADFS encrypted key Aug 3, 2017
@joshwetzel
Copy link
Contributor Author

@pitbulk let me know what you think of the latest changes I pushed up.

pitbulk
pitbulk reviewed Aug 14, 2017
View reviewed changes
lib/onelogin/ruby-saml/utils.rb Outdated
{ "ds" => DSIG, "xenc" => XENC },
{ "id" => self.retrieve_symetric_key_reference(encrypt_data) }
"./ds:KeyInfo/xenc:EncryptedKey | ./KeyInfo/e:EncryptedKey | //xenc:EncryptedKey[@Id=$id]",
{ "ds" => DSIG, "xenc" => XENC, "e" => XENC },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why add "e" => XENC if already was defined "xenc" => XENC? It should be:

"./ds:KeyInfo/xenc:EncryptedKey | ./KeyInfo/xenc:EncryptedKey | //xenc:EncryptedKey[@id=$id]",
{ "ds" => DSIG, "xenc" => XENC},

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated.

@pitbulk pitbulk merged commit f435c48 into SAML-Toolkits:master Aug 14, 2017
@pitbulk
Copy link
Collaborator

pitbulk commented Aug 14, 2017

I will release a new version soon.

@joshwetzel joshwetzel deleted the dynamic_encryption_namespace branch August 14, 2017 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitbulk pitbulk pitbulk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@joshwetzel @pitbulk