| Message ID | cover.1639509048.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Conditional config includes based on remote URL | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > Thanks, everyone, for your comments. I've followed Glen's code > suggestion and Junio's documentation suggestion, as you can see in the > range-diff. > > Jonathan Tan (2): > config: make git_config_include() static > config: include file if remote URL matches a glob > > Documentation/config.txt | 27 ++++++++ > config.c | 132 ++++++++++++++++++++++++++++++++++++--- > config.h | 46 ++++---------- > t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++ > 4 files changed, 282 insertions(+), 41 deletions(-) > > Range-diff against v6: > 1: b2dcae03ed = 1: b2dcae03ed config: make git_config_include() static > 2: de2be06818 ! 2: 7c70089074 config: include file if remote URL matches a glob > @@ Documentation/config.txt: all branches that begin with `foo/`. This is useful if > +Files included by this option (directly or indirectly) are not allowed > +to contain remote URLs. > ++ > -+This keyword is designed to be forwards compatible with a naming > -+scheme that supports more variable-based include conditions, but > -+currently Git only supports the exact keyword described above. > ++Note that unlike other includeIf conditions, resolving this condition > ++relies on information that is not yet known at the point of reading the > ++condition. A typical use case is this option being present as a > ++system-level or global-level config, and the remote URL being in a > ++local-level config; hence the need to scan ahead when resolving this > ++condition. In order to avoid the chicken-and-egg problem in which > ++potentially-included files can affect whether such files are potentially > ++included, Git breaks the cycle by prohibiting these files from affecting > ++the resolution of these conditions (thus, prohibiting them from > ++declaring remote URLs). > +++ > ++As for the naming of this keyword, it is for forwards compatibiliy with > ++a naming scheme that supports more variable-based include conditions, > ++but currently Git only supports the exact keyword described above. > + > A few more notes on matching via `gitdir` and `gitdir/i`: > > @@ config.c: static int git_config_include(const char *var, const char *value, void > + if (inc->opts->unconditional_remote_url) > + inc->fn = forbid_remote_url; > ret = handle_path_include(value, inc); > -+ if (inc->opts->unconditional_remote_url) > -+ inc->fn = old_fn; > ++ inc->fn = old_fn; > + } > > return ret; > -- > 2.34.1.173.g76aa8bc2d0-goog The implementation looks good, and I think that the precedent we are setting with "hasconfig:" is pretty well captured on this thread. This looks good to me, though I'm not an expert in this area, so it would be good for others to chime in. Reviewed-by: Glen Choo <chooglen@google.com>
On Wed, Dec 15, 2021 at 7:25 AM Jonathan Tan <jonathantanmy@google.com> wrote: > > Thanks, everyone, for your comments. I've followed Glen's code > suggestion and Junio's documentation suggestion, as you can see in the > range-diff. So, the basic idea is, in a setting like Google's, you can have users install additional files on their system out-of-band, and have the users specify a simple line in their configuration to make use of those additional files -- or portions thereof. It's a way of easily providing potentially large blocks of pre-vetted configuration for users. Seems to make sense. (and I've read over the code lightly, so feel free to take this as an Acked-by.) But can I back up and comment on a bigger picture item? This mechanism requires somehow getting additional files to the user separately; projects that span companies (git.git, linux.git, etc.) won't likely be able to make use of this. Scalar also has a mechanism for providing potentially large blocks of pre-vetted configuration for users. It does so as part of a new top-level command. And it does so with a very opinionated set of values that are not configurable. Thus, while I'd like to use it, they use a configuration option that would break things badly at my $DAYJOB. (Too many gradle plugins using jgit, which doesn't understand index.version=4 and will blow up with a very suboptimal error message when they see it.) And, it's very specific to scalar; we probably don't want to add a new toplevel command everytime someone wants common configuration to be easily grabbed by some user. It would be nice if we could find some more generic solution. Granted, I can't think of any, and I don't think this comment should block this particular series (nor the scalar one), but I am worrying a little bit that we're getting multiple completely different solutions for the same general problem, and each brings caveats big enough to preclude many (most?) potential users. I don't know what to do about that, especially since configuration that is too easy to propagate comes with big security problems, but I wanted to at least raise the issue and hope others have good ideas. If nothing else, I want to raise awareness to avoid proliferation of similar pre-vetted-configuration-deployment mechanisms. I'm CC'ing a couple scalar folks as well for that point.
Elijah Newren <newren@gmail.com> writes: > But can I back up and comment on a bigger picture item? > > This mechanism requires somehow getting additional files to the user > separately; projects that span companies (git.git, linux.git, etc.) > won't likely be able to make use of this. > > Scalar also has a mechanism for providing potentially large blocks of > pre-vetted configuration for users. It does so as part of a new > top-level command. And it does so with a very opinionated set of > values that are not configurable. Thus, while I'd like to use it, > they use a configuration option that would break things badly at my > $DAYJOB. (Too many gradle plugins using jgit, which doesn't > understand index.version=4 and will blow up with a very suboptimal > error message when they see it.) And, it's very specific to scalar; > we probably don't want to add a new toplevel command everytime someone > wants common configuration to be easily grabbed by some user. > > It would be nice if we could find some more generic solution. > Granted, I can't think of any, and I don't think this comment should > block this particular series (nor the scalar one), but I am worrying a > little bit that we're getting multiple completely different solutions > for the same general problem, and each brings caveats big enough to > preclude many (most?) potential users. I don't know what to do about > that, especially since configuration that is too easy to propagate > comes with big security problems, but I wanted to at least raise the > issue and hope others have good ideas. If nothing else, I want to > raise awareness to avoid proliferation of similar > pre-vetted-configuration-deployment mechanisms. I'm CC'ing a couple > scalar folks as well for that point. Yes, that's an accurate description. To reiterate what Jonathan said in his first cover letter [1], the primary motivation is that we want to be able to 'suggest' hooks to users. There was an RFC for this 'remote-suggested hooks feature' (docs [2], RFC implementation [3]) but it ultimately stalled due to security concerns I believe (this was before I joined the team, so I'm not the most familiar with this). It might be worth re-reading those threads since they tread on pretty much the same ground of shipping pre-vetted config (this is directed at me too, since I haven't read through those in detail). I've also been told that we're (aka Google) still looking for feedback on [2], so feel free to share any thoughts on that thread too. [1] https://lore.kernel.org/git/cover.1634077795.git.jonathantanmy@google.com [2] https://lore.kernel.org/git/pull.908.v4.git.1620241892929.gitgitgadget@gmail.com/ [3] https://lore.kernel.org/git/cover.1623881977.git.jonathantanmy@google.com/
Elijah Newren <newren@gmail.com> writes: > On Wed, Dec 15, 2021 at 7:25 AM Jonathan Tan <jonathantanmy@google.com> wrote: > > > > Thanks, everyone, for your comments. I've followed Glen's code > > suggestion and Junio's documentation suggestion, as you can see in the > > range-diff. > > So, the basic idea is, in a setting like Google's, you can have users > install additional files on their system out-of-band, and have the > users specify a simple line in their configuration to make use of > those additional files -- or portions thereof. It's a way of easily > providing potentially large blocks of pre-vetted configuration for > users. > > Seems to make sense. (and I've read over the code lightly, so feel > free to take this as an Acked-by.) Thanks. > But can I back up and comment on a bigger picture item? > > This mechanism requires somehow getting additional files to the user > separately; projects that span companies (git.git, linux.git, etc.) > won't likely be able to make use of this. Yes, they would also need to use a separate mechanism in addition to Git. > Scalar also has a mechanism for providing potentially large blocks of > pre-vetted configuration for users. It does so as part of a new > top-level command. And it does so with a very opinionated set of > values that are not configurable. Thus, while I'd like to use it, > they use a configuration option that would break things badly at my > $DAYJOB. (Too many gradle plugins using jgit, which doesn't > understand index.version=4 and will blow up with a very suboptimal > error message when they see it.) And, it's very specific to scalar; > we probably don't want to add a new toplevel command everytime someone > wants common configuration to be easily grabbed by some user. Do you have more information on this? The closest thing I've seen is "Scalar Config" under "Modifying Configuration Values" in [1], which seems to be more about bundling additional tools (which may change config, of course). Unless you're referring to the config bundled in the Scalar tool itself, in which case this patch set seems orthogonal and potentially complementary - I was envisioning config being provided by a package manager package, but Scalar could provide some too for users to use at their own discretion. [1] https://github.com/microsoft/git/blob/7a514b4c2d5df7fdd2f66f048010d8ddcb412d0b/contrib/scalar/docs/troubleshooting.md > It would be nice if we could find some more generic solution. > Granted, I can't think of any, and I don't think this comment should > block this particular series (nor the scalar one), but I am worrying a > little bit that we're getting multiple completely different solutions > for the same general problem, and each brings caveats big enough to > preclude many (most?) potential users. I don't know what to do about > that, especially since configuration that is too easy to propagate > comes with big security problems, but I wanted to at least raise the > issue and hope others have good ideas. If nothing else, I want to > raise awareness to avoid proliferation of similar > pre-vetted-configuration-deployment mechanisms. I'm CC'ing a couple > scalar folks as well for that point. That's a good point. As Glen said [2], it seems like transmitting config itself (or, at least, hooks) through Git is something that we (the Git project) don't want to do, so I have been working from the basis that Git should just make use of config/hooks delivered through a non-Git mechanism, and not deliver the config/hooks itself. [2] https://lore.kernel.org/git/kl6lee5w5nng.fsf@chooglen-macbookpro.roam.corp.google.com/
On Mon, Jan 10, 2022 at 11:22 AM Jonathan Tan <jonathantanmy@google.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > On Wed, Dec 15, 2021 at 7:25 AM Jonathan Tan <jonathantanmy@google.com> wrote: > > > > > > Thanks, everyone, for your comments. I've followed Glen's code > > > suggestion and Junio's documentation suggestion, as you can see in the > > > range-diff. > > > > So, the basic idea is, in a setting like Google's, you can have users > > install additional files on their system out-of-band, and have the > > users specify a simple line in their configuration to make use of > > those additional files -- or portions thereof. It's a way of easily > > providing potentially large blocks of pre-vetted configuration for > > users. > > > > Seems to make sense. (and I've read over the code lightly, so feel > > free to take this as an Acked-by.) > > Thanks. > > > But can I back up and comment on a bigger picture item? > > > > This mechanism requires somehow getting additional files to the user > > separately; projects that span companies (git.git, linux.git, etc.) > > won't likely be able to make use of this. > > Yes, they would also need to use a separate mechanism in addition to > Git. > > > Scalar also has a mechanism for providing potentially large blocks of > > pre-vetted configuration for users. It does so as part of a new > > top-level command. And it does so with a very opinionated set of > > values that are not configurable. Thus, while I'd like to use it, > > they use a configuration option that would break things badly at my > > $DAYJOB. (Too many gradle plugins using jgit, which doesn't > > understand index.version=4 and will blow up with a very suboptimal > > error message when they see it.) And, it's very specific to scalar; > > we probably don't want to add a new toplevel command everytime someone > > wants common configuration to be easily grabbed by some user. > > Do you have more information on this? The closest thing I've seen is > "Scalar Config" under "Modifying Configuration Values" in [1], which > seems to be more about bundling additional tools (which may change > config, of course). > > Unless you're referring to the config bundled in the Scalar tool itself, > in which case this patch set seems orthogonal and potentially > complementary - I was envisioning config being provided by a package > manager package, but Scalar could provide some too for users to use at > their own discretion. > > [1] https://github.com/microsoft/git/blob/7a514b4c2d5df7fdd2f66f048010d8ddcb412d0b/contrib/scalar/docs/troubleshooting.md Yes, I was referring to the config hardcoded in the Scalar tool itself (see set_recommended_config() in https://lore.kernel.org/git/4439ab4de0bc3f48a6bdcf4b5165b16fad792ebd.1638538470.git.gitgitgadget@gmail.com/). I agree they are different solutions to "help others setup config in a pre-vetted way", that the two don't seem to conflict, and one can't be implemented in terms of the other. It might even be possible for someone somewhere to simultaneously take advantage of both (not sure if anyone would try, but I don't forsee problems in doing so, except in the narrow case that both schemes try to set the same config and there are worries about which one "wins", which might boil down to whether the include directive came first in the config file or the specific config value that scalar set). > > It would be nice if we could find some more generic solution. > > Granted, I can't think of any, and I don't think this comment should > > block this particular series (nor the scalar one), but I am worrying a > > little bit that we're getting multiple completely different solutions > > for the same general problem, and each brings caveats big enough to > > preclude many (most?) potential users. I don't know what to do about > > that, especially since configuration that is too easy to propagate > > comes with big security problems, but I wanted to at least raise the > > issue and hope others have good ideas. If nothing else, I want to > > raise awareness to avoid proliferation of similar > > pre-vetted-configuration-deployment mechanisms. I'm CC'ing a couple > > scalar folks as well for that point. > > That's a good point. As Glen said [2], it seems like transmitting config > itself (or, at least, hooks) through Git is something that we (the Git > project) don't want to do, so I have been working from the basis that > Git should just make use of config/hooks delivered through a non-Git > mechanism, and not deliver the config/hooks itself. > > [2] https://lore.kernel.org/git/kl6lee5w5nng.fsf@chooglen-macbookpro.roam.corp.google.com/ Yeah, makes sense. And I don't know any better solutions. I guess all I'm really saying is that if a third narrowly targetted way to provide pre-vetted configuration shows up on the list, it may be time to ask folks to step back and try to find a more generic solution.
Hi Elijah, On Mon, 10 Jan 2022, Elijah Newren wrote: > On Mon, Jan 10, 2022 at 11:22 AM Jonathan Tan <jonathantanmy@google.com> wrote: > > > > Elijah Newren <newren@gmail.com> writes: > > > > > Scalar also has a mechanism for providing potentially large blocks > > > of pre-vetted configuration for users. It does so as part of a new > > > top-level command. And it does so with a very opinionated set of > > > values that are not configurable. Thus, while I'd like to use it, > > > they use a configuration option that would break things badly at my > > > $DAYJOB. (Too many gradle plugins using jgit, which doesn't > > > understand index.version=4 and will blow up with a very suboptimal > > > error message when they see it.) And, it's very specific to scalar; > > > we probably don't want to add a new toplevel command everytime > > > someone wants common configuration to be easily grabbed by some > > > user. > > > > Do you have more information on this? The closest thing I've seen is > > "Scalar Config" under "Modifying Configuration Values" in [1], which > > seems to be more about bundling additional tools (which may change > > config, of course). > > > > Unless you're referring to the config bundled in the Scalar tool itself, > > in which case this patch set seems orthogonal and potentially > > complementary - I was envisioning config being provided by a package > > manager package, but Scalar could provide some too for users to use at > > their own discretion. > > > > [1] https://github.com/microsoft/git/blob/7a514b4c2d5df7fdd2f66f048010d8ddcb412d0b/contrib/scalar/docs/troubleshooting.md > > Yes, I was referring to the config hardcoded in the Scalar tool itself > (see set_recommended_config() in > https://lore.kernel.org/git/4439ab4de0bc3f48a6bdcf4b5165b16fad792ebd.1638538470.git.gitgitgadget@gmail.com/). I was kind of thinking that such problems might be solved via introducing e.g. `scalar.ensureJGitCompatibility = true` (which should be a relatively trivial patch to write). What do you think? Ciao, Dscho
Thanks, everyone, for your comments. I've followed Glen's code suggestion and Junio's documentation suggestion, as you can see in the range-diff. Jonathan Tan (2): config: make git_config_include() static config: include file if remote URL matches a glob Documentation/config.txt | 27 ++++++++ config.c | 132 ++++++++++++++++++++++++++++++++++++--- config.h | 46 ++++---------- t/t1300-config.sh | 118 ++++++++++++++++++++++++++++++++++ 4 files changed, 282 insertions(+), 41 deletions(-) Range-diff against v6: 1: b2dcae03ed = 1: b2dcae03ed config: make git_config_include() static 2: de2be06818 ! 2: 7c70089074 config: include file if remote URL matches a glob @@ Documentation/config.txt: all branches that begin with `foo/`. This is useful if +Files included by this option (directly or indirectly) are not allowed +to contain remote URLs. ++ -+This keyword is designed to be forwards compatible with a naming -+scheme that supports more variable-based include conditions, but -+currently Git only supports the exact keyword described above. ++Note that unlike other includeIf conditions, resolving this condition ++relies on information that is not yet known at the point of reading the ++condition. A typical use case is this option being present as a ++system-level or global-level config, and the remote URL being in a ++local-level config; hence the need to scan ahead when resolving this ++condition. In order to avoid the chicken-and-egg problem in which ++potentially-included files can affect whether such files are potentially ++included, Git breaks the cycle by prohibiting these files from affecting ++the resolution of these conditions (thus, prohibiting them from ++declaring remote URLs). +++ ++As for the naming of this keyword, it is for forwards compatibiliy with ++a naming scheme that supports more variable-based include conditions, ++but currently Git only supports the exact keyword described above. + A few more notes on matching via `gitdir` and `gitdir/i`: @@ config.c: static int git_config_include(const char *var, const char *value, void + if (inc->opts->unconditional_remote_url) + inc->fn = forbid_remote_url; ret = handle_path_include(value, inc); -+ if (inc->opts->unconditional_remote_url) -+ inc->fn = old_fn; ++ inc->fn = old_fn; + } return ret;