search

tjcsl / director4

Director 4: Website Hosting for the Masses
https://director.tjhsst.edu
MIT License
9 stars 12 forks source link

manager: Update secret key generation comment #15

Closed sumanthratna closed 4 years ago

sumanthratna commented 4 years ago

very small + insignificant PR that suggests that users use Django's secret key generation functon instead of random.SystemRandom()

etnguyen03 commented 4 years ago

I don't see this function mentioned anywhere in the Django docs. Is this an internal function?

It's defined here.

ghost commented 4 years ago

@etnguyen03 yes, I found that in the source too. However, it's not documented on http://docs.djangoproject.com/.

It definitely looks like it's supposed to be internal, and I don't want to recommend use of internal functions that could change. Besides, get_random_secret_key() appears to only choose from ascii lowercase + digits + selected punctuation, so the currently recommended method should actually have greater entropy.

sumanthratna commented 4 years ago

I don't want to recommend use of internal functions that could change.

I agree, but even if the method does change, why wouldn't we want to use Django's updated method? I assume that if the Django team changes a function, it's either an improvement or it limits functionality due to potential errors from other parts of the codebase. Both of these seem like changes we should follow in Director's codebase.

get_random_secret_key() appears to only choose from ascii lowercase + digits + selected punctuation, so the currently recommended method should actually have greater entropy.`

True, but going off of what I just mentioned, don't we want to stick to Django's method for creating secrets (for consistency)?

@anonymoose2 did bring up good points; feel free to close this if you're still concerned about recommending an internal function

ghost commented 4 years ago

Some more thoughts.

I don't want to recommend use of internal functions that could change.

I agree, but even if the method does change, why wouldn't we want to use Django's updated method? I assume that if the Django team changes a function, it's either an improvement or it limits functionality due to potential errors from other parts of the codebase. Both of these seem like changes we should follow in Director's codebase.

Internal methods could change in other ways that would make this stop working. For example, the Django team could move it to another module.

In my opinion, you should only use internal functions if there is no good alternative.

get_random_secret_key() appears to only choose from ascii lowercase + digits + selected punctuation, so the currently recommended method should actually have greater entropy.

True, but going off of what I just mentioned, don't we want to stick to Django's method for creating secrets (for consistency)?

I don't see why we necessarily should, especially if they result in lower entropy.

In my opinion, 1) this isn't a huge deal either way, but 2) I don't really see a benefit in switching from recommending a more-secure, future-proof code snippet to recommending a less-secure code snippet that uses an internal function and could break at any time without warning.

@etnguyen03 I would recommend closing this PR.