-
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
Mlflow weak credential tester #455
Conversation
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 for your pull request. Please review the comments.
Regarding failure to build - did you follow instructions for setup in README of the main Tsunami repo? Particularly, what is the - gradle --version
? We need 6.5 for building Tsunami.
|
||
private boolean isMlFlowAccessible(NetworkService networkService, TestCredential credential) { | ||
var uriAuthority = NetworkEndpointUtils.toUriAuthority(networkService.getNetworkEndpoint()); | ||
var url = String.format("http://%s/%s", uriAuthority, "api/2.0/mlflow/users/create"); |
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.
Don't create a user - the scanner acts on live systems and this will create a user with a username and password that can be found in the code below.
Instead, use the Get User
endpoint to check for the existence of the user https://mlflow.org/docs/latest/auth/rest-api.html#get-user
url, credential.username(), credential.password().orElse("")); | ||
HttpResponse response = sendRequestWithCredentials(url, credential); | ||
return response.status().isSuccess() | ||
&& response |
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.
Remove this second check for UserRegistration
. See above - just check for a successful response.
* {"user":{"experiment_permissions":[],"id":4,"is_admin":false,"registered_model_permissions":[], | ||
* "username":"googleTsunamiSecurityScanner"}} | ||
*/ | ||
private static boolean bodyContainsSuccessfulUserRegistration(String responseBody) { |
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.
Rename or remove this method as discussed above.
@pisqu4red The error is because of the tsunami CLI, when I compiled it with the docker command and copied the final tsunami CLI jar file from docker to my environment, everything was normal. the Gradle version was the same from the beginning. Maybe The reason is that I was building the tsunami CLI with OpenJDK 11 and it should be 13 instead. |
@lanced00m could you please sync with master branch? We have upgraded gradle to 7.0 in this commit so this should resolve the last failures. The reason for asking for the Could you please address my previous comments before doing another round of review. Thanks! |
@pisqu4red Sorry I didn't understand fully your last comment. |
@pisqu4red Now you can compare the initial commit with the last commit easier |
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 for the changes, small nitpicks from my side before we merge.
...ors/credentials/genericweakcredentialdetector/testers/mlflow/MlFlowCredentialTesterTest.java
Outdated
Show resolved
Hide resolved
...ors/credentials/genericweakcredentialdetector/testers/mlflow/MlFlowCredentialTesterTest.java
Outdated
Show resolved
Hide resolved
...ors/credentials/genericweakcredentialdetector/testers/mlflow/MlFlowCredentialTesterTest.java
Outdated
Show resolved
Hide resolved
...ors/credentials/genericweakcredentialdetector/testers/mlflow/MlFlowCredentialTesterTest.java
Outdated
Show resolved
Hide resolved
...ors/credentials/genericweakcredentialdetector/testers/mlflow/MlFlowCredentialTesterTest.java
Outdated
Show resolved
Hide resolved
...tectors/credentials/genericweakcredentialdetector/testers/mlflow/MlFlowCredentialTester.java
Outdated
Show resolved
Hide resolved
...tectors/credentials/genericweakcredentialdetector/testers/mlflow/MlFlowCredentialTester.java
Outdated
Show resolved
Hide resolved
...tectors/credentials/genericweakcredentialdetector/testers/mlflow/MlFlowCredentialTester.java
Outdated
Show resolved
Hide resolved
Hi @lanced00m, Your PR has been merged. This usually means a reward will be granted. Google will start the internal QC process and the reward amount will be determined based on the quality of the detector report. Please be patient and allow up to a week for the QC process to finish. You'll be notified once the decision is made. Thanks! |
#412
Hi @maoning
I couldn't test the plugin on a vulnerable target because of this error:
but I'm sure it works because of the Comprehensive test cases I've implemented.
what I'm doing is simply running
./gradlew build
ingoogle/detectors/credentials/generic_weak_credential_detector
directory and then I copy the Nmap plugin togoogle/detectors/credentials/generic_weak_credential_detector/build/libs/
because it is needed.finally, I run this command:
java -cp "tsunami-main-0.0.22-SNAPSHOT-cli.jar:/absolute/path/to/tsunami-security-scanner-plugins/google/detectors/credentials/generic_weak_credential_detector/build/libs/*" com.google.tsunami.main.cli.TsunamiCli --uri-target=http://127.0.0.1:5000
I'm working with
openjdk-11
.