From 1ae3674390f32cdd1b1913d82e94c78e53a928f6 Mon Sep 17 00:00:00 2001 From: Michael Sverdlov Date: Sun, 10 Sep 2023 11:25:51 +0300 Subject: [PATCH] Improve python audit logs (#939) * copy dir Signed-off-by: Michael Sverdlov * copy dir Signed-off-by: Michael Sverdlov * copy dir Signed-off-by: Michael Sverdlov * copy dir Signed-off-by: Michael Sverdlov * copy dir Signed-off-by: Michael Sverdlov * copy dir Signed-off-by: Michael Sverdlov * copy dir Signed-off-by: Michael Sverdlov * copy dir Signed-off-by: Michael Sverdlov --------- Signed-off-by: Michael Sverdlov --- artifactory/commands/python/python.go | 2 +- utils/coreutils/utils.go | 15 +++++ utils/coreutils/utils_test.go | 15 +++++ utils/python/utils.go | 6 +- xray/commands/audit/sca/common.go | 14 ++-- xray/commands/audit/sca/python/python.go | 66 ++++++++----------- xray/commands/audit/sca/python/python_test.go | 7 +- xray/commands/audit/scarunner.go | 4 +- 8 files changed, 78 insertions(+), 51 deletions(-) diff --git a/artifactory/commands/python/python.go b/artifactory/commands/python/python.go index 8d1afeea6..8a32a689c 100644 --- a/artifactory/commands/python/python.go +++ b/artifactory/commands/python/python.go @@ -106,7 +106,7 @@ func (pc *PythonCommand) SetPypiRepoUrlWithCredentials() error { if err != nil { return err } - pc.args = append(pc.args, python.GetPypiRemoteRegistryFlag(pc.pythonTool), rtUrl.String()) + pc.args = append(pc.args, python.GetPypiRemoteRegistryFlag(pc.pythonTool), rtUrl) return nil } diff --git a/utils/coreutils/utils.go b/utils/coreutils/utils.go index ae81d84f9..15bb5809f 100644 --- a/utils/coreutils/utils.go +++ b/utils/coreutils/utils.go @@ -584,3 +584,18 @@ func GetServerIdAndRepo(remoteEnv string) (serverID string, repoName string, err } return } + +func GetMaskedCommandString(cmd *exec.Cmd) string { + cmdString := strings.Join(cmd.Args, " ") + // Mask url if required + matchedResult := regexp.MustCompile(utils.CredentialsInUrlRegexp).FindString(cmdString) + if matchedResult != "" { + cmdString = strings.ReplaceAll(cmdString, matchedResult, "***@") + } + + matchedResults := regexp.MustCompile(`--(?:password|access-token)=(\S+)`).FindStringSubmatch(cmdString) + if len(matchedResults) > 1 && matchedResults[1] != "" { + cmdString = strings.ReplaceAll(cmdString, matchedResults[1], "***") + } + return cmdString +} diff --git a/utils/coreutils/utils_test.go b/utils/coreutils/utils_test.go index 1bebe9d9c..8ddffbee4 100644 --- a/utils/coreutils/utils_test.go +++ b/utils/coreutils/utils_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "os/exec" "path/filepath" "reflect" "testing" @@ -264,3 +265,17 @@ func TestGetFullPathsWorkingDirs(t *testing.T) { }) } } + +func TestGetMaskedCommandString(t *testing.T) { + assert.Equal(t, + "pip -i ***@someurl.com/repo", + GetMaskedCommandString(exec.Command("pip", "-i", "https://user:pass@someurl.com/repo"))) + + assert.Equal(t, + "pip -i ***@someurl.com/repo --password=***", + GetMaskedCommandString(exec.Command("pip", "-i", "https://user:pass@someurl.com/repo", "--password=123"))) + + assert.Equal(t, + "pip -i ***@someurl.com/repo --access-token=***", + GetMaskedCommandString(exec.Command("pip", "-i", "https://user:pass@someurl.com/repo", "--access-token=123"))) +} diff --git a/utils/python/utils.go b/utils/python/utils.go index d5057b894..c44c0385a 100644 --- a/utils/python/utils.go +++ b/utils/python/utils.go @@ -51,15 +51,15 @@ func GetPypiRemoteRegistryFlag(tool pythonutils.PythonTool) string { return pipenvRemoteRegistryFlag } -func GetPypiRepoUrl(serverDetails *config.ServerDetails, repository string) (*url.URL, error) { +func GetPypiRepoUrl(serverDetails *config.ServerDetails, repository string) (string, error) { rtUrl, username, password, err := GetPypiRepoUrlWithCredentials(serverDetails, repository) if err != nil { - return nil, err + return "", err } if password != "" { rtUrl.User = url.UserPassword(username, password) } - return rtUrl, err + return rtUrl.String(), err } func ConfigPoetryRepo(url, username, password, configRepoName string) error { diff --git a/xray/commands/audit/sca/common.go b/xray/commands/audit/sca/common.go index 3176308e3..cb4513978 100644 --- a/xray/commands/audit/sca/common.go +++ b/xray/commands/audit/sca/common.go @@ -108,14 +108,18 @@ func GetModule(modules []*xrayUtils.GraphNode, moduleId string) *xrayUtils.Graph // GetExecutableVersion gets an executable version and prints to the debug log if possible. // Only supported for package managers that use "--version". -func GetExecutableVersion(executable string) (version string, err error) { +func LogExecutableVersion(executable string) { verBytes, err := exec.Command(executable, "--version").CombinedOutput() - if err != nil || len(verBytes) == 0 { - return "", err + if err != nil { + log.Debug(fmt.Sprintf("'%q --version' command received an error: %s", executable, err.Error())) + return } - version = strings.TrimSpace(string(verBytes)) + if len(verBytes) == 0 { + log.Debug(fmt.Sprintf("'%q --version' command received an empty response", executable)) + return + } + version := strings.TrimSpace(string(verBytes)) log.Debug(fmt.Sprintf("Used %q version: %s", executable, version)) - return } // BuildImpactPathsForScanResponse builds the full impact paths for each vulnerability found in the scanResult argument, using the dependencyTrees argument. diff --git a/xray/commands/audit/sca/python/python.go b/xray/commands/audit/sca/python/python.go index e23dc25b0..4efb193ac 100644 --- a/xray/commands/audit/sca/python/python.go +++ b/xray/commands/audit/sca/python/python.go @@ -1,11 +1,13 @@ package python import ( + "errors" "fmt" biutils "github.com/jfrog/build-info-go/utils" "github.com/jfrog/build-info-go/utils/pythonutils" "github.com/jfrog/gofrog/datastructures" "github.com/jfrog/jfrog-cli-core/v2/utils/config" + "github.com/jfrog/jfrog-cli-core/v2/utils/coreutils" utils "github.com/jfrog/jfrog-cli-core/v2/utils/python" "github.com/jfrog/jfrog-cli-core/v2/xray/commands/audit/sca" "github.com/jfrog/jfrog-client-go/utils/errorutils" @@ -72,15 +74,11 @@ func getDependencies(auditPython *AuditPython) (dependenciesGraph map[string][]s } defer func() { - e := os.Chdir(wd) - if err == nil { - err = errorutils.CheckError(e) - } - - e = fileutils.RemoveTempDir(tempDirPath) - if err == nil { - err = e - } + err = errors.Join( + err, + errorutils.CheckError(os.Chdir(wd)), + fileutils.RemoveTempDir(tempDirPath), + ) }() err = biutils.CopyDir(wd, tempDirPath, true, nil) @@ -90,10 +88,7 @@ func getDependencies(auditPython *AuditPython) (dependenciesGraph map[string][]s restoreEnv, err := runPythonInstall(auditPython) defer func() { - e := restoreEnv() - if err == nil { - err = e - } + err = errors.Join(err, restoreEnv()) }() if err != nil { return @@ -105,12 +100,8 @@ func getDependencies(auditPython *AuditPython) (dependenciesGraph map[string][]s } dependenciesGraph, directDependencies, err = pythonutils.GetPythonDependencies(auditPython.Tool, tempDirPath, localDependenciesPath) if err != nil { - if _, innerErr := sca.GetExecutableVersion("python"); innerErr != nil { - log.Error(innerErr) - } - if _, innerErr := sca.GetExecutableVersion(string(auditPython.Tool)); innerErr != nil { - log.Error(innerErr) - } + sca.LogExecutableVersion("python") + sca.LogExecutableVersion(string(auditPython.Tool)) } return } @@ -168,14 +159,19 @@ func installPipDeps(auditPython *AuditPython) (restoreEnv func() error, err erro if err != nil { return } + + remoteUrl := "" if auditPython.RemotePypiRepo != "" { - return restoreEnv, runPipInstallFromRemoteRegistry(auditPython.Server, auditPython.RemotePypiRepo, auditPython.PipRequirementsFile) + remoteUrl, err = utils.GetPypiRepoUrl(auditPython.Server, auditPython.RemotePypiRepo) + if err != nil { + return + } } - pipInstallArgs := getPipInstallArgs(auditPython.PipRequirementsFile) + pipInstallArgs := getPipInstallArgs(auditPython.PipRequirementsFile, remoteUrl) err = executeCommand("python", pipInstallArgs...) if err != nil && auditPython.PipRequirementsFile == "" { log.Debug(err.Error() + "\nTrying to install using a requirements file...") - pipInstallArgs = getPipInstallArgs("requirements.txt") + pipInstallArgs = getPipInstallArgs("requirements.txt", remoteUrl) reqErr := executeCommand("python", pipInstallArgs...) if reqErr != nil { // Return Pip install error and log the requirements fallback error. @@ -189,18 +185,17 @@ func installPipDeps(auditPython *AuditPython) (restoreEnv func() error, err erro func executeCommand(executable string, args ...string) error { installCmd := exec.Command(executable, args...) - log.Debug(fmt.Sprintf("Running %q", strings.Join(installCmd.Args, " "))) + maskedCmdString := coreutils.GetMaskedCommandString(installCmd) + log.Debug("Running", maskedCmdString) output, err := installCmd.CombinedOutput() if err != nil { - if _, innerErr := sca.GetExecutableVersion(executable); innerErr != nil { - log.Error(innerErr) - } - return errorutils.CheckErrorf("%q command failed: %s - %s", strings.Join(installCmd.Args, " "), err.Error(), output) + sca.LogExecutableVersion(executable) + return errorutils.CheckErrorf("%q command failed: %s - %s", maskedCmdString, err.Error(), output) } return nil } -func getPipInstallArgs(requirementsFile string) []string { +func getPipInstallArgs(requirementsFile, remoteUrl string) []string { args := []string{"-m", "pip", "install"} if requirementsFile == "" { // Run 'pip install .' @@ -209,17 +204,10 @@ func getPipInstallArgs(requirementsFile string) []string { // Run pip 'install -r requirements ' args = append(args, "-r", requirementsFile) } - return args -} - -func runPipInstallFromRemoteRegistry(server *config.ServerDetails, depsRepoName, pipRequirementsFile string) (err error) { - rtUrl, err := utils.GetPypiRepoUrl(server, depsRepoName) - if err != nil { - return err + if remoteUrl != "" { + args = append(args, utils.GetPypiRemoteRegistryFlag(pythonutils.Pip), remoteUrl) } - args := getPipInstallArgs(pipRequirementsFile) - args = append(args, utils.GetPypiRemoteRegistryFlag(pythonutils.Pip), rtUrl.String()) - return executeCommand("python", args...) + return args } func runPipenvInstallFromRemoteRegistry(server *config.ServerDetails, depsRepoName string) (err error) { @@ -227,7 +215,7 @@ func runPipenvInstallFromRemoteRegistry(server *config.ServerDetails, depsRepoNa if err != nil { return err } - args := []string{"install", "-d", utils.GetPypiRemoteRegistryFlag(pythonutils.Pipenv), rtUrl.String()} + args := []string{"install", "-d", utils.GetPypiRemoteRegistryFlag(pythonutils.Pipenv), rtUrl} return executeCommand("pipenv", args...) } diff --git a/xray/commands/audit/sca/python/python_test.go b/xray/commands/audit/sca/python/python_test.go index 49c3b1f74..bf8ae768d 100644 --- a/xray/commands/audit/sca/python/python_test.go +++ b/xray/commands/audit/sca/python/python_test.go @@ -136,6 +136,9 @@ func TestBuildPoetryDependencyList(t *testing.T) { } func TestGetPipInstallArgs(t *testing.T) { - assert.Equal(t, []string{"-m", "pip", "install", "."}, getPipInstallArgs("")) - assert.Equal(t, []string{"-m", "pip", "install", "-r", "requirements.txt"}, getPipInstallArgs("requirements.txt")) + assert.Equal(t, []string{"-m", "pip", "install", "."}, getPipInstallArgs("", "")) + assert.Equal(t, []string{"-m", "pip", "install", "-r", "requirements.txt"}, getPipInstallArgs("requirements.txt", "")) + + assert.Equal(t, []string{"-m", "pip", "install", ".", "-i", "https://user@pass:remote.url/repo"}, getPipInstallArgs("", "https://user@pass:remote.url/repo")) + assert.Equal(t, []string{"-m", "pip", "install", "-r", "requirements.txt", "-i", "https://user@pass:remote.url/repo"}, getPipInstallArgs("requirements.txt", "https://user@pass:remote.url/repo")) } diff --git a/xray/commands/audit/scarunner.go b/xray/commands/audit/scarunner.go index 231a45c1e..3f8e6144e 100644 --- a/xray/commands/audit/scarunner.go +++ b/xray/commands/audit/scarunner.go @@ -127,8 +127,10 @@ func getDirectDependenciesFromTree(dependencyTrees []*xrayCmdUtils.GraphNode) [] } func GetTechDependencyTree(params *xrayutils.AuditBasicParams, tech coreutils.Technology) (flatTree *xrayCmdUtils.GraphNode, fullDependencyTrees []*xrayCmdUtils.GraphNode, err error) { + logMessage := fmt.Sprintf("Calculating %s dependencies", tech.ToFormal()) + log.Info(logMessage) if params.Progress() != nil { - params.Progress().SetHeadlineMsg(fmt.Sprintf("Calculating %v dependencies", tech.ToFormal())) + params.Progress().SetHeadlineMsg(logMessage) } serverDetails, err := params.ServerDetails() if err != nil {