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.

Sign up for GitHub

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

Jump to bottom

implement tri-state verification in FTP detector #1604

Merged
merged 8 commits into from
Aug 9, 2023
Merged

implement tri-state verification in FTP detector #1604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4162658
add determinacy framework
rosecodym Aug 3, 2023
8583f9b
fix redaction in existing tests
rosecodym Aug 3, 2023
e7e17ae
fixup! add determinacy framework
rosecodym Aug 3, 2023
3e6a6cc
account for determinacy
rosecodym Aug 3, 2023
8081c4d
inject timeout
rosecodym Aug 3, 2023
1101929
actually check error code
rosecodym Aug 3, 2023
fa982bf
actually accept timeout
rosecodym Aug 3, 2023
1b31e80
make comment const
rosecodym Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 35 additions & 12 deletions pkg/detectors/ftp/ftp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package ftp

import (
"context"
"errors"
"net/textproto"
"net/url"
"regexp"
"strings"
Expand All @@ -13,7 +15,17 @@ import (
"github.com/trufflesecurity/trufflehog/v3/pkg/pb/detectorspb"
)

type Scanner struct{}
const (
// https://datatracker.ietf.org/doc/html/rfc959
ftpNotLoggedIn = 530

defaultVerificationTimeout = 5 * time.Second
)

type Scanner struct {
// Verification timeout. Defaults to 5 seconds if unset.
verificationTimeout time.Duration
}

// Ensure the Scanner satisfies the interface at compile time.
var _ detectors.Detector = (*Scanner)(nil)
Expand Down Expand Up @@ -58,48 +70,59 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
rawURL.Path = ""
redact := strings.TrimSpace(strings.Replace(rawURL.String(), password, "********", -1))

s := detectors.Result{
r := detectors.Result{
DetectorType: detectorspb.DetectorType_FTP,
Raw: []byte(rawURL.String()),
Redacted: redact,
}

if verify {
s.Verified = verifyFTP(ctx, parsedURL)
timeout := s.verificationTimeout
if timeout == 0 {
timeout = defaultVerificationTimeout
}
verificationErr := verifyFTP(timeout, parsedURL)
r.Verified = verificationErr == nil
if !isErrDeterminate(verificationErr) {
r.VerificationError = verificationErr
}
}

if !s.Verified {
if !r.Verified {
// Skip unverified findings where the password starts with a `$` - it's almost certainly a variable.
if strings.HasPrefix(password, "$") {
continue
}
}

if detectors.IsKnownFalsePositive(string(s.Raw), []detectors.FalsePositive{"@ftp.freebsd.org"}, false) {
if detectors.IsKnownFalsePositive(string(r.Raw), []detectors.FalsePositive{"@ftp.freebsd.org"}, false) {
continue
}

results = append(results, s)
results = append(results, r)
}

return results, nil
}

func verifyFTP(ctx context.Context, u *url.URL) bool {
func isErrDeterminate(e error) bool {
ftpErr := &textproto.Error{}
return errors.As(e, &ftpErr) && ftpErr.Code == ftpNotLoggedIn
Comment on lines +109 to +110
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is cool

}

func verifyFTP(timeout time.Duration, u *url.URL) error {
host := u.Host
if !strings.Contains(host, ":") {
host = host + ":21"
}

c, err := ftp.Dial(host, ftp.DialWithTimeout(5*time.Second))
c, err := ftp.Dial(host, ftp.DialWithTimeout(timeout))
if err != nil {
return false
return err
}

password, _ := u.User.Password()
err = c.Login(u.User.Username(), password)

return err == nil
return c.Login(u.User.Username(), password)
}

func (s Scanner) Type() detectorspb.DetectorType {
Expand Down
69 changes: 57 additions & 12 deletions pkg/detectors/ftp/ftp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@

import (
"context"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"testing"
"time"

"github.com/kylelemons/godebug/pretty"
"github.com/trufflesecurity/trufflehog/v3/pkg/detectors"
"github.com/trufflesecurity/trufflehog/v3/pkg/pb/detectorspb"
)
Expand All @@ -19,11 +21,12 @@
verify bool
}
tests := []struct {
name string
s Scanner
args args
want []detectors.Result
wantErr bool
name string
s Scanner
args args
want []detectors.Result
wantErr bool
wantVerificationErr bool
}{
{
name: "bad scheme",
Expand All @@ -48,7 +51,7 @@
{
DetectorType: detectorspb.DetectorType_FTP,
Verified: true,
Redacted: "ftp://dlpuser:*************************@ftp.dlptest.com",
Redacted: "ftp://dlpuser:********@ftp.dlptest.com",
Comment on lines -51 to +54
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why some of the tests were adjusted? Did our redaction code update?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i guess so? they were failing on main and i didn't think that figuring out why was worth it

},
},
wantErr: false,
Expand All @@ -66,11 +69,49 @@
{
DetectorType: detectorspb.DetectorType_FTP,
Verified: false,
Redacted: "ftp://dlpuser:*******@ftp.dlptest.com",
Redacted: "ftp://dlpuser:********@ftp.dlptest.com",
},
},
wantErr: false,
},
{
name: "bad host",
s: Scanner{},
args: args{
ctx: context.Background(),
// https://dlptest.com/ftp-test/
data: []byte("ftp://dlpuser:[email protected]"),
verify: true,
},
want: []detectors.Result{
{
DetectorType: detectorspb.DetectorType_FTP,
Verified: false,
Redacted: "ftp://dlpuser:********@ftp.dlptest.com.badhost",
},
},
wantErr: false,
wantVerificationErr: true,
},
{
name: "timeout",
s: Scanner{verificationTimeout: 1 * time.Microsecond},
args: args{
ctx: context.Background(),
// https://dlptest.com/ftp-test/
data: []byte("ftp://dlpuser:[email protected]"),

Check warning on line 102 in pkg/detectors/ftp/ftp_test.go

View workflow job for this annotation

GitHub Actions / test 

Found verified FTP result 🐷🔑
verify: true,
},
want: []detectors.Result{
{
DetectorType: detectorspb.DetectorType_FTP,
Verified: false,
Redacted: "ftp://dlpuser:********@ftp.dlptest.com",
},
},
wantErr: false,
wantVerificationErr: true,
},
{
name: "blocked FP",
s: Scanner{},
Expand All @@ -84,8 +125,7 @@
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
s := Scanner{}
got, err := s.FromData(tt.args.ctx, tt.args.verify, tt.args.data)
got, err := tt.s.FromData(tt.args.ctx, tt.args.verify, tt.args.data)
if (err != nil) != tt.wantErr {
t.Errorf("URI.FromData() error = %v, wantErr %v", err, tt.wantErr)
return
Expand All @@ -95,9 +135,14 @@
// }
for i := range got {
got[i].Raw = nil

if (got[i].VerificationError != nil) != tt.wantVerificationErr {
t.Fatalf("wantVerificationError = %v, verification error = %v", tt.wantVerificationErr, got[i].VerificationError)
}
}
if diff := pretty.Compare(got, tt.want); diff != "" {
t.Errorf("URI.FromData() %s diff: (-got +want)\n%s", tt.name, diff)
opts := cmpopts.IgnoreFields(detectors.Result{}, "VerificationError")
if diff := cmp.Diff(got, tt.want, opts); diff != "" {
t.Errorf("FTP.FromData() %s diff: (-got +want)\n%s", tt.name, diff)
}
Comment on lines 136 to 146
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit / question: Could we add "Raw" to the cmpopts.IgnoreFields and remove setting it to nil on line 137? If so, we could convert that loop into iterating over values also.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good thought moving Raw into the ignorelist, but i don't follow the second part of your suggestion

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the only reason we were iterating over the indices was to clear out the Raw value in the object, so now that Raw is being ignored we don't need to do that and could update the loop to be:

for _, gotResult := range got {
    if (gotResult.Verification != nil) != tt.wantVerificationErr {
        // ...
    }
}

Copy link
Collaborator Author

@rosecodym rosecodym Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

oh, no, it's dumber than that. the root problem is that we know when we want to see a verification error, but we don't always know the exact shape of the error, so there's a top-level "is there an error" flag. but verification errors are per result, not per overall chunk flow, so we loop through and check the expected flag against each individual result object.

there's probably a better way to do it, though - i haven't thought about it too hard.

})
}
Expand Down
Loading