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.

Sign up for GitHub

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

Jump to bottom

Turn warning about no signature verification into an error #866

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

Turn warning about no signature verification into an error #866

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ab4d4e
Turn not verifying any signature into an error
thijskh Aug 8, 2022
8c2f880
Default want_assertions_or_response_signed to True
thijskh Aug 8, 2022
2218697
Reordering the phrases to put emphasis on insecure.Update src/saml2/c…
thijskh Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/howto/config.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1174,7 +1174,7 @@ want_assertions_or_response_signed
Indicates that *either* the Authentication Response *or* the assertions
contained within the response to this SP must be signed.

Valid values are True or False. Default value is False.
Valid values are True or False. Default value is True.

This configuration directive **does not** override ``want_response_signed``
or ``want_assertions_signed``. For example, if ``want_response_signed`` is True
Expand Down
10 changes: 4 additions & 6 deletions src/saml2/client_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
import time
import logging
from typing import Mapping
from warnings import warn as _warn

from saml2.entity import Entity

Expand Down Expand Up @@ -174,7 +173,7 @@ def __init__(self, config=None, identity_cache=None, state_cache=None,
"authn_requests_signed": False,
"want_assertions_signed": False,
"want_response_signed": True,
Copy link
Member

Choose a reason for hiding this comment

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

This should change to False to allow messages with signed assertions only to be processed. This should also change on the documentation.

Suggested change
"want_response_signed": True,
"want_response_signed": False,

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this is saml2int compliant to change this default. However of course there are some arguments to make that reponse signing is more secure/more defense in depth than assertion signing so there is some case also to make to by default check response signatures and let people change that if they need to.

Copy link
Author

Choose a reason for hiding this comment

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

If your IdP signs responses it's probably more secure/defense in depth to not allow this to change. So defaulting to response signing as a requirement might make sense, so only installs that knowingly require only the assertion to be signed will change this?

"want_assertions_or_response_signed": False,
"want_assertions_or_response_signed": True,
}
for attr, val_default in attribute_defaults.items():
val_config = self.config.getattr(attr, "sp")
Expand All @@ -194,15 +193,14 @@ def __init__(self, config=None, identity_cache=None, state_cache=None,
self.want_assertions_or_response_signed,
]
):
warn_msg = (
error_msg = (
"The SAML service provider accepts "
"unsigned SAML Responses and Assertions. "
"This configuration is insecure. "
thijskh marked this conversation as resolved.
Show resolved Hide resolved
"Consider setting want_assertions_signed, want_response_signed "
"Set at least one of want_assertions_signed, want_response_signed "
"or want_assertions_or_response_signed configuration options."
)
logger.warning(warn_msg)
_warn(warn_msg)
raise SAMLError(error_msg)

self.artifact2response = {}

Expand Down
12 changes: 8 additions & 4 deletions tests/test_51_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1860,7 +1860,8 @@ def set_client_want(response, assertion, either):
parse_authn_response(response)

set_client_want(False, False, False)
parse_authn_response(response)
with raises(SAMLError):
parse_authn_response(response)

# Response is not signed but assertion is signed.
kwargs["sign_response"] = False
Expand Down Expand Up @@ -1893,7 +1894,8 @@ def set_client_want(response, assertion, either):
parse_authn_response(response)

set_client_want(False, False, False)
parse_authn_response(response)
with raises(SAMLError):
parse_authn_response(response)

# Both response and assertion are signed.
kwargs["sign_response"] = True
Expand Down Expand Up @@ -1922,7 +1924,8 @@ def set_client_want(response, assertion, either):
parse_authn_response(response)

set_client_want(False, False, False)
parse_authn_response(response)
with raises(SAMLError):
parse_authn_response(response)

# Neither response nor assertion is signed.
kwargs["sign_response"] = False
Expand Down Expand Up @@ -1958,7 +1961,8 @@ def set_client_want(response, assertion, either):
parse_authn_response(response)

set_client_want(False, False, False)
parse_authn_response(response)
with raises(SAMLError):
parse_authn_response(response)


class TestClientNonAsciiAva:
Expand Down