Skip to content

Conversation

claudep
Copy link
Member

@claudep claudep commented Jan 29, 2020

This commit prepares the way for different algorithms.

@claudep
Copy link
Member Author

claudep commented Jan 29, 2020

I'm not sure about the deprecation period for the legacy format. I wonder if we should wait for 4.1, so as when Django 4.0 is out, it is still able to unsign a Django2.2 value (still supported at that time).

@claudep claudep requested a review from charettes January 29, 2020 17:56
Copy link
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

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

Thanks for spearheading this work @claudep 🙇

Except for the comments I left I had a more general question. Now that we add support for multiple digestmods it feels like it could be valuable to have unsign and loads accept a set of allowed algorithms. This way you could ensure that only values signed with a specified algorithm are considered valid without subclassing Signer.

I realize it might be more appropriate to discuss this on the mailing list but I thought I'd throw the idea around here to gather some feedback given you're already spent a bit of time tackling this issue.

@claudep
Copy link
Member Author

claudep commented Jan 30, 2020

Absolutely, the idea is to allow a set of approved algorithms in valid_algorithms (see #12291). This PR was dedicated to just add the (current used) algorithm in the signature. This is just a step towards the goal of changing the default Signer algorithm, while keeping compatibility with current hashes.

This commit prepares the way for different algorithms.

Co-Authored-By: Simon Charette <charettes@users.noreply.github.com>
@apollo13
Copy link
Member

@PaulMcMillan @ubernostrum could we have some input on those changes?

@ubernostrum
Copy link
Member

Like @charettes I would want to see support in unsign() and loads() for specifying the expected algorithm, and I think it's important to have that happen as part of this PR. I also wonder whether that should just become the interface, skipping the valid_algorithms attribute altogether.

The thing I mostly want to avoid is a situation where we do negotiation on the algorithm, because that puts the ultimate choice in the hands of an attacker: if they know a Django install accepts any out of a list of algorithms, they can choose the worst/weakest one and craft a value to exploit it. Which is generally why things like cipher negotiation are frowned on in cryptography.

So what would people think about making the signatures of unsign() and loads() start taking the extra parameter now, and default it to SHA-1? Then when we're ready we could change the default, do a short deprecation cycle in which we still support SHA-1 fallback even though it's no longer the specified allowed algorithm, and then drop it altogether.

@claudep
Copy link
Member Author

claudep commented Feb 13, 2020

Thanks ubernostrum for your input.
I'll rework this patch. In fact, my plan is to set the algorithm in the Signer.__init__ signature. In this way, the unsign process can limit the accepted algorithm to this selected algorithm only. The question is whether we should provide some sort of legacy algorithm support to ease algorithm changes from API clients.

@claudep
Copy link
Member Author

claudep commented Feb 13, 2020

@ubernostrum, is #12454 more in line with what you imagined?

@claudep
Copy link
Member Author

claudep commented Mar 5, 2020

We'll concentrate on PR #12454.

@claudep claudep closed this Mar 5, 2020
@claudep claudep deleted the 27468d branch February 10, 2021 11:41
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.

5 participants