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

Enable conan support #781

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from
Open

Enable conan support #781

wants to merge 16 commits into from

Conversation

orto17
Copy link
Contributor

@orto17 orto17 commented Nov 6, 2024

This PR includes support of conan requirements file update. It does not support installation of the suggested fixes. In order to update the dependencies, the user should run a 'conan install' command with the suggested conan file..

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • This pull request is on the dev branch.
  • I used gofmt for formatting the code before submitting the pull request.
  • Update documentation about new features / new supported technologies

@orto17 orto17 added the safe to test Approve running integration tests on a pull request label Nov 6, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 6, 2024
@orto17 orto17 added the safe to test Approve running integration tests on a pull request label Nov 6, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 6, 2024
}

func (conan *ConanPackageHandler) installConanDependencies(conanFile string) (err error) {
commandArgs := []string{"install", conanFile, "--build=missing"}
Copy link
Contributor

@eranturgeman eranturgeman Nov 7, 2024

Choose a reason for hiding this comment

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

Before installing Conan dependencies with a default command we must check if an Install command was provided by the user (it is found in auditParams if exist), and if so- we should run it instead of the default command (since it might not work)

if err = conan.updateConanFile(conanFileTxt, vulnDetails); err != nil {
return
}
return conan.installConanDependencies(conanFileTxt)
Copy link
Contributor

@eranturgeman eranturgeman Nov 7, 2024

Choose a reason for hiding this comment

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

Why do we have to install after fixing the descriptor? Installation always comes with a risk of failure. Can't we just update the descriptor?

if err = conan.updateConanFile(conanFilePy, vulnDetails); err != nil {
return
}
return conan.installConanDependencies(conanFilePy)
Copy link
Contributor

@eranturgeman eranturgeman Nov 7, 2024

Choose a reason for hiding this comment

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

Why do we have to install after fixing the descriptor? Installation always comes with a risk of failure. Can't we just update the descriptor?

return
}

func (conan *ConanPackageHandler) installConanDependencies(conanFile string) (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Before running an install command we must check if we should resolve dependencies from Artifactory. if a deps_repo was provided we must configure Artifactory along with this deps repo and the registry we resolve from

@orto17 orto17 added the safe to test Approve running integration tests on a pull request label Nov 10, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 10, 2024
@orto17 orto17 added the safe to test Approve running integration tests on a pull request label Nov 10, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Nov 10, 2024
return fmt.Errorf("impacted package %s not found, fix failed", vulnDetails.ImpactedDependencyName)
}
if err = os.WriteFile(conanFileName, []byte(fixedFile), 0600); err != nil {
err = fmt.Errorf("an error occured while writing the fixed version of %s to the requirements file:\n%s", vulnDetails.ImpactedDependencyName, err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err = fmt.Errorf("an error occured while writing the fixed version of %s to the requirements file:\n%s", vulnDetails.ImpactedDependencyName, err.Error())
err = fmt.Errorf("an error occured while writing the fixed version of %s to the requirements file '%s': %s", conanFileName, vulnDetails.ImpactedDependencyName, err.Error())

Copy link
Contributor

@eranturgeman eranturgeman left a comment

Choose a reason for hiding this comment

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

Overall looks good! Most of the comments are "cosmetics" and things we talked about that we can further discuss

@orto17 orto17 added the safe to test Approve running integration tests on a pull request label Dec 1, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Dec 1, 2024
@orto17 orto17 added the safe to test Approve running integration tests on a pull request label Dec 1, 2024
@github-actions github-actions bot removed the safe to test Approve running integration tests on a pull request label Dec 1, 2024
Copy link
Contributor

github-actions bot commented Dec 1, 2024

🚨 Frogbot scanned this pull request and found the below:

📦 Vulnerable Dependencies

✍️ Summary

SEVERITY CONTEXTUAL ANALYSIS DIRECT DEPENDENCIES IMPACTED DEPENDENCY FIXED VERSIONS CVES

Low
Not Covered github.com/golang-jwt/jwt/v4:v4.5.0
github.com/jfrog/jfrog-cli-core/v2:v2.56.4
github.com/jfrog/jfrog-cli-security:v1.12.3
github.com/jfrog/jfrog-client-go:v1.47.3
github.com/golang-jwt/jwt/v4 v4.5.0 [4.5.1] CVE-2024-51744

🔬 Research Details

Description:
golang-jwt is a Go implementation of JSON Web Tokens. Unclear documentation of the error behavior in ParseWithClaims can lead to situation where users are potentially not checking errors in the way they should be. Especially, if a token is both expired and invalid, the errors returned by ParseWithClaims return both error codes. If users only check for the jwt.ErrTokenExpired using error.Is, they will ignore the embedded jwt.ErrTokenSignatureInvalid and thus potentially accept invalid tokens. A fix has been back-ported with the error handling logic from the v5 branch to the v4 branch. In this logic, the ParseWithClaims function will immediately return in "dangerous" situations (e.g., an invalid signature), limiting the combined errors only to situations where the signature is valid, but further validation failed (e.g., if the signature is valid, but is expired AND has the wrong audience). This fix is part of the 4.5.1 release. We are aware that this changes the behaviour of an established function and is not 100 % backwards compatible, so updating to 4.5.1 might break your code. In case you cannot update to 4.5.0, please make sure that you are properly checking for all errors ("dangerous" ones first), so that you are not running in the case detailed above.


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