allow setting django secret with enironment variable #8566
Conversation
WalkthroughThe changes made in the pull request focus on the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
cvat/settings/base.py (2)
76-82: Approve changes to SECRET_KEY import logic with a minor suggestion.The modifications to the SECRET_KEY import logic improve the robustness of secret key management. It now allows for multiple ways to set the secret key (environment variable, file, or auto-generation) with a good fallback mechanism.
Consider adding a comment to explain the fallback logic for better clarity:
if not SECRET_KEY: + # Attempt to import SECRET_KEY from file, generate if not found try: sys.path.append(BASE_DIR) from keys.secret_key import SECRET_KEY # pylint: disable=unused-import except ModuleNotFoundError: generate_secret_key() from keys.secret_key import SECRET_KEY
43-43: Overall approval with suggestion for documentation update.The changes successfully implement the ability to set the Django secret key using an environment variable while maintaining backwards compatibility. This enhancement aligns well with the PR objectives and improves flexibility for containerized environments.
Consider updating the project documentation to reflect these changes:
- Explain the new method of setting the secret key via the
DJANGO_APP_SECRETenvironment variable.- Describe the fallback mechanism and the order of precedence for secret key sources.
- Provide best practices for secret management in different deployment scenarios.
This documentation will help administrators understand and properly utilize the new secret management options.
Also applies to: 76-82
| @@ -40,6 +40,7 @@ | |||
|
|
|||
| ALLOWED_HOSTS = os.environ.get('ALLOWED_HOSTS', 'localhost,127.0.0.1').split(',') | |||
| INTERNAL_IPS = ['127.0.0.1'] | |||
| SECRET_KEY = os.environ.get("DJANGO_APP_SECRET", "") | |||
There was a problem hiding this comment.
Improve secret key handling to enhance security.
While using an environment variable for the SECRET_KEY is a good practice, defaulting to an empty string could pose a security risk if the environment variable is not set. Consider raising an exception or generating a random secret key if the environment variable is missing.
Here's a suggested improvement:
-SECRET_KEY = os.environ.get("DJANGO_APP_SECRET", "")
+SECRET_KEY = os.environ.get("DJANGO_APP_SECRET")
+if not SECRET_KEY:
+ raise ImproperlyConfigured("DJANGO_APP_SECRET environment variable is not set")This change ensures that the application won't start with an empty secret key, reducing the risk of running with insecure settings.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SECRET_KEY = os.environ.get("DJANGO_APP_SECRET", "") | |
| SECRET_KEY = os.environ.get("DJANGO_APP_SECRET") | |
| if not SECRET_KEY: | |
| raise ImproperlyConfigured("DJANGO_APP_SECRET environment variable is not set") |
#8565
Motivation and context
In container deployment scenarios, it may be cleaner for an admin to provide an app secret via env vars.
This may also be a solution for #8002, if you don't want fix in #8003.
How has this been tested?
Ran this is k8s environment.
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
SECRET_KEYfor enhanced application security and robustness.SECRET_KEYto prevent application errors if the key is missing.