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.

Sign up for GitHub

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

Jump to bottom

Zenml weak credentials #491

Merged
merged 6 commits into from
Jun 25, 2024
Merged

Zenml weak credentials #491

merged 6 commits into from
Jun 25, 2024

Conversation

secureness
Copy link
Contributor

@secureness secureness mentioned this pull request May 27, 2024
Closed
leonardo-doyensec
leonardo-doyensec reviewed Jun 11, 2024
View reviewed changes
Copy link
Collaborator

@leonardo-doyensec leonardo-doyensec left a comment

Choose a reason for hiding this comment

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

Hello @secureness.
Thank you for your contribution. There are several issues to address. You can find all of them in the comments.

Feel free to reach out.
~ Leonardo (Doyensec)

...detectors/credentials/genericweakcredentialdetector/testers/zenml/ZenMlCredentialTester.java Outdated Show resolved Hide resolved
...detectors/credentials/genericweakcredentialdetector/testers/zenml/ZenMlCredentialTester.java Outdated Show resolved Hide resolved
...detectors/credentials/genericweakcredentialdetector/testers/zenml/ZenMlCredentialTester.java Outdated Show resolved Hide resolved
...detectors/credentials/genericweakcredentialdetector/testers/zenml/ZenMlCredentialTester.java Show resolved Hide resolved
...detectors/credentials/genericweakcredentialdetector/testers/zenml/ZenMlCredentialTester.java Outdated Show resolved Hide resolved
...tectors/credentials/genericweakcredentialdetector/data/service_default_credentials.textproto Show resolved Hide resolved
...ctors/credentials/genericweakcredentialdetector/testers/zenml/ZenMlCredentialTesterTest.java Outdated Show resolved Hide resolved
...ctors/credentials/genericweakcredentialdetector/testers/zenml/ZenMlCredentialTesterTest.java Outdated Show resolved Hide resolved
...ctors/credentials/genericweakcredentialdetector/testers/zenml/ZenMlCredentialTesterTest.java Outdated Show resolved Hide resolved
...web/src/main/java/com/google/tsunami/plugins/fingerprinters/web/WebServiceFingerprinter.java Outdated Show resolved Hide resolved
@leonardo-doyensec leonardo-doyensec self-assigned this Jun 11, 2024
@leonardo-doyensec
Copy link
Collaborator

LGTM - Approved
@maoning we can merge this. Moreover we can also merge the security testbed

Reviewer: Leonardo, Doyensec
Plugin: ZenML Weak Credentials Detector
Feedback: The overall quality is decent. The testbed required several back and forth in order to make it working properly. The plugin is working correctly, however several unused packages were found.
Drawback: None.

pisqu4red
pisqu4red requested changes Jun 25, 2024
View reviewed changes
...detectors/credentials/genericweakcredentialdetector/testers/zenml/ZenMlCredentialTester.java Outdated
}

private boolean isZenMlAccessible(NetworkService networkService, TestCredential credential) {
logger.atWarning().log(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have test breaking on our side:

error: [FloggerLogString] Arguments to log(String) must be compile-time constants or parameters annotated with @CompileTimeConstant. If possible, use Flogger's formatting log methods instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, it is weird, I think it is not good to log every single username and password and fill the stdout, I should've removed this logger, it was for debugging purposes.

@copybara-service copybara-service bot merged commit 8716342 into google:master Jun 25, 2024
5 checks passed
@secureness secureness mentioned this pull request Jul 17, 2024
Closed
@secureness
Copy link
Contributor Author

@tooryx this PR is merged for one month! but I haven't received any further updates about this PR.

@tooryx
Copy link
Member

tooryx commented Jul 29, 2024

Hey @secureness, I will check with the rest of the team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leonardo-doyensec leonardo-doyensec leonardo-doyensec left review comments

@pisqu4red pisqu4red pisqu4red requested changes

Assignees

@leonardo-doyensec leonardo-doyensec

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@secureness @leonardo-doyensec @tooryx @pisqu4red