fix: paranoid mode now blocks susp packages as like malicious.#152
fix: paranoid mode now blocks susp packages as like malicious.#152atharvamhaske wants to merge 1 commit intosafedep:mainfrom
Conversation
SafeDep Report Summary
No dependency changes detected. Nothing to scan. This report is generated by SafeDep Github App |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Sahilb315
left a comment
Sahilb315
left a comment
There was a problem hiding this comment.
@atharvamhaske There are 2 flows in pmg. One is proxy & other is guard flow which would also require changes.
| if cfg.Config.Paranoid { | ||
| log.Warnf("[%s] Package %s/%s@%s is suspicious and paranoid mode is enabled, blocking", | ||
| ctx.RequestID, ecosystem.String(), packageName, packageVersion) | ||
|
|
||
| eventlog.LogMalwareBlocked(packageName, packageVersion, ecosystem.String(), result.Summary, map[string]interface{}{ | ||
| "analysis_id": result.AnalysisID, | ||
| "reference_url": result.ReferenceURL, | ||
| }) | ||
|
|
||
| if b.statsCollector != nil { | ||
| b.statsCollector.RecordBlocked(result) | ||
| } | ||
|
|
||
| message := fmt.Sprintf("Suspicious package blocked in paranoid mode: %s/%s@%s\n\nReason: %s\n\nReference: %s", | ||
| ecosystem.String(), | ||
| packageName, | ||
| packageVersion, | ||
| result.Summary, | ||
| result.ReferenceURL, | ||
| ) | ||
|
|
||
| return &proxy.InterceptorResponse{ | ||
| Action: proxy.ActionBlock, | ||
| BlockCode: http.StatusForbidden, | ||
| BlockMessage: message, | ||
| }, nil | ||
|
|
||
| } |
There was a problem hiding this comment.
As the paranoid policy needs to be applied in 2 different flows, please create a common function which takes result *analyzer.PackageVersionAnalysisResult as input & based on the config (paranoid flag) it returns the Action to be performed (ActionConfirm or ActionBlock). This way both flows can depend on the returned Action and source of truth is not duplicated across both flows
There was a problem hiding this comment.
where n which folder i should keep this common function? @Sahilb315
There was a problem hiding this comment.
I think a better place to handle this will be in the analyser itself that makes the decision if the action is to block or query. Then the flows need not worry about it at all.
But there is an issue.
When paranoid mode is enabled, we are choosing the active analyser.
@Sahilb315 I think the active analyzer just don't fit in to PMG. We should get rid of it. But keep the paranoid mode as an opinionated high security mode where
- We consider suspicious as malicious (change in analyzer)
- Enforce sandbox as if
--sandbox-enforceis enabled
Future security choices should honour --paranoid and choose the most secure options when set.
Thoughts?
There was a problem hiding this comment.
I think a better place to handle this will be in the analyser itself that makes the decision if the action is to block or query. Then the flows need not worry about it at all.
I had thought of it but it would have made the active_scan_analyser coupled with config. And the job of an analyser is to analyse an package & return the result (independent concern), making changes directly would lead it to being an dependent analyser / concern.
Thoughts?
Agreed. For an package installation (which needs to be fast), active analyser doesn't make sense which takes a lot of time for each package and also creates a bad UX (even though its an opt-in feature)
There was a problem hiding this comment.
Introduce a new package named policy
Lets not name it policy please. We have plans to implement a policy layer in PMG where users can write policy in CEL (like we do in vet), to evaluate proxy request and response and produce an action for the proxy to ensure. In simple terms, I can write a policy to block or filter out all packages published in less than 3 days ago. This requires thinking and writing the specs. But policy package is reserved for that.
Introduce a policy (or maybe just a function) which checks paranoid flag, and takes result *analyzer.PackageVersionAnalysisResult as an input to decide the Action
Currently analyzer makes this decision. The reason it is like this is because Analyzer (like Malysis) knows the context of the risk. For example suspicious, malicious etc. is a concept of Malysis and not a generic concep. So it can make a decision (Action) based on config.
Similarly, in future, we may choose to introduce a vulnerability analyser which blocks packages with critical vulnerabilities. Not saying we will, but we may. At what severity to block or confirm with user, is a function of config + analyzer type. Which is I feel it is already coupled with the analyzer and should not be separated into a different concern because it will anyway depend (coupled) with analyzers because it need the analyzer output to make a decision.
There was a problem hiding this comment.
Umm so i will change the name i started with folder named policy what should we keep this as? I take anaylzer as input and then decide it i have sent images on dc.
There was a problem hiding this comment.
@atharvamhaske Two things, feel free to do it in separate PR
- Deprecate the
malysis_active_scan.goanalyzer and make sure Query analyzer is always used - In Query analyzer, based on config (paranoid mode), return appropriate action
[2] is important logic. Must have appropriate test cases.
3 participants
Fixed the issue of paranoid mode of treating suspicious pkg as malicious pkg and block installation w/o asking user permission.
passed test: added test for new code addition
This PR fixes #91 issue.
let me know if any changes or suggestions.
cc: @Sahilb315