Skip to content

Support ADFS encrypted key#405

Merged
pitbulk merged 1 commit intoSAML-Toolkits:masterfrom
joshwetzel:dynamic_encryption_namespace
Aug 14, 2017
Merged

Support ADFS encrypted key#405
pitbulk merged 1 commit intoSAML-Toolkits:masterfrom
joshwetzel:dynamic_encryption_namespace

Conversation

@joshwetzel
Copy link
Copy Markdown
Contributor

@joshwetzel joshwetzel commented Jun 23, 2017 •
edited
Loading

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

@pitbulk
Copy link
Copy Markdown
Collaborator

pitbulk commented Jun 28, 2017

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

@joshwetzel
Copy link
Copy Markdown
Contributor Author

joshwetzel commented Jul 29, 2017

@pitbulk have you had a chance to review this?

@pitbulk
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

joshwetzel commented Aug 11, 2017

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

pitbulk
pitbulk reviewed Aug 14, 2017
View reviewed changes
Comment thread 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
Copy Markdown
Collaborator

@pitbulk pitbulk Aug 14, 2017

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
Copy Markdown
Contributor Author

@joshwetzel joshwetzel Aug 14, 2017

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
Copy Markdown
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