Hi, On 14 September 2017 at 06:05, Alex Deparvu <[email protected]> wrote:
> Hi, > > > I don't think this patch bypasses Oak security, and since the API can > only > > be implemented by Oak itself. I am sure any future patch would be subject > > to the same scrutiny. If it can be implemented outside Oak, then Oak has > > already been breached, something I can see no evidence of. > > I don't think I agree with this statement. Sure you start with a proper Oak > session, but the patch facilitates any consumer code to bypass Oak security > and generate stateless links to any resource, no session involved anymore > but it's a signed url bound by a ttl. so your trust boundary goes from > (oak-security) to (oak-security + any consumer code) and here I'm not > talking about a possible breach, but simply bugs in the code, you now > delegate the security concerns to code that might not be at the same > quality level as oak. So it looks like yes, you are bypassing Oak security > to achieve this goal. > Fair point, Any data that passes over the JCR API boundary has the same 'oak-security+any consumer code' model, which is not the oak-security model. The biggest difference here is that Oak knows this bit of data provides delegated access, whereas it has not knowledge of the sensitivity of other bits of data that pass over the bounday. > > > > I feel that Oak is weaker without the ability to offload bulk data > streaming to infrastructure designed for that purpose. > > I pasted an older comment to come back to the reasoning behind this need. > If you bind the urls to a ttl, how do you guarantee that the workflows (or > whatever process does bulk data streaming) is completed within that time > frame? Will there be a retry policy? And more specifically how to make sure > this ttl will not get bumped up to a point where it becomes a real problem > (1 min/5mins is probably fine, what if someone sets it to a few hours). > Workflows should make the request back to Sling/AEM as they do now with the appropriate credentials. Those requests will get redirected, if appropriate with a fresh signed URL. The signed URL should never be put into a message that cant be acted on within the TTL time period. It is quite possible, that the current CloudFront Singed URL implementation will not be suitable for a Workflow to use, due to network topology, although that was out so scope for the requirements in OAK-6575. There was some suggestions r adding a getPrivateURI to the URIProvider interface. btw, thanks for your comments in Jira, I have updated the patch. Best Regards Ian > > > > alex > > > > On Wed, Sep 13, 2017 at 11:34 PM, Ian Boston <[email protected]> wrote: > > > Hi Angela, > > > > On 13 September 2017 at 06:50, Angela Schreiber > <[email protected] > > > > > wrote: > > > > > Hi Ian > > > > > > The new proposal looks a lot better to me. > > > > > > The only concern from a security perspective I could come up with is > the > > > one we expressed already with the very first proposal (see initial word > > of > > > caution mail sent by Francesco): applications built on top of Oak can > up > > > to now be sure that all access to the repository content is subject to > > the > > > same permission evaluation as configured with the repository setup. > This > > > is no longer guaranteed when we offer the ability to plug application > > code > > > that may or may not by-pass the evaluation by allowing it to directly > > > access binaries. > > > > > > > I don't think this patch bypasses Oak security, and since the API can > only > > be implemented by Oak itself. I am sure any future patch would be subject > > to the same scrutiny. If it can be implemented outside Oak, then Oak has > > already been breached, something I can see no evidence of. > > > > In this case, the signed url is only issued after Oak security has > granted > > access to the binary, and only returned over the JCR API to the JCR > Session > > that made the call, in the same way that an InputStream allows the bytes > of > > the binary to be read by that session. The URL only allows read access. > > > > What the session does with that data, is outside the control of Oak. > > Unlike the byte[] from the that has no protection, the signed URL is > > protected. It may only be used unmodified for the purpose it was intended > > by Oak and only for a well defined period of time. In that respect, > > arguably, its is more secure than the byte[] or InputStream. > > > > > > > > > > > > While I know that this is actually the goal of the whole exercise, we > > have > > > to be aware that this also is a change in our Oak security model. As > such > > > this may look like a security breach and I have been told by my > > colleagues > > > at Adobe that the 'single-way-to-access' is a relevant security > question > > > with a lot of potential customers. > > > > > > That doesn't mean that I am opposed to the patch in it's current form > as > > I > > > see the benefits from an Oak pov, I just want to highlight that we are > > > going to make a fundamental change and we should treat and document it > > > with the necessary care... maybe we should take this opportunity to > > > finally create a threat model for Oak? Doing so at this stage would > allow > > > us to visualise the proposed change to all parties involved. > > > > > > wdyt? > > > > > > > Agreed. > > Having a fully developed threat model which clarified all the risks for > > every aspect of Oak would, imho, be much better than not defining the > risks > > that exist. Even the most secure application has risks, best exposed in a > > threat model, however brief. > > > > Unfortunately Oak now exists in a world which is distributed, where > > applications need to embrace the network. This is a fundamental change, > > which Oak has to embrace. An Oak Threat model that recognises this will > be > > a great step forwards. > > > > On the other hand, if you are saying that the Oak Threat model has to be > > developed and agreed, before this patch can be added, then I am concerned > > that will take too long. Doing justice to an Oak Treat model will require > > resource. > > > > Best `Regards > > Ian > > > > > > > > > > Kind regards > > > Angela > > > > > > > > > On 07/09/17 16:39, "Ian Boston" <[email protected]> wrote: > > > > > > >On 7 September 2017 at 14:41, Francesco Mari < > [email protected]> > > > >wrote: > > > > > > > >> On Thu, Sep 7, 2017 at 11:05 AM, Ian Boston <[email protected]> wrote: > > > >> > On 7 September 2017 at 07:22, Ian Boston <[email protected]> wrote: > > > >> > > > > >> >> Hi, > > > >> >> > > > >> >> On 6 September 2017 at 22:43, Michael Dürig <[email protected]> > > > >>wrote: > > > >> >> > > > >> >>> > > > >> >>> > > > >> >>> On 06.09.17 23:08, Michael Dürig wrote: > > > >> >>> > > > >> >>>> > > > >> >>>> Hi, > > > >> >>>> > > > >> >>>> On 05.09.17 14:09, Ian Boston wrote: > > > >> >>>> > > > >> >>>>> Repeating the comment to on OAK-6575 here for further > > discussion. > > > >>2 > > > >> new > > > >> >>>>> Patches exploring both options. > > > >> >>>>> > > > >> >>>> > > > >> >>>> I would actually prefer the original patch ( > > > >> >>>> https://github.com/ieb/jackrabbit-oak/compare/trunk...ieb:O > > > >> >>>> AK-6575?expand=1) in most parts. However I have concerns > > regarding > > > >>the > > > >> >>>> generality of the new OakConversionService API as mentioned in > my > > > >> previous > > > >> >>>> mail. I would be more comfortable if this could be restricted > to > > > >> something > > > >> >>>> that resembles more like a "URIProvider", which given a blob > > > >>returns > > > >> an URI. > > > >> >>>> > > > >> >>>> On the implementation side, why do we need to introduce the > > > >>adaptable > > > >> >>>> machinery? Couldn't we re-use the Whiteboard and OSGiWhiteBoard > > > >> mechanisms > > > >> >>>> instead? I think these could be used to track URIProvider > > instances > > > >> >>>> registered by the various blob stores. > > > >> >>>> > > > >> >>>> > > > >> >>> See https://github.com/mduerig/jackrabbit-oak/commit/ > 2709c097b01 > > > >> >>> a006784b7011135efcbbe3ce1ba88 for a *really* quickly hacked > > together > > > >> and > > > >> >>> entirely untested POC. But it should get the idea across though. > > > >> >> > > > >> >> > > > >> >> > > > >> >> Thank you. > > > >> >> That makes sense. > > > >> >> I think it only needs the java/org/apache/jackrabbit/ > > > >> >> oak/blob/cloud/aws/s3/CloudFrontS3SignedUrlAdapterFactory.java > and > > > >>the > > > >> >> API to be inside Oak, everything else can be in Sling. > > > >> >> I'll update my patch and do a 2 options for Sling. > > > >> >> > > > >> > > > > >> > > > > >> > > > > >> > > > > >> > https://github.com/ieb/jackrabbit-oak/compare/trunk.. > > > >> .ieb:OAK-6575-3?expand=1 > > > >> > > > > >> > and > > > >> > > > > >> > > > > >>https://github.com/apache/sling/compare/trunk...ieb:OAK- > > 6575-3?expand=1 > > > >> > > > > >> > wdyt ? > > > >> > > > >> I like this a lot. It keeps Oak's side simple and cleanly integrates > > > >> Oak's lower-level services in Sling. > > > >> > > > > > > > > > > > >Good news. > > > >I think we should hold off committing the patch until Monday or > Tuesday > > to > > > >give those who may be offline this week a chance to comment. In > > particular > > > >I have not seen a comment from Angela who I would expect to have a > view > > as > > > >this is acl/security related. That is assuming she is back online next > > > >week. > > > > > > > >Best Regards > > > >Ian > > > > > > > > > > > >> > > > >> > Obviously the second patch needs to be discussed with Sling dev, > but > > > >>is > > > >> > should not be too contentious. > > > >> > > > > >> > Best Regards > > > >> > Ian > > > >> > > > > >> > > > > >> > > > > >> >> > > > >> >> I think that should address others concerns since it drops all > > signs > > > >>of > > > >> >> any generic object to object conversion from Oak (Francesco), and > > > >> doesn't > > > >> >> require wide scale fragile changes with implied requirements > being > > > >> placed > > > >> >> on how intermediate classes are connected and behave (mine). > > > >> >> > > > >> >> Best Regards > > > >> >> Ian > > > >> >> > > > >> >> > > > >> >>> > > > >> >>> Michael > > > >> >>> > > > >> >> > > > >> >> > > > >> > > > > > > > > >
