Implementing Link: rel=serviceworker (in http headers)

[Marijn Kruisselbrink]

I'm working on implementing support for a "serviceworker" link type in Link: headers (design doc here), and was hoping to get some input from people more familiar with the networking stack. Specifically:

• Does the proposed move of the functionality in blink::LinkHeader (code to parse an HTTP Link: header) to somewhere in net/http sound reasonable?
• Does it make sense to implement this by overriding MaybeInterceptResponse in some URLRequestInterceptor implementation, or this there some better place to inspect responses for this header and act on it if the request was one we care about (made from a secure context, to a secure origin primarily).
• Is there already some way to figure out (in the browser process) if a particular URLRequest was made from a secure context, or would this be something I would need to add myself?
  

Thanks for any advice you can give.

[Matt Menke]

On Thu, Feb 18, 2016 at 6:15 PM, 'Marijn Kruisselbrink' via net-dev <net...@chromium.org> wrote:

I'm working on implementing support for a "serviceworker" link type in Link: headers (design doc here), and was hoping to get some input from people more familiar with the networking stack. Specifically:

• Does the proposed move of the functionality in blink::LinkHeader (code to parse an HTTP Link: header) to somewhere in net/http sound reasonable?

This seems kinda weird to me, for two reasons:

*  net currently neither knows nor cares about link headers.  With the change, it would know about them, but not care about them, which seems to imply a layering problem (Much the same problem with having a bunch of MIME Type logic in net).

* Seems like the code to handle <link> tags and Link headers should be in the same place, which seems to imply they should be handled in the renderer process.

• Does it make sense to implement this by overriding MaybeInterceptResponse in some URLRequestInterceptor implementation, or this there some better place to inspect responses for this header and act on it if the request was one we care about (made from a secure context, to a secure origin primarily).

This seems like a bad idea.  URLRequestInterceptors just exist to return URLRequestJobs, not to do anything else.  A ResourceThrottle seems a better hook.  Could add something directly to ResourceDispatcherHostImpl or ResourceLoader, but throttles allow for better isolation. 

• Is there already some way to figure out (in the browser process) if a particular URLRequest was made from a secure context, or would this be something I would need to add myself?
  

Thanks for any advice you can give.

--
You received this message because you are subscribed to the Google Groups "net-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to net-dev+u...@chromium.org.
To post to this group, send email to net...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/net-dev/CA%2BOSsVYvZ%3Dpy0xmtizmuMWwAJuOdcQWcZxLNoeRBHAQ%3DgpZUzg%40mail.gmail.com.

[Ryan Sleevi]

On Thu, Feb 18, 2016 at 3:20 PM, Matt Menke <mme...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 6:15 PM, 'Marijn Kruisselbrink' via net-dev <net...@chromium.org> wrote:

I'm working on implementing support for a "serviceworker" link type in Link: headers (design doc here), and was hoping to get some input from people more familiar with the networking stack. Specifically:

• Does the proposed move of the functionality in blink::LinkHeader (code to parse an HTTP Link: header) to somewhere in net/http sound reasonable?

This seems kinda weird to me, for two reasons:

*  net currently neither knows nor cares about link headers.  With the change, it would know about them, but not care about them, which seems to imply a layering problem (Much the same problem with having a bunch of MIME Type logic in net).

* Seems like the code to handle <link> tags and Link headers should be in the same place, which seems to imply they should be handled in the renderer process.

• Does it make sense to implement this by overriding MaybeInterceptResponse in some URLRequestInterceptor implementation, or this there some better place to inspect responses for this header and act on it if the request was one we care about (made from a secure context, to a secure origin primarily).

This seems like a bad idea.  URLRequestInterceptors just exist to return URLRequestJobs, not to do anything else.  A ResourceThrottle seems a better hook.  Could add something directly to ResourceDispatcherHostImpl or ResourceLoader, but throttles allow for better isolation. 

• Is there already some way to figure out (in the browser process) if a particular URLRequest was made from a secure context, or would this be something I would need to add myself?

+1 to what Matt said, but to this question, the answer is "No, not in a trustworthy fashion". Work is being done to enable this, but it's not done, and I'm not sure the status (this is a superset of the Site Isolation work + PlzNavigate work) 

[Marijn Kruisselbrink]

On Thu, Feb 18, 2016 at 3:21 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 3:20 PM, Matt Menke <mme...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 6:15 PM, 'Marijn Kruisselbrink' via net-dev <net...@chromium.org> wrote:

I'm working on implementing support for a "serviceworker" link type in Link: headers (design doc here), and was hoping to get some input from people more familiar with the networking stack. Specifically:

• Does the proposed move of the functionality in blink::LinkHeader (code to parse an HTTP Link: header) to somewhere in net/http sound reasonable?

This seems kinda weird to me, for two reasons:

*  net currently neither knows nor cares about link headers.  With the change, it would know about them, but not care about them, which seems to imply a layering problem (Much the same problem with having a bunch of MIME Type logic in net).

Hmm, okay. It seemed pretty natural to me to have code for parsing a particular HTTP header in with the rest of the HTTP code, even if the rest of that HTTP code doesn't actually do anything with the parsed data. Although I guess this isn't really a core part of HTTP so I can see how maybe net wouldn't be the best place.

Not sure though how this would imply a layering problem? The whole reason this is being suggested is to prevent a layering problem: both blink and content will need to be able to parse Link: headers; there are very few places where code can live that both depend on, so with net/ being potentially one of those few places this seemed like a good candidate (base/ would be a lot worse, and there aren't really any other places both content and blink can depend on).

* Seems like the code to handle <link> tags and Link headers should be in the same place, which seems to imply they should be handled in the renderer process.

 As explained in the design document, support for the Link header can not live in the renderer process as that would require pretty much completely getting rid of any origin/security checks in the browser. So even though both <link> tags and Link: headers can do some of the same things, in this case the implementation of the two will be pretty much fully independent (until both eventually reach the same code in the browser process). And even if handling of Link headers would take place in the renderer process, that doesn't necessarily mean that parsing of the raw bytes in the link header into a format that can be fed into the same code as to where data parsed from <link> elements is fed into would have to live in blink. Similar to how blink doesn't do all the header parsing from scratch but gets fed in somewhat parsed headers (although in a slightly broken way; I'd love to see blink just use net::Http*Headers to represent http headers rather than having its own HTTPHeaderMap class), it doesn't seem that weird to hand off parsing to some lover level library.

• Does it make sense to implement this by overriding MaybeInterceptResponse in some URLRequestInterceptor implementation, or this there some better place to inspect responses for this header and act on it if the request was one we care about (made from a secure context, to a secure origin primarily).

This seems like a bad idea.  URLRequestInterceptors just exist to return URLRequestJobs, not to do anything else.  A ResourceThrottle seems a better hook.  Could add something directly to ResourceDispatcherHostImpl or ResourceLoader, but throttles allow for better isolation. 

Any pointers to documentation/examples of how I could use a ResourceThrottle for this? It seems like it has methods that get called at various points during a request, but I don't see how it actually gets access to the request and response itself? 

• Is there already some way to figure out (in the browser process) if a particular URLRequest was made from a secure context, or would this be something I would need to add myself?

+1 to what Matt said, but to this question, the answer is "No, not in a trustworthy fashion". Work is being done to enable this, but it's not done, and I'm not sure the status (this is a superset of the Site Isolation work + PlzNavigate work) 

That makes it sound like the data is actually there as best as it can be, but due to the way chrome currently works it isn't possible to do this in a way to doesn't rely on trusting the renderer? Since pretty much all "is this done in a secure context" checks that currently exist are done purely renderer side, that doesn't seem like too much of a problem for this. 

[Matt Menke]

On Thu, Feb 18, 2016 at 6:40 PM, Marijn Kruisselbrink <m...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 3:21 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 3:20 PM, Matt Menke <mme...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 6:15 PM, 'Marijn Kruisselbrink' via net-dev <net...@chromium.org> wrote:

I'm working on implementing support for a "serviceworker" link type in Link: headers (design doc here), and was hoping to get some input from people more familiar with the networking stack. Specifically:

• Does the proposed move of the functionality in blink::LinkHeader (code to parse an HTTP Link: header) to somewhere in net/http sound reasonable?

This seems kinda weird to me, for two reasons:

*  net currently neither knows nor cares about link headers.  With the change, it would know about them, but not care about them, which seems to imply a layering problem (Much the same problem with having a bunch of MIME Type logic in net).

Hmm, okay. It seemed pretty natural to me to have code for parsing a particular HTTP header in with the rest of the HTTP code, even if the rest of that HTTP code doesn't actually do anything with the parsed data. Although I guess this isn't really a core part of HTTP so I can see how maybe net wouldn't be the best place.

Not sure though how this would imply a layering problem? The whole reason this is being suggested is to prevent a layering problem: both blink and content will need to be able to parse Link: headers; there are very few places where code can live that both depend on, so with net/ being potentially one of those few places this seemed like a good candidate (base/ would be a lot worse, and there aren't really any other places both content and blink can depend on).

* Seems like the code to handle <link> tags and Link headers should be in the same place, which seems to imply they should be handled in the renderer process.

 As explained in the design document, support for the Link header can not live in the renderer process as that would require pretty much completely getting rid of any origin/security checks in the browser. So even though both <link> tags and Link: headers can do some of the same things, in this case the implementation of the two will be pretty much fully independent (until both eventually reach the same code in the browser process). And even if handling of Link headers would take place in the renderer process, that doesn't necessarily mean that parsing of the raw bytes in the link header into a format that can be fed into the same code as to where data parsed from <link> elements is fed into would have to live in blink. Similar to how blink doesn't do all the header parsing from scratch but gets fed in somewhat parsed headers (although in a slightly broken way; I'd love to see blink just use net::Http*Headers to represent http headers rather than having its own HTTPHeaderMap class), it doesn't seem that weird to hand off parsing to some lover level library.

• Does it make sense to implement this by overriding MaybeInterceptResponse in some URLRequestInterceptor implementation, or this there some better place to inspect responses for this header and act on it if the request was one we care about (made from a secure context, to a secure origin primarily).

This seems like a bad idea.  URLRequestInterceptors just exist to return URLRequestJobs, not to do anything else.  A ResourceThrottle seems a better hook.  Could add something directly to ResourceDispatcherHostImpl or ResourceLoader, but throttles allow for better isolation. 

Any pointers to documentation/examples of how I could use a ResourceThrottle for this? It seems like it has methods that get called at various points during a request, but I don't see how it actually gets access to the request and response itself? 

No docs.  Their constructor is generally passed a pointer to the URLRequest, which they can hang onto if they need it (Longer term, I'd prefer to just pass them what they're likely to want at each stage, and make it so they can't access the URLRequest directly, but that shouldn't affect you - headers are definitely something consumers want).

[Ryan Sleevi]

On Thu, Feb 18, 2016 at 3:40 PM, Marijn Kruisselbrink <m...@chromium.org> wrote:

Not sure though how this would imply a layering problem? The whole reason this is being suggested is to prevent a layering problem: both blink and content will need to be able to parse Link: headers; there are very few places where code can live that both depend on, so with net/ being potentially one of those few places this seemed like a good candidate (base/ would be a lot worse, and there aren't really any other places both content and blink can depend on).

Blink cannot depend on //net or //base; unless things have changed substantially. But just because Blink could depend on //net doesn't mean it's the right place.

Think of //net as being the low-level bits, particularly focused on IETF side of things - protocols and implementations. Semantic-level things - such as the handling and parsing of links, metadata - or things specific to HTML - belong in //content or Blink.

You can use the MIME code as an example, in which it was moved from //net into //components, where it can better interact with both //content and with Mojo; perhaps that's the answer for you, but I can't guarantee it is - the layering is definitely something that needs to be worked out.

 

 As explained in the design document, support for the Link header can not live in the renderer process as that would require pretty much completely getting rid of any origin/security checks in the browser.

Correction: For *this* Link header, right? It's not in general an issue for link headers, it's an issue with this particular header and it's semantic values.

And it's unclear from the doc why this is intrinsically true.

 

So even though both <link> tags and Link: headers can do some of the same things, in this case the implementation of the two will be pretty much fully independent (until both eventually reach the same code in the browser process). And even if handling of Link headers would take place in the renderer process, that doesn't necessarily mean that parsing of the raw bytes in the link header into a format that can be fed into the same code as to where data parsed from <link> elements is fed into would have to live in blink.

Why?

 

Similar to how blink doesn't do all the header parsing from scratch but gets fed in somewhat parsed headers (although in a slightly broken way; I'd love to see blink just use net::Http*Headers to represent http headers rather than having its own HTTPHeaderMap class), it doesn't seem that weird to hand off parsing to some lover level library.

Blink's usage and interpretation of headers are somewhat specific to the Web Platform and quirks inherited from browsers. They're not intrinsically good behaviours - nor are they intrinsically bad.

 

That makes it sound like the data is actually there as best as it can be, but due to the way chrome currently works it isn't possible to do this in a way to doesn't rely on trusting the renderer?

I'm not aware of that data being there in the browser process was the point - you'd have to send that data from the renderer, and at that point, you're in hostile territory.

 

Since pretty much all "is this done in a secure context" checks that currently exist are done purely renderer side, that doesn't seem like too much of a problem for this. 

Correct, this is true at present to the best of my knowledge. 

[Marijn Kruisselbrink]

On Fri, Feb 19, 2016 at 10:41 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

On Thu, Feb 18, 2016 at 3:40 PM, Marijn Kruisselbrink <m...@chromium.org> wrote:

Not sure though how this would imply a layering problem? The whole reason this is being suggested is to prevent a layering problem: both blink and content will need to be able to parse Link: headers; there are very few places where code can live that both depend on, so with net/ being potentially one of those few places this seemed like a good candidate (base/ would be a lot worse, and there aren't really any other places both content and blink can depend on).

Blink cannot depend on //net or //base; unless things have changed substantially. But just because Blink could depend on //net doesn't mean it's the right place.

Things have changed substantially. Blink is slowly moving to a world where at least parts of blink can depend on //base.

 

Think of //net as being the low-level bits, particularly focused on IETF side of things - protocols and implementations. Semantic-level things - such as the handling and parsing of links, metadata - or things specific to HTML - belong in //content or Blink.

Well, all I was suggesting to put in //net/http would be the IETF side of Link header support (it's defined by an IETF RFC). Not any semantics on top of that. Nothing specifc to HTML (okay, the current blink::LinkHeader code has a bit of both, but there is no reason for a shared link header parsing implementation to have any knowledge of any specific link header types. I wasn't planning on keeping that part in a //net/http implementation).

You can use the MIME code as an example, in which it was moved from //net into //components, where it can better interact with both //content and with Mojo; perhaps that's the answer for you, but I can't guarantee it is - the layering is definitely something that needs to be worked out.

Is that //components/mime_util you're taking about? I can see how that makes sense to not live in //net. But it still depends on some mime-type parsing code in //net for the actual parsing. Here I'm proposing a similar split. Have the raw Link: header parsing in //net/http, and code to give semantics to specific link types and attributes in content and/or blink. But of course it could be made to work by having a separate component for this. It just seems a bit silly to have yet another component with a single source file.

 

 As explained in the design document, support for the Link header can not live in the renderer process as that would require pretty much completely getting rid of any origin/security checks in the browser.

Correction: For *this* Link header, right? It's not in general an issue for link headers, it's an issue with this particular header and it's semantic values.

Yes, the existing preload/prefetch etc link headers are just fine where they currently are in blink. 

And it's unclear from the doc why this is intrinsically true.

This is the case because it is very undesirable to introduce an IPC to allow any renderer to install a service worker for any arbitrary origin (which would be needed to implement this feature fully in blink). Allowing this would go against all the attempts at having origin isolation for renderer processes.

 

 

So even though both <link> tags and Link: headers can do some of the same things, in this case the implementation of the two will be pretty much fully independent (until both eventually reach the same code in the browser process). And even if handling of Link headers would take place in the renderer process, that doesn't necessarily mean that parsing of the raw bytes in the link header into a format that can be fed into the same code as to where data parsed from <link> elements is fed into would have to live in blink.

Why?

When a <link> element exists in a document, it is pretty much the same as calling the existing javascript API from that document. So implementing this is pretty much literally one line of code: pass the atrtibutes of the link element to the implementation of the javascript API. This is not the case for the link header. The Link header can be set on subresources (which in particular is the use case I'm interested in), and should then be interpreted in the context of the origin of that subresource. This is very different from when a Link header/element is associated with a document. As such different security concerns exist, and different checks have to be done.

 

Similar to how blink doesn't do all the header parsing from scratch but gets fed in somewhat parsed headers (although in a slightly broken way; I'd love to see blink just use net::Http*Headers to represent http headers rather than having its own HTTPHeaderMap class), it doesn't seem that weird to hand off parsing to some lover level library.

Blink's usage and interpretation of headers are somewhat specific to the Web Platform and quirks inherited from browsers. They're not intrinsically good behaviours - nor are they intrinsically bad.

I was mostly referring to the  various fixme's in the blink code dealing with headers that could be relatively easily solved by stopping duplicating code that already exists outside of blink (getting rid of this kind of duplication/unnecessary abstractions is one of the main goals of the onion soup effort to refactor blink and the way it interacts with the rest of chrome).

 

 

That makes it sound like the data is actually there as best as it can be, but due to the way chrome currently works it isn't possible to do this in a way to doesn't rely on trusting the renderer?

I'm not aware of that data being there in the browser process was the point - you'd have to send that data from the renderer, and at that point, you're in hostile territory.

Okay, makes sense. I can live with that.