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

[NEW] SAML: Adds possibility to decrypt encrypted assertions #12153

Merged
merged 3 commits into from
Jan 19, 2019

Conversation

gerbsen
Copy link
Contributor

@gerbsen gerbsen commented Sep 25, 2018

Dear Rocket.Chat team,
First of all, thank you very much for providing this software as open source. Very much appreciated.

We have added the possiblity to also use encrypted assertions with the SAML login. We have developed and tested this extension on an instance with a couple of thousand users. We originally have started to develop this fix for the underlying SAML library and it was approved there. Since we discovered to late that Rocket.Chat uses a custom version of this library, our changes are kind of obsolete now. We have asked here if a PR is welcome and herewith deliver the PR. We are aware of the fact that this might take some time. In case we can do anything to speed up this process, we are happy to help. :)

Here is the documentation that we added in the underlying project.

Kind regards,
Daniel (from team netzbegruenung)

Encryption

The <EncryptedAssertion> element represents an assertion in encrypted fashion, as defined by the XML Encryption Syntax and Processing specification XMLEnc. Encrypted assertions are intended as a confidentiality protection mechanism when the plain-text value passes through an intermediary.

The following schema fragment defines the <EncryptedAssertion> element:

<element name="EncryptedAssertion" type="saml:EncryptedElementType"/>

In case the SAML response contains an <EncryptedAssertion> element and the configuration key privateKey is set, the assertion get's decrypted and handled like it would be an unencrypted one.

@CLAassistant
Copy link

CLAassistant commented Sep 25, 2018

CLA assistant check
All committers have signed the CLA.

@Hudell Hudell self-assigned this Sep 27, 2018
Hudell
Hudell previously approved these changes Oct 2, 2018
@rodrigok
Copy link
Member

rodrigok commented Nov 6, 2018

@gerbsen can you fix the conflict?

@gerbsen
Copy link
Contributor Author

gerbsen commented Nov 7, 2018

@rodrigogs I tried fixing the problems but now there are even more :( could you help me, and tell me what needs to be done?

@gerbsen
Copy link
Contributor Author

gerbsen commented Jan 14, 2019

@rodrigok @Hudell All tests pass now. Could we please get an approval? :)

@Hudell Hudell self-requested a review January 15, 2019 13:11
@Hudell Hudell added this to the 0.74.0 milestone Jan 15, 2019
Copy link
Member

@rodrigok rodrigok left a comment

Choose a reason for hiding this comment

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

Added some comments to later improvements

let assertion = response.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'Assertion')[0];
const encAssertion = response.getElementsByTagNameNS('urn:oasis:names:tc:SAML:2.0:assertion', 'EncryptedAssertion')[0];

const xmlenc = require('xml-encryption');
Copy link
Member

Choose a reason for hiding this comment

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

Move this to the top of the file as an import

const options = { key: this.options.privateKey };

if (typeof encAssertion !== 'undefined') {
xmlenc.decrypt(encAssertion.getElementsByTagNameNS('*', 'EncryptedData')[0], options, function(err, result) {
Copy link
Member

Choose a reason for hiding this comment

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

Move this to a function to prevent code duplication and prefer to use arrow functions

@rodrigok rodrigok merged commit 483f7d5 into RocketChat:develop Jan 19, 2019
@rodrigok rodrigok changed the title [NEW] adds possibility to decrypt encrypted assertions [NEW] SAML: Adds possibility to decrypt encrypted assertions Jan 19, 2019
alansikora pushed a commit that referenced this pull request Jan 23, 2019
@sampaiodiego sampaiodiego mentioned this pull request Jan 28, 2019
@ChessSpider
Copy link

ChessSpider commented Jan 28, 2019

Can someone please make unit tests for saml? Willing to help..

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.

6 participants