search

trinodb / aws-proxy

Proxy for S3
Apache License 2.0
7 stars 3 forks source link

Address OPARequest #148

Closed pranavr12 closed 23 hours ago

pranavr12 commented 3 weeks ago

Currently, in OpaS3SecurityFacadeProvider, the function, does two operations:

  1. Builds an OPA request - Builds a request containing the OPA document and the headers for the OPA request
  2. Sends the request to OPA server The return type of the function is SecurityResponse.

The issue we are currently facing is that while building the OPA request, based on some rules/ conditions we want to shortcut the OPA decision to either SecurityResponse - SUCCESS or FAILURE without sending a request to OPA server. We could have the opaS3SecurityMapper.toRequest returning a wrapper containing (OpaRequest | SecurityResponse), and based on SecurityResponse we shortcut the decision or proceed with further steps. WDYT ? How should we handle this ?

@Randgalt , @vagaerg , @mosiac1

Randgalt commented 3 weeks ago

You'd only short circuit on FAILURE or might it be both SUCCESS and FAILURE? If the short circuit is just FAILURE is this an exceptional (rare) case? If so, I'd have opaS3SecurityMapper throw an exception indicating failure.

pranavr12 commented 3 weeks ago

@Randgalt We would want to shortcut it mostly on FAILURE. The failure scenario could be fairly common - In our case, we call an external service to fetch the table name. This external service can send a 404, error response or it could be unavailable.

Randgalt commented 3 weeks ago

I suggest throwing an exception or making a new response as you offered: (OpaRequest | Optional<SecurityResponse>)

mosiac1 commented 3 weeks ago

I prefer creating a new response type, I like the no-throw contract we have for the security classes, its easier to track where decisions are made.

I was thinking of OpaRequest | SecurityResponse, which we can now do with sealed interfaces.

In (OpaRequest | Optional<SecurityResponse>), what would an empty Optional represent?

Randgalt commented 3 weeks ago

I'm assuming the SecurityResponse is optional.

pranavr12 commented 3 weeks ago

As @mosiac1 , we can do this by using sealed interfaces where the types are OpaRequest and SecurityResponse. I can raise a draft PR for this

pranavr12 commented 23 hours ago

Closed by https://github.com/trinodb/aws-proxy/pull/150