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

Added parameter to config list in order to be able to set authn_reque… #495

Closed
wants to merge 1 commit into from

Conversation

pcingola
Copy link

@pcingola pcingola commented Feb 21, 2018

Added parameter to config list in order to be able to set authn_requests_signed using sha256 (particularly from djangosaml2)

When configuring (e.g. from djangosaml2) the parameter
'authn_requests_signed': True,
works OK, whereas
'authn_requests_signed_alg': 'sha256'
does not work because the parameter is not in the list of allowed parameters (SP_ARGS). As far as I know, there is no other way to set sha-256 algorithm from config without this small change in the code.

The solution is a one-liner that adds 'authn_requests_signed_alg' to SP_ARGS list of parameters in src/saml2/config.py right below 'authn_requests_signed'

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

…sts_signed using sha256 (particularly from djangosaml2)
@c00kiemon5ter c00kiemon5ter self-assigned this Feb 21, 2018
@c00kiemon5ter
Copy link
Member

There is conversation about this at the mailing list. I will report progress on this here, but this is not going to be just an option that acts as a placeholder. If set, it should change pysaml's behaviour.

@peppelinux
Copy link
Member

peppelinux commented Jun 25, 2018

Probably I need this PR. I faced this problem today with djangosaml2.
I followed the calls into pysaml2 source code to find out the problem regarding this parameter.
Actually djangosaml2 manage IDP SAML2 response, through pySAML2, as SHA1 format and not like a ShibIDPv3 standard as SHA512. It also be able to configure SHA1 for a specific relying parties but probably SHA1 is not a good choice for the future because of its weakness:
https://wiki.shibboleth.net/confluence/display/IDP30/SecurityConfiguration

Furthermore, regarding on what I read on SaToSa-dev mailing list, as @c00kiemon5ter mentioned, I think that #396 could be merged

@c00kiemon5ter I also agree about what you wrote in SaToSa-dev ml, sign algorithm should be defined in pysaml2 and it could be also global to avoid too much complexity all around in the code (also worst documentable and maintenable!) BUT it could be necessary to deal, in case of an advanced IDP deployment, with different SP and, as ShibIDPv3 also does, it could be mandatory set a sign_alg for every SP to talk to. Different approach could be done with a generic SP deployment, sign_alg could be configured in config['sp'] node.

Also #450 talks about this issue.
Furthermore, this, coupled with #421 could suggest us to draw a big picture of the next feature-set regarding how to manage (enable and disable) the Signing Methods suported in pysaml2, if you agree, with the opportunity to exclude the weakness one.

Furthermore, about djangosaml2 implementation based on pysaml2, this topic seems to became very popular. This issues is mentioned also here IdentityPython/djangosaml2#115 with different, but not less interesting, approach. Regarding about this issue the solution could be adding authn_requests_signed_alg paramenter in the the pysaml2 config, as described in the following link: https://github.com/knaperek/djangosaml2/blob/master/djangosaml2/views.py#L181

@peppelinux
Copy link
Member

peppelinux commented Jun 26, 2018

A usefull example came from Shibboleth IDP, where the encryption and signing methods are defined globally in conf/idp.properties and then they could be also customized for every SP in conf/relying-party.xml

@c00kiemon5ter
Copy link
Member

This will be fixed by a changeset based on #396. As I explained, by itself this does nothing useful. I will close this and the discussion should move to #396

Thanks @peppelinux for gathering all the relevant info on the comment above 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants