Git issue #5009 Asset cache can lead to denial of service if asset database is deleted -Fix#5050
Git issue #5009 Asset cache can lead to denial of service if asset database is deleted -Fix#5050SudhanshuBawane wants to merge 9 commits intodevelop/6from
Conversation
| 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) |
There was a problem hiding this comment.
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
sensu-go/backend/pipeline/filter/legacy.go
Line 126 in 76c7a16
| 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 |
There was a problem hiding this comment.
Nitpick: no need to add your name and issue number :)
|
A couple of minor things but looks good otherwise! |
|
Don't forget to add a changelog entry. |
echlebek
left a comment
echlebek
left a comment
There was a problem hiding this comment.
Please add a test that shows that the boltdb manager now exhibits correct behaviour when the asset.db has been deleted.
|
|
||
| // cleanup of the assetSHA when cache dir gets force deleted | ||
| func CleanUp(fullPath string) error { | ||
| return os.RemoveAll(fullPath) | ||
| } |
There was a problem hiding this comment.
Please remove this and use os.RemoveAll directly in boltdb_manager.go.
|
|
||
| // ---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") | ||
| } | ||
| } |
There was a problem hiding this comment.
This only tests os.RemoveAll and is therefore unnecessary
|
May I know what kind of behavior is expected like what I need to check ? |
Signed-off-by: SudhanshuBawane <sudhanshu.bawane.ctr@sumologic.com>
Signed-off-by: SudhanshuBawane <sudhanshu.bawane.ctr@sumologic.com>
3 participants
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
Changed
Fixed
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.