Skip to content

Fix for issue #1972: Fixed typecasting, and added more debug logging.#1974

Merged
kzantow merged 3 commits intoanchore:mainfrom
markgalpin:main
Aug 17, 2023
Merged

Fix for issue #1972: Fixed typecasting, and added more debug logging.#1974
kzantow merged 3 commits intoanchore:mainfrom
markgalpin:main

Conversation

@markgalpin
Copy link
Copy Markdown
Contributor

@markgalpin markgalpin commented Jul 28, 2023

I'm not sure if this is adequately tested... Feel free to just use it as a POC if its wrong.

Fixes: #1972

@markgalpin
Copy link
Copy Markdown
Contributor Author

So my environment had had an error with an extra ` after my email address, which I fixed, but DCO seems to be remaining confused. Sorry about that... LMK if I'm doing something wrong.

@markgalpin
Copy link
Copy Markdown
Contributor Author

After playing around with some other code so I could POC java deps, I think I fixed this issue at the wrong end... I'm still investigating, but it seems like I maybe should have fixed this at the APK end instead of the cyclonedx end.

@markgalpin
Copy link
Copy Markdown
Contributor Author

Seems like I was right the first time based on: syft/pkg/cataloger/kernel/cataloger.go line 117: // note: relationships should have Package objects, not pointers

exists := isExpressiblePackageRelationship(r.Type)
if !exists {
log.Debugf("unable to convert relationship from CycloneDX 1.4 JSON, dropping: %+v", r)
log.Debugf("unable to convert relationship type to CycloneDX 1.4 JSON, dropping: %+v", r)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe just remove the 1.4 in these messages?

@kzantow
Copy link
Copy Markdown
Contributor

kzantow commented Jul 31, 2023

To fix DCO I usually git reset HEAD~1 and re-commit with -s, then force push

@markgalpin
Copy link
Copy Markdown
Contributor Author

Hmmmm. So... the itest was failing because the decode is lossy, as far as I can tell. Looking at: syft/formats/common/cyclonedxhelpers/decoder.go it looks like the /FIXME in line 230 is a red herring, I don't think there's anything to fix there (cyclonedx relationships imply a type, probably, though there might be a broader discussion of what sort they imply)

I fixed this piece, but now we're getting to the fact that package_ids are changed in the encode/decode process... The current test redacts the ids for individual packages, so I could go in and redact the ids in the dependencies section as well, although it feels a bit gross? I'm a bit curious why in the decoder we're not trying to preserve IDs? is it because you can't rely on them being present and/or syft formatted? Is there a better way?

Still getting the weird DCO behavior on commit a4... is it because I used amend instead of rebase? (I did amend because otherwise it was trying to sign off twice with the mistake I made)

@markgalpin
Copy link
Copy Markdown
Contributor Author

@kzantow this seems to be an issue related to PR #1872 after my fix to decoder, your thoughts on if I should just proceed with redacting from the itest given the decision not to proceed with that?

@kzantow
Copy link
Copy Markdown
Contributor

kzantow commented Aug 3, 2023

@markgalpin are you able to rebase/squash this to fix the DCO issue and fix the static analysis issues?

To fix the static analysis: you can try to run: make lint-fix (I see comments which need spaces)

To fix the DCO... I think the problem is there might not be an empty line on this commit? (it looks fine in my git log, I'm a little unclear what's wrong)

One way: if you have an upstream remote of this repo (e.g. git remote add upstream https://github.com/anchore/syft.git):

git fetch upstream
git merge upstream/main

solve any merge conflicts (there currently are none)

git reset upstream/main

re-commit your changes with --signoff and then force-push the new single commit to this branch.

If you could do this, I'd love to help adding a few tests (or you're welcome to).

…tently expect and generate relationships with objects instead of pointers. KNOWN ISSUE: encode_decode cycle has an issue with choking on the dependsOn section for cycloneDX json objects (but not XML)

Signed-off-by: Mark Galpin <mark@tidelift.com>
@markgalpin
Copy link
Copy Markdown
Contributor Author

@kzantow Sorry for the delay, got busy with other work -- I really appreciate the advice you've given. This commit does most things discussed above, but I need some help figuring out how to suppress the regex in the encode/decode cycle for the dependsOn block. I haven't been able to figure out a REGEX for it that works quite (admittedly I haven't had a whole lot of time to spend on the project, and next time I get a chance I'll take another stab at it)

And then, yes, any help you have on other ways to test this, I'll gladly take, though it looks like mostly the test cycle is relying on encode/decode itest to catch things.

@kzantow
Copy link
Copy Markdown
Contributor

kzantow commented Aug 17, 2023

Thanks for getting this started @markgalpin -- I pushed a few changes and added some more testing

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.

Relationships section of CycloneDX is not outputting even when the data is present

3 participants