| Message ID | cover.1552073690.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | CDN offloading of fetch response | expand |
On 2019.03.08 13:55, Jonathan Tan wrote: > Here's my current progress - the only thing that is lacking is more > tests, maybe, so I think it's ready for review. This series looks good to me. Reviewed-by: Josh Steadmon <steadmon@google.com>
On Fri, Mar 08, 2019 at 01:55:12PM -0800, Jonathan Tan wrote: > Here's my current progress - the only thing that is lacking is more > tests, maybe, so I think it's ready for review. A bit belated, but here are some overall thoughts. A lot of my thinking comes from comparing this to previous work I had done on teaching the client to fetch first from a bundle and then do a follow-up fetch from the server. > This code now starts CDN downloads after closing the first HTTP request, > and it holds on to the .keep files until after the refs are set. I'll > leave parallelization of the CDN downloads for later work. Good. One of the problems with the bundle+followup approach was either having to hold open or re-establish the connection to the original server, since that fetch had to be put on hold. I agree that parallelizing can come later. I do wonder if it's worth making a new tool rather than trying to reuse git-http-fetch. Yes, it basically does what we want already, but there's quite a lot of cruft in that dumb-http code base. Using http_get_file(), or even curl more directly might be a bit easier. One problem in particular I'd worry about is that the http code generally expects authentication to happen through the initial ref-discovery, and then be set for all subsequent requests. So I doubt an http_pack_request actually handles auth at all, and retro-fitting it may be tricky. > One relatively significant change: someone pointed out that the issue fixed by > 50d3413740 ("http: make redirects more obvious", 2016-12-06) could also > occur here, so I have changed it so that the server is required to send > the packfile's hash along with the URI. I have mixed feelings on that. I like how it clearly makes the server the source of authority. You don't have to care about CDN tampering, because you know you are getting the bytes the server wanted you to. I think even without that this is still _mostly_ true, though. You're going to compute the hash of every object the CDN sends you, and those objects must fit into the missing gaps in what the server sends you. So the worst case is that a malicious CDN could send you some extra objects. That's probably not the end of the world, but I do like the extra guarantee of knowing that you got byte-for-byte what the server wanted you to. > This does mean that Ævar's workflow described in [1] would not work. > Quoting Ævar: > > > More concretely, I'd like to have a setup where a server can just dumbly > > point to some URL that probably has most of the data, without having any > > idea what OIDs are in it. So that e.g. some machine entirely > > disconnected from the server (and with just a regular clone) can > > continually generating an up-to-date-enough packfile. > > With 50d3413740, it seems to me that it's important for the server to > know details about the URIs that it points to, so such a disconnection > would not work. I think even without 50d3413740, this is important for your scheme. One of the weaknesses (and strengths, I suppose) of the bundle+followup scheme was that the initial bundle generally had to meet the git repository guarantee. After you fetched the bundle, you'd tell the server "OK, now I have commit X" just like you were a regular client. But I really like that your proposal does away with that, and asks for tighter cooperation between the server and the CDN. It means the CDN can serve some random subset of the objects. But once we do that, now the server _has_ to know what was in those URLs it pointed to, because our protocol has no good way of communicating a random subset of objects (if they're just blobs, we could enumerate all of them to the server as haves; yuck. But as soon as you get a tree or commit from the CDN, you have to have all of the reachable objects to be able to claim it). So I think this is pretty inherent to your proposal, and but it's the right tradeoff to be making here (I think there could still be room for the other thing, too, but it's just a different feature). I have a few other comments on the design, but I'll send them in response to patch 5. -Peff
> On Fri, Mar 08, 2019 at 01:55:12PM -0800, Jonathan Tan wrote: > > > Here's my current progress - the only thing that is lacking is more > > tests, maybe, so I think it's ready for review. > > A bit belated, but here are some overall thoughts. A lot of my thinking > comes from comparing this to previous work I had done on teaching the > client to fetch first from a bundle and then do a follow-up fetch from > the server. Thanks for your thoughts. > > This code now starts CDN downloads after closing the first HTTP request, > > and it holds on to the .keep files until after the refs are set. I'll > > leave parallelization of the CDN downloads for later work. > > Good. One of the problems with the bundle+followup approach was either > having to hold open or re-establish the connection to the original > server, since that fetch had to be put on hold. > > I agree that parallelizing can come later. I do wonder if it's worth > making a new tool rather than trying to reuse git-http-fetch. Yes, it > basically does what we want already, but there's quite a lot of cruft in > that dumb-http code base. Using http_get_file(), or even curl more > directly might be a bit easier. > > One problem in particular I'd worry about is that the http code > generally expects authentication to happen through the initial > ref-discovery, and then be set for all subsequent requests. So I doubt > an http_pack_request actually handles auth at all, and retro-fitting it > may be tricky. Thanks - I'll keep that in mind. If auth isn't handled, then we'll definitely need something new. > > One relatively significant change: someone pointed out that the issue fixed by > > 50d3413740 ("http: make redirects more obvious", 2016-12-06) could also > > occur here, so I have changed it so that the server is required to send > > the packfile's hash along with the URI. > > I have mixed feelings on that. I like how it clearly makes the server > the source of authority. You don't have to care about CDN tampering, > because you know you are getting the bytes the server wanted you to. I was thinking not of CDN tampering but of a malicious server. Suppose a server knew of (1) the hash of a target commit, and (2) the URL of a packfile containing that target commit. The server can craft a commit whose parent is that target commit and advertise it, and serves both that commit as a packfile and the URL as packfile-uri. When the user subsequently pushes, they will probably inadvertently push everything including the target commit. This is similar to the 50d3413740 case except that now the server needs an additional datum (2). The packfile's hash is yet another additional datum. I admit that it may not be that useful, though. > I think even without that this is still _mostly_ true, though. You're > going to compute the hash of every object the CDN sends you, and those > objects must fit into the missing gaps in what the server sends you. So > the worst case is that a malicious CDN could send you some extra > objects. That's probably not the end of the world, but I do like the > extra guarantee of knowing that you got byte-for-byte what the server > wanted you to. Yes, the guarantee is nice. > > This does mean that Ævar's workflow described in [1] would not work. > > Quoting Ævar: > > > > > More concretely, I'd like to have a setup where a server can just dumbly > > > point to some URL that probably has most of the data, without having any > > > idea what OIDs are in it. So that e.g. some machine entirely > > > disconnected from the server (and with just a regular clone) can > > > continually generating an up-to-date-enough packfile. > > > > With 50d3413740, it seems to me that it's important for the server to > > know details about the URIs that it points to, so such a disconnection > > would not work. > > I think even without 50d3413740, this is important for your scheme. One > of the weaknesses (and strengths, I suppose) of the bundle+followup > scheme was that the initial bundle generally had to meet the git > repository guarantee. After you fetched the bundle, you'd tell the > server "OK, now I have commit X" just like you were a regular client. > > But I really like that your proposal does away with that, and asks for > tighter cooperation between the server and the CDN. It means the CDN can > serve some random subset of the objects. But once we do that, now the > server _has_ to know what was in those URLs it pointed to, because our > protocol has no good way of communicating a random subset of objects (if > they're just blobs, we could enumerate all of them to the server as > haves; yuck. But as soon as you get a tree or commit from the CDN, you > have to have all of the reachable objects to be able to claim it). Ah...you're right that the server still needs some inkling of what the CDN's packfile has. Without the packfile hash, the knowledge needed is slightly less: the server just has to know a subset of the objects instead of the exact list of objects, but I can't think of how this can benefit us other than that we don't require so much consistency (it's OK if the server's knowledge of the CDN is updated significantly later than the CDN is updated, if the CDN reuses the same URL for each version of the packfile). > So I think this is pretty inherent to your proposal, and but it's the > right tradeoff to be making here (I think there could still be room for > the other thing, too, but it's just a different feature). Thanks.
On Fri, Mar 08 2019, Jonathan Tan wrote: > One relatively significant change: someone pointed out that the issue fixed by > 50d3413740 ("http: make redirects more obvious", 2016-12-06) could also > occur here, so I have changed it so that the server is required to send > the packfile's hash along with the URI. > > This does mean that Ævar's workflow described in > https://public-inbox.org/git/87k1hv6eel.fsf@evledraar.gmail.com/ would > not work. Quoting Ævar: > >> More concretely, I'd like to have a setup where a server can just dumbly >> point to some URL that probably has most of the data, without having any >> idea what OIDs are in it. So that e.g. some machine entirely >> disconnected from the server (and with just a regular clone) can >> continually generating an up-to-date-enough packfile. > > With 50d3413740, it seems to me that it's important for the server to > know details about the URIs that it points to, so such a disconnection > would not work. For what it's worth I'm fine with that, and as is obvious in some of the previous threads I hadn't thought about that threat model. This "I know XYZ has a pack that should have most of it, check what's in it and get back to me with HAVEs" was a nice-to-have, but not a must. And you can still get most of the way there with this proposal and the techniques described in the follow-up to https://public-inbox.org/git/87bmhiykvw.fsf@evledraar.gmail.com/ if I'm reading this series correctly. I.e. the server would know the CDNs are going to be generating a pack from the "master" history with the tip being the Nth commit, and since both agree on the pack algorithm they can generate the same OID/pack hash pair without further coordination, the server would then optimistically advertise that. This would give you the sort of "lazy" CDN setup I was describing with slightly more work than just "here's some random latest pack". But isn't in the case that if a malicious server ever got a hold of a pack SHA-1 it knows to be on a private CDN *and* a commit that's in it we'd have the same problem? Shouldn't this security model be prominently documented in Documentation/technical/packfile-uri.txt? I.e. "THOU MUST NEVER LEAK A COMBINATION OF THE PACK SHA-1 OF A PRIVATE CDN AND A PRIVATE COMMIT SHA-1!". It seems likely that once we have git CDN support the first thing CDNs are going to do is to serve up such packs via a URL that includes the pack SHA-1. Once the combination of that & a commit in it leaks we're back to square one, no? *My* default approach in setting up such infrastructure without knowing about 50d3413740 would be not to care about the privacy of the SHA-1s, even in the case of private data. Isn't there also overlap here with non-CDN shallow/narrow fetching? Where a server can detect such a client, rely on it to get the objects from elsewhere (e.g. via adding an unrelated remote), and then get a push that gives it secret data? I don't see any fool-proof way out of it, but maybe a mode where we: 1. Get the CDN data URLs (which for the purposes of this example I'm assuming are "base packs" containing at least some commits...) 2. Do a follow-up request to the server where we e.g. pretend not to have the last N commits for tips it advertises and ask it to serve that up from a non-CDN. If it can't we know it's lying and trying an attack similar to the one described in 50d3413740. Conversely, can't know if it can that it isn't, but at that point it seems unlikely, since surely the useful thing is a delta against the recent-but-server-lied-about-having-it commit, at least in most cases.... This would also have the advantage of addressing some of the reliability concerns brought up elsewhere. I.e. once we're doing a sanity-check post-fetch anyway a failure to download 1/3 packs from a CDN becomes recoverable, although the design outlined here (understandably) would prefer that to be done in another "fetch" dialog so the server can keep the "CDN + my data should be 100% of the data" state.