Vat v2 #1
Vat v2 #1
Conversation
-removed db -created new packages
| for _, h := range a.DiscoveredTargets { | ||
| if (h.ActualStatus() == New) || (h.ActualStatus() == Expired) { | ||
| wg.Add(1) | ||
| temp := h |
There was a problem hiding this comment.
👍
or could have been passed as an argument to the anonymous function launched on the go routine
cmd/vat/analysis/analyzer.go
Outdated
| } | ||
|
|
||
| func (a *Analyzer) DiscoverTargets() { | ||
| a.DiscoveredTargets = a.discoverer.DiscoverNewTargets(a.DiscoveredTargets) |
There was a problem hiding this comment.
there are some concurrency issues here as someone else can call this method in an asynchronous manner.
We have a sort of "good practices guidelines" that state the following:
- Try to make all exported items concurrent safe as someone else can misuse your components leading to some hard to find bugs
- Unless we are talking about a struct used as a DTO (Data Transfer Object), try to export as few as you can properties in order to help achieve point 1 & increase the possibility that the struct can implement a defined interface (this will help code decoupling & creation of unit tests)
- You can have concurrent "unsafe" methods, but try to keep them unexported so no other package can mess with them.
cmd/vat/analysis/analyzer.go
Outdated
|
|
||
| var log = logger.GetOrCreate("vat") | ||
|
|
||
| func NewAnalyzer(discoverer Discoverer, sf ScannerFactory, pf ParserFactory) (*Analyzer, error) { |
There was a problem hiding this comment.
we generally add comments on all exported items
| if (h.ActualStatus() == New) || (h.ActualStatus() == Expired) { | ||
| wg.Add(1) | ||
| temp := h | ||
| go func() { |
There was a problem hiding this comment.
in the future, you might think of a way to create only a predefined number of workers in order to not cripple the host in case a large bunch of targets are discovered at once.
There was a problem hiding this comment.
True. Maybe a queue list with no more than 10 workers deployed at once.
Thanks.
cmd/vat/analysis/analyzer.go
Outdated
| } | ||
|
|
||
| func (a *Analyzer) changeTargetStatus(address string, status utils.TargetStatus) string { | ||
| for idx, _ := range a.DiscoveredTargets { |
There was a problem hiding this comment.
concurrent problems detected here as a.DiscoveredTargets can be asynced changed. A mutex locking will properly fix this
|
|
||
| var log = logger.GetOrCreate("vat") | ||
|
|
||
| type NmapScanner struct { |
There was a problem hiding this comment.
not a concurrent safe struct
cmd/vat/scan/politeScanner.go
Outdated
|
|
||
| // InsecureIgnoreHostKey returns a function that can be used for ClientConfig.HostKeyCallback to accept any host key. | ||
| sshConfig.HostKeyCallback = ssh.InsecureIgnoreHostKey() | ||
| _, err = ssh.Dial("tcp", fmt.Sprintf("%s:%d", pS.Host, pS.Port), sshConfig) |
There was a problem hiding this comment.
interface separation required in order to make the unit tests pass on all hosts.
cmd/vat/scan/port.go
Outdated
| if p.isPortInRange(37373, 38383) { | ||
| p.Type = utils.ElrondPort | ||
| p.Importance = utils.JudgementNoRisk | ||
| } else if (p.Number == 80) || (p.Number == 8080) { |
There was a problem hiding this comment.
else(s) can be removed by the help of early exists
cmd/vat/utils/constants.go
Outdated
| @@ -0,0 +1,148 @@ | |||
| package utils | |||
There was a problem hiding this comment.
what the "util" package should contain? Strongly suggest to dissolve this package and move the consts where used or, in the case of consts used in multiple package, can create a "core" package just for those very-high level definitions
There was a problem hiding this comment.
utils was intended as the core .
change done. all constants left in core package are used in multiple packages.
cmd/vat/utils/constants.go
Outdated
| type Judgement string | ||
|
|
||
| const ( | ||
| JudgementSshUserPermited Judgement = "5$ SMALL RISK - SSH User permitted" |
There was a problem hiding this comment.
what does the "$" should suppose to mean? :D
| "golang.org/x/crypto/ssh" | ||
| ) | ||
|
|
||
| type sshDialer struct { |
please review.
TBD:
Analysis result example:
trace.txt