Skip to content

Add support for configuring the hash algorithm used to sign CSRF tokens - #700

Open
phinjensen wants to merge 7 commits into
pallets-eco:mainfrom
phinjensen:main
Open

phinjensen wants to merge 7 commits into
pallets-eco:mainfrom
phinjensen:main

Conversation

@phinjensen

Copy link
Copy Markdown

This fixes the issue described in #699 by adding a configuration option that allows the user to set what hash algorithm is used by the itsdangerous Signer object, allowing users on FIPS-enabled systems (or those wanting a better algorithm than sha1 for any reason) to choose which hash algorithm is used.

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in docs/changes.rst summarizing the change and linking to the issue. Add .. versionchanged:: entries in any relevant code docs.

Comment thread docs/config.rst Outdated
Also set to ``False`` if you want to use WTForms's
built-in messages directly, see more info `here`_.
Default is ``True``.
``WTF_CSRF_SIGNER_DIGEST_METHOD`` Set to an algorithm from the `hashlib`_ library to

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

:mod:`hashlib` to reference a module

Comment thread src/flask_wtf/csrf.py Outdated
__all__ = ("generate_csrf", "validate_csrf", "csrf_meta_tag", "CSRFProtect")
logger = logging.getLogger(__name__)

DEFAULT_CSRF_SIGNER_DIGEST_METHOD = hashlib.sha1

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is unneeded

Comment thread src/flask_wtf/csrf.py Outdated
salt="wtf-csrf-token",
signer_kwargs={
"digest_method": current_app.config.get(
"WTF_CSRF_SIGNER_DIGEST_METHOD", DEFAULT_CSRF_SIGNER_DIGEST_METHOD

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should not pass at all (still use itsdangerous default) unless it's set.

Comment thread src/flask_wtf/csrf.py Outdated
salt="wtf-csrf-token",
signer_kwargs={
"digest_method": current_app.config.get(
"WTF_CSRF_SIGNER_DIGEST_METHOD", DEFAULT_CSRF_SIGNER_DIGEST_METHOD

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should not pass at all (still use itsdangerous default) unless it's set.

Comment thread src/flask_wtf/csrf.py Outdated
app.config.setdefault("WTF_CSRF_TIME_LIMIT", 3600)
app.config.setdefault("WTF_CSRF_SSL_STRICT", True)
app.config.setdefault(
"WTF_CSRF_SIGNER_DIGEST_METHOD", DEFAULT_CSRF_SIGNER_DIGEST_METHOD

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Default to None

@davidism

Copy link
Copy Markdown
Member

I think I'd prefer a way to override the signer in general, rather than adding specific configs for specific (potentially nested) arguments.

@phinjensen

Copy link
Copy Markdown
Author

I think I'd prefer a way to override the signer in general, rather than adding specific configs for specific (potentially nested) arguments.

I considered doing that, but the signer is passed as a type + properties and gets built by Serializer's __init__, so it doesn't really work to pass one in. But I agree that having a more general configuration would be better, maybe something like WTF_CSRF_SIGNER and WTF_CSRF_SIGNER_KWARGS?

@phinjensen

Copy link
Copy Markdown
Author

@davidism I just pushed some commits that address all of your concerns, including making the Signer as a whole configurable. let me know what you think.

@phinjensen

Copy link
Copy Markdown
Author

@davidism Any thoughts on this?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Error generating tokens on FIPS-enabled systems: Unsupported digestmod <function _lazy_sha1>

2 participants