#30360 -- Support rotation of secret keys

terrameijar · November 16, 2020, 9:17pm

@carltongibson
The test run gives me this
FAILED (failures=1, errors=50, skipped=1169, expected failures=4)

Are the 50 errors expected?

The failing test is this one:

terrameijar/django/blob/fix_30360_pr/tests/settings_tests/tests.py#L500


def test_secret_key(self):
    settings = self.make_settings(SECRET_KEY='foo')
    self.assertEqual(settings.SECRET_KEY, 'foo')
    self.assertEqual(settings.SECRET_KEYS, ['foo'])
def test_secret_keys(self):
    settings = self.make_settings(SECRET_KEYS=['foo', 'bar'])
    self.assertEqual(settings.SECRET_KEY, 'foo')
    self.assertEqual(settings.SECRET_KEYS, ['foo', 'bar'])
def test_both_specified(self):
    msg = 'Only one of SECRET_KEY and SECRET_KEYS must be specified, not both.'
    with self.assertRaisesRegex(ImproperlyConfigured, msg):
        self.make_settings(SECRET_KEY='a', SECRET_KEYS=['a'])
def test_empty(self):
    msg = 'The SECRET_KEY or SECRET_KEYS setting must not be empty.'
    with self.assertRaisesRegex(ImproperlyConfigured, msg):
        self.make_settings()

I understand what the test does but I am unsure how to make it pass. I tried to remove the assertRaisesRegex(ImproperlyConfigured, msg) call but the test raises an ImproperlyConfigured error either way.

carltongibson · November 17, 2020, 7:26am

Hi @terrameijar — no the errors aren’t expected. (Compare running the tests on master…)

What’s happened is the patch has made some changes which have broken other changes which were subsequently made. (Florian’s PR that moved the secret key check to first access most likely.) So we need to look at the errors and work out what changed. It’s a puzzle.

I’ll give it a run today, and see if I can spot the issue. (Likely it’s one thing causing all the errors.)

carltongibson · November 17, 2020, 11:08am

Hi @terrameijar.

I had a quick look. The various errors were caused by a bad rebase on tokens.py. I looked at the error, looked at the diff, and the file history, and adjusted to fix it here.

That just leave the SECRET_KEY failures which I think are coming from the block in django/conf/__init__.py — That logic was adjusted to make the check on first access — See my comment on the PR — Have a look at the linked commit there and see if you can work out how to update to match the intent of both commits to resolve. (Hopefully that’s the fun bit — let me know if you need input!)

Once we have the tests passing, we can work on the documentation and release notes — There’s a new setting, and here’s how you use it.

carltongibson · November 24, 2020, 11:21am

Hey @terrameijar — did you have any bandwidth to get back to this PR as yet? Do you need any input? Do you think you will have capacity? (No stress my end — I just want to make sure you’ve got support that you need.)

terrameijar · November 25, 2020, 2:25pm

Hi @carltongibson, sorry I had not, it has been pretty busy on my end. I had a look and will post an update to the PR with questions later today. I think i’m on the right track but need clarification on one small thing. Thanks

carltongibson · November 25, 2020, 2:38pm

Hey @terrameijar — no problem — you always look like you’re doing loads of stuff!

terrameijar · November 26, 2020, 9:47pm

I’ve just pushed a commit to the PR to address the failing tests. One test still fails. Please review when you have a moment and let me know if i’m on the right track @carltongibson. Thank you. I appreciate your help with this.

carltongibson · November 27, 2020, 9:53am

Super. Let me have a look and I’ll get back to you.

terrameijar · December 18, 2020, 1:48pm

Hey @carltongibson Were you able to take a look at this? I’ll have some free time next week and would like to work on it. Thank you.

carltongibson · December 18, 2020, 2:23pm

HI @terrameijar, I hope you’re well!

Were you able to take a look at this?

Yes I did. I got so far through updating the SECRET_KEY validation move, and then had to stop. It’s a bit more complex than it was previously, because we need to check both possibilities and validate on first access, and then adjust the tests for those.

It’s on my list to pick up.

I’ll have some free time next week and would like to work on it.

Super! If we assume I’ll resolve the test failure, the thing that’s not there at all (and was really the TODO before all this started) was the Docs and Release Notes changes.

So, (first) to add docs for the SECRET_KEYS setting, and then (second) a release note matching that. If you want to focus there, that would be awesome! Just ping me here or on GitHub if you need input.

For reference, the feature freeze for Django 3.2 is January 14th. That’s our deadline. It has to be merged by then.

Thanks for the follow up! Good hustle.

terrameijar · January 12, 2021, 4:57am

Hey @carltongibson . My apologies for the late response. I really wanted to do this but I haven’t been able to, there’s been a lot happening on my end and I just couldn’t get the time. Let me try again with a different ticket when things settle down for me.

Thank you.

carltongibson · January 25, 2021, 5:17pm

Hey @terrameijar — sorry for the slow reply — I’ve only just been head about water for the last few weeks.

No stress at all! Life > OSS. Never forget that.

In the end we bumped the ticket to 4.0, so it’s on my list to pick it up over the early part of the cycle. If you’d like to carry on with it, please take a look at the new branch (although it needs rebasing again — Or, if you fancy working on another ticket, that’s cool too. If you need input on that let me know — but as time and space allow for you — no pressure our end.

Kind regards, keep safe.

Topic		Replies	Views
Unsure of how to contribute to issue 32819 Mentorship	14	538	October 12, 2022
Getting feedback on WIP patches Mentorship	6	741	October 22, 2020
Main development branch renamed. Announcements	1	5265	March 9, 2021
how to revise other people's code ? Django Internals	4	212	January 16, 2024
Technical Board vote on DEP 44: Clarifying Release Process Django Internals	6	532	December 14, 2022

#30360 -- Support rotation of secret keys

Related topics