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

Allow raw (unsigned) assertions #10

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

rordeix
Copy link

@rordeix rordeix commented Sep 11, 2020

According to SAML specifications assertions may be signed.

This PR allows to add raw assertions on responses.
I branched from signed_response_message which has an updated README about signing responses and assertions

oTsogbadrakhChinzorig and others added 30 commits January 17, 2019 21:56
Some SP required to do sign SAML response message it self.
Follow the origin coding style


Signed message opts


signed was the instance method
Small test for Entity ID
ID should be same for sign and document
Install bundle for RVM version
Exclude build with issue because, can't check rails version before install it.
Update library for cloud SAML services.
Small test for Entity ID


Name space should be defined
@Zogoo
Copy link
Owner

Zogoo commented Sep 14, 2020

According to SAML specifications assertions may be signed.

This PR allows to add raw assertions on responses.
I branched from signed_response_message which has an updated README about signing responses and assertions

Thanks for your contribution, I like to have an optional config for Assertion signature. But, I just wonder that is there any use case for non signed assertion, because for security reason assertion or response must be signed right?
https://cheatsheetseries.owasp.org/cheatsheets/SAML_Security_Cheat_Sheet.html

@rordeix
Copy link
Author

rordeix commented Sep 14, 2020

According to SAML specifications assertions may be signed.
This PR allows to add raw assertions on responses.
I branched from signed_response_message which has an updated README about signing responses and assertions

Thanks for your contribution, I like to have an optional config for Assertion signature. But, I just wonder that is there any use case for non signed assertion, because for security reason assertion or response must be signed right?
https://cheatsheetseries.owasp.org/cheatsheets/SAML_Security_Cheat_Sheet.html

I agree with you, for security reasons assertions should be signed.
In our case we have an idp initiated flow to a server which requires the responses the to be signed but does not support signed assertions.
I thought someone else could face same problem (it could be fixed on the other side too, if they add support for signed assertions). Perhaps is a really particular case and does not add much value to anyone else?

@Zogoo
Copy link
Owner

Zogoo commented Sep 15, 2020

According to SAML specifications assertions may be signed.
This PR allows to add raw assertions on responses.
I branched from signed_response_message which has an updated README about signing responses and assertions

Thanks for your contribution, I like to have an optional config for Assertion signature. But, I just wonder that is there any use case for non signed assertion, because for security reason assertion or response must be signed right?
https://cheatsheetseries.owasp.org/cheatsheets/SAML_Security_Cheat_Sheet.html

I agree with you, for security reasons assertions should be signed.
In our case we have an idp initiated flow to a server which requires the responses the to be signed but does not support signed assertions.
I thought someone else could face same problem (it could be fixed on the other side too, if they add support for signed assertions). Perhaps is a really particular case and does not add much value to anyone else?

Actually, I was missing that if response itself get signed by "IdP", assertion could be as raw. It won't bring critical security issue.
And it might be good that print warning if someone mistakenly allows unsigned response with unsigned assertion.
About as merge request if you are not rushing, can I wait for some other guys also give some review of your idea?

@rordeix
Copy link
Author

rordeix commented Sep 17, 2020

I addressed your comments. I'm not sure about the controller test, please take a look and any feedback is welcome.
About the merge request, there is no rush, we are using our own fork in the meantime. When this gets merged we will update our gemfile.
One unrelated question to this PR, if I can ask: is saml-idp#88 is merged on this repo?

@rordeix rordeix requested a review from Zogoo September 17, 2020 10:22
@Zogoo
Copy link
Owner

Zogoo commented Sep 23, 2020

I addressed your comments. I'm not sure about the controller test, please take a look and any feedback is welcome.
About the merge request, there is no rush, we are using our own fork in the meantime. When this gets merged we will update our gemfile.
One unrelated question to this PR, if I can ask: is saml-idp#88 is merged on this repo?

I'm not merged yet. I have solved the issue with lambda which return what I wanted to return. I could achieve that which overrides SAML IdP config before generate response.
But, I just created a new pull request for saml_idp#88 in my repo and checking how will affect to an existing project.
BTW, I just thought that we can communicate each other on Gitter and I have recently created room. If you want to join, here is the link https://gitter.im/saml_idp

@rordeix
Copy link
Author

rordeix commented Sep 28, 2020

I addressed your comments. I'm not sure about the controller test, please take a look and any feedback is welcome.
About the merge request, there is no rush, we are using our own fork in the meantime. When this gets merged we will update our gemfile.
One unrelated question to this PR, if I can ask: is saml-idp#88 is merged on this repo?

I'm not merged yet. I have solved the issue with lambda which return what I wanted to return. I could achieve that which overrides SAML IdP config before generate response.
But, I just created a new pull request for saml_idp#88 in my repo and checking how will affect to an existing project.
BTW, I just thought that we can communicate each other on Gitter and I have recently created room. If you want to join, here is the link https://gitter.im/saml_idp

Thank you, Zogoo! For your answers and the invitation, I will join the Gitter room

@Zogoo
Copy link
Owner

Zogoo commented Jan 6, 2021

@rordeix my all pull requests are merged into original repo, if you create your pull request in original repo, probably other guys also take look at it and we might can easily merge 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