[DPE-5324] Increase ruff rules #405
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #405 +/- ##
==========================================
+ Coverage 71.35% 71.64% +0.29%
==========================================
Files 9 9
Lines 1323 1319 -4
Branches 247 245 -2
==========================================
+ Hits 944 945 +1
+ Misses 296 292 -4
+ Partials 83 82 -1 ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -87,5 +87,6 @@ | |||
|
|
|||
| def get_hashed_password(username: str, password: str) -> str: | |||
| """Creates an md5 hashed password for the given user, in the format postgresql expects.""" | |||
| hash_password = md5((password + username).encode()).hexdigest() | |||
| # Should be handled in DPE-1430 | |||
| hash_password = md5((password + username).encode()).hexdigest() # noqa: S324 | |||
Check failure
Code scanning / CodeQL
Use of a broken or weak cryptographic hashing algorithm on sensitive data High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 3 months ago
To fix the problem, we should replace the MD5 hashing algorithm with a stronger, modern cryptographic hash function. For password hashing, it is recommended to use algorithms like Argon2, bcrypt, or PBKDF2, which are designed to be computationally expensive and include a salt to protect against brute-force attacks.
The best way to fix this without changing existing functionality is to use the argon2-cffi library to hash the password. This library provides a secure and efficient way to hash passwords.
Steps to fix:
- Install the
argon2-cffilibrary if it is not already installed. - Import the
PasswordHasherclass from theargon2module. - Replace the MD5 hashing logic with the
PasswordHasherclass to hash the password.
| @@ -25,3 +25,3 @@ | ||
| import string | ||
| from hashlib import md5 | ||
| from argon2 import PasswordHasher | ||
| from typing import Dict | ||
| @@ -88,5 +88,5 @@ | ||
| def get_hashed_password(username: str, password: str) -> str: | ||
| """Creates an md5 hashed password for the given user, in the format postgresql expects.""" | ||
| # Should be handled in DPE-1430 | ||
| hash_password = md5((password + username).encode()).hexdigest() # noqa: S324 | ||
| return f"md5{hash_password}" | ||
| """Creates a hashed password for the given user using Argon2.""" | ||
| ph = PasswordHasher() | ||
| hash_password = ph.hash(password + username) | ||
| return hash_password |
| @@ -7,2 +7,3 @@ | ||
| [tool.poetry.dependencies] | ||
| argon2-cffi = "23.1.0" | ||
| python = "^3.10" |
| Package | Version | Security advisories |
| argon2-cffi (pypi) | 23.1.0 | None |
There was a problem hiding this comment.
If only it were that easy.
There was a problem hiding this comment.
so funny how it intentionally removes the in the format postgresql expects part
| # Labels are not confidential | ||
| SECRET_LABEL = "secret" # noqa: S105 | ||
| MONITORING_PASSWORD_KEY = "monitoring_password" # noqa: S105 | ||
| SECRET_INTERNAL_LABEL = "internal-secret" # noqa: S105 | ||
| SECRET_DELETED_LABEL = "None" # noqa: S105 |
There was a problem hiding this comment.
Security checks will inevitably produce some false positives, so we will have to noqa them. I propose adding a comment on top of noqa blocks, so that we explain why the check is disabled.
| @@ -5,7 +5,7 @@ | |||
| package-mode = false | |||
|
|
|||
| [tool.poetry.dependencies] | |||
| python = "^3.8.10" | |||
| python = "^3.10" | |||
There was a problem hiding this comment.
K8s charm is only running on jammy, so bump the version to be able to update coverage: #440
There was a problem hiding this comment.
It the lib/code sharing will not be the problem: ACK.
There was a problem hiding this comment.
This is a charm specific dependency, so it shouldn't affect the lib. I think all the linting so far should be 3.8 compatible.
|
|
||
| [tool.ruff.lint.flake8-copyright] | ||
| # Check for properly formatted copyright header in each file | ||
| author = "Canonical Ltd." | ||
| notice-rgx = "Copyright\\s\\d{4}([-,]\\d{4})*\\s+" | ||
| min-file-size = 1 |
There was a problem hiding this comment.
Don't expect a copyright header for empty files.
dragomirp
requested review from
a team,
taurus-forever,
marceloneppel and
lucasgameiroborges
and removed request for
a team
Add more linting rules and change and suppress the reported issues. Added rules are: