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

feat(java): add support licenses and graph for gradle lock files #6140

Merged
merged 31 commits into from
Mar 19, 2024

Conversation

DmitriyLewen
Copy link
Contributor

@DmitriyLewen DmitriyLewen commented Feb 15, 2024

Description

We can parse *.pom files from gradle cache dir to detect:

  • licenses
  • child dependencies

nuances of dependency parsing:

  • GroupID and Version are resolved using filePath (if empty)
  • variables are currently only resolved using property fields.
    maven local repository and gradle cache dir use different structs. So we can't just use maven pom parser to parse gradle *.pom files.
    If we get issues about this, - we will update maven logic (as option - we will add interface to detecting *.pom files in local dir)

One important change - All gradle dependencies mark as Indirect now.
This is required to use logic to guess direct deps (There is no reliable way to determine direct dependencies (even using other files)) - see comments below.

examples:

➜  GRADLE_USER_HOME=./cache trivy -d fs ./gradle.lockfile --scanners vuln,license --dependency-tree
...
gradle.lockfile (gradle)

Total: 1 (UNKNOWN: 0, LOW: 0, MEDIUM: 1, HIGH: 0, CRITICAL: 0)

┌─────────────┬────────────────┬──────────┬────────┬───────────────────┬───────────────┬───────────────────────────────────────────────────────────┐
│   Library   │ Vulnerability  │ Severity │ Status │ Installed Version │ Fixed Version │                           Title                           │
├─────────────┼────────────────┼──────────┼────────┼───────────────────┼───────────────┼───────────────────────────────────────────────────────────┤
│ junit:junit │ CVE-2020-15250 │ MEDIUM   │ fixed  │ 4.13              │ 4.13.1        │ TemporaryFolder is shared between all users across system │
│             │                │          │        │                   │               │ which could result in...                                  │
│             │                │          │        │                   │               │ https://avd.aquasec.com/nvd/cve-2020-15250                │
└─────────────┴────────────────┴──────────┴────────┴───────────────────┴───────────────┴───────────────────────────────────────────────────────────┘

Dependency Origin Tree (Reversed)
=================================
gradle.lockfile
└── junit:junit:4.13, (UNKNOWN: 0, LOW: 0, MEDIUM: 1, HIGH: 0, CRITICAL: 0)

gradle.lockfile (license)

Total: 1 (UNKNOWN: 1, LOW: 0, MEDIUM: 0, HIGH: 0, CRITICAL: 0)

┌─────────────┬────────────────────────────┬────────────────┬──────────┐
│   Package   │          License           │ Classification │ Severity │
├─────────────┼────────────────────────────┼────────────────┼──────────┤
│ junit:junit │ Eclipse Public License 1.0 │ Non Standard   │ UNKNOWN  │
└─────────────┴────────────────────────────┴────────────────┴──────────┘

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@DmitriyLewen DmitriyLewen self-assigned this Feb 15, 2024
@ChristianCiach
Copy link

ChristianCiach commented Feb 19, 2024

I see you're trying to parse the build.gradle(.kts). My two cents: This may not be worth the effort:

  • You don't distinguish between the configuration of the respective dependency. testCompileOnly("my:dependency:1.0.0") is very different from implementation("my:dependency:1.0.0"). This could easily lead to wrong results, because an application could have my:dependency:1.0.0 both as a transitive test-only dependency and a direct runtime dependency (or vice-versa).
  • Gradle encourages users to use version catalogs. Even before the invention of version catalogs, many developers (especially for Android projects) declare their dependencies as constants in settings.gradle or even gradle.properties.

I applaud you for what you're trying to do, but I think it may just not be worth the effort and could easily lead to some confusing behaviour.

Edit: That being said, I don't have any better idea how we could retrieve the direct dependencies of a gradle project, at least not without executing the gradle command. This is really a tricky problem.

@DmitriyLewen
Copy link
Contributor Author

Hello @ChristianCiach
Thank you very much for your opinion and help!

I don't have a better idea of how we could get direct Gradle project dependencies.

I haven't found a better way either. I thought I'd start with this and get some feedback from users. Perhaps together we will find a better way.

@ChristianCiach
Copy link

I didn't test your changes yet, but looking at the code, it will probably break on this construct:

dependencies {
    implementation("com.google.guava:guava:32.1.3-jre") {
        exclude(group = "com.google.code.findbugs", module = "jsr305")
    }
    implementation("com.fasterxml.jackson.core:jackson-databind:2.16.1")
}

The } in line 4 will probably be interpreted as the end of the dependencies block.

Please don't interpret my comments as negative critique. I love what you're doing! Since gradle-support is important to me, I just want to write down my thoughts and ideas.

@DmitriyLewen
Copy link
Contributor Author

DmitriyLewen commented Feb 20, 2024

it will probably break on this construct:

Great! Thanks! I will add logic for this case.

exclude(group = "com.google.code.findbugs", module = "jsr305")

Are other words possible here (I mean non exclude)?

UPD:
@ChristianCiach I added excludes support.
Looks like you have more experience than me. If you have time, can you take a look at my code (or at least the tests) and tell me what common cases I missed?

@ChristianCiach
Copy link

ChristianCiach commented Feb 20, 2024

Are other words possible here (I mean non exclude)?

That's the issue: Gradle-build-files are just scripts (Groovy or Kotlin) and can contain anything. For example:

dependencies {
  val maybeDependency = "com.fasterxml.jackson.core:jackson-databind:2.16.1"
  if (project.hasProperty("someProperty")) {
    implementation(maybeDependency)
  }
}

Now it would break again on the closing } of the if.

