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

Vat v2 #1

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Vat v2 #1

wants to merge 30 commits into from

Conversation

schimih
Copy link
Owner

@schimih schimih commented Jan 30, 2022

please review.

TBD:

  • manager to collaborate via GIN and add requested data;
  • tests with real data for interpreters;
  • add errors;
  • ssh algos if needed;
  • brute force if permitted;
  • etc.

Analysis result example:
trace.txt

for _, h := range a.DiscoveredTargets {
if (h.ActualStatus() == New) || (h.ActualStatus() == Expired) {
wg.Add(1)
temp := h
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍
or could have been passed as an argument to the anonymous function launched on the go routine

}

func (a *Analyzer) DiscoverTargets() {
a.DiscoveredTargets = a.discoverer.DiscoverNewTargets(a.DiscoveredTargets)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

  1. Try to make all exported items concurrent safe as someone else can misuse your components leading to some hard to find bugs
  2. 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)
  3. You can have concurrent "unsafe" methods, but try to keep them unexported so no other package can mess with them.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Fixed.
thanks.


var log = logger.GetOrCreate("vat")

func NewAnalyzer(discoverer Discoverer, sf ScannerFactory, pf ParserFactory) (*Analyzer, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we generally add comments on all exported items

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.

if (h.ActualStatus() == New) || (h.ActualStatus() == Expired) {
wg.Add(1)
temp := h
go func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

True. Maybe a queue list with no more than 10 workers deployed at once.
Thanks.

}

func (a *Analyzer) changeTargetStatus(address string, status utils.TargetStatus) string {
for idx, _ := range a.DiscoveredTargets {
Copy link
Collaborator

Choose a reason for hiding this comment

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

concurrent problems detected here as a.DiscoveredTargets can be asynced changed. A mutex locking will properly fix this

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks. Refactored.


var log = logger.GetOrCreate("vat")

type NmapScanner struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not a concurrent safe struct

Copy link
Owner Author

Choose a reason for hiding this comment

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

fixed.


// 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

interface separation required in order to make the unit tests pass on all hosts.

Copy link
Owner Author

Choose a reason for hiding this comment

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

created dialer.

if p.isPortInRange(37373, 38383) {
p.Type = utils.ElrondPort
p.Importance = utils.JudgementNoRisk
} else if (p.Number == 80) || (p.Number == 8080) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

else(s) can be removed by the help of early exists

Copy link
Owner Author

Choose a reason for hiding this comment

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

changed. ✌️

@@ -0,0 +1,148 @@
package utils
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Owner Author

@schimih schimih Feb 13, 2022

Choose a reason for hiding this comment

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

utils was intended as the core .

change done. all constants left in core package are used in multiple packages.

type Judgement string

const (
JudgementSshUserPermited Judgement = "5$ SMALL RISK - SSH User permitted"
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does the "$" should suppose to mean? :D

Copy link
Owner Author

Choose a reason for hiding this comment

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

nothing. fixed.
🔑

Copy link
Collaborator

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

👍 GJ!
looks way better now!

"golang.org/x/crypto/ssh"
)

type sshDialer struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

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.

2 participants