| Message ID | pull.1814.git.git.1729112794671.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled | expand |
On Wed, Oct 16, 2024 at 09:06:34PM +0000, Piotr Szlazak via GitGitGadget wrote: > From: Piotr Szlazak <piotr.szlazak@gmail.com> > > ALLOW_ANY_SHA1 implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. > Yet ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 flags can be enabled > independently. > If uploadpack.allowAnySHA1InWant is not enabled in config file, > other flags should not be disabled together with ALLOW_ANY_SHA1. > They should be kept enabled if they were separately enabled in > config file with they respective options. > > Signed-off-by: Piotr Szlazak <piotr.szlazak@gmail.com> > --- > upload-pack: fix how ALLOW_ANY_SHA1 flag is disabled > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1814%2Fpszlazak%2Fupload-pack-allow-flags-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1814/pszlazak/upload-pack-allow-flags-v1 > Pull-Request: https://github.com/git/git/pull/1814 > > upload-pack.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/upload-pack.c b/upload-pack.c > index 6d6e0f9f980..cf99b228719 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -53,6 +53,7 @@ enum allow_uor { > /* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */ > ALLOW_REACHABLE_SHA1 = 0x02, > /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */ > + /* As this flag implies other two flags, be careful when it must be disabled. */ > ALLOW_ANY_SHA1 = 0x07 > }; > > @@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value, > if (git_config_bool(var, value)) > data->allow_uor |= ALLOW_ANY_SHA1; > else > - data->allow_uor &= ~ALLOW_ANY_SHA1; > + data->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1)); Subtracting the result of a bitwise-OR feels a little odd to me. Since ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 are defined as 0x1 and 0x2, respectively, I think the end result is as you described it, but the route to get there feels a little odd to me. I think it would probably make more sense to write this as: data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)); Stepping back a moment, I suppose this is handling the case where a user writes: [uploadpack] allowTipSHA1InWant = true allowReachableSHA1InWant = true allowAnySHA1InWant = false and is surprised when the final "uploadPack.allowAnySHA1InWant" unsets the previous two options. I'm not sure that the current behavior is actually wrong. The final line in the example above seems to indicate "do not allow any SHA-1 in the 'wants'", which would indeed imply that the other two options should be set to false as well. And that is what the current code is doing, which I think is correct. I do see that our upload-pack section of "git-config(1)" is lacking in this area, though, as it does not indicate that uploadPack.allowAnySHA1InWant implies the other two options. It may be worth saying something like: NOTE: this option implies both uploadPack.allowTipSHA1InWant and uploadPack.allowReachableSHA1InWant. Setting this option to "false" will do the same for the implied ones. Thanks, Taylor
On Wed, Oct 16, 2024 at 05:18:44PM -0400, Taylor Blau wrote: > > diff --git a/upload-pack.c b/upload-pack.c > > index 6d6e0f9f980..cf99b228719 100644 > > --- a/upload-pack.c > > +++ b/upload-pack.c > > @@ -53,6 +53,7 @@ enum allow_uor { > > /* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */ > > ALLOW_REACHABLE_SHA1 = 0x02, > > /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */ > > + /* As this flag implies other two flags, be careful when it must be disabled. */ > > ALLOW_ANY_SHA1 = 0x07 > > }; > > > > @@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value, > > if (git_config_bool(var, value)) > > data->allow_uor |= ALLOW_ANY_SHA1; > > else > > - data->allow_uor &= ~ALLOW_ANY_SHA1; > > + data->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1)); > > Subtracting the result of a bitwise-OR feels a little odd to me. > > Since ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1 are defined as 0x1 and > 0x2, respectively, I think the end result is as you described it, but > the route to get there feels a little odd to me. > > I think it would probably make more sense to write this as: > > data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)); I think we have to treat them as a complete unit, as we don't know which bits were set by independent config lines and which were OR-ed in by ALLOW_ANY. So this case: > Stepping back a moment, I suppose this is handling the case where a user > writes: > > [uploadpack] > allowTipSHA1InWant = true > allowReachableSHA1InWant = true > allowAnySHA1InWant = false > > and is surprised when the final "uploadPack.allowAnySHA1InWant" unsets > the previous two options. is the one that Piotr is thinking about. But what about: [uploadpack] allowAnySHA1InWant = true allowAnySHA1InWant = false Right now that pair is a noop, which is what I'd expect. But after the proposed patch, it quietly enables ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. So I think the code has to stay the same, but we perhaps should document that "allow any" has the user-visible side effect of enabling/disabling the other two. -Peff
On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote: > > I think it would probably make more sense to write this as: > > > > data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)); > > I think we have to treat them as a complete unit, as we don't know which > bits were set by independent config lines and which were OR-ed in by > ALLOW_ANY. > > So this case: > > > Stepping back a moment, I suppose this is handling the case where a user > > writes: > > > > [uploadpack] > > allowTipSHA1InWant = true > > allowReachableSHA1InWant = true > > allowAnySHA1InWant = false > > > > and is surprised when the final "uploadPack.allowAnySHA1InWant" unsets > > the previous two options. Yeah, I think that you and I are in agreement here. > is the one that Piotr is thinking about. But what about: > > [uploadpack] > allowAnySHA1InWant = true > allowAnySHA1InWant = false > > Right now that pair is a noop, which is what I'd expect. But after the > proposed patch, it quietly enables ALLOW_TIP_SHA1 and > ALLOW_REACHABLE_SHA1. That's an even clearer example of a new gotcha that would occur with this proposed patch, IMHO. I don't think in general that successive $ git config core.foo true $ git config core.foo false should have any user-visible effect, as the latter should nullify the former. > So I think the code has to stay the same, but we perhaps should document > that "allow any" has the user-visible side effect of enabling/disabling > the other two. That would be a useful direction, I think. Double checking git-config(1), there is in deed no mention of allowAnySHA1InWant implying the other two options, which seems like a gap that would be good to address. Piotr: what do you think? Thanks, Taylor
On 17.10.2024 17:23, Taylor Blau wrote: > On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote: >>> I think it would probably make more sense to write this as: >>> >>> data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)); Much better! :-) >> I think we have to treat them as a complete unit, as we don't know which >> bits were set by independent config lines and which were OR-ed in by >> ALLOW_ANY. >> >> So this case: >> >>> Stepping back a moment, I suppose this is handling the case where a user >>> writes: >>> >>> [uploadpack] >>> allowTipSHA1InWant = true >>> allowReachableSHA1InWant = true >>> allowAnySHA1InWant = false >>> >>> and is surprised when the final "uploadPack.allowAnySHA1InWant" unsets >>> the previous two options. > Yeah, I think that you and I are in agreement here. > >> is the one that Piotr is thinking about. But what about: >> >> [uploadpack] >> allowAnySHA1InWant = true >> allowAnySHA1InWant = false >> >> Right now that pair is a noop, which is what I'd expect. But after the >> proposed patch, it quietly enables ALLOW_TIP_SHA1 and >> ALLOW_REACHABLE_SHA1. Rather not as config file is parsed only once: https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368 >> So I think the code has to stay the same, but we perhaps should document >> that "allow any" has the user-visible side effect of enabling/disabling >> the other two. > That would be a useful direction, I think. Double checking > git-config(1), there is in deed no mention of allowAnySHA1InWant > implying the other two options, which seems like a gap that would be > good to address. > > Piotr: what do you think? I agree. I completely missed to test it like that (which works OK): [uploadpack] allowTipSHA1InWant = true allowReachableSHA1InWant = true EOF I was always testing with allowAnySHAInWant either set to 'true' or 'false'. But always in place. And having it set to 'false' was disabling my previously set other allowXyzInWant options. Which was a surprise, as I was considering allowAnySHAInWant as a final level of openness for client-side requests[1]. In contradiction to what Taylor expressed here: > I'm not sure that the current behavior is actually wrong. The final line in the example above seems to indicate "do not allow any SHA-1 in the 'wants'", which would indeed imply that the other two options should be set to false as well. So as suggested I will prepare a patch for documentation, so it will be also clear for others. Should it be done using same thread or new one should be created? [1] For example client can request not reachable objects, trees, blobs. Regards, Piotr
On Thu, Oct 17, 2024 at 05:59:46PM +0200, Piotr Szlazak wrote: > On 17.10.2024 17:23, Taylor Blau wrote: > > On Wed, Oct 16, 2024 at 10:37:35PM -0400, Jeff King wrote: > > > > I think it would probably make more sense to write this as: > > > > > > > > data->allow_uor &= ~(ALLOW_ANY_SHA1 ^ (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)); > > Much better! :-) > > > > I think we have to treat them as a complete unit, as we don't know which > > > bits were set by independent config lines and which were OR-ed in by > > > ALLOW_ANY. > > > > > > So this case: > > > > > > > Stepping back a moment, I suppose this is handling the case where a user > > > > writes: > > > > > > > > [uploadpack] > > > > allowTipSHA1InWant = true > > > > allowReachableSHA1InWant = true > > > > allowAnySHA1InWant = false > > > > > > > > and is surprised when the final "uploadPack.allowAnySHA1InWant" unsets > > > > the previous two options. > > Yeah, I think that you and I are in agreement here. > > > > > is the one that Piotr is thinking about. But what about: > > > > > > [uploadpack] > > > allowAnySHA1InWant = true > > > allowAnySHA1InWant = false > > > > > > Right now that pair is a noop, which is what I'd expect. But after the > > > proposed patch, it quietly enables ALLOW_TIP_SHA1 and > > > ALLOW_REACHABLE_SHA1. > > Rather not as config file is parsed only once: > > https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368 I am not sure I follow... upload_pack_config() is invoked on every configuration line that we see. So the first time when we read "allowAnySHA1InWant = true", we take the first path through that function you linked. The second time, we take the else branch, and correspondingly unset the bits from ALLOW_ANY_SHA1. So today that is doing the right thing (it will end with the same bits set that we started with). But under the proposed patch that behavior would change, and the lower two bits would still be set. > So as suggested I will prepare a patch for documentation, so it will be also > clear for others. Should it be done using same thread or new one should be > created? Either is fine. Thanks, Taylor
On Thu, Oct 17, 2024 at 02:46:45PM -0400, Taylor Blau wrote: > > Rather not as config file is parsed only once: > > > > https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368 > > I am not sure I follow... upload_pack_config() is invoked on every > configuration line that we see. So the first time when we read > "allowAnySHA1InWant = true", we take the first path through that > function you linked. The second time, we take the else branch, and > correspondingly unset the bits from ALLOW_ANY_SHA1. > > So today that is doing the right thing (it will end with the same bits > set that we started with). But under the proposed patch that behavior > would change, and the lower two bits would still be set. Reading Piotr's response I wondered if upload-pack might be using the configset API instead of a config callback. But no, it does look like a config callback. But it does hint at a possible path if we wanted to process these independently: we could read each of the config options separately, resolving them in the usual last-one-wins way, and then turning the result into a bitfield. Something like this, outside of the callback (totally untested): int v; unsigned bits = 0; if (!git_config_get_bool("uploadpack.allowtipsha1inwant", &v) && v) bits |= ALLOW_TIP_SHA1; if (!git_config_get_bool("uploadpack.allowreachablesha1inwant", &v) && v) bits |= ALLOW_REACHABLE_SHA1; if (!git_config_get_bool("uploadpack.allowanysha1inwant", &v) && v) bits |= ALLOW_ANY_SHA1; That makes the config flags independent. But the features themselves are not really independent. If you do: [uploadpack] allowAnySHA1InWant = true allowTipSHA1InWant = false it is still going to allow the "tip" flag, because it's a subset of the "any" flag. And you can't escape that. So I still think we're better off to just document (and of course in the long run all of these become less and less interesting as they are v0-only options). -Peff
On Fri, Oct 18, 2024 at 12:33:06AM -0400, Jeff King wrote: > On Thu, Oct 17, 2024 at 02:46:45PM -0400, Taylor Blau wrote: > > > > Rather not as config file is parsed only once: > > > > > > https://github.com/git/git/blob/15030f9556f545b167b1879b877a5d780252dc16/upload-pack.c#L1368 > > > > I am not sure I follow... upload_pack_config() is invoked on every > > configuration line that we see. So the first time when we read > > "allowAnySHA1InWant = true", we take the first path through that > > function you linked. The second time, we take the else branch, and > > correspondingly unset the bits from ALLOW_ANY_SHA1. > > > > So today that is doing the right thing (it will end with the same bits > > set that we started with). But under the proposed patch that behavior > > would change, and the lower two bits would still be set. > > Reading Piotr's response I wondered if upload-pack might be using the > configset API instead of a config callback. But no, it does look like a > config callback. Yeah... I was pretty sure that was the case thinking back to 6dd3456a8c (upload-pack.c: allow banning certain object filter(s), 2020-08-03), which somehow was already more than four years ago :-<. > But it does hint at a possible path if we wanted to process these > independently: we could read each of the config options separately, > resolving them in the usual last-one-wins way, and then turning the > result into a bitfield. Something like this, outside of the callback > (totally untested): > > int v; > unsigned bits = 0; > > if (!git_config_get_bool("uploadpack.allowtipsha1inwant", &v) && v) > bits |= ALLOW_TIP_SHA1; > if (!git_config_get_bool("uploadpack.allowreachablesha1inwant", &v) && v) > bits |= ALLOW_REACHABLE_SHA1; > if (!git_config_get_bool("uploadpack.allowanysha1inwant", &v) && v) > bits |= ALLOW_ANY_SHA1; > > That makes the config flags independent. But the features themselves are > not really independent. If you do: > > [uploadpack] > allowAnySHA1InWant = true > allowTipSHA1InWant = false > > it is still going to allow the "tip" flag, because it's a subset of the > "any" flag. And you can't escape that. Yup. > So I still think we're better off to just document (and of course in the > long run all of these become less and less interesting as they are > v0-only options). I agree completely. I think the consensus here seems to be that, if anything, we should update the documentation and move on :-). Thanks, Taylor
diff --git a/upload-pack.c b/upload-pack.c index 6d6e0f9f980..cf99b228719 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -53,6 +53,7 @@ enum allow_uor { /* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */ ALLOW_REACHABLE_SHA1 = 0x02, /* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */ + /* As this flag implies other two flags, be careful when it must be disabled. */ ALLOW_ANY_SHA1 = 0x07 }; @@ -1368,7 +1369,7 @@ static int upload_pack_config(const char *var, const char *value, if (git_config_bool(var, value)) data->allow_uor |= ALLOW_ANY_SHA1; else - data->allow_uor &= ~ALLOW_ANY_SHA1; + data->allow_uor &= ~(ALLOW_ANY_SHA1 -(ALLOW_TIP_SHA1|ALLOW_REACHABLE_SHA1)); } else if (!strcmp("uploadpack.keepalive", var)) { data->keepalive = git_config_int(var, value, ctx->kvi); if (!data->keepalive)