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

Detect golang boring crypto and fipsonly modules #2021

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

bathina2
Copy link
Contributor

This PR adds functionality to Syft to derive the crypto settings of a go executable.

It imports a library that scans the symbols and determines whether an executable contains the boring crypto module as well as the fipsonly module.

@kzantow
Copy link
Contributor

kzantow commented Aug 14, 2023

Thanks for the contribution @bathina2 -- I'm a little hesitant to merge this as-is with a forked repo to add reader functionality (as you've submitted in a PR to the main repo here). We try to avoid forking repositories and just use upstream when we can (there have been a couple cases where the upstream was unresponsive to necessary fixes, etc.). Could we see if your PR gets accepted upstream to rsc/goversion in some sort of reasonable time frame?

@bathina2
Copy link
Contributor Author

I was hesitant as well. I would have liked to have the reader functionality merged upstream, however, the upstream project hasn't changed much in a while and we don't have real time frame. My thoughts were that we could get this PR merged in syft and once the goversion changes got merged upstream we could pull them in.

Signed-off-by: Sirish Bathina <sirish@kasten.io>
Signed-off-by: Sirish Bathina <sirish@kasten.io>
Signed-off-by: Sirish Bathina <sirish@kasten.io>
@wagoodman
Copy link
Contributor

@bathina2 if you run make lint-fix you should be good to go CI-wise.

Signed-off-by: Sirish Bathina <sirish@kasten.io>
@wagoodman
Copy link
Contributor

@kzantow is right that we try to keep references to upstreams before forking. That being said, I would be OK with getting the functionality under the fork then create an issue for removing the fork and tracking the upstream PR there.

Signed-off-by: Sirish Bathina <sirish@kasten.io>
@bathina2
Copy link
Contributor Author

@wagoodman Thanks for the feedback! I've addressed them. It would be great to get this change into upstream Syft.

GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
* Extending build info to include crypto settings

Signed-off-by: Sirish Bathina <sirish@kasten.io>

* Use kasten fork for goversion module

Signed-off-by: Sirish Bathina <sirish@kasten.io>

* go mod tidy

Signed-off-by: Sirish Bathina <sirish@kasten.io>

* change key to GoCryptoSettings and lint fix

Signed-off-by: Sirish Bathina <sirish@kasten.io>

* Addressing feedback

Signed-off-by: Sirish Bathina <sirish@kasten.io>

---------

Signed-off-by: Sirish Bathina <sirish@kasten.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants