-
Notifications
You must be signed in to change notification settings - Fork 363
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
- add conan.lock to the list #59
Conversation
It needs to be implemented in https://github.com/google/osv-scanner/tree/main/pkg/lockfile 🙊 |
Signed-off-by: SSE4 <[email protected]>
@prince-chrismc I've added some parser |
pkg/lockfile/parse-conan-lock.go
Outdated
PythonRequires []string `json:"python_requires,omitempty"` | ||
} | ||
|
||
const ConanEcosystem Ecosystem = "conan.io" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just flagging this isn't a valid ecosystem currently per the OSV spec - I'll leave it to @another-rex & @oliverchang to decide what to do about this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed!
We should at the very least add a TODO here to say that this is tentative and subject to change depending on the OSV schema.
This is definitely a useful addition to have for the OSV-Scanner, but the database itself also currently does not have conan.io advisories. Is there also an existing Conan vuln DB that we could have publishing OSV entries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there also an existing Conan vuln DB that we could have publishing OSV entries?
Conan is the C++ package manager 😉 We have ConanCenter which offers a ton of C and C++ packages. https://cve.mitre.org/index.html or https://nvd.nist.gov/ are the types of places I'd expect find know vulnerabilities.
https://conan.io/center/openssl has numerous ones https://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=openssl.
openssl/3.0.5 has CVE-2022-3602 for example.
I saw in the readme these are listed as alias could they not be the IDs as well? Some like the Openssl Alpine that I see on https://osv.dev/list?ecosystem=Alpine&q=openssl was my initial reaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I've opened a PR to ovs-schema: ossf/osv-schema#101
for the database, sorry, but I have no idea. maybe @prince-chrismc or @jcar87 know and can comment. perhaps, X-Ray might have some sort of database, but I am not familiar with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not a public database 😉 But public database that already exists are welcomed!
pkg/lockfile/parse-conan-lock.go
Outdated
|
||
func parseConanV2Lock(lockfile ConanLockFile) []PackageDetails { | ||
|
||
packages := make([]PackageDetails, 0, len(lockfile.Requires)+len(lockfile.BuildRequires)+len(lockfile.PythonRequires)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to wrap these in uint64
to make CodeQL happy, otherwise it will flag this as a potential vulnerability since the return type of len
technically allows negative numbers even though in practice that should never be possible.
See here for an example of how I've addressed that in the past
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I am not that familiar with Go or GoLang, so negative length or size type sounds a bit suspicious for me. fixed it.
@SSE4 I've done an initial review and nothing huge has jumped out at me as needing to be changed - I'm not familiar with this ecosystem/tool at all; would you happen to have any references to the lockfile (ideally a spec would be great, but not all ecosystems have those). It would also be good to know how the version comparison is actually done if you know that too. |
Signed-off-by: SSE4 <[email protected]>
thanks, I've updated the PR description. lemme know if you have any further question. |
@G-Rath how do we move this forward? please let me know if you need anything else from my side. |
@SSE4 the main thing that needs addressing is this comment - otherwise everything looks ok |
okay, I've changed ecosystem name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a couple of nitty comments about newlines
pkg/lockfile/parse-conan-lock.go
Outdated
for _, node := range lockfile.GraphLock.Nodes { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
for _, node := range lockfile.GraphLock.Nodes { | |
for _, node := range lockfile.GraphLock.Nodes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a change here 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I think GitHub bugged out - I've remade the change here across more lines so hopefully it'll stick
pkg/lockfile/parse-conan-lock.go
Outdated
parts = strings.SplitN(ref, "/", 2) | ||
if len(parts) == 2 { | ||
reference.Name = parts[0] | ||
reference.Version = parts[1] | ||
} else { | ||
reference.Version = ref | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect I'm about to open a can of worms with this (which I missed in my first review, sorry about that), but this looks to me like it's possible for a package to not have a name - is that true? technically the osv.dev
API allows that if you have a commit, but it's not really expected elsewhere.
it also looks like none of your tests are actually covering this line (unless I've missed something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it's possible from the code perspective: https://github.com/conan-io/conan/blob/478395863b7b0ac0ec400c91410f43ba66754e75/conans/model/ref.py#L54
however I don't remember when exactly does it happen
@prince-chrismc do you remember if it's a legit situation? if not, we probably can just raise for invalid reference.
(in practice I think I've encountered some CLI errors trying to make a package without name 🤔 - e.g. ERROR: conanfile didn't specify name
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the official packages that are open-source that could possibly be in public databases with CVE will have a name.
It's technically possible for someone to have private packages but I would safe those are safe to skip.
Perhaps a warning or just skipping the package is an option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just got some responses:
It is possible to have self.version defined for a consumer, but not have self.name defined
In conan export or conan create it should never happen, I understand that it has reached the lockfile because it is a consumer
Well, not having to define a name, a conanfile.txt has neither a name nor a version, a conanfile.py also works fine without a name or version
But if you want to give it a version, you can.
so, I think they might theoretically appear in a lockfile, but they should be completely safe to skip.
I'll try to add some test case, if I can generate such a lockfile, otherwise, I'll just skip them in code (if Name is missing or empty).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added code to skip packages with no name
Co-authored-by: Gareth Jones <[email protected]>
Co-authored-by: Gareth Jones <[email protected]>
Co-authored-by: Gareth Jones <[email protected]>
Co-authored-by: Gareth Jones <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just waiting for (ossf/osv-schema#101) to be accepted before merging this in.
it's merged now |
* - add conan.lock to the list * - conan lockfile parser Signed-off-by: SSE4 <[email protected]> * - make CodeQL happy, use explicit uint64 cast Signed-off-by: SSE4 <[email protected]> * Update pkg/lockfile/parse-conan-lock.go * Update pkg/lockfile/parse-conan-lock.go * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * - skip references with no name Signed-off-by: SSE4 <[email protected]> * - add test for packages with no name specified Signed-off-by: SSE4 <[email protected]> * Update README.md * Update parse_test.go * Update parse.go * - fix test Signed-off-by: SSE4 <[email protected]> Signed-off-by: SSE4 <[email protected]> Co-authored-by: Gareth Jones <[email protected]>
* - add conan.lock to the list * - conan lockfile parser Signed-off-by: SSE4 <[email protected]> * - make CodeQL happy, use explicit uint64 cast Signed-off-by: SSE4 <[email protected]> * Update pkg/lockfile/parse-conan-lock.go * Update pkg/lockfile/parse-conan-lock.go * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * - skip references with no name Signed-off-by: SSE4 <[email protected]> * - add test for packages with no name specified Signed-off-by: SSE4 <[email protected]> * Update README.md * Update parse_test.go * Update parse.go * - fix test Signed-off-by: SSE4 <[email protected]> Signed-off-by: SSE4 <[email protected]> Co-authored-by: Gareth Jones <[email protected]>
* - add conan.lock to the list * - conan lockfile parser Signed-off-by: SSE4 <[email protected]> * - make CodeQL happy, use explicit uint64 cast Signed-off-by: SSE4 <[email protected]> * Update pkg/lockfile/parse-conan-lock.go * Update pkg/lockfile/parse-conan-lock.go * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * Update pkg/lockfile/parse-conan-lock.go Co-authored-by: Gareth Jones <[email protected]> * - skip references with no name Signed-off-by: SSE4 <[email protected]> * - add test for packages with no name specified Signed-off-by: SSE4 <[email protected]> * Update README.md * Update parse_test.go * Update parse.go * - fix test Signed-off-by: SSE4 <[email protected]> Signed-off-by: SSE4 <[email protected]> Co-authored-by: Gareth Jones <[email protected]>
/cc @prince-chrismc
there are several formats of conan lockfile available in the wild (lockfiles are versioned separately from the conan client itself, as not every conan release changes the format of lockfile):
0.0
) - added in conan 1.170.1
added in conan 1.18, added versioning to the lockfile format0.2
added in conan 1.21, added deterministic lockfiles0.3
added in conan 1.22, changed how nodes reference each other0.4
added in conan 1.28, the format mostly used currently in production and encountered most often0.5
is used by conan 2.0+ (currently, as of December 2022, still in beta state, and not yet released)formats 0.0 - 0.4 are mostly similar and use
graph_lock
with a list of nodes.format 0.5 is a major overhaul, it's a completely new implementation developed from scratch.
some information about 0.0 - 0.4 lockfiles (current format used in production):
more information about 0.5 lockfiles used in conan 2.0:
the format itself is not documented, but can be learnt by studying the source code graph_lock.py. as it's currently not expected lockfiles to be manually or automatically edited by users or tools, they are only generated by the conan client itself during the graph resolution process, and afterwards, lockfiles are fully immutable.
for the version comparison, conan client itself uses version ranges to compare versions, but it's only for packages that follow SemVer (a majority of C and C++ libraries don't follow SemVer, or even predate it). as different libraries use various versioning schemes, conan client cannot make any assumtions.
however, lockfile represents a resolved graph state, with all the version ranges already computed. therefore, versions in lockfiles are compared as regular strings.