-
Notifications
You must be signed in to change notification settings - Fork 8
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
Bind Identity to S3SecurityController #145
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.
LGTM.
I would to add at least one test that binds an CredentialsProvider
that will return a non-empty identify and check that the identity reaches S3SecurityFacadeProvider#securityFacadeForRequest
. We should also bind a custom Identity class. I don't think we currently have any tests that combine all these functionalities.
Updating TestS3SecurityController
with this might make sense. @vagaerg WDYT?
Yup that makes sense. (Edit): Also, my initial reaction to this was that we can rely on the tests of plugins like the OPA security interface (admittedly, once we create some more thorough tests) to indirectly assert that the core is passing down identities properly. But perhaps it makes sense to bring these tests into the core now I was also going to suggest waiting for @Randgalt to have a look given this is an SPI change |
|
||
/** | ||
* Return a validated/authenticated facade for the given request or | ||
* throw {@link jakarta.ws.rs.WebApplicationException} | ||
*/ | ||
S3SecurityFacade securityFacadeForRequest(ParsedS3Request request) | ||
S3SecurityFacade securityFacadeForRequest(ParsedS3Request request, Optional<Identity> identity) |
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.
wdyt about passing the full Credentials
object instead of just the identity? That might be more flexible for the future. Thoughts?
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.
Thought about the same, but would OPA really need to know emulated/ remote credentials ?
The credential provider can be used to customise the identity entities that can be passed to OPA module.
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.
One change to consider. Otherwise LGTM
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.
Currently, the TestingCredentials - https://github.com/trinodb/aws-proxy/blob/main/trino-aws-proxy/src/test/java/io/trino/aws/proxy/server/testing/TestingUtil.java#L57 is used by the s3 client. should we add Identity to this ?
If not, how should we plan to test this or should we rely on other tests that the identity from credentials provider is passed to OPA correctly?
Another workaround - In the test class, we could modify the stored credentials in the TestingCredentialsProvider to fetch and restore new credential with identity
d28c489
to
ff4f633
Compare
Added the Identoty to TestingCredentials. I have also added an assert in the TestOpaSecurity class to verify the identity |
ff4f633
to
5a6a714
Compare
No description provided.