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

Add want_assertions_or_response_signed functionality #485

Conversation

skoranda
Copy link
Contributor

Add the ability to configure an SP to require either a signed response
or signed assertions.

@rohe
Copy link
Contributor

rohe commented Dec 29, 2017

LGTM

Copy link
Member

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

Ideally, I would like us to have tests in https://github.com/rohe/pysaml2/blob/master/tests/test_41_response.py ensuring we discard incorrectly signed responses/assertions with any valid combination of the want_X_signed set

Assuming this is not too much work, I would feel more comfortable having those before we merge this change.

@jkakavas
Copy link
Member

I agree with Scott that this is the easiest way forward for now without a major refactoring as this was a breaking change we introduced recently and needs to be fixed.

Down the line, I think we should open an issue and discuss how this can be handled so that we refactor the logic accordingly. My opinion is that administrators don't need the granularity we currently give them. It should be enough to give the option to disable signature verification for testing/development ( with BIG FAT warnings ) and when this is not the case :

  • Check if the response is signed.
    • If yes, validate the signature and discard if not valid
    • If no, move on and check if the assertion in the response is signed
      • If yes, validate the signature and discard if not valid
      • If no, discard.

@surfnet-niels
Copy link

What is keeping us from merging this? Is it just the conflicts?

@c00kiemon5ter
Copy link
Member

Tests is the main reason (same for #483). Conflicts are easily resolved.

@skoranda
Copy link
Contributor Author

skoranda commented Nov 5, 2018

Just a "heads up" that I have rebased my commits off of today's master and spun up my testbed and verified that my commits (after fixing some merge conflicts) still work. I will be attempting to write tests in the next few days.

@skoranda skoranda force-pushed the want_assertions_or_response_signed branch from 4a3e427 to a397968 Compare November 5, 2018 20:13
@codecov-io
Copy link

codecov-io commented Nov 5, 2018

Codecov Report

Merging #485 into master will increase coverage by 0.04%.
The diff coverage is 94.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #485      +/-   ##
==========================================
+ Coverage   65.56%   65.61%   +0.04%     
==========================================
  Files         103      103              
  Lines       25568    25602      +34     
==========================================
+ Hits        16764    16799      +35     
+ Misses       8804     8803       -1
Impacted Files Coverage Δ
src/saml2/client_base.py 76.86% <ø> (+1.2%) ⬆️
src/saml2/config.py 84.42% <ø> (ø) ⬆️
src/saml2/response.py 72.77% <100%> (+0.68%) ⬆️
src/saml2/entity.py 85.2% <93.75%> (+0.85%) ⬆️
src/saml2/validate.py 74.5% <0%> (-3.93%) ⬇️
src/saml2/__init__.py 86.33% <0%> (-0.19%) ⬇️
src/saml2/sigver.py 71.7% <0%> (+0.11%) ⬆️
src/saml2/time_util.py 87.42% <0%> (+0.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40a3699...8b79846. Read the comment docs.

skoranda and others added 4 commits November 21, 2018 17:36
Add the ability to configure an SP to require either a signed response
or signed assertions.

Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Add logic to test client configuration options
want_response_signed, want_assertions_signed, and
want_assertions_or_response_signed.
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
Signed-off-by: Ivan Kanakarakis <ivan.kanak@gmail.com>
@c00kiemon5ter c00kiemon5ter force-pushed the want_assertions_or_response_signed branch from 14e21bb to 8b79846 Compare November 21, 2018 15:43
@c00kiemon5ter c00kiemon5ter merged commit 3989b99 into IdentityPython:master Nov 21, 2018
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.

None yet

6 participants