-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
[backend] Security exploit in mlpipeline-UI #9889
Comments
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Let's keep it open |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
/lifecycle-frozen |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
not stale |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
/lifecycle frozen |
@HumairAK where exactly did you fix #9889 in #10787 ? I see only "Because we don't do namespace verification server side, #9889 where users can dupe the namespace client side and access object stores from another profile namespace. Because we are storing the provider's creds location in mlmd and querying it from client side, client can technically dupe this, and attempt to read object stores if their secrets are stored else where on the cluster. However, once we resolve #9889 this should no longer be an issue, because any attempt to access resources outside the user's profile namespace should be rejected." which says the opposite. |
/reopen until this is clarified |
@juliusvonkohout: Reopened this issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Environment
Steps to reproduce
This image here shows Bobs S3 artifacts produced by a pipeline run (e.g. tensorflow model)
Sadly the KFP UI allows Alice to read Bobs artifacts content
But how?
If Alice spies the S3 artifact location from Bob, as seen at the bottom of the image,
She can just can remove the yellow namespace parameter and the UI server will skip permission checks
So you still need to know the S3/GCS path which you can for example get from an insecure (that is the default in KFP) not-disabled ML-metadata
Expected result
The namespace parameter of the readartifact api call should not be user-configurable.
That should have been achieved for the APIserver in another PR from me and @difince https://github.com/difince/pipelines/blob/d0424bf86de5217d41dd03b50f91ac5ec3489df3/backend/src/apiserver/server/run_server.go#L313
But with mlpipeline-ui you can circumvent the permission check in the apiserver as seen on the screenshot.
You can just remove the namespace parameter and the artifact-proxy mode is disabled.
So mlpipeline-ui must set the namespace parameter based on the namespace dropdown selection for an authenticated user and not expose the namespace parameter to the user in the URL.
#8074 is also important to secure the apiserver in the future and
in the long term i would really like to get rid of the artifactproxy altogether as stated in #8406 (comment) and #4790 (comment) but that is for another day.
Materials and Reference
This has been discussed in the last KFP meeting (16th August 2023) and acknowledged by @zijianjoy on a technical level.
From the Kubecon presentation https://static.sched.com/hosted_files/kccnceu2023/f8/Hardening%20Kubeflow%20Security%20for%20Enterprise%20Environments%20-%20Julius%20von%20Kohout%2C%20DPDHL%20%26%20Diana%20Dimitrova%20Atanasova%2C%20VMware.pdf
Impacted by this bug? Give it a 👍.
The text was updated successfully, but these errors were encountered: