OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Chetan Mehrotra
Recently we had internal discussion for Ian's requirement in OAK-6575.
See issue for complete details. In brief

1. Need a way to provide a signed url [1] for Blobs stored in Oak if
they are stored in S3
2. The url would only be created if the user can access the Binary.
3.  The url would only be valid for certain time

To meet this requirement various approaches were suggested like using
Adaptable pattern in Sling, or having a new api in Binary object.

Would follow up with a sketch for such an API

Chetan Mehrotra
[1] http://docs.aws.amazon.com/AmazonS3/latest/dev/ShareObjectPreSignedURL.html


Re: OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Chetan Mehrotra
Below is one possible sketch of the proposed api. We introduce a new
AdaptableBinary which allows adapting a Binary to some other form.

API
===

public interface AdaptableBinary {

/**
 * Adapts the binary to another type
 *
 * @param  The generic type to which this binary is adapted
 *to
 * @param type The Class object of the target type
 * @return The adapter target or null if the binary cannot
 * adapt to the requested type
 */
 AdapterType adaptTo(Class type);
}



Usage
=

Binary binProp = node.getProperty("jcr:data").getBinary();

//Check if Binary is of type AdaptableBinary
if (binProp instanceof AdaptableBinary){
AdaptableBinary adaptableBinary = (AdaptableBinary) binProp;
SignedBinary url = adaptableBinary.adaptTo(SignedBinary.class);
}

Where SignedBinary is one of the supported adaptables.

public interface SignedBinary {

URL getUrl(int ttl, TimeUnit unit)
}

The user can specify ttl. The implementation may enforce an upper
bound on the allowed ttl.

This proposal is meant to provide base. If we agree on the general
approach then we can decide further details like

1. Under which package to expose AdaptableBinary

Proposal 'org.apache.jackrabbit.oak.jcr.binary'. We would also later
possibly need an AdaptableBlob for Oak layer

2. Under which package to expose SignedBinary

Proposal 'org.apache.jackrabbit.oak.api.blob' in oak-api

Thoughts?
Chetan Mehrotra


On Wed, Aug 23, 2017 at 4:25 AM, Chetan Mehrotra
 wrote:
> Recently we had internal discussion for Ian's requirement in OAK-6575.
> See issue for complete details. In brief
>
> 1. Need a way to provide a signed url [1] for Blobs stored in Oak if
> they are stored in S3
> 2. The url would only be created if the user can access the Binary.
> 3.  The url would only be valid for certain time
>
> To meet this requirement various approaches were suggested like using
> Adaptable pattern in Sling, or having a new api in Binary object.
>
> Would follow up with a sketch for such an API
>
> Chetan Mehrotra
> [1] 
> http://docs.aws.amazon.com/AmazonS3/latest/dev/ShareObjectPreSignedURL.html


Re: OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Julian Reschke

On 2017-08-23 13:39, Chetan Mehrotra wrote:

Below is one possible sketch of the proposed api. We introduce a new
AdaptableBinary which allows adapting a Binary to some other form.

API
===

public interface AdaptableBinary {

 /**
  * Adapts the binary to another type
  *
  * @param  The generic type to which this binary is adapted
  *to
  * @param type The Class object of the target type
  * @return The adapter target or null if the binary cannot
  * adapt to the requested type
  */
  AdapterType adaptTo(Class type);
}


Can we make that more generic, not relying on Binary?


Usage
=

Binary binProp = node.getProperty("jcr:data").getBinary();

//Check if Binary is of type AdaptableBinary
if (binProp instanceof AdaptableBinary){
 AdaptableBinary adaptableBinary = (AdaptableBinary) binProp;
 SignedBinary url = adaptableBinary.adaptTo(SignedBinary.class);
}

Where SignedBinary is one of the supported adaptables.

public interface SignedBinary {

 URL getUrl(int ttl, TimeUnit unit)
}


Use URI, not URL.


...


Best regards, Julian


Re: OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Ian Boston
Hi,

On 23 August 2017 at 13:06, Julian Reschke  wrote:

> On 2017-08-23 13:39, Chetan Mehrotra wrote:
>
>> Below is one possible sketch of the proposed api. We introduce a new
>> AdaptableBinary which allows adapting a Binary to some other form.
>>
>> API
>> ===
>>
>> public interface AdaptableBinary {
>>
>>  /**
>>   * Adapts the binary to another type
>>   *
>>   * @param  The generic type to which this binary is
>> adapted
>>   *to
>>   * @param type The Class object of the target type
>>   * @return The adapter target or null if the binary
>> cannot
>>   * adapt to the requested type
>>   */
>>   AdapterType adaptTo(Class type);
>> }
>>
>
> Can we make that more generic, not relying on Binary?



adapting from something already Adaptable would work.
Sling Resource might be a good option, since a) its adaptable, and b) its
resolvable and already available to the code that would use this.




>
>
> Usage
>> =
>>
>> Binary binProp = node.getProperty("jcr:data").getBinary();
>>
>> //Check if Binary is of type AdaptableBinary
>> if (binProp instanceof AdaptableBinary){
>>  AdaptableBinary adaptableBinary = (AdaptableBinary) binProp;
>>  SignedBinary url = adaptableBinary.adaptTo(SignedBinary.class);
>> }
>>
>>

This breaks the Adaptable pattern, which should not require intermediate
interfaces.
If the implementation needs instanceof we can drop the Adaptable pattern
and just use APIs.

if (binProp instanceof SignedBinary) {
 response.sendRedirect(((SingedBinary)binProp).getURL());
}

better would be

SingedBinary sb = resource.adaptTo(SignedBinary.class);
if ( sb != null ) { // oak might decide for this invocation a signed binary
is not appropriate, and so return null.
  response.sendRedirect(sb.getURL());
}

If the DS didn't have a AdapterFactory implemented, null will be returned.
Optionally implementing an interface on a call by call basis requires more
work, potentially, than adapting.




> Where SignedBinary is one of the supported adaptables.
>>
>> public interface SignedBinary {
>>
>>  URL getUrl(int ttl, TimeUnit unit)
>> }
>>
>
> Use URI, not URL.
>

Is there something so wrong with an immutable String in this case ?

Also, As a caller I could specify 20 years, or 30s. Oak should decide what
the ttl is, not leave it to the caller.

Best Regards
Ian


> ...
>>
>
> Best regards, Julian
>


Re: OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Ian Boston
Somehow the following between me and Chetan slipped off list, so copying it
back here to continue on list, probably my fault, apologies.



Hi,

On 23 August 2017 at 13:29, Chetan Mehrotra 
 wrote:

> > >> Can we make that more generic, not relying on Binary?
>
> Was being conservative there and hence not proposed a generic
> adaptable api. But +1 for making it generic
>
> > adapting from something already Adaptable would work.
> > Sling Resource might be a good option, since a) its adaptable, and b) its
> > resolvable and already available to the code that would use this.
>
> So far Oak does not have a dependency on Sling API and this api should
> be usable in non Sling setups also
>


In that case, Oak can't implement the AdapterFactory interface either or
implement Adaptables. AdaptableBinary should
strictly speaking extend Adaptable if it is to implement adaptTo and be
used in a Sling based deployment.

Since the AdapterFactory and Adaptable interface are not available, the
benefits are not available to Oak.

Hence, why not simply use  binaryProp instanceof SignedBinary ?

Oak would have to provide multiple versions of the Binary implementation,
differing depending on if or not a SignedBinary was appropriate. That might
be dependent on the properties of the node or one of its parents. It would
certainly be dependent on if the Binary was in S3 (or equiv) or NodeStore
(<16K).



>
> > > SingedBinary sb = resource.adaptTo(SignedBinary.class);
>
> At Oak level we do not have resource. What we have is JCR Binary
> interface which does not extend Adaptable
>

ok, understood.


>
> > Also, As a caller I could specify 20 years, or 30s. Oak should decide
> what
> > the ttl is, not leave it to the caller.
>
> As mentioned earlier implementation can enforce an upper limit. My
> take was that different usecase require different ttls. For e.g. if
> the asset needs to be shared for some workflow processing then it may
> require longer ttl. If other think its not useful we can drop that
>

If a long ttl is possible for certain use cases, then what is to stop
someone using that long ttl where it is not appropriate (publishing it to
public html).?
The ttl should be configurable by the deployer, but not by the caller.

OAK-6575 was only really thinking about requests being converted into
redirects that would be used immediately.

If something like workflow (or any client) needed direct access some time
later, it should authenticate against Oak, be issued a fresh signed url as
a redirect and access the binary using its own signed url.

No client should be issued a signed url that could be used in the distant
(relatively) future bypassing fresh ACL constraints saved to Oak.

Best Regards
Ian




>
> Chetan Mehrotra





On 23 August 2017 at 13:22, Ian Boston  wrote:

> Hi,
>
> On 23 August 2017 at 13:06, Julian Reschke  wrote:
>
>> On 2017-08-23 13:39, Chetan Mehrotra wrote:
>>
>>> Below is one possible sketch of the proposed api. We introduce a new
>>> AdaptableBinary which allows adapting a Binary to some other form.
>>>
>>> API
>>> ===
>>>
>>> public interface AdaptableBinary {
>>>
>>>  /**
>>>   * Adapts the binary to another type
>>>   *
>>>   * @param  The generic type to which this binary is
>>> adapted
>>>   *to
>>>   * @param type The Class object of the target type
>>>   * @return The adapter target or null if the binary
>>> cannot
>>>   * adapt to the requested type
>>>   */
>>>   AdapterType adaptTo(Class type);
>>> }
>>>
>>
>> Can we make that more generic, not relying on Binary?
>
>
>
> adapting from something already Adaptable would work.
> Sling Resource might be a good option, since a) its adaptable, and b) its
> resolvable and already available to the code that would use this.
>
>
>
>
>>
>>
>> Usage
>>> =
>>>
>>> Binary binProp = node.getProperty("jcr:data").getBinary();
>>>
>>> //Check if Binary is of type AdaptableBinary
>>> if (binProp instanceof AdaptableBinary){
>>>  AdaptableBinary adaptableBinary = (AdaptableBinary) binProp;
>>>  SignedBinary url = adaptableBinary.adaptTo(SignedBinary.class);
>>> }
>>>
>>>
>
> This breaks the Adaptable pattern, which should not require intermediate
> interfaces.
> If the implementation needs instanceof we can drop the Adaptable pattern
> and just use APIs.
>
> if (binProp instanceof SignedBinary) {
>  response.sendRedirect(((SingedBinary)binProp).getURL());
> }
>
> better would be
>
> SingedBinary sb = resource.adaptTo(SignedBinary.class);
> if ( sb != null ) { // oak might decide for this invocation a signed
> binary is not appropriate, and so return null.
>   response.sendRedirect(sb.getURL());
> }
>
> If the DS didn't have a AdapterFactory implemented, null will be returned.
> Optionally implementing an interface on a call by call basis requires more
> work, potentially, than adapting.
>
>
>
>
>> Where SignedBinary is one of the supported adaptables.
>>>
>>> public inter

Re: OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Julian Reschke

On 2017-08-23 14:22, Ian Boston wrote:

...
This breaks the Adaptable pattern, which should not require intermediate
interfaces.
If the implementation needs instanceof we can drop the Adaptable pattern
and just use APIs.
...


Not really. It depends on how many extensions there might be in the 
future. Using instanceOf doesn't work well if there may multiple 
extensions in the future thatcan be combined.


...we should have a generic "Adaptable" pattern in Oak for a long time...

Best regards, Julian


Re: OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Chetan Mehrotra
> Hence, why not simply use  binaryProp instanceof SignedBinary ?

As Julian mentioned it would make it tricky to support multiple
extensions with various permutations. Having adapter support for
simplify the implementation

> No client should be issued a signed url that could be used in the distant
> (relatively) future bypassing fresh ACL constraints saved to Oak.

Fair point. Then lets drop the ttl paramater

Chetan Mehrotra


Re: OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Chetan Mehrotra
Based on the feedback so far below is revised proposal

1. Define a new Adaptable interface in 'org.apache.jackrabbit.oak.api'

public interface Adaptable {

/**
 * Adapts the binary to another type
 *
 * @param  The generic type to which this type is adapted
 *to
 * @param type The Class object of the target type
 * @return The adapter target or null if the type cannot
 * adapt to the requested type
 */
 AdapterType adaptTo(Class type);
}

2. Have the binary implementation in Oak implement Adaptable
3. Have a minimal implementation in Oak on line of Sling Adaptor support [1]