That's why I am so sceptical of your approach of parsing the build.gradle(.kts). Again, I don't really have a better idea (or maybe I do, keep reading :) ). But this approach is so full of pitfalls and undeterministic behavior that I pretty much see this as a non-starter.

Looks like you have more experience than me. If you have time, can you take a look at my code (or at least the tests) and tell me what common cases I missed?

Yeah, I see myself as a bit of a gradle expert. I have been thinking about other approaches to detect direct dependencies of a gradle project and I think I've come up with something nicer:

As said in a previous comment, many people are using version catalogs to declare their direct dependencies. By convention (see documentation), the version catalog is a TOML file located at {gradle-root-project}/gradle/libs.versions.toml. A TOML-file is surely way easier to parse than a build.gradle.kts!

Please keep in mind that a "version-catalog" declares dependencies that a gradle project may (or may not!) use in their build.gradle(.kts). Instead of parsing the build.gradle(.kts) (or maybe in addition to that, but I would personally totally ignore the build* files), I propose to just check if a dependency found in gradle.lockfile is found inside the gradle/libs.versions.toml. If a dependency is found in gradle/libs.versions.toml, we can assume with very high probability that the project is using this dependency as a direct dependency.

Or to put it another way: If we could reliably parse a build.gradle(.kts), why do we even bother with gradle.lockfile at all? In this case we could extract all dependencies from the build file - but we can't. And I recommend to not even try.

@ChristianCiach
Copy link

ChristianCiach commented Feb 20, 2024

Another approach, but this is also not guaranteed to lead to correct results:

When constructing the dependency graph, maybe we should just declare the "roots" (the opposite of the leafs) as "direct dependencies"? As "roots" I mean dependencies that are not dependencies of other dependencies of the graph.

Of course, this is a bit wonky, but so is the current approach. It's wonky because a dependency could be both a direct and a transitive dependency of a gradle project.

Edit: I think I vastly prefer this approach, even if it results in a few missing direct dependencies. Of all the approaches proposed so far, this is the only one that would mostly work for us, because we use external version-catalogs: https://docs.gradle.org/current/userguide/platforms.html#sec:importing-published-catalog

@DmitriyLewen
Copy link
Contributor Author

Thanks for your opinion!

But this approach is so full of pitfalls and undeterministic behavior that I pretty much see this as a non-starter.

The more I work on this problem the more I agree with you.

When constructing the dependency graph, maybe we should just declare the "roots" (the opposite of the leafs) as "direct dependencies"? As "roots" I mean dependencies that are not dependencies of other dependencies of the graph.

IIUC we already have similar logic :

// Direct dependencies cannot be identified in some package managers like "package-lock.json" v1,
// then the "Indirect" field can be always true. We try to guess direct dependencies in this case.
// A dependency with no parents must be a direct dependency.
//
// e.g.
// -> styled-components
// -> fbjs
// -> isomorphic-fetch
// -> node-fetch
//
// Even if `styled-components` is not marked as a direct dependency, it must be a direct dependency
// as it has no parents. Note that it doesn't mean `fbjs` is an indirect dependency.

We can mark all dependencies as Indirect and you this logic.
wdyt?

@ChristianCiach
Copy link

ChristianCiach commented Feb 20, 2024

We can mark all dependencies as Indirect and you this logic.
wdyt?

Yes, this is exactly what I was saying :) I like it, and I think this is more than "good enough" for now!

I am sorry that I cannot contribute any code for this. I am fluent in Java and Python, but I am not yet confident enough in my Go skills, unfortunately.

Edit:

and you this logic

Sorry, I do not understand this part.

@DmitriyLewen
Copy link
Contributor Author

DmitriyLewen commented Feb 20, 2024

Sorry, I do not understand this part.

Sorry, this is typo. I meant We can mark all dependencies as Indirect and use this logic.

Yes, this is exactly what I was saying :) I like it, and I think this is more than "good enough" for now!
I am sorry that I cannot contribute any code for this. I am fluent in Java and Python, but I am not yet confident enough in my Go skills, unfortunately

Thank you for your help. I thought about using this logic, but had my doubts. But now I think we really should have chosen this way.

@DmitriyLewen DmitriyLewen marked this pull request as ready for review February 21, 2024 08:32
// There is no reliable way to determine direct dependencies (even using other files).
// Therefore, we mark all dependencies as Indirect.
// This is necessary to try to guess direct dependencies and build a dependency tree.
Indirect: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see a second way:
we can mark Gradle dependencies as indirect when before building the dependency tree -

func (r *vulnerabilityRenderer) renderDependencyTree() {

@knqyf263 knqyf263 added this pull request to the merge queue Mar 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 18, 2024
@knqyf263 knqyf263 added this pull request to the merge queue Mar 19, 2024
Merged via the queue into aquasecurity:main with commit f6c5d58 Mar 19, 2024
17 checks passed
@ChristianCiach
Copy link

But this approach is so full of pitfalls and undeterministic behavior that I pretty much see this as a non-starter.

The more I work on this problem the more I agree with you.

It may be worth re-visiting this idea when "declarative gradle" becomes a thing:

Parsing a declarative gradle build file surely is a lot more feasible than parsing a gradle script file. This will be very similar to parsing a maven pom.xml.

@DmitriyLewen DmitriyLewen deleted the feat-gradle/license-support branch June 25, 2024 05:55
@oleg-nenashev
Copy link

@ChristianCiach a more interesting approach would be to use Gradle's own tooling to generate dependency trees and BOMs/license information. For example, you can see how the GitHub Dependency submission action does it for GitHub Security https://github.com/gradle/actions/tree/main/dependency-submission

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.

Gradle dependency tree support Gradle license support
4 participants