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

Add MinIO admin client functionality #1221

Merged
merged 16 commits into from
Sep 16, 2021

Conversation

Sam-Kramer
Copy link
Contributor

Adds the following calls for MinIO Admin functionality:

  • Add/Set/List/Delete policies
  • Add/Set/List/Delete users

I had to add a dependency on bouncy castle and implement sio-go's encryption methodology in-order to encrypt/decrypt UserInfo payloads.

Seeing as there is no integration test, I tested against my local MinIO server running on Minikube. I commented out the tests to show how/which I tested each endpoints.

@harshavardhana
Copy link
Member

Seeing as there is no integration test, I tested against my local MinIO server running on Minikube. I commented out the tests to show how/which I tested each endpoints.

The integration tests are here functional/* - you can add them there.

@Sam-Kramer
Copy link
Contributor Author

Seeing as there is no integration test, I tested against my local MinIO server running on Minikube. I commented out the tests to show how/which I tested each endpoints.

The integration tests are here functional/* - you can add them there.

Excellent, I'll add them there! Will re-push some time tomorrow (I live in the U.K., so it's slightly late for me here 😄 )

Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

Keep MinIO Admin functionality as a separate package i.e. gradle subproject.

@harshavardhana
Copy link
Member

Keep MinIO Admin functionality as a separate package i.e. gradle subproject.

Agreed this is the clean approach.

build.gradle Outdated Show resolved Hide resolved
additional rollback

spotless
Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

Please remove gradle/wrapper/gradle-wrapper.jar file in this PR.

build.gradle Outdated Show resolved Hide resolved
functional/FunctionalTest.java Outdated Show resolved Hide resolved
Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

Minor comments; otherwise LGTM

Please address Github workflow error

gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
api/src/main/java/io/minio/S3Base.java Outdated Show resolved Hide resolved
functional/FunctionalTest.java Outdated Show resolved Hide resolved
Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

Please add copy right headers to all the newly added files.

build.gradle Show resolved Hide resolved
functional/TestMinioAdminClient.java Show resolved Hide resolved
@harshavardhana
Copy link
Member

strange the same test is failing on windows @Sam-Kramer are you using randomized usernames?

@Sam-Kramer
Copy link
Contributor Author

strange the same test is failing on windows @Sam-Kramer are you using randomized usernames?

Hmmm, this is a bit strange, I shouldn't be: https://github.com/minio/minio-java/pull/1221/files#diff-4d85893fe8f0da0e54f6e37e5bc7ae66ae7ed183606f30081ce5b48ca8670789R31-R33

And I am just using the same accessKey/secretKey as the regular client. It's an unauthorized message, so I wonder if we are signing the request differently on a windows host?

@balamurugana
Copy link
Member

strange the same test is failing on windows @Sam-Kramer are you using randomized usernames?

Hmmm, this is a bit strange, I shouldn't be: https://github.com/minio/minio-java/pull/1221/files#diff-4d85893fe8f0da0e54f6e37e5bc7ae66ae7ed183606f30081ce5b48ca8670789R31-R33

And I am just using the same accessKey/secretKey as the regular client. It's an unauthorized message, so I wonder if we are signing the request differently on a windows host?

Github workflow on windows uses play.minio.io. You would need to create random names for the testing.

harshavardhana
harshavardhana previously approved these changes Sep 1, 2021
@Sam-Kramer
Copy link
Contributor Author

@harshavardhana / @balamurugana , does the user for play.minio.io have admin permissions?

Friendly bump on this ^

Which user are you using? @Sam-Kramer

I'm simply re-using whatever the previous functional tests used for their user:

client = MinioClient.builder().endpoint(endpoint).credentials(accessKey, secretKey).build();
MinioAdminClient adminClient =
MinioAdminClient.builder().endpoint(endpoint).credentials(accessKey, secretKey).build();
.

I'd imagine the credentials being used are stored in the circleci test, which I don't believe I cannot see.

@balamurugana
Copy link
Member

@harshavardhana The credential information is available at https://github.com/minio/minio-java/blob/master/build.gradle#L284

@harshavardhana
Copy link
Member

I'm simply re-using whatever the previous functional tests used for their user:

Let me check locally what is failing @Sam-Kramer

@harshavardhana
Copy link
Member

Looks like all tests run fine locally

createUser()
addCannedPolicy()
setPolicy()
listUsers()
listCannedPolicies()
deleteUser()
removeCannedPolicy()

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.6.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 1m 31s
6 actionable tasks: 6 executed

@balamurugana
Copy link
Member

@harshavardhana Yep. It fails only with play.min.io as showed in Github workflow.

@Sam-Kramer
Copy link
Contributor Author

Looks like all tests run fine locally

createUser()
addCannedPolicy()
setPolicy()
listUsers()
listCannedPolicies()
deleteUser()
removeCannedPolicy()

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.6.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 1m 31s
6 actionable tasks: 6 executed

Was this ran against a local minio or against play.min.io w/ this user (https://github.com/minio/minio-java/blob/master/build.gradle#L284)? Seeing as it's an Access Denied exception, I'd imagine there's nothing wrong with the message signing itself, and rather, a permission issue?

@harshavardhana
Copy link
Member

Was this ran against a local minio or against play.min.io w/ this user (https://github.com/minio/minio-java/blob/master/build.gradle#L284)? Seeing as it's an Access Denied exception, I'd imagine there's nothing wrong with the message signing itself, and rather, a permission issue?

This is a root user - it can never return "Access Denied" @Sam-Kramer that's why I am perplexed.

@harshavardhana
Copy link
Member

Found the problem - it is the usage of AdminClient using the signature v4 style when TLS is enabled it ends up passing UNSIGNED-PAYLOAD for x-amz-content-sha256 this is not supported with Admin API on MinIO server.

i.e MinIO server expects a proper x-amz-content-sha256 value not a special string even with TLS. This is unlike S3 API behavior. If you fix that it will work fine.

Additionally I am also going to allow x-amz-content-sha256 -> UNSIGNED-PAYLOAD value.

@Sam-Kramer
Copy link
Contributor Author

Found the problem - it is the usage of AdminClient using the signature v4 style when TLS is enabled it ends up passing UNSIGNED-PAYLOAD for x-amz-content-sha256 this is not supported with Admin API on MinIO server.

i.e MinIO server expects a proper x-amz-content-sha256 value not a special string even with TLS. This is unlike S3 API behavior. If you fix that it will work fine.

Additionally I am also going to allow x-amz-content-sha256 -> UNSIGNED-PAYLOAD value.

Thanks for digging into this! Excellent, I'll fix this and push

@harshavardhana
Copy link
Member

Thanks for digging into this! Excellent, I'll fix this and push

Take a look at the PR #1231 - these fixes should be incorporated in your build.

@harshavardhana
Copy link
Member

I also took the opportunity to fix the MinIO build service windows runs on CI/CD - now MinIO on windows runs a local instance, instead of talking to https://play.min.io

It is also doing TLS tests locally as well.

harshavardhana
harshavardhana previously approved these changes Sep 14, 2021
balamurugana
balamurugana previously approved these changes Sep 15, 2021
functional/FunctionalTest.java Outdated Show resolved Hide resolved
functional/FunctionalTest.java Show resolved Hide resolved
@harshavardhana harshavardhana dismissed stale reviews from balamurugana and themself via 273d2d2 September 15, 2021 08:29
@balamurugana balamurugana merged commit c21b24c into minio:master Sep 16, 2021
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.

4 participants