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

Git issue #5009 Asset cache can lead to denial of service if asset database is deleted -Fix #5050

Open
wants to merge 9 commits into
base: develop/6
Choose a base branch
from

Conversation

SudhanshuBawane
Copy link

Closed #5009

Description

The change is regarding the denial of service from agent when asset.db gets deleted due to some external scenario.
Which creates a DOS as SHA contains the some reference to the previous asset.db and hence forth the new one does not get created properly.

Change in behavior

To prevent the DOS of the agent in above scenario and keep the agent working properly. For new asset.db creation.

Added

  • The new cleanUp function for cleaning up the SHA dir from cache to prevent a DOS in case of asset.db deletion.
  • Added the test to check all scenarios.

Changed

  • The expandWithDuration function in boltdb_manager has been changed a little for the cleanUp function support.

Fixed

  • This fixes the DOS in agent in case of asset database is deleted.

Change verification

The changes can be verified not only by the test cases but also by checking the same behavior. That is delete the asset.db and make note of it's size. Then re-run the agent and it will run without populating error file exits and asset.db get recreated with same size.

Signed-off-by: Sudhanshu Bawane <[email protected]>
@SudhanshuBawane SudhanshuBawane marked this pull request as ready for review February 1, 2024 05:31
fullPath := filepath.Join(CacheDir, assetSHA)

if err := CleanUp(fullPath); err != nil { //fix for git issue 5009
logger.Printf("error cleaning up the SHA dir: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if more context information such as the SHA and full path could be provided with the log message. You can use WithFields and WithError .

Additionally please use an explicit log level such as Errorf or Warnf instead of using the generic Printf. In this case I believe warning would be an appropriate log level.

You can see an example at

logger.WithFields(fields).WithError(err).

@@ -90,3 +91,14 @@ func sniffType(f io.ReadSeeker) (filetype_types.Type, error) {

return ft, nil
}

// Sudhanshu - CleanUp the SHA for the git issue 5009 fix. Making sure that in case of DOS when asset.db gets deleted it gets cleanUp so that asset can be re-downloded
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: no need to add your name and issue number :)

@fguimond
Copy link
Contributor

A couple of minor things but looks good otherwise!

Signed-off-by: SudhanshuBawane <[email protected]>
@fguimond
Copy link
Contributor

Don't forget to add a changelog entry.

Signed-off-by: SudhanshuBawane <[email protected]>
asset/expander.go Outdated Show resolved Hide resolved
CHANGELOG-6.md Outdated Show resolved Hide resolved
Signed-off-by: SudhanshuBawane <[email protected]>
Copy link
Contributor

@echlebek echlebek left a comment

Choose a reason for hiding this comment

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

Please add a test that shows that the boltdb manager now exhibits correct behaviour when the asset.db has been deleted.

Comment on lines 94 to 98

// cleanup of the assetSHA when cache dir gets force deleted
func CleanUp(fullPath string) error {
return os.RemoveAll(fullPath)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this and use os.RemoveAll directly in boltdb_manager.go.

Comment on lines 116 to 145

// ---Test to check CleanUp
func TestCleanUp(t *testing.T) {
t.Parallel()

// Create a temporary directory for testing
tmpDir := t.TempDir()

// Define the SHA and file name
SHAName := "shaAsset.tar"
SHAFilePath := filepath.Join(tmpDir, SHAName)

// Create a dummy file inside the temporary directory
SHAFile, err := os.Create(SHAFilePath)
if err != nil {
t.Fatalf("Failed to create dummy file: %v", err)
}
SHAFile.Close()

// Call CleanUp with the SHA of the dummy file and the temporary directory
err = CleanUp(SHAFilePath)
if err != nil {
t.Errorf("CleanUp returned an error: %v", err)
}

_, err = os.Stat(SHAFilePath)
if !os.IsNotExist(err) {
t.Errorf("CleanUp did not remove the dummy file as expected")
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This only tests os.RemoveAll and is therefore unnecessary

Signed-off-by: SudhanshuBawane <[email protected]>
Signed-off-by: SudhanshuBawane <[email protected]>
@SudhanshuBawane
Copy link
Author

Please add a test that shows that the boltdb manager now exhibits correct behaviour when the asset.db has been deleted

@SudhanshuBawane
Copy link
Author

Please add a test that shows that the boltdb manager now exhibits correct behaviour when the asset.db has been deleted

May I know what kind of behavior is expected like what I need to check ?

Signed-off-by: SudhanshuBawane <[email protected]>
Signed-off-by: SudhanshuBawane <[email protected]>
Signed-off-by: SudhanshuBawane <[email protected]>
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.

Asset cache can lead to denial of service if asset database is deleted
3 participants