Message ID | 20200124033436.81097-10-emilyshaffer@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | add git-bugreport tool | expand |
On Fri, 24 Jan 2020 at 04:35, <emilyshaffer@google.com> wrote: > Add a new step to the build to generate a safelist of git-config > variables which are appropriate to include in the output of > git-bugreport. New variables can be added to the safelist by annotating > their documentation in Documentation/config with the > "bugreport" macro, which is recognized by AsciiDoc and > AsciiDoctor. > > Some configs are private in nature, and can contain remote URLs, > passwords, or other sensitive information. In the event that a user > doesn't notice their information while reviewing a bugreport, that user > may leak their credentials to other individuals, mailing lists, or bug > tracking tools inadvertently. Heuristic blocklisting of configuration > keys is imperfect and prone to false negatives; given the nature of the > information which can be leaked, a safelist is more reliable. I sort of wonder whether safelist/blocklist is to prefer over whitelist/blacklist, or if it's the other way round. The former are new to me, whereas the latter are the terms I would have used. But that's just me, of course. I was a little surprised, that's all. > Implement a new no-op "bugreport" macro for use as > "bugreport:include[x]" to annotate the config keys that should be > included in the automatically generated safelist. Use "exclude" for the > others. > > With Asciidoctor, it's ok to say "bugreport:include[]", but AsciiDoc > seems to want something between the brackets. A bit unfortunate, but > not a huge problem -- we'll just provide an "x". I recognize this reasoning :-) and I'm not terribly opposed to it, but after some nights' sleeping on this, I have to wonder if "annotate:bugreport[include]" wouldn't be better than "bugreport[x]" with that ugly "x". Maybe this isn't the biggest problem, but if we expect this macro to eventually sit right next to ~90% of all our config variables... > "doc-diff" reports that this macro doesn't render at all. That is, > these are both empty after this commit: > > cd Documentation > ./doc-diff --asciidoctor :/"bugreport: add tool" HEAD > ./doc-diff --asciidoc :/"bugreport: add tool" HEAD That was true in [1], but alas, no more. In that patch, it's sort of obvious from the diff how it adds a "class" which "end"s. [1] https://lore.kernel.org/git/20190817203846.31609-1-martin.agren@gmail.com/ > --- a/Documentation/asciidoctor-extensions.rb > +++ b/Documentation/asciidoctor-extensions.rb > @@ -37,6 +37,10 @@ module Git > output = output.sub(/<\/refmeta>/, new_tags + "</refmeta>") > end > output > + > + class BugReportProcessor < Asciidoctor::Extensions::InlineMacroProcessor > + def process(parent, action, attrs) > + "" > end > end > end But this one doesn't add an "end", and Asciidoctor trips up badly. > + # The bugreport macro does nothing as far as rendering is > + # concerned -- we just grep for it in the sources. > + inline_macro Git::Documentation::BugReportProcessor, :bugreport (I never much liked this copy-paste comment then, and I still don't, to be honest.) Martin
On Fri, 24 Jan 2020 at 04:35, <emilyshaffer@google.com> wrote: > Add a new step to the build to generate a safelist of git-config > variables which are appropriate to include in the output of > git-bugreport. New variables can be added to the safelist by annotating > their documentation in Documentation/config with the > "bugreport" macro, which is recognized by AsciiDoc and > AsciiDoctor. "which is recognized by" sounds like it's built-in. Maybe "with a new no-op 'bugreport' macro" or something like that. > -sendemail.signedoffcc (deprecated):: > +sendemail.signedoffcc (deprecated) bugreport:exclude[x] :: > Deprecated alias for `sendemail.signedoffbycc`. > > -sendemail.smtpBatchSize:: > +sendemail.smtpBatchSize bugreport:include[x] :: > Number of messages to be sent per connection, after that a relogin > will happen. If the value is 0 or undefined, send all messages in > one connection. > See also the `--batch-size` option of linkgit:git-send-email[1]. Do we really need to list includes *and* excludes? I could see the point of adding an exclude here and there to signal that "this might look innocent enough, but trust me, we really need to exclude this" in order to avoid future commits to more or less accidentally over-include. Should we add some internal documentation and/or a remark in the commit message about this? As an example, is "sendemail.signedofcc" sensitive enough that we need to explicitly exclude it? If someone wants to come along and include it, they don't just need to argue for an inclusion, but also for lifting the exclusion. Hmm? Martin
On Fri, Jan 31, 2020 at 10:20:05PM +0100, Martin Ågren wrote: > On Fri, 24 Jan 2020 at 04:35, <emilyshaffer@google.com> wrote: > > Add a new step to the build to generate a safelist of git-config > > variables which are appropriate to include in the output of > > git-bugreport. New variables can be added to the safelist by annotating > > their documentation in Documentation/config with the > > "bugreport" macro, which is recognized by AsciiDoc and > > AsciiDoctor. > > "which is recognized by" sounds like it's built-in. Maybe "with a new > no-op 'bugreport' macro" or something like that. Sure, I'll wordsmith this a little. > > > -sendemail.signedoffcc (deprecated):: > > +sendemail.signedoffcc (deprecated) bugreport:exclude[x] :: > > Deprecated alias for `sendemail.signedoffbycc`. > > > > -sendemail.smtpBatchSize:: > > +sendemail.smtpBatchSize bugreport:include[x] :: > > Number of messages to be sent per connection, after that a relogin > > will happen. If the value is 0 or undefined, send all messages in > > one connection. > > See also the `--batch-size` option of linkgit:git-send-email[1]. > > Do we really need to list includes *and* excludes? Need? Nah. The exclude markers are not even acknowledged by the current implementation. > I could see the point of adding an exclude here and there to signal > that "this might look innocent enough, but trust me, we really need to > exclude this" in order to avoid future commits to more or less > accidentally over-include. This was more of my line of thinking, along with the thought that some folks may use bug reporting internally, where they have A) more control over which extensions/tools are in use, and B) less concern about sharing things like remote URLs, branch names, etc. and may want to go for a blocklist as opposed to a safelist, to make it easier for their support folks to diagnose. > Should we add some internal documentation and/or a remark in the commit > message about this? As an example, is "sendemail.signedofcc" sensitive > enough that we need to explicitly exclude it? If someone wants to come > along and include it, they don't just need to argue for an inclusion, > but also for lifting the exclusion. Hmm? Yeah, I think this is a good point. I'll try to figure out how to reword the commit message, and take another look at the sample include/exclude change I made to see where I can omit entirely. - Emily
On Thu, Jan 30, 2020 at 11:34:24PM +0100, Martin Ågren wrote: > On Fri, 24 Jan 2020 at 04:35, <emilyshaffer@google.com> wrote: > > Add a new step to the build to generate a safelist of git-config > > variables which are appropriate to include in the output of > > git-bugreport. New variables can be added to the safelist by annotating > > their documentation in Documentation/config with the > > "bugreport" macro, which is recognized by AsciiDoc and > > AsciiDoctor. > > > > Some configs are private in nature, and can contain remote URLs, > > passwords, or other sensitive information. In the event that a user > > doesn't notice their information while reviewing a bugreport, that user > > may leak their credentials to other individuals, mailing lists, or bug > > tracking tools inadvertently. Heuristic blocklisting of configuration > > keys is imperfect and prone to false negatives; given the nature of the > > information which can be leaked, a safelist is more reliable. > > I sort of wonder whether safelist/blocklist is to prefer over > whitelist/blacklist, or if it's the other way round. The former are new > to me, whereas the latter are the terms I would have used. But that's > just me, of course. I was a little surprised, that's all. Eh. I think the following things are true: - Whitelist/blacklist has a "smell" of discrimination, whether that's the true etymology or not. - Those with experience in the field can easily understand what whitelist or blacklist means. - Safelist/blocklist do not "smell" the same way. - It is easy to tell what "safelist" means: "a list of stuff which is safe." No experience needed. So, while it's new, I think it's not harmful. I see only a no-op or positive impact from using this term instead of whitelist/blacklist. Computer science seems to have quite a few terms which fall into this long-standing but potentially negative area, so I don't mind looking for alternatives where it's harmless to do so. > > > Implement a new no-op "bugreport" macro for use as > > "bugreport:include[x]" to annotate the config keys that should be > > included in the automatically generated safelist. Use "exclude" for the > > others. > > > > With Asciidoctor, it's ok to say "bugreport:include[]", but AsciiDoc > > seems to want something between the brackets. A bit unfortunate, but > > not a huge problem -- we'll just provide an "x". > > I recognize this reasoning :-) and I'm not terribly opposed to it, but > after some nights' sleeping on this, I have to wonder if > "annotate:bugreport[include]" wouldn't be better than "bugreport[x]" > with that ugly "x". Maybe this isn't the biggest problem, but if we > expect this macro to eventually sit right next to ~90% of all our config > variables... Hm. I wanted to say, "Ok, but I don't know how to do that, so can you help?" But I think that's all the more reason that I should do it ;) Ok. I will try and change it to annotate:bugreport[include] like you suggested, and hopefully learn more about asciidoc macros :) > > > "doc-diff" reports that this macro doesn't render at all. That is, > > these are both empty after this commit: > > > > cd Documentation > > ./doc-diff --asciidoctor :/"bugreport: add tool" HEAD > > ./doc-diff --asciidoc :/"bugreport: add tool" HEAD > > That was true in [1], but alas, no more. In that patch, it's sort of > obvious from the diff how it adds a "class" which "end"s. > > [1] https://lore.kernel.org/git/20190817203846.31609-1-martin.agren@gmail.com/ > > > --- a/Documentation/asciidoctor-extensions.rb > > +++ b/Documentation/asciidoctor-extensions.rb > > @@ -37,6 +37,10 @@ module Git > > output = output.sub(/<\/refmeta>/, new_tags + "</refmeta>") > > end > > output > > + > > + class BugReportProcessor < Asciidoctor::Extensions::InlineMacroProcessor > > + def process(parent, action, attrs) > > + "" > > end > > end > > end > > But this one doesn't add an "end", and Asciidoctor trips up badly. Ok, I'll have a look. I'm sure I copied something badly. > > > + # The bugreport macro does nothing as far as rendering is > > + # concerned -- we just grep for it in the sources. > > + inline_macro Git::Documentation::BugReportProcessor, :bugreport > > (I never much liked this copy-paste comment then, and I still don't, to > be honest.) I'll see if I can find a reasonable alternative (or remove it). - Emily
On Tue, Feb 04, 2020 at 04:30:43PM -0800, Emily Shaffer wrote: > Yeah, I think this is a good point. I'll try to figure out how to reword > the commit message, and take another look at the sample include/exclude > change I made to see where I can omit entirely. Eh. I'm going to try and reword the whole commit message. I combined your scissors patch, plus something from Dscho's scissors patch, plus my original commit message, and now it's huge and hard to follow. - Emily
On Wed, 5 Feb 2020 at 01:45, Emily Shaffer <emilyshaffer@google.com> wrote: > > > I sort of wonder whether safelist/blocklist is to prefer over > > whitelist/blacklist, or if it's the other way round. The former are new > > to me, whereas the latter are the terms I would have used. But that's > > just me, of course. I was a little surprised, that's all. > > Eh. I think the following things are true: > > - Whitelist/blacklist has a "smell" of discrimination, whether that's > the true etymology or not. > - Those with experience in the field can easily understand what > whitelist or blacklist means. > - Safelist/blocklist do not "smell" the same way. > - It is easy to tell what "safelist" means: "a list of stuff which is > safe." No experience needed. > > So, while it's new, I think it's not harmful. I see only a no-op or > positive impact from using this term instead of whitelist/blacklist. > Computer science seems to have quite a few terms which fall into this > long-standing but potentially negative area, so I don't mind looking for > alternatives where it's harmless to do so. Ok, that all makes sense. Thanks Martin
diff --git a/.gitignore b/.gitignore index d89bf9e11e..bd2f49b996 100644 --- a/.gitignore +++ b/.gitignore @@ -192,6 +192,7 @@ /gitweb/static/gitweb.min.* /config-list.h /command-list.h +/bugreport-config-safelist.h *.tar.gz *.dsc *.deb diff --git a/Documentation/asciidoc.conf b/Documentation/asciidoc.conf index 8fc4b67081..5d5359fcf9 100644 --- a/Documentation/asciidoc.conf +++ b/Documentation/asciidoc.conf @@ -6,9 +6,13 @@ # # Show Git link as: <command>(<section>); if section is defined, else just show # the command. +# +# The bugreport macro does nothing as far as rendering is +# concerned -- we just grep for it in the sources. [macros] (?su)[\\]?(?P<name>linkgit):(?P<target>\S*?)\[(?P<attrlist>.*?)\]= +(?su)[\\]?(?P<name>bugreport):(?P<action>\S*?)\[(?P<attrlist>.*?)\]= [attributes] asterisk=* @@ -28,6 +32,8 @@ ifdef::backend-docbook[] {0#<citerefentry>} {0#<refentrytitle>{target}</refentrytitle><manvolnum>{0}</manvolnum>} {0#</citerefentry>} +[bugreport-inlinemacro] +{0#} endif::backend-docbook[] ifdef::backend-docbook[] @@ -94,4 +100,6 @@ ifdef::backend-xhtml11[] git-relative-html-prefix= [linkgit-inlinemacro] <a href="{git-relative-html-prefix}{target}.html">{target}{0?({0})}</a> +[bugreport-inlinemacro] +<!-- --> endif::backend-xhtml11[] diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb index d906a00803..750bdff9af 100644 --- a/Documentation/asciidoctor-extensions.rb +++ b/Documentation/asciidoctor-extensions.rb @@ -37,6 +37,10 @@ module Git output = output.sub(/<\/refmeta>/, new_tags + "</refmeta>") end output + + class BugReportProcessor < Asciidoctor::Extensions::InlineMacroProcessor + def process(parent, action, attrs) + "" end end end @@ -45,4 +49,7 @@ end Asciidoctor::Extensions.register do inline_macro Git::Documentation::LinkGitProcessor, :linkgit postprocessor Git::Documentation::DocumentPostProcessor + # The bugreport macro does nothing as far as rendering is + # concerned -- we just grep for it in the sources. + inline_macro Git::Documentation::BugReportProcessor, :bugreport end diff --git a/Documentation/config/sendemail.txt b/Documentation/config/sendemail.txt index 0006faf800..92f5082013 100644 --- a/Documentation/config/sendemail.txt +++ b/Documentation/config/sendemail.txt @@ -1,63 +1,63 @@ -sendemail.identity:: +sendemail.identity bugreport:exclude[x] :: A configuration identity. When given, causes values in the 'sendemail.<identity>' subsection to take precedence over values in the 'sendemail' section. The default identity is the value of `sendemail.identity`. -sendemail.smtpEncryption:: +sendemail.smtpEncryption bugreport:include[x] :: See linkgit:git-send-email[1] for description. Note that this setting is not subject to the 'identity' mechanism. -sendemail.smtpssl (deprecated):: +sendemail.smtpssl (deprecated) bugreport:exclude[x] :: Deprecated alias for 'sendemail.smtpEncryption = ssl'. -sendemail.smtpsslcertpath:: +sendemail.smtpsslcertpath bugreport:exclude[x] :: Path to ca-certificates (either a directory or a single file). Set it to an empty string to disable certificate verification. -sendemail.<identity>.*:: +sendemail.<identity>.* bugreport:exclude[x] :: Identity-specific versions of the 'sendemail.*' parameters found below, taking precedence over those when this identity is selected, through either the command-line or `sendemail.identity`. -sendemail.aliasesFile:: -sendemail.aliasFileType:: -sendemail.annotate:: -sendemail.bcc:: -sendemail.cc:: -sendemail.ccCmd:: -sendemail.chainReplyTo:: -sendemail.confirm:: -sendemail.envelopeSender:: -sendemail.from:: -sendemail.multiEdit:: -sendemail.signedoffbycc:: -sendemail.smtpPass:: -sendemail.suppresscc:: -sendemail.suppressFrom:: -sendemail.to:: -sendemail.tocmd:: -sendemail.smtpDomain:: -sendemail.smtpServer:: -sendemail.smtpServerPort:: -sendemail.smtpServerOption:: -sendemail.smtpUser:: -sendemail.thread:: -sendemail.transferEncoding:: -sendemail.validate:: -sendemail.xmailer:: +sendemail.aliasesFile bugreport:exclude[x] :: +sendemail.aliasFileType bugreport:exclude[x] :: +sendemail.annotate bugreport:include[x] :: +sendemail.bcc bugreport:include[x] :: +sendemail.cc bugreport:include[x] :: +sendemail.ccCmd bugreport:include[x] :: +sendemail.chainReplyTo bugreport:include[x] :: +sendemail.confirm bugreport:include[x] :: +sendemail.envelopeSender bugreport:include[x] :: +sendemail.from bugreport:include[x] :: +sendemail.multiEdit bugreport:include[x] :: +sendemail.signedoffbycc bugreport:include[x] :: +sendemail.smtpPass bugreport:exclude[x] :: +sendemail.suppresscc bugreport:include[x] :: +sendemail.suppressFrom bugreport:include[x] :: +sendemail.to bugreport:include[x] :: +sendemail.tocmd bugreport:include[x] :: +sendemail.smtpDomain bugreport:include[x] :: +sendemail.smtpServer bugreport:include[x] :: +sendemail.smtpServerPort bugreport:include[x] :: +sendemail.smtpServerOption bugreport:include[x] :: +sendemail.smtpUser bugreport:exclude[x] :: +sendemail.thread bugreport:include[x] :: +sendemail.transferEncoding bugreport:include[x] :: +sendemail.validate bugreport:include[x] :: +sendemail.xmailer bugreport:include[x] :: See linkgit:git-send-email[1] for description. -sendemail.signedoffcc (deprecated):: +sendemail.signedoffcc (deprecated) bugreport:exclude[x] :: Deprecated alias for `sendemail.signedoffbycc`. -sendemail.smtpBatchSize:: +sendemail.smtpBatchSize bugreport:include[x] :: Number of messages to be sent per connection, after that a relogin will happen. If the value is 0 or undefined, send all messages in one connection. See also the `--batch-size` option of linkgit:git-send-email[1]. -sendemail.smtpReloginDelay:: +sendemail.smtpReloginDelay bugreport:include[x] :: Seconds wait before reconnecting to smtp server. See also the `--relogin-delay` option of linkgit:git-send-email[1]. diff --git a/Makefile b/Makefile index a01a050aa3..2bc9f112ea 100644 --- a/Makefile +++ b/Makefile @@ -817,6 +817,7 @@ VCSSVN_LIB = vcs-svn/lib.a GENERATED_H += config-list.h GENERATED_H += command-list.h +GENERATED_H += bugreport-config-safelist.h LIB_H := $(sort $(patsubst ./%,%,$(shell git ls-files '*.h' ':!t/' ':!Documentation/' 2>/dev/null || \ $(FIND) . \ @@ -2163,6 +2164,12 @@ command-list.h: $(wildcard Documentation/git*.txt) Documentation/*config.txt Doc $(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \ command-list.txt >$@+ && mv $@+ $@ +bugreport-config-safelist.h: generate-bugreport-config-safelist.sh + +bugreport-config-safelist.h: Documentation/config/*.txt + $(QUIET_GEN)$(SHELL_PATH) ./generate-bugreport-config-safelist.sh \ + >$@+ && mv $@+ $@ + SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\ $(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\ $(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\ diff --git a/generate-bugreport-config-safelist.sh b/generate-bugreport-config-safelist.sh new file mode 100755 index 0000000000..44612d5538 --- /dev/null +++ b/generate-bugreport-config-safelist.sh @@ -0,0 +1,17 @@ +#!/bin/sh + +cat <<EOF +/* Automatically generated by bugreport-generate-config-safelist.sh */ + + +static const char *bugreport_config_safelist[] = { +EOF + +# cat all regular files in Documentation/config +find Documentation/config -type f -exec cat {} \; | +# print the command name which matches the bugreport-include macro +sed -n 's/^\(.*\) \+bugreport:include.* ::$/ "\1",/p' | sort + +cat <<EOF +}; +EOF