For current usecase we would provide an adaptation to SignedBinary

public interface SignedBinary {

URI getUri()
}

Chetan Mehrotra

[1] 
https://github.com/apache/sling/tree/trunk/bundles/api/src/main/java/org/apache/sling/api/adapter


On Wed, Aug 23, 2017 at 10:04 PM, Chetan Mehrotra
 wrote:
>> Hence, why not simply use  binaryProp instanceof SignedBinary ?
>
> As Julian mentioned it would make it tricky to support multiple
> extensions with various permutations. Having adapter support for
> simplify the implementation
>
>> No client should be issued a signed url that could be used in the distant
>> (relatively) future bypassing fresh ACL constraints saved to Oak.
>
> Fair point. Then lets drop the ttl paramater
>
> Chetan Mehrotra


Re: OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Amit Jain
Hi,

+1 from my side for the broad contours as proposed above.

Thanks
Amit

On Thu, Aug 24, 2017 at 10:58 AM, Chetan Mehrotra  wrote:

> Based on the feedback so far below is revised proposal
>
> 1. Define a new Adaptable interface in 'org.apache.jackrabbit.oak.api'
>
> public interface Adaptable {
>
> /**
>  * Adapts the binary to another type
>  *
>  * @param  The generic type to which this type is adapted
>  *to
>  * @param type The Class object of the target type
>  * @return The adapter target or null if the type cannot
>  * adapt to the requested type
>  */
>  AdapterType adaptTo(Class type);
> }
>
> 2. Have the binary implementation in Oak implement Adaptable
> 3. Have a minimal implementation in Oak on line of Sling Adaptor support
> [1]
>
> For current usecase we would provide an adaptation to SignedBinary
>
> public interface SignedBinary {
>
> URI getUri()
> }
>
> Chetan Mehrotra
>
> [1] https://github.com/apache/sling/tree/trunk/bundles/api/
> src/main/java/org/apache/sling/api/adapter
>
>
> On Wed, Aug 23, 2017 at 10:04 PM, Chetan Mehrotra
>  wrote:
> >> Hence, why not simply use  binaryProp instanceof SignedBinary ?
> >
> > As Julian mentioned it would make it tricky to support multiple
> > extensions with various permutations. Having adapter support for
> > simplify the implementation
> >
> >> No client should be issued a signed url that could be used in the
> distant
> >> (relatively) future bypassing fresh ACL constraints saved to Oak.
> >
> > Fair point. Then lets drop the ttl paramater
> >
> > Chetan Mehrotra
>


Re: OAK-6575 - Provide a secure external URL to a DataStore binary.

2017-08-23 Thread Michael Dürig


Hi,

I think the discussion about the adapter pattern is orthogonal to the 
binary issue. According to YAGNI we should stick with instance of checks 
unless we already have a somewhat clear picture of future extensions.


Michael

On 24.08.17 07:28, Chetan Mehrotra wrote:

Based on the feedback so far below is revised proposal

1. Define a new Adaptable interface in 'org.apache.jackrabbit.oak.api'

public interface Adaptable {

 /**
  * Adapts the binary to another type
  *
  * @param  The generic type to which this type is adapted
  *to
  * @param type The Class object of the target type
  * @return The adapter target or null if the type cannot
  * adapt to the requested type
  */
  AdapterType adaptTo(Class type);
}

2. Have the binary implementation in Oak implement Adaptable
3. Have a minimal implementation in Oak on line of Sling Adaptor support [1]

For current usecase we would provide an adaptation to SignedBinary

public interface SignedBinary {

 URI getUri()
}

Chetan Mehrotra

[1] 
https://github.com/apache/sling/tree/trunk/bundles/api/src/main/java/org/apache/sling/api/adapter


On Wed, Aug 23, 2017 at 10:04 PM, Chetan Mehrotra
 wrote:

Hence, why not simply use  binaryProp instanceof SignedBinary ?


As Julian mentioned it would make it tricky to support multiple
extensions with various permutations. Having adapter support for
simplify the implementation


No client should be issued a signed url that could be used in the distant
(relatively) future bypassing fresh ACL constraints saved to Oak.


Fair point. Then lets drop the ttl paramater

Chetan Mehrotra