Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(integrity check): update hash + handle collectstatic #10241

Merged
merged 4 commits into from
May 22, 2024

Conversation

kiblik
Copy link
Contributor

@kiblik kiblik commented May 21, 2024

This PR fix:

@kiblik kiblik changed the title trigger fix(integrity check): update hash May 21, 2024
Copy link
Contributor

@manuel-sommer manuel-sommer left a comment

Choose a reason for hiding this comment

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

@kiblik : I don't have enough insights into the release process, but maybe it makes sense to add automatic recalculation for the release process.
Let's assume hash changes in both bugfix and dev branches. Then, the new release should have new hash codes automatically recalculated and commited for all branches. Is this automatically possible somehow?

Copy link

dryrunsecurity bot commented May 21, 2024

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Configured Codepaths Analyzer 0 findings
Secrets Analyzer 0 findings
Sensitive Files Analyzer 1 finding
AppSec Analyzer 0 findings
Authn/Authz Analyzer 0 findings

Note

🟢 Risk threshold not exceeded.

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The changes in this pull request focus on improving the security and integrity of the Django application's configuration settings. The key changes include:

  1. Updating the SHA-256 hash value for the .settings.dist.py.sha256sum file, which suggests that the underlying .settings.dist.py configuration file has been updated. It's important to review the actual changes to the configuration file to ensure that no security-sensitive information has been introduced or modified.

  2. Modifying the condition for checking the DEBUG setting in the settings.py file. The changes now allow editing the settings.dist.py file during the collectstatic command execution, which could potentially expose sensitive information. This change should be carefully reviewed to ensure that no security vulnerabilities are introduced.

  3. Implementing a security-conscious approach to verifying the integrity of the settings.dist.py file using SHA-256 hashes. This helps mitigate the risk of unauthorized modifications to the critical configuration settings.

Files Changed:

  1. dojo/settings/.settings.dist.py.sha256sum: The SHA-256 hash value for the .settings.dist.py file has been updated, suggesting that the underlying configuration file has been modified. It's important to review the actual changes to ensure that no security-sensitive information has been introduced or modified.

  2. dojo/settings/settings.py: The code changes in this file include:

    • Importing the sys module, which allows accessing system-level variables and functionality. This should be reviewed to ensure that any access to system-level resources is properly handled.
    • Modifying the condition for checking the DEBUG setting to also allow editing the settings.dist.py file during the collectstatic command execution. This change could potentially expose sensitive information and should be carefully reviewed.
    • Implementing a security-conscious approach to verifying the integrity of the settings.dist.py file using SHA-256 hashes. This helps ensure the authenticity of the critical configuration settings.

Powered by DryRun Security

@kiblik kiblik changed the title fix(integrity check): update hash fix(integrity check): update hash + handle collectstatic May 21, 2024
@kiblik kiblik marked this pull request as ready for review May 21, 2024 11:16
@Maffooch
Copy link
Contributor

I agree with @manuel-sommer that would be the ideal approach imo. Changes could be made

This should probably wait until after this PR is merged to get the dev branch corrected sooner

Copy link
Contributor

@cneill cneill left a comment

Choose a reason for hiding this comment

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

I'm a bit confused about why we have to create an exception for collectstatic since I can't see any reason the hash of settings.dist.py file would differ from our expected value in that step, but in the interest of getting this fixed quickly we can leave it for now.

@quirinziessler
Copy link
Contributor

I'm a bit confused about why we have to create an exception for collectstatic since I can't see any reason the hash of settings.dist.py file would differ from our expected value in that step, but in the interest of getting this fixed quickly we can leave it for now.

@cneill please see my comment on #10212 - the integrity checker wrongly stopped the build process when running ./dc-build.sh when running in debug mode. Thanks @kiblik for looking into this so quickly.

Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

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

Approved

@cneill
Copy link
Contributor

cneill commented May 21, 2024

I'm a bit confused about why we have to create an exception for collectstatic since I can't see any reason the hash of settings.dist.py file would differ from our expected value in that step, but in the interest of getting this fixed quickly we can leave it for now.

@cneill please see my comment on #10212 - the integrity checker wrongly stopped the build process when running ./dc-build.sh when running in debug mode. Thanks @kiblik for looking into this so quickly.

Right, I can see that it has been failing at that step in our unit tests runs, but taking this branch and removing the collectstatic condition from the settings.py check still doesn't produce issues for me. Changing the profile/etc has no effect. I can only get it to fail when I modify the checksum to be intentionally incorrect.

@quirinziessler
Copy link
Contributor

I'm a bit confused about why we have to create an exception for collectstatic since I can't see any reason the hash of settings.dist.py file would differ from our expected value in that step, but in the interest of getting this fixed quickly we can leave it for now.

@cneill please see my comment on #10212 - the integrity checker wrongly stopped the build process when running ./dc-build.sh when running in debug mode. Thanks @kiblik for looking into this so quickly.

Right, I can see that it has been failing at that step in our unit tests runs, but taking this branch and removing the collectstatic condition from the settings.py check still doesn't produce issues for me. Changing the profile/etc has no effect. I can only get it to fail when I modify the checksum to be intentionally incorrect.

@cneill it's all about when developing and testing things. When preparing a fix for an Issue e.g. for dedupe, this behavior hinders contributor to correctly test their stuff. Skipping the file is the best way to provide a smooth access for contributor to change Hash fields as well as other stuff there and test it correctly. Please see this as a Hotfix, until the hash code is created during release. It is really annoying right now.

@mtesauro mtesauro merged commit 94abb0d into DefectDojo:dev May 22, 2024
123 checks passed
@kiblik kiblik deleted the fix_integrity_check branch May 22, 2024 18:30
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.

7 participants