On Tue, Dec 11 2018, Jeff King wrote:
> When using the v2 protocol, hidden-ref config is not respected at all:
>
> $ git config transfer.hiderefs refs/tags/
> $ git -c protocol.version=0 ls-remote . | grep -c refs/tags
> 0
> $ git -c protocol.version=2 ls-remote . | grep -c refs/tags
> 1424
>
> The fix in patch 3 is pretty straightforward, but note:
>
> - I'm a little worried this may happen again with future features. The
> root cause is that "ls-refs" follows a different code path than the
> ref advertisement for "upload-pack". So if we add any new config,
> it needs to go both places (non ref-advertisement config is OK, as
> the v2 "fetch" command is a lot closer to a v0 upload-pack).
>
> I think this is just an issue for any future features. I looked for
> other existing features which might be missing in v2, but couldn't
> find any.
>
> I don't know if there's a good solution. I tried running the whole
> test suite with v2 as the default. It does find this bug, but it has
> a bunch of other problems (notably fetch-pack won't run as v2, but
> some other tests I think also depend on v0's reachability rules,
> which v2 is documented not to enforce).
I think a global test mode for it would be a very good idea.
> - The "serve" command is funky, because it has no concept of whether
> the "ls-refs" is for fetching or pushing. Is git-serve even a thing
> that we want to support going forward? I know part of the original
> v2 conception was that one would be able to just connect to
> "git-serve" and do a number of operations. But in practice the v2
> probing requires saying "I'd like to git-upload-pack, and v2 if you
> please". So no client ever calls git-serve.
>
> Is this something we plan to eventually move to? Or can it be
> considered a funny vestige of the development? In the latter case, I
> think we should consider removing it.
>
> I've worked around it here with patch 2, but note that "git serve"
> would not ever respect uploadpack.hiderefs nor receive.hiderefs
> (since it has no idea which operation it's doing).
>
> The patches are:
>
> [1/3]: serve: pass "config context" through to individual commands
> [2/3]: parse_hide_refs_config: handle NULL section
> [3/3]: upload-pack: support hidden refs with protocol v2
Does this issue rise to the level of needing a security point-release
(which I'm discussing here as the details are already public). The
transfer.hideRefs docs have said:
Even if you hide refs, a client may still be able to steal the
target objects via the techniques described in the "SECURITY"
section of the gitnamespaces(7) man page; it’s best to keep private
data in a separate repository.
So we never promised to hide the objects, but definitely promised to
hide the ref names. I don't know if anyone uses this in practice for
secret ref names, but if they do they have a data leak if they enable
protocol v2.
More importantly, the docs for receive.hideRefs say. "An attempt to
update or delete a hidden ref by git push is rejected.". It seems this
bit was enforced, i.e. this passes before and after your 3/3, but I have
not dug enough to be 100% satisfied with that.
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index ca69636fd5..20059c3308 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -210,6 +210,13 @@ test_expect_success 'protocol v2 supports hiderefs' '
! grep refs/tags actual
'
+test_expect_success 'protocol v2 respects hiderefs when pushing' '
+ git init --bare server.git &&
+ git -C server.git config transfer.hideRefs refs/tags &&
+ test_must_fail git -c protocol.version=0 push "file://$PWD/server.git"
mark &&
+ test_must_fail git -c protocol.version=2 push "file://$PWD/server.git"
mark
+'
+
test_expect_success 'ls-remote --symref' '
git fetch origin &&
cat >expect <<-EOF &&
If there's some bug where you can bypass this push protection that would
be much worse. E.g. GitLab uses "keep-around" refs to track its own
internal state, and it would be bad if users could manipulate it.
> builtin/upload-pack.c | 1 +
> ls-refs.c | 16 +++++++++++++++-
> ls-refs.h | 3 ++-
> refs.c | 3 ++-
> serve.c | 9 +++++----
> serve.h | 7 +++++++
> t/t5512-ls-remote.sh | 6 ++++++
> upload-pack.c | 4 ++--
> upload-pack.h | 4 ++--
> 9 files changed, 42 insertions(+), 11 deletions(-)
>
> -Peff
