* Re: send-email garbled header with trailing doublequote in email
From: Jeff King @ 2016-11-04 0:11 UTC (permalink / raw)
To: Andrea Arcangeli; +Cc: git
In-Reply-To: <20161104000351.GP4611@redhat.com>
On Fri, Nov 04, 2016 at 01:03:51AM +0100, Andrea Arcangeli wrote:
> > can see why it is confused and does what it does (the whole email is
> > inside a double-quoted portion that is never closed, so it probably
> > thinks there is no hostname portion at all).
>
> I would see it too, if it actually sent the email to the garbled email
> address it generated, but it actually got the right email address
> (because it sent the email to the right address), but it decided to
> show a different email address in the mail header than the one it sent
> the email to. But I figure this is the wrong list for such questions :).
It has to do with the "envelope recipient" versus the RFC822 header. Git
provides the envelope recipient on the command-line, and that is what is
used in the SMTP conversation. In theory the MTA does not need to ever
even look at the contents of the message itself. Some do not, but some
have features like rewriting the in-message headers (e.g., to turn
"to: user" into "to: user@hostname"). You can probably disable that
header-rewriting feature in your config, but I don't know very much
about configuring postfix.
-Peff
^ permalink raw reply
* Re: send-email garbled header with trailing doublequote in email
From: Andrea Arcangeli @ 2016-11-04 0:03 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161103141848.42pg6iow24prign5@sigill.intra.peff.net>
On Thu, Nov 03, 2016 at 10:18:48AM -0400, Jeff King wrote:
> bogus header. The munging that postfix does makes things worse, but I
I agree it makes things worse.
> can see why it is confused and does what it does (the whole email is
> inside a double-quoted portion that is never closed, so it probably
> thinks there is no hostname portion at all).
I would see it too, if it actually sent the email to the garbled email
address it generated, but it actually got the right email address
(because it sent the email to the right address), but it decided to
show a different email address in the mail header than the one it sent
the email to. But I figure this is the wrong list for such questions :).
> I think if any change were to be made, it would be to recognize this
> bogosity and either clean it up or abort. That ideally would happen via
If postfix continues to do what it does, a strict checking would be my
preference of course, assuming it won't break anything that currently
works...
If not it's fine too, as nothing particularly "unexpected" happened in
git.
At this point double checking that what postfix does is legit is
perhaps more worthwhile than adding strictness to git.
Thanks,
Andrea
^ permalink raw reply
* Re: [PATCH 2/4] t0021: put $TEST_ROOT in $PATH
From: Johannes Sixt @ 2016-11-03 21:09 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Lars Schneider, Junio C Hamano, git
In-Reply-To: <20161103204438.zfe653c2bsv3zqkx@sigill.intra.peff.net>
Am 03.11.2016 um 21:44 schrieb Jeff King:
> On Thu, Nov 03, 2016 at 09:40:01PM +0100, Johannes Sixt wrote:
>> We have to use $PWD instead of $(pwd) because on Windows...
>
> Thanks. I have to admit I remain confused about which one to use at any
> given invocation
No worries. It *is* complex, and I don't expect anyone who is not deep
in the business to get this thing right except by mere chance.
> (I would have thought that switching to $PWD causes
> problems elsewhere in the script, but I guess file redirection is
> capable of handling either type).
Correct.
-- Hannes
^ permalink raw reply
* Re: [PATCHv2 00/36] Revamp the attr subsystem!
From: Stefan Beller @ 2016-11-03 20:53 UTC (permalink / raw)
To: Johannes Sixt
Cc: Junio C Hamano, Brandon Williams, Duy Nguyen, git@vger.kernel.org
In-Reply-To: <18e8d0bb-ceb0-17dd-1318-f07585f8709b@kdbg.org>
On Thu, Nov 3, 2016 at 1:47 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 28.10.2016 um 20:54 schrieb Stefan Beller:
>>
>> previous discussion at
>> https://public-inbox.org/git/20161022233225.8883-1-sbeller@google.com
>>
>> This implements the discarded series':
>> jc/attr
>> jc/attr-more
>> sb/pathspec-label
>> sb/submodule-default-paths
>>
>> This includes
>> * The fixes for windows
>
>
> I've tested the incarnation currently in pu (1928fcc65dc6), which also has
> these fixes, and they work well after a cursory test (it at least passes the
> 3 test scripts that the series touches).
Thanks!
^ permalink raw reply
* Re: [PATCHv2 00/36] Revamp the attr subsystem!
From: Johannes Sixt @ 2016-11-03 20:47 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, bmwill, pclouds, git
In-Reply-To: <20161028185502.8789-1-sbeller@google.com>
Am 28.10.2016 um 20:54 schrieb Stefan Beller:
> previous discussion at https://public-inbox.org/git/20161022233225.8883-1-sbeller@google.com
>
> This implements the discarded series':
> jc/attr
> jc/attr-more
> sb/pathspec-label
> sb/submodule-default-paths
>
> This includes
> * The fixes for windows
I've tested the incarnation currently in pu (1928fcc65dc6), which also
has these fixes, and they work well after a cursory test (it at least
passes the 3 test scripts that the series touches).
> t/t0003-attributes.sh | 26 ++
> t/t6134-pathspec-with-labels.sh | 185 +++++++++
> t/t7400-submodule-basic.sh | 134 +++++++
-- Hannes
^ permalink raw reply
* Re: [PATCH 2/4] t0021: put $TEST_ROOT in $PATH
From: Jeff King @ 2016-11-03 20:44 UTC (permalink / raw)
To: Johannes Sixt
Cc: Torsten Bögershausen, Lars Schneider, Junio C Hamano, git
In-Reply-To: <d36d8b51-f2d7-a2f5-89ea-369f49556e10@kdbg.org>
On Thu, Nov 03, 2016 at 09:40:01PM +0100, Johannes Sixt wrote:
> > TEST_ROOT="$(pwd)"
> > +PATH=$TEST_ROOT:$PATH
>
> This causes problems on Windows. We need the following squashed in.
>
> ---- 8< ----
> [PATCH] squash! t0021: put $TEST_ROOT in $PATH
>
> We have to use $PWD instead of $(pwd) because on Windows the
> latter would add a C: style path to bash's Unix-style $PATH
> variable, which becomes confused by the colon after the
> drive letter. ($PWD is a Unix-style path.)
Thanks. I have to admit I remain confused about which one to use at any
given invocation (I would have thought that switching to $PWD causes
problems elsewhere in the script, but I guess file redirection is
capable of handling either type).
-Peff
^ permalink raw reply
* Re: [PATCH 2/4] t0021: put $TEST_ROOT in $PATH
From: Johannes Sixt @ 2016-11-03 20:40 UTC (permalink / raw)
To: Jeff King; +Cc: Torsten Bögershausen, Lars Schneider, Junio C Hamano, git
In-Reply-To: <20161102181824.mi6lmfnfckvrav7n@sigill.intra.peff.net>
Am 02.11.2016 um 19:18 schrieb Jeff King:
> We create a rot13.sh script in the trash directory, but need
> to call it by its full path when we have moved our cwd to
> another directory. Let's just put $TEST_ROOT in our $PATH so
> that the script is always found.
>
> This is a minor convenience for rot13.sh, but will be a
> major one when we switch rot13-filter.pl to a script in the
> same directory, as it means we will not have to deal with
> shell quoting inside the filter-process config.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t0021-conversion.sh | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index dfde22549..c1ad20c61 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -5,6 +5,7 @@ test_description='blob conversion via gitattributes'
> . ./test-lib.sh
>
> TEST_ROOT="$(pwd)"
> +PATH=$TEST_ROOT:$PATH
This causes problems on Windows. We need the following squashed in.
---- 8< ----
[PATCH] squash! t0021: put $TEST_ROOT in $PATH
We have to use $PWD instead of $(pwd) because on Windows the
latter would add a C: style path to bash's Unix-style $PATH
variable, which becomes confused by the colon after the
drive letter. ($PWD is a Unix-style path.)
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
t/t0021-conversion.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 4e4b9a6be3..cff2b7259d 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -4,7 +4,7 @@ test_description='blob conversion via gitattributes'
. ./test-lib.sh
-TEST_ROOT="$(pwd)"
+TEST_ROOT="$PWD"
PATH=$TEST_ROOT:$PATH
write_script <<\EOF "$TEST_ROOT/rot13.sh"
--
2.11.0.rc0.55.gd967357
^ permalink raw reply related
* [PATCH (optional)] t0021: use arithmetic expansion to trim whitespace from wc -c output
From: Johannes Sixt @ 2016-11-03 20:22 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, jnareb, peff, ramsay, tboegi
In-Reply-To: <fb4d62de-fbb5-a2b4-8eba-b135125dafa9@kdbg.org>
Instead of a pipeline with sed and a useless use of cat, return the
unmodified text of wc -c from function file_size, but substitute the
result with arithmetic expansion to get rid of the leading whitespace
that some version of wc -c print.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
This is a pure optimization that reduces the number of forks, which
helps a bit on Windows.
There would be a solution with perl that does not require trimming
of whitespace, but perl startup times are unbearable on Windows.
wc -c is better.
t/t0021-conversion.sh | 50 +++++++++++++++++++++++++-------------------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index db71acacb3..42b529f615 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -22,7 +22,7 @@ generate_random_characters () {
}
file_size () {
- cat "$1" | wc -c | sed "s/^[ ]*//"
+ wc -c <"$1"
}
filter_git () {
@@ -369,10 +369,10 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
- IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: clean test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
- IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
STOP
EOF
test_cmp_count expected.log rot13-filter.log &&
@@ -381,14 +381,14 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
- IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: clean test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
- IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
- IN: clean test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: clean test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
- IN: clean testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: clean testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
STOP
EOF
test_cmp_count expected.log rot13-filter.log &&
@@ -399,8 +399,8 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
- IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
+ IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -409,7 +409,7 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -418,10 +418,10 @@ test_expect_success PERL 'required process filter should filter data' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge test.r $S [OK] -- OUT: $S . [OK]
- IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: smudge test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]
- IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $S3 [OK] -- OUT: $S3 . [OK]
+ IN: smudge testsubdir/test3 '\''sq'\'',\$x.r $(($S3)) [OK] -- OUT: $(($S3)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -451,7 +451,7 @@ test_expect_success PERL 'required process filter takes precedence' '
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
STOP
EOF
test_cmp_count expected.log rot13-filter.log
@@ -474,7 +474,7 @@ test_expect_success PERL 'required process filter should be used only for "clean
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: clean test.r $S [OK] -- OUT: $S . [OK]
+ IN: clean test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
STOP
EOF
test_cmp_count expected.log rot13-filter.log &&
@@ -603,11 +603,11 @@ test_expect_success PERL 'process filter should restart after unexpected write f
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge smudge-write-fail.r $SF [OK] -- OUT: $SF [WRITE FAIL]
+ IN: smudge smudge-write-fail.r $(($SF)) [OK] -- OUT: $(($SF)) [WRITE FAIL]
START
init handshake complete
- IN: smudge test.r $S [OK] -- OUT: $S . [OK]
- IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: smudge test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -649,9 +649,9 @@ test_expect_success PERL 'process filter should not be restarted if it signals a
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge error.r $SE [OK] -- OUT: 0 [ERROR]
- IN: smudge test.r $S [OK] -- OUT: $S . [OK]
- IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
+ IN: smudge error.r $(($SE)) [OK] -- OUT: 0 [ERROR]
+ IN: smudge test.r $(($S)) [OK] -- OUT: $(($S)) . [OK]
+ IN: smudge test2.r $(($S2)) [OK] -- OUT: $(($S2)) . [OK]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
@@ -688,7 +688,7 @@ test_expect_success PERL 'process filter abort stops processing of all further f
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge abort.r $SA [OK] -- OUT: 0 [ABORT]
+ IN: smudge abort.r $(($SA)) [OK] -- OUT: 0 [ABORT]
STOP
EOF
test_cmp_exclude_clean expected.log rot13-filter.log &&
--
2.11.0.rc0.55.gd967357
^ permalink raw reply related
* [PATCH] t0021: expect more variations in the output of uniq -c
From: Johannes Sixt @ 2016-11-03 20:12 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, gitster, jnareb, peff, ramsay, tboegi
In-Reply-To: <3763DDDB-9D53-4877-8399-32DF1780CAB7@gmail.com>
Some versions of uniq -c write the count left-justified, other version
write it right-justified. Be prepared for both kinds.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Here it is as a proper patch.
Am 03.11.2016 um 01:41 schrieb Lars Schneider:
>> On 2 Nov 2016, at 18:03, Johannes Sixt <j6t@kdbg.org> wrote:
>> + sort "$FILE" | uniq -c |
>> + sed -e "s/^ *[0-9][0-9]* *IN: /x IN: /" >"$FILE.tmp" &&
>
> This looks good (thanks for cleaning up the redundant clean/smudge
> stuff - that was a refactoring artifact!). One minor nit: doesn't sed
> understand '[0-9]+' ?
I don't think so.
>> + mv "$FILE.tmp" "$FILE" || return
>
> Why '|| return' here?
If there is an error in the pipeline or in the mv command, the for loop
would not exit otherwise. The subsequent test_cmp most likely fails,
but the || return is more correct.
t/t0021-conversion.sh | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index a20b9f58e3..db71acacb3 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -40,10 +40,9 @@ test_cmp_count () {
actual=$2
for FILE in "$expect" "$actual"
do
- sort "$FILE" | uniq -c | sed "s/^[ ]*//" |
- sed "s/^\([0-9]\) IN: clean/x IN: clean/" |
- sed "s/^\([0-9]\) IN: smudge/x IN: smudge/" >"$FILE.tmp" &&
- mv "$FILE.tmp" "$FILE"
+ sort "$FILE" | uniq -c |
+ sed -e "s/^ *[0-9][0-9]*[ ]*IN: /x IN: /" >"$FILE.tmp" &&
+ mv "$FILE.tmp" "$FILE" || return
done &&
test_cmp "$expect" "$actual"
}
--
2.11.0.rc0.55.gd967357
^ permalink raw reply related
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 18:56 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103185052.q46x4hlcwne7kpc5@sigill.intra.peff.net>
On 11/03, Jeff King wrote:
> On Thu, Nov 03, 2016 at 11:45:38AM -0700, Brandon Williams wrote:
>
> > On 11/03, Jeff King wrote:
> > >
> > > So this seems like a reasonable direction to me. It obviously needs
> > > documentation and tests. Arguably there should be a fallback "allow"
> > > value when a protocol is not mentioned in the config so that you could
> > > convert the default from "user" to "never" if you wanted your config to
> > > specify a pure whitelist.
> >
> > Yes I agree there should probably be a fallback value of 'never' maybe?
> > What you currently have preserves the behavior of what git does
> > now, if we did instead have a fallback of 'never' it would break current
> > users who don't already use GIT_ALLOW_PROTOCOL (well only if they use
> > crazy protocols). We could ease into it though and start with default
> > to allow and then transition to a true whitelist sometime after this
> > change has been made?
>
> I don't see the value in moving the out-of-the-box install to any
> default except "user". Right now the experience of using a third-party
> helper is something like:
>
> cp git-remote-hg /somewhere/in/your/PATH
> git clone hg::whatever
>
> We restrict its use in submodules by default, which is unlikely to bite
> many people. But if we started falling back to "never" all the time,
> then that second command would break until you officially "approve"
> remote-hg in your config.
>
> I was thinking of just something to let people decide to have that level
> of paranoia themselves (especially if they want to just set up a
> whole-system white list via the config without bothering with
> environment variables). Like:
>
> git config --system protocol.allow never
> git config --system protocol.https.allow always
>
> That behaves exactly like:
>
> export GIT_ALLOW_PROTOCOL=https
>
> except it just works everywhere, without having to tweak the environment
> of every process.
>
Ah ok, so essentially letting the user specify a default behaviour
themselves.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 18:50 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103184538.GE182568@google.com>
On Thu, Nov 03, 2016 at 11:45:38AM -0700, Brandon Williams wrote:
> On 11/03, Jeff King wrote:
> >
> > So this seems like a reasonable direction to me. It obviously needs
> > documentation and tests. Arguably there should be a fallback "allow"
> > value when a protocol is not mentioned in the config so that you could
> > convert the default from "user" to "never" if you wanted your config to
> > specify a pure whitelist.
>
> Yes I agree there should probably be a fallback value of 'never' maybe?
> What you currently have preserves the behavior of what git does
> now, if we did instead have a fallback of 'never' it would break current
> users who don't already use GIT_ALLOW_PROTOCOL (well only if they use
> crazy protocols). We could ease into it though and start with default
> to allow and then transition to a true whitelist sometime after this
> change has been made?
I don't see the value in moving the out-of-the-box install to any
default except "user". Right now the experience of using a third-party
helper is something like:
cp git-remote-hg /somewhere/in/your/PATH
git clone hg::whatever
We restrict its use in submodules by default, which is unlikely to bite
many people. But if we started falling back to "never" all the time,
then that second command would break until you officially "approve"
remote-hg in your config.
I was thinking of just something to let people decide to have that level
of paranoia themselves (especially if they want to just set up a
whole-system white list via the config without bothering with
environment variables). Like:
git config --system protocol.allow never
git config --system protocol.https.allow always
That behaves exactly like:
export GIT_ALLOW_PROTOCOL=https
except it just works everywhere, without having to tweak the environment
of every process.
> > Do you have interest in picking this up and running with it?
>
> Yep! Thanks for the help in shaping this.
Great, thanks. I'm happy to review or discuss further as necessary.
-Peff
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 18:45 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103182428.j3r574evsk7ypfie@sigill.intra.peff.net>
On 11/03, Jeff King wrote:
>
> So this seems like a reasonable direction to me. It obviously needs
> documentation and tests. Arguably there should be a fallback "allow"
> value when a protocol is not mentioned in the config so that you could
> convert the default from "user" to "never" if you wanted your config to
> specify a pure whitelist.
Yes I agree there should probably be a fallback value of 'never' maybe?
What you currently have preserves the behavior of what git does
now, if we did instead have a fallback of 'never' it would break current
users who don't already use GIT_ALLOW_PROTOCOL (well only if they use
crazy protocols). We could ease into it though and start with default
to allow and then transition to a true whitelist sometime after this
change has been made?
>
> Without that, I think we'd want to keep GIT_ALLOW_PROTOCOL for the truly
> paranoid (though we should keep it indefinitely either way for backwards
> compatibility).
>
> Do you have interest in picking this up and running with it?
Yep! Thanks for the help in shaping this.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 18:25 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103181954.GD182568@google.com>
On Thu, Nov 03, 2016 at 11:19:54AM -0700, Brandon Williams wrote:
> On 11/03, Jeff King wrote:
> > +
> > + /* unknown; let them be used only directly by the user */
> > + return PROTOCOL_ALLOW_USER_ONLY;
> > +}
> > +
> > int is_transport_allowed(const char *type)
> > {
> > - const struct string_list *allowed = protocol_whitelist();
> > - return !allowed || string_list_has_string(allowed, type);
> > + const struct string_list *whitelist = protocol_whitelist();
> > + if (whitelist)
> > + return string_list_has_string(whitelist, type);
> > +
> > + switch (get_protocol_config(type)) {
> > + case PROTOCOL_ALLOW_ALWAYS:
> > + return 1;
> > + case PROTOCOL_ALLOW_NEVER:
> > + return 0;
> > + case PROTOCOL_ALLOW_USER_ONLY:
> > + return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
> > + }
>
> I know this is just a rough patch you wiped up but one question:
> With the 'user' state, how exactly do you envision this env variable
> working? Do we want the user to have to explicitly set
> GIT_PROTOCOL_FROM_USER in their environment and then have these other
> commands (like git-submodule) explicitly clear the env var or would we
> rather these subcommands set a variable indicating they aren't coming
> from the user and the deafult state (no var set) is a user run command?
See the follow-up I just posted, but basically, the rules are:
- if you don't say anything, then the URL is from the user
- git-submodule would set it to "0" (i.e., tell us to be more careful)
- tools like "go get" would similarly set it to "0" if they are
passing untrusted URLs
-Peff
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 18:24 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103175327.nn2yasvlsxsy22be@sigill.intra.peff.net>
On Thu, Nov 03, 2016 at 01:53:27PM -0400, Jeff King wrote:
> I'd design the new system from scratch, and have it kick in _only_ when
> GIT_ALLOW_PROTOCOL is not set. That lets existing callers continue to
> have the safe behavior until they are ready to move to the new format.
>
> Something like the patch below (which is just for illustration, and not
> tested beyond compilation).
Here's that same patch with a few tweaks:
- it changes git-submodule to use the new, more flexible system (which
also gets a it a lot more test coverage)
- it tweaks two tests which use the "ext" helper to enable it (since
it's blacklisted by default; I have mixed feelings on that, but I
see why Blake wants it, as it would have protected things like "go
get" out of the box).
- it adds "file://" as a known-good protocol, even for submodules,
which matches the current code. I am not sure if this is reasonable
or not. A malicious repository probably can't do much by pointing
you to cloning your own repo as a submodule unless you then _also_
run some arbitrary code to expose it, at which point it's generally
game-over anyway.
And I'd expect automated services (like GitHub Pages) to already
have a cut-down whitelist via GIT_ALLOW_PROTOCOL (and I happen to
know that it goes).
So this seems like a reasonable direction to me. It obviously needs
documentation and tests. Arguably there should be a fallback "allow"
value when a protocol is not mentioned in the config so that you could
convert the default from "user" to "never" if you wanted your config to
specify a pure whitelist.
Without that, I think we'd want to keep GIT_ALLOW_PROTOCOL for the truly
paranoid (though we should keep it indefinitely either way for backwards
compatibility).
Do you have interest in picking this up and running with it?
-Peff
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d..0a477b4c9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -21,14 +21,10 @@ require_work_tree
wt_prefix=$(git rev-parse --show-prefix)
cd_to_toplevel
-# Restrict ourselves to a vanilla subset of protocols; the URLs
-# we get are under control of a remote repository, and we do not
-# want them kicking off arbitrary git-remote-* programs.
-#
-# If the user has already specified a set of allowed protocols,
-# we assume they know what they're doing and use that instead.
-: ${GIT_ALLOW_PROTOCOL=file:git:http:https:ssh}
-export GIT_ALLOW_PROTOCOL
+# Tell the rest of git that any URLs we get don't come
+# directly from the user, so it can apply policy as appropriate.
+GIT_PROTOCOL_FROM_USER=0
+export GIT_PROTOCOL_FROM_USER
command=
branch=
diff --git a/t/t5509-fetch-push-namespaces.sh b/t/t5509-fetch-push-namespaces.sh
index bc44ac36d..75c570adc 100755
--- a/t/t5509-fetch-push-namespaces.sh
+++ b/t/t5509-fetch-push-namespaces.sh
@@ -4,6 +4,7 @@ test_description='fetch/push involving ref namespaces'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git init original &&
(
diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
index b7a7f9d58..c6c266187 100755
--- a/t/t5802-connect-helper.sh
+++ b/t/t5802-connect-helper.sh
@@ -4,6 +4,7 @@ test_description='ext::cmd remote "connect" helper'
. ./test-lib.sh
test_expect_success setup '
+ git config --global protocol.ext.allow user &&
test_tick &&
git commit --allow-empty -m initial &&
test_tick &&
diff --git a/transport.c b/transport.c
index d57e8dec2..cd603fbf5 100644
--- a/transport.c
+++ b/transport.c
@@ -664,10 +664,70 @@ static const struct string_list *protocol_whitelist(void)
return enabled ? &allowed : NULL;
}
+enum protocol_allow_config {
+ PROTOCOL_ALLOW_NEVER = 0,
+ PROTOCOL_ALLOW_USER_ONLY,
+ PROTOCOL_ALLOW_ALWAYS
+};
+
+static enum protocol_allow_config parse_protocol_config(const char *key,
+ const char *value)
+{
+ if (!strcasecmp(value, "always"))
+ return PROTOCOL_ALLOW_ALWAYS;
+ else if (!strcasecmp(value, "never"))
+ return PROTOCOL_ALLOW_NEVER;
+ else if (!strcasecmp(value, "user"))
+ return PROTOCOL_ALLOW_USER_ONLY;
+
+ /* XXX maybe also interpret git_config_bool() here? */
+ die("unknown value for config '%s': %s", key, value);
+}
+
+static enum protocol_allow_config get_protocol_config(const char *type)
+{
+ char *key = xstrfmt("protocol.%s.allow", type);
+ char *value;
+
+ if (!git_config_get_string(key, &value)) {
+ enum protocol_allow_config ret =
+ parse_protocol_config(key, value);
+ free(key);
+ free(value);
+ return ret;
+ }
+ free(key);
+
+ /* known safe */
+ if (!strcmp(type, "http") ||
+ !strcmp(type, "https") ||
+ !strcmp(type, "git") ||
+ !strcmp(type, "ssh") ||
+ !strcmp(type, "file"))
+ return PROTOCOL_ALLOW_ALWAYS;
+
+ /* known scary; err on the side of caution */
+ if (!strcmp(type, "ext"))
+ return PROTOCOL_ALLOW_NEVER;
+
+ /* unknown; let them be used only directly by the user */
+ return PROTOCOL_ALLOW_USER_ONLY;
+}
+
int is_transport_allowed(const char *type)
{
- const struct string_list *allowed = protocol_whitelist();
- return !allowed || string_list_has_string(allowed, type);
+ const struct string_list *whitelist = protocol_whitelist();
+ if (whitelist)
+ return string_list_has_string(whitelist, type);
+
+ switch (get_protocol_config(type)) {
+ case PROTOCOL_ALLOW_ALWAYS:
+ return 1;
+ case PROTOCOL_ALLOW_NEVER:
+ return 0;
+ case PROTOCOL_ALLOW_USER_ONLY:
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ }
}
void transport_check_allowed(const char *type)
^ permalink raw reply related
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 18:19 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103175327.nn2yasvlsxsy22be@sigill.intra.peff.net>
On 11/03, Jeff King wrote:
> +
> + /* unknown; let them be used only directly by the user */
> + return PROTOCOL_ALLOW_USER_ONLY;
> +}
> +
> int is_transport_allowed(const char *type)
> {
> - const struct string_list *allowed = protocol_whitelist();
> - return !allowed || string_list_has_string(allowed, type);
> + const struct string_list *whitelist = protocol_whitelist();
> + if (whitelist)
> + return string_list_has_string(whitelist, type);
> +
> + switch (get_protocol_config(type)) {
> + case PROTOCOL_ALLOW_ALWAYS:
> + return 1;
> + case PROTOCOL_ALLOW_NEVER:
> + return 0;
> + case PROTOCOL_ALLOW_USER_ONLY:
> + return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
> + }
I know this is just a rough patch you wiped up but one question:
With the 'user' state, how exactly do you envision this env variable
working? Do we want the user to have to explicitly set
GIT_PROTOCOL_FROM_USER in their environment and then have these other
commands (like git-submodule) explicitly clear the env var or would we
rather these subcommands set a variable indicating they aren't coming
from the user and the deafult state (no var set) is a user run command?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 18:08 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, Jonathan Nieder, git@vger.kernel.org,
Blake Burkhart
In-Reply-To: <20161103180224.7mhpziotpinmd3tr@sigill.intra.peff.net>
On 11/03, Jeff King wrote:
> On Thu, Nov 03, 2016 at 10:51:31AM -0700, Brandon Williams wrote:
>
> > > > I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> > > > because I view a whitelist or blacklist as well black and white and
> > > > having this user state adds in a gray area.
> > >
> > > Well the "user" state is to differentiate between the
> > > * "I consciously typed `git clone ...` (and e.g. I know what happens as
> > > I know the server admin and they are trustworthy.)
> > > * a repository contains a possible hostile .gitmodules file such
> > > that I am not aware of the network connection.
> >
> > This is still a gray area to me. I think that if we have a whitelist of
> > protocols then it should be a true whitelist and not have some means of
> > going around it. It just seems like something that could be exploited.
>
> How do you implement:
>
> git clone --recursive trusted:foo.git
>
> and use your ssh keys for the "trusted" server, but not for any servers
> mentioned in .gitmodules?
>
> You need some way of distinguishing between the two contexts (and
> setting policy for each).
>
> -Peff
Interesting. Ok I can see how this would be a useful now. Thanks for
the example :)
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 18:02 UTC (permalink / raw)
To: Brandon Williams
Cc: Stefan Beller, Jonathan Nieder, git@vger.kernel.org,
Blake Burkhart
In-Reply-To: <20161103175131.GB182568@google.com>
On Thu, Nov 03, 2016 at 10:51:31AM -0700, Brandon Williams wrote:
> > > I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> > > because I view a whitelist or blacklist as well black and white and
> > > having this user state adds in a gray area.
> >
> > Well the "user" state is to differentiate between the
> > * "I consciously typed `git clone ...` (and e.g. I know what happens as
> > I know the server admin and they are trustworthy.)
> > * a repository contains a possible hostile .gitmodules file such
> > that I am not aware of the network connection.
>
> This is still a gray area to me. I think that if we have a whitelist of
> protocols then it should be a true whitelist and not have some means of
> going around it. It just seems like something that could be exploited.
How do you implement:
git clone --recursive trusted:foo.git
and use your ssh keys for the "trusted" server, but not for any servers
mentioned in .gitmodules?
You need some way of distinguishing between the two contexts (and
setting policy for each).
-Peff
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 18:00 UTC (permalink / raw)
To: Stefan Beller
Cc: Brandon Williams, Jonathan Nieder, git@vger.kernel.org,
Blake Burkhart
In-Reply-To: <CAGZ79kbvs+ryGRjCkHYO=3iNK4tPPhBhYjRMZaH7rP0QMrULhg@mail.gmail.com>
On Thu, Nov 03, 2016 at 10:39:35AM -0700, Stefan Beller wrote:
> >> protocol.X.allow = always | user | never
> >
> > It sounds like there is interest for this sort of behavior, it would
> > definitely require a larger change than what I initially proposed. One
> > problem I see though is that with this we have support for both a
> > blacklist and a whitelist. Which wins?
>
> For the submodule operations we'll use a whitelist, because we want to
> provide security and for the other case we can offer a blacklist as a bandaid.
>
> My opinion on blacklists is roughly aligned with e.g. :
> https://blog.codinghorror.com/blacklists-dont-work/
> http://blog.deepinstinct.com/2016/02/04/when-blacklists-dont-really-work/
>
> So IMHO we could drop the "never" and substitute it with a "warn" or
> "ask-user", such that this configuration becomes a white list for both cases:
>
> protocol.X.allow = always | user | warn
I don't think blacklists work in the general case, because they grow out
of date and fail-open. But you want to have _some_ blacklisting
mechanism, in order override a decision of the whitelist.
For instance, the default submodule whitelist would probably include
https and ssh. But if I'm cloning potentially malicious repos and I
don't ever want them to trigger ssh (because I don't want them to use my
ssh keys, whereas I have explicitly set up my credentials such that http
is safe to use), I would want to be able to do:
git config protocol.ssh.allow never
(or "git -c", or whatever).
True, a whitelist is safer. If we add a new "foo" protocol that also
looks at your ssh keys, you're screwed. And that's why I designed it as
a pure-whitelist in the first place. But it comes at the price of
convenience, because you have to manually add each new innocent protocol
to the whitelist.
> So you're suggesting that setting it to "never" doesn't have any effect
> except for cluttering the config file?
> I don't think we should do that; each setting should have an impact.
> So maybe the "never" would be there to disallow protocols of the hardcoded
> white list (e.g. http)
Exactly.
> > I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> > because I view a whitelist or blacklist as well black and white and
> > having this user state adds in a gray area.
>
> Well the "user" state is to differentiate between the
> * "I consciously typed `git clone ...` (and e.g. I know what happens as
> I know the server admin and they are trustworthy.)
> * a repository contains a possible hostile .gitmodules file such
> that I am not aware of the network connection.
Right. I had assumed that we would tell the difference between those
automatically (by seeing if we got the URL on the command line), but
things like "go get" show that the context is often before git is even
called.
-Peff
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Jeff King @ 2016-11-03 17:53 UTC (permalink / raw)
To: Brandon Williams; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103172515.GA182568@google.com>
On Thu, Nov 03, 2016 at 10:25:15AM -0700, Brandon Williams wrote:
> > So I think this probably needs to be a separate parallel system where
> > each protocol can be white- or black-listed in a context-specific way.
> > Like:
> >
> > protocol.X.allow = always | user | never
>
> It sounds like there is interest for this sort of behavior, it would
> definitely require a larger change than what I initially proposed. One
> problem I see though is that with this we have support for both a
> blacklist and a whitelist. Which wins? Or do we simply generate a
> whitelist of allowed protocols which includes all protocols with allow
> set to 'always' and if it is set to 'never' then it just isn't included
> in the whitelist?
I think trying to combine the two or generate the whitelist from the
more flexible format is a recipe for madness.
I'd design the new system from scratch, and have it kick in _only_ when
GIT_ALLOW_PROTOCOL is not set. That lets existing callers continue to
have the safe behavior until they are ready to move to the new format.
Something like the patch below (which is just for illustration, and not
tested beyond compilation).
> I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> because I view a whitelist or blacklist as well black and white and
> having this user state adds in a gray area.
The lack of understanding the "user" context is what makes the current
system so painful. The only way a caller can influence the system is to
hand over the policy directly: this is allowed, this is not. But systems
like "go get" should not be setting policy. They should be giving us a
hint about the context, and letting git implement the policy.
-- >8 --
diff --git a/transport.c b/transport.c
index d57e8dec2..0d5b382db 100644
--- a/transport.c
+++ b/transport.c
@@ -664,10 +664,69 @@ static const struct string_list *protocol_whitelist(void)
return enabled ? &allowed : NULL;
}
+enum protocol_allow_config {
+ PROTOCOL_ALLOW_NEVER,
+ PROTOCOL_ALLOW_USER_ONLY,
+ PROTOCOL_ALLOW_ALWAYS
+};
+
+static enum protocol_allow_config parse_protocol_config(const char *key,
+ const char *value)
+{
+ if (!strcasecmp(value, "always"))
+ return PROTOCOL_ALLOW_ALWAYS;
+ else if (!strcasecmp(value, "never"))
+ return PROTOCOL_ALLOW_NEVER;
+ else if (!strcasecmp(value, "user"))
+ return PROTOCOL_ALLOW_USER_ONLY;
+
+ /* XXX maybe also interpret git_config_bool() here? */
+ die("unknown value for config '%s': %s", key, value);
+}
+
+static enum protocol_allow_config get_protocol_config(const char *type)
+{
+ char *key = xstrfmt("protocol.%s.allow", type);
+ char *value;
+
+ if (!git_config_get_string(key, &value)) {
+ enum protocol_allow_config ret =
+ parse_protocol_config(key, value);
+ free(key);
+ free(value);
+ return ret;
+ }
+ free(key);
+
+ /* known safe */
+ if (!strcmp(type, "http") ||
+ !strcmp(type, "https") ||
+ !strcmp(type, "git") ||
+ !strcmp(type, "ssh"))
+ return PROTOCOL_ALLOW_ALWAYS;
+
+ /* known scary; err on the side of caution */
+ if (!strcmp(type, "ext"))
+ return PROTOCOL_ALLOW_NEVER;
+
+ /* unknown; let them be used only directly by the user */
+ return PROTOCOL_ALLOW_USER_ONLY;
+}
+
int is_transport_allowed(const char *type)
{
- const struct string_list *allowed = protocol_whitelist();
- return !allowed || string_list_has_string(allowed, type);
+ const struct string_list *whitelist = protocol_whitelist();
+ if (whitelist)
+ return string_list_has_string(whitelist, type);
+
+ switch (get_protocol_config(type)) {
+ case PROTOCOL_ALLOW_ALWAYS:
+ return 1;
+ case PROTOCOL_ALLOW_NEVER:
+ return 0;
+ case PROTOCOL_ALLOW_USER_ONLY:
+ return git_env_bool("GIT_PROTOCOL_FROM_USER", 1);
+ }
}
void transport_check_allowed(const char *type)
^ permalink raw reply related
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 17:51 UTC (permalink / raw)
To: Stefan Beller
Cc: Jeff King, Jonathan Nieder, git@vger.kernel.org, Blake Burkhart
In-Reply-To: <CAGZ79kbvs+ryGRjCkHYO=3iNK4tPPhBhYjRMZaH7rP0QMrULhg@mail.gmail.com>
On 11/03, Stefan Beller wrote:
> >> protocol.X.allow = always | user | never
> >
> > It sounds like there is interest for this sort of behavior, it would
> > definitely require a larger change than what I initially proposed. One
> > problem I see though is that with this we have support for both a
> > blacklist and a whitelist. Which wins?
>
> For the submodule operations we'll use a whitelist, because we want to
> provide security and for the other case we can offer a blacklist as a bandaid.
>
> My opinion on blacklists is roughly aligned with e.g. :
> https://blog.codinghorror.com/blacklists-dont-work/
> http://blog.deepinstinct.com/2016/02/04/when-blacklists-dont-really-work/
>
> So IMHO we could drop the "never" and substitute it with a "warn" or
> "ask-user", such that this configuration becomes a white list for both cases:
>
> protocol.X.allow = always | user | warn
>
> > Or do we simply generate a
> > whitelist of allowed protocols which includes all protocols with allow
> > set to 'always' and if it is set to 'never' then it just isn't included
> > in the whitelist?
>
> So you're suggesting that setting it to "never" doesn't have any effect
> except for cluttering the config file?
> I don't think we should do that; each setting should have an impact.
> So maybe the "never" would be there to disallow protocols of the hardcoded
> white list (e.g. http)
Thats what I meant, if a protocol is listed as 'never' then it just
removes that protocol from the whitelist. That way we still have the
benefit of using a whitelist vs a blacklist. Also, if we move in this
direction should we setup a default whitelist of allowed protocols?
> >
> > I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> > because I view a whitelist or blacklist as well black and white and
> > having this user state adds in a gray area.
>
> Well the "user" state is to differentiate between the
> * "I consciously typed `git clone ...` (and e.g. I know what happens as
> I know the server admin and they are trustworthy.)
> * a repository contains a possible hostile .gitmodules file such
> that I am not aware of the network connection.
This is still a gray area to me. I think that if we have a whitelist of
protocols then it should be a true whitelist and not have some means of
going around it. It just seems like something that could be exploited.
--
Brandon Williams
^ permalink raw reply
* [PATCH] SQUASH to: submodule update: add `--init-default-path` switch
From: Stefan Beller @ 2016-11-03 17:46 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, pclouds, Stefan Beller
In-Reply-To: <20161028185502.8789-35-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
I discovered this, when going over this series myself once again, as
I am thinking about how this setting may affect the
"git checkout --recurse-submodules" that I am currently working on.
I'll include this in a resend, if a resend is needed (i.e. more changes
are asked for by reviewers)
Thanks,
Stefan
Documentation/config.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 72901ef90e..0f13e75e52 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2888,7 +2888,7 @@ submodule.alternateErrorStrategy
submodule.defaultUpdatePath::
Specifies a set of submodules to initialize when calling
- `git submodule --init-default-group` by using the pathspec
+ `git submodule --init-default-path` by using the pathspec
syntax.
tag.forceSignAnnotated::
--
2.10.2.621.g399b625.dirty
^ permalink raw reply related
* Re: [PATCH] transport: add core.allowProtocol config option
From: Stefan Beller @ 2016-11-03 17:39 UTC (permalink / raw)
To: Brandon Williams
Cc: Jeff King, Jonathan Nieder, git@vger.kernel.org, Blake Burkhart
In-Reply-To: <20161103172515.GA182568@google.com>
>> protocol.X.allow = always | user | never
>
> It sounds like there is interest for this sort of behavior, it would
> definitely require a larger change than what I initially proposed. One
> problem I see though is that with this we have support for both a
> blacklist and a whitelist. Which wins?
For the submodule operations we'll use a whitelist, because we want to
provide security and for the other case we can offer a blacklist as a bandaid.
My opinion on blacklists is roughly aligned with e.g. :
https://blog.codinghorror.com/blacklists-dont-work/
http://blog.deepinstinct.com/2016/02/04/when-blacklists-dont-really-work/
So IMHO we could drop the "never" and substitute it with a "warn" or
"ask-user", such that this configuration becomes a white list for both cases:
protocol.X.allow = always | user | warn
> Or do we simply generate a
> whitelist of allowed protocols which includes all protocols with allow
> set to 'always' and if it is set to 'never' then it just isn't included
> in the whitelist?
So you're suggesting that setting it to "never" doesn't have any effect
except for cluttering the config file?
I don't think we should do that; each setting should have an impact.
So maybe the "never" would be there to disallow protocols of the hardcoded
white list (e.g. http)
>
> I don't know if I'm sold on a 'user' state just yet, perhaps that's just
> because I view a whitelist or blacklist as well black and white and
> having this user state adds in a gray area.
Well the "user" state is to differentiate between the
* "I consciously typed `git clone ...` (and e.g. I know what happens as
I know the server admin and they are trustworthy.)
* a repository contains a possible hostile .gitmodules file such
that I am not aware of the network connection.
^ permalink raw reply
* Re: [PATCH] transport: add core.allowProtocol config option
From: Brandon Williams @ 2016-11-03 17:25 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git, Stefan Beller, Blake Burkhart
In-Reply-To: <20161103143806.hce4msk3dhxtgpre@sigill.intra.peff.net>
On 11/03, Jeff King wrote:
> On Wed, Nov 02, 2016 at 05:22:25PM -0700, Jonathan Nieder wrote:
>
> > Another difficulty with setting GIT_ALLOW_PROTOCOL globally is that it
> > requires copy/pasting the default value from upstream and then adding
> > the values I want. There's no straightforward way to get the current
> > value and add to it, in case I want to benefit from future upstream
> > fixes to the default list.
>
> I agree that this is a big drawback of the current scheme, and it would
> be nice to be able to say "also allow".
>
> > That is, would it be possible to use something like
> >
> > [protocol "sso"]
> > allow = always
> >
> > instead of
> >
> > [core]
> > allowProtocol = file:git:http:https:....:sso
> >
> > ?
>
> One complication is that the whitelist has multiple states:
>
> 1. if it's not used at all, anything goes
>
> 2. if it exists and has zero or more entries, only those entries are
> allowed
>
> And then submodules are an exception to (1), because it's not anything
> goes. It's "this default safe whitelist".
>
> So when does protocol.sso.allow kick in? We wouldn't want it to trigger
> case (2) for things like fetch (disabling other non-allowed protocols).
> Nor do I think we'd only want it for the submodule case, as I would
> assume that "protocol.sso.allow = false" should disable it.
>
> So I think this probably needs to be a separate parallel system where
> each protocol can be white- or black-listed in a context-specific way.
> Like:
>
> protocol.X.allow = always | user | never
It sounds like there is interest for this sort of behavior, it would
definitely require a larger change than what I initially proposed. One
problem I see though is that with this we have support for both a
blacklist and a whitelist. Which wins? Or do we simply generate a
whitelist of allowed protocols which includes all protocols with allow
set to 'always' and if it is set to 'never' then it just isn't included
in the whitelist?
I don't know if I'm sold on a 'user' state just yet, perhaps that's just
because I view a whitelist or blacklist as well black and white and
having this user state adds in a gray area.
> > An example approach would be a GIT_ALLOW_PROTOCOL var returned by
> > "git var".
> >
> > That way git-submodule.sh could do
> >
> > : ${GIT_ALLOW_PROTOCOL=$(git var GIT_ALLOW_PROTOCOL)}
> >
> > and it would just work. Other tools could do the same, with a
> > fallback to the current default until new enough git is in widespread
> > use.
>
> ...some automated way to say "is this protocol supported"? I think it is
> not just "give me ALLOW_PROTOCOL" anymore, though, but "apply your rules
> to this protocol, and tell me if it is supported".
I agree, if we do add different states to a protocol then we couldn't
simply ask for a whitelist/blacklist of protocols anymore since its more
of a graylist :) (if such a thing exits).
--
Brandon Williams
^ permalink raw reply
* Re: `git stash apply` deleted a random dir from my working copy
From: Stefan Monov @ 2016-11-03 17:04 UTC (permalink / raw)
To: git
In-Reply-To: <CAJtFkWu1qf3xFg1-B-xVONOmqQZfJhe4+qAb4E3H2A5-st_2dw@mail.gmail.com>
Scratch my last message, I figured out how to restore it. In the
TortoiseGit context menu I selected "Diff", and in the list the
deleted files were listed as "missing". I selected them all, right
clicked them, and selected "revert". Still don't know why the dir got
deleted in the first place though.
On Thu, Nov 3, 2016 at 7:01 PM, Stefan Monov <logixoul@gmail.com> wrote:
> Actually, I just tried restoring my dir with `git pull origin
> clipping`, but that didn't restore it. So, besides the question "why
> did it get deleted", I'd like to ask "how do I restore it?", as well.
>
> On Thu, Nov 3, 2016 at 6:06 PM, Stefan Monov <logixoul@gmail.com> wrote:
>> Hi.
>>
>> I just tried `git stash save` for the first time. It worked fine. Then
>> I tried `git stash apply` and while my uncommitted changes were
>> restored, another effect was that a random dir from the root of my
>> working copy was deleted. I don't know why it chose that exact dir,
>> there's lots of other dirs like it in the root.
>>
>> `git stash save` output and shortened `git stash apply` output are attached.
>>
>> Note: The dir that got deleted was a committed and pushed dir. It had
>> no uncommitted local changes.
>>
>> It's not a huge problem, because there was no data loss - I can pull
>> the deleted data back from the repo. But I'd still like to know why
>> this problem happened.
>>
>> TIA, Stefan Monov
^ permalink raw reply
* Re: `git stash apply` deleted a random dir from my working copy
From: Stefan Monov @ 2016-11-03 17:01 UTC (permalink / raw)
To: git
In-Reply-To: <CAJtFkWsUL=9K=VGTDejE+4RR3HJevk+2aRAhUyf8-mDrqi5MRg@mail.gmail.com>
Actually, I just tried restoring my dir with `git pull origin
clipping`, but that didn't restore it. So, besides the question "why
did it get deleted", I'd like to ask "how do I restore it?", as well.
On Thu, Nov 3, 2016 at 6:06 PM, Stefan Monov <logixoul@gmail.com> wrote:
> Hi.
>
> I just tried `git stash save` for the first time. It worked fine. Then
> I tried `git stash apply` and while my uncommitted changes were
> restored, another effect was that a random dir from the root of my
> working copy was deleted. I don't know why it chose that exact dir,
> there's lots of other dirs like it in the root.
>
> `git stash save` output and shortened `git stash apply` output are attached.
>
> Note: The dir that got deleted was a committed and pushed dir. It had
> no uncommitted local changes.
>
> It's not a huge problem, because there was no data loss - I can pull
> the deleted data back from the repo. But I'd still like to know why
> this problem happened.
>
> TIA, Stefan Monov
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox