-
Notifications
You must be signed in to change notification settings - Fork 180
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: add bentoml insecure deserlization plugin #520
Conversation
@tooryx could you help me on this issue too? In case your colleague was busy. |
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.
Hey @secureness, thanks for your contribution!
First of all, about your issues with Tsunami, the two CLI switches you're referencing were added pretty recently. Just make sure you're not using the Tsunami JAR from the release section, but rather compile everything from source. I'd suggest just purging everything and running the quick_start_advanced.sh
script from scratch.
Also, I've found that I needed to append this to my yaml config, otherwise I'd get a Nullptr Exception:
common:
net:
http:
trust_all_certificates: true
connect_timeout_seconds: 30
About the plugin, I confirm it's working after fixing the small bugs I've highlighted in the review.
Please after addressing the review comments, also make sure to do the following:
- Try to fix the indentation, some lines seem extra-indented and a few multiline strings start and end at different indentation levels
- Add a file with the test cases. You can take inspiration from the other Python plugins in the repo
I'll add the test cases soon, sorry I didn't notice that Python plugins have test cases too :) |
@lokiuox I added the tests. |
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.
Hi @secureness, I added a few more comments
py_plugins/bentoml_deserialization_rce/bentoml_rce_detector_tests.py
Outdated
Show resolved
Hide resolved
py_plugins/bentoml_deserialization_rce/bentoml_rce_detector_tests.py
Outdated
Show resolved
Hide resolved
…mistake about wrong indent thanks to the lokiuox
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.
Hey @secureness, I've left two comments on a a few minor things, besides those the plugin looks good.
py_plugins/bentoml_deserialization_rce/bentoml_rce_detector_tests.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Savio Sisco <[email protected]>
Hi @secureness, I just noticed that the plugin lacks a check to ensure that the service is indeed BentoML before calling |
LGTM - Approved Reviewer: Savio, Doyensec |
Hi @secureness, This is being reviewed internally and should be merged in a few days. ~tooryx |
The issue of this PR: #482
The testbed: google/security-testbeds#73