| Message ID | 20210411125431.28971-3-sir@cmpwn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-send-email: improve SSL configuration | expand |
On Sun, Apr 11 2021, Drew DeVault wrote: > Signed-off-by: Drew DeVault <sir@cmpwn.com> > --- > Documentation/git-send-email.txt | 4 ++-- > git-send-email.perl | 3 +++ > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt > index c17c3b400a..520b355e50 100644 > --- a/Documentation/git-send-email.txt > +++ b/Documentation/git-send-email.txt > @@ -171,8 +171,8 @@ Sending > Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables > generic SSL/TLS support and is typically used on port 465. 'tls' > enables in-band STARTTLS support and is typically used on port 25 or > - 587. Use whichever option is recommended by your mail provider. Any > - other value reverts to plain SMTP. Default is the value of > + 587. Use whichever option is recommended by your mail provider. Leave > + empty to disable encryption and use plain SMTP. Default is the value of > `sendemail.smtpEncryption`. > > --smtp-domain=<FQDN>:: > diff --git a/git-send-email.perl b/git-send-email.perl > index f5bbf1647e..bda5211f0d 100755 > --- a/git-send-email.perl > +++ b/git-send-email.perl > @@ -495,6 +495,9 @@ sub read_config { > > # 'default' encryption is none -- this only prevents a warning > $smtp_encryption = '' unless (defined $smtp_encryption); > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") { > + die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n"); > +} Having not tested this but just eyeballed the code, I'm fairly sure you're adding a logic error here, or is $smtp_encryption guaranteed to be defined at this point? We start with it as undef, then read the config, then the CLI options. If we've got neither it'll still be undef here, no? Thus the string comparison will emit a warning. Maybe I'm overly used to regexen in Perl, but I'd also find this more readable as: $smtp_encryption !~ /^(?:|ssl|tls)$/s Or something like: my @valid_smtp_encryption = ('', qw(ssl tls)); if (!grep { $_ eq $smtp_encryption } @valid_smtp_encryption) { .... }
On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote: > > # 'default' encryption is none -- this only prevents a warning > > $smtp_encryption = '' unless (defined $smtp_encryption); > > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") { > > + die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n"); > > +} > > Having not tested this but just eyeballed the code, I'm fairly sure > you're adding a logic error here, or is $smtp_encryption guaranteed to > be defined at this point? I will admit to being ignorant of much of Perl's semantics, but I had assumed that the line prior to my additions addresses this: $smtp_encryption = '' unless (defined $smtp_encryption); > $smtp_encryption !~ /^(?:|ssl|tls)$/s Yeah, that would probably be better.
On Sun, Apr 11 2021, Drew DeVault wrote: > On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote: >> > # 'default' encryption is none -- this only prevents a warning >> > $smtp_encryption = '' unless (defined $smtp_encryption); >> > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") { >> > + die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n"); >> > +} >> >> Having not tested this but just eyeballed the code, I'm fairly sure >> you're adding a logic error here, or is $smtp_encryption guaranteed to >> be defined at this point? > > I will admit to being ignorant of much of Perl's semantics, but I had > assumed that the line prior to my additions addresses this: > > $smtp_encryption = '' unless (defined $smtp_encryption); You're right, I just misread the diff. Nevermind.
On Sun, Apr 11 2021, Ævar Arnfjörð Bjarmason wrote: > On Sun, Apr 11 2021, Drew DeVault wrote: > >> On Sun Apr 11, 2021 at 10:20 AM EDT, Ævar Arnfjörð Bjarmason wrote: >>> > # 'default' encryption is none -- this only prevents a warning >>> > $smtp_encryption = '' unless (defined $smtp_encryption); >>> > +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") { >>> > + die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n"); >>> > +} >>> >>> Having not tested this but just eyeballed the code, I'm fairly sure >>> you're adding a logic error here, or is $smtp_encryption guaranteed to >>> be defined at this point? >> >> I will admit to being ignorant of much of Perl's semantics, but I had >> assumed that the line prior to my additions addresses this: >> >> $smtp_encryption = '' unless (defined $smtp_encryption); > > You're right, I just misread the diff. Nevermind. So on a second reading. So first, I've been sitting on some fairly extensive send-email patches locally, but have been trying to focus on re-rolling some of my outstanding stuff. But I just sent two patches directly relevant to this series as https://lore.kernel.org/git/cover-0.2-00000000000-20210411T144128Z-avarab@gmail.com/ Something felt a bit wrong about the approach in your series, I wasn't quite sure what initially, but here it is; So, the only reason we have that "encryption is none -- this only prevents a warning" so late in the file (as opposed to setting it to '' when we declare the variable) was because of the smtp.{smtpssl,smtpencryption} interaction, i.e. we relied on it being undef to see if we needed to parse the secondary variable. But with it gone with my small series (it already didn't work) we can get rid of that special case. But, on the specifics of the "felt funny": 1. Your 2/3 changes a long standing existing "any other value = no encryption" to "die on unrecognized". I happen to think this is probably a good idea, but let's be explicit in the commit message, e.g.: We don't think it's a good idea to silently degrade to non-encrypted as we've been promising just because your version doesn't support something, let's die instead. 2. If we're breaking the "any other value" we should not be documenting the "or nothing", the distinction between "" and undef on the Perl-level was just a leaky implementation detail. But let's not conflate that with how we present something to the user. It's not the same to not set a variable v.s. setting it to the empty string. With my 2-part series it's even more trivial to detect that, but even on top of master you just move your check above the "set to empty unless defined". 3. While I'm very much leaning to #1 being a good idea, I'm very much leaning towards introducing this "starttls" alias being a bad idea for the same reason. I.e. let's not create a new 'starttls' if we can avoid it explicitly because we used to have the long-standing "anything unrecognized is empty == no encryption" behavior. A lot of users read documentation for the latest version online, but may have an older version installed. To the extent that anyone cares about the transport security of git-send-email (I'm kind of "meh" on it, but if we're making sendemail.smtpEncryption parsing strict you probably disagree), then such silent downgrading seems worse than just not accepting starttls at all. I.e. to have a new behavior of something like: if (defined $smtp_encryption) { die "we call 'starttls' 'tls' for historical reasons, sorry!" if $smtp_encryption eq 'starttls'; die "unknown mode '$smtp_encryption'" unless $smtp_encryption =~ /^(?:ssl|tls)$/s; } else { $smtp_encryption = ''; } I.e. I get that it's confusing, but isn't it enough to address the TLS v.s. STARTTLS confusion in the docs, as opposed to supporting it in the config format, which as noted will silently downgrade on older versions?
On Sun Apr 11, 2021 at 11:06 AM EDT, Ævar Arnfjörð Bjarmason wrote: > 3. While I'm very much leaning to #1 being a good idea, I'm very much > leaning towards introducing this "starttls" alias being a bad idea > for the same reason. > > i.e. let's not create a new 'starttls' if we can avoid it explicitly > because we used to have the long-standing "anything unrecognized is > empty == no encryption" behavior. > > A lot of users read documentation for the latest version online, but > may have an older version installed. I feel quite strongly that the options here are a grave failure of usability, and that it needs to be corrected. I help people troubleshoot git send-email problems quite often, and this is a recurring error. However, you make a good point in that someone might see some online documentation which does not match their git version and end up with a surprisingly unencrypted connection. As a compromise, let's consider making this a gradual change. We can start by clarifying the docs and forbiding the use of any value other than 'ssl' or 'tls'. If an unknown value is set, the user is not getting the encryption they expected anyway, and this should cause an error. Then we can leave the issue aside for some agreed upon period of time to allow the change to proliferate in the ecosystem, and then revisit this at some point in the future to rename the options to make more sense. Does this seem like a reasonable compromise?
On Sun, Apr 11 2021, Drew DeVault wrote: > On Sun Apr 11, 2021 at 11:06 AM EDT, Ævar Arnfjörð Bjarmason wrote: >> 3. While I'm very much leaning to #1 being a good idea, I'm very much >> leaning towards introducing this "starttls" alias being a bad idea >> for the same reason. >> >> i.e. let's not create a new 'starttls' if we can avoid it explicitly >> because we used to have the long-standing "anything unrecognized is >> empty == no encryption" behavior. >> >> A lot of users read documentation for the latest version online, but >> may have an older version installed. > > I feel quite strongly that the options here are a grave failure of > usability, and that it needs to be corrected. I help people troubleshoot > git send-email problems quite often, and this is a recurring error. > However, you make a good point in that someone might see some online > documentation which does not match their git version and end up with a > surprisingly unencrypted connection. > > As a compromise, let's consider making this a gradual change. We can > start by clarifying the docs and forbiding the use of any value other > than 'ssl' or 'tls'. If an unknown value is set, the user is not getting > the encryption they expected anyway, and this should cause an error. > > Then we can leave the issue aside for some agreed upon period of time to > allow the change to proliferate in the ecosystem, and then revisit this > at some point in the future to rename the options to make more sense. > > Does this seem like a reasonable compromise? I suggest we don't compromise and just go with whatever you're OK with :) I really don't care enough about #1 and #3 in my E-Mail to in any way push for it, sorry if it came off that way. I just wanted to check your assumptions when reviewing the series. I do think that it would make sense to more prominently note something to the effect of "this was documented to do X all along, now we do Y, but that's OK because ABC", and to note why the new starttls = plaintext on older versions is OK, maybe it's just fine. I really don't know. Isn't it pretty common in any case that SMTP servers in the wild just refuse plaintext these days when dealing with auth'd connections? I don't know. I do think it makes sense to fixup for my suggested #2, i.e. not leaking the internal detail of the "empty string".
On Sun Apr 11, 2021 at 3:56 PM EDT, Ævar Arnfjörð Bjarmason wrote:
> I suggest we don't compromise and just go with whatever you're OK with :)
Well, if you're giving me an opportunity to not drag this out into a
multi-phase rollout, then I'll take it :)
Another option is to forbid an unknown value (which is almost certainly
(1) wrong and (2) causing users to unexpectedly use plaintext when they
expected encryption), file a CVE, and pitch it as a security fix - then
we can expect a reasonably quick rollout of the change to the ecosystem
at large.
On Mon, Apr 12 2021, Drew DeVault wrote: > On Sun Apr 11, 2021 at 3:56 PM EDT, Ævar Arnfjörð Bjarmason wrote: >> I suggest we don't compromise and just go with whatever you're OK with :) > > Well, if you're giving me an opportunity to not drag this out into a > multi-phase rollout, then I'll take it :) Just to be clear even if I was insisting on that I'm still just one guy on the ML reviewing your patch. As a first approximation the opinion of regular contributors counts for more when the topic is some tricky interaction of code they wrote/are familiar with. In this case we're just discussing the general interaction of security, optional switches, software versioning and how SMTP servers in the wild work. I'd think someone who e.g. needs to regularly deal with SMTP servers in the wild would have a much better idea of those trade-offs than someone (like me) who happens to have some existing patches in git.git to git-send-email.perl. > Another option is to forbid an unknown value (which is almost certainly > (1) wrong and (2) causing users to unexpectedly use plaintext when they > expected encryption), file a CVE, and pitch it as a security fix - then > we can expect a reasonably quick rollout of the change to the ecosystem > at large. I think anyone would agree that in retrospect "unknown is plaintext" for the "what encryption do you want" option is at best a something approaching a shotgun to your foot of a UI pattern. But I think it falls far short of a CVE. We *do* prominently document it, a potential CVE would be if we had silent degration to plaintext (well, in a mode whose inherent workings aren't to be vulnerable to that attack, as STARTTLS is...). FWIW since my upthread <87zgy4egtp.fsf@evledraar.gmail.com> I tried sending mail through GMail's plain-text smtp gateway as an authenticated user. Testing with: nc smtp.gmail.com 25 openssl s_client -connect smtp.gmail.com:465 It will emit a 530 if you try to AUTH in plain-text (telling you to use STARTTLS), it will also only say "AUTH" in the EHLO response to the latter. And indeed Net::SMTP picks up on this, and doesn't even send your user/password: https://metacpan.org/release/libnet/source/lib/Net/SMTP.pm#L169 So this hypothetical degradation of the connection and sending auth over plain-text I suggested in upthread #3 seems to mostly/entirely be a non-issue as far as e.g. accidentally sending your password on some open WiFi network goes due to a local misconfiguration. As long as the SMTP server is functional enough to say it doesn't support AUTH on plain-text you'll be OK. I'm assuming that these days with the push for "SSL everywhere" most/all big providers/MTAs have moved away from supporing plain-text auth by default.
Can I get one of the maintainers to chime in on this thread and explain, in their opinion, what this patchset needs before it is acceptable? I'm not sure where I should go from this discussion.
On Tue, Apr 13 2021, Drew DeVault wrote: > Can I get one of the maintainers to chime in on this thread and explain, > in their opinion, what this patchset needs before it is acceptable? I'm > not sure where I should go from this discussion. Git just has the one maintainer, Junio C Hamano. Ultimately getting your patch in is up to his whimsy. Maybe he'll reply, but in an attempt to save him some time (which I understand I'm taking too much of these days): Getting your patch in is ultimately up to you. So you submitted a patch, got some feedback/review. Through some combination of this thread (mainly <875z0sg8t9.fsf@evledraar.gmail.com> and <87o8ejej8m.fsf@evledraar.gmail.com>) I suggested making some changes in a v3. I.e. cleaning up some of the semantics (config docs/handling leaking implementation details) and improving the commit message(s) to summarize the trade-offs, why this approach is safe/isn't safe/why it's OK in the cases it's not etc. So, whatever Junio or anyone else thinks of my opinion I think it's fair to say that at this point he's most likely to skim this thread and see that there's some outstanding feedback that hasn't been addressed. "Addressed" means either re-rolling into a v3, or deciding that nothing (or only part of the feedback) needs to be changed and/or addressed. Both/some of those/that are perfectly acceptable approaches, but in either case making things easily digestable to Junio will help your series along.
"Drew DeVault" <sir@cmpwn.com> writes: > Can I get one of the maintainers to chime in on this thread and explain, > in their opinion, what this patchset needs before it is acceptable? I'm > not sure where I should go from this discussion. What you called "compromise" in the upthread didn't even smelled like a compromise but the safest way forward. My reasoning goes (thinking aloud, so that you and Ævar can correct me if I am talking nonsense and discount my input based on it): - If we were designing this from scratch, we would have called the smtps:// tunnelled transport SSL/TLS and in-place upgrade transport STARTTLS, like e-mail providers and client programs do, but unfortunately we didn't. We ended up with 'ssl' vs 'tls'. - We could introduce and advertise STARTTLS as a synonym to 'tls', but then those whose send-email does not understand STARTTLS but read about the new way of spelling would end up having no encryption due to another earlier mistake we made, i.e. an unrecognised option value silently turns into no encryption. - To avoid the above problem, the first phase is not to change the status quo that 'ssl' vs 'tls' are the only two choices. What we do is to make the program error out if we see an unrecognised value given to the option. We release this to the wild, and wait for the current versions of send-email that turns unrecognised words into no-encryption die out. It may take several years, though. - After waiting, we add 'starttls' as a synonym to 'tls'. We may also add 'ssl/tls' as a synonym to 'ssl'. Unfortunately 'tls' alone cannot be repurposed as a synonym for smtps:// without another deprecation dance, and it is not in scope of the transtion. Am I on the same page as you two?
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index c17c3b400a..520b355e50 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -171,8 +171,8 @@ Sending Specify the encryption to use, either 'ssl' or 'tls'. 'ssl' enables generic SSL/TLS support and is typically used on port 465. 'tls' enables in-band STARTTLS support and is typically used on port 25 or - 587. Use whichever option is recommended by your mail provider. Any - other value reverts to plain SMTP. Default is the value of + 587. Use whichever option is recommended by your mail provider. Leave + empty to disable encryption and use plain SMTP. Default is the value of `sendemail.smtpEncryption`. --smtp-domain=<FQDN>:: diff --git a/git-send-email.perl b/git-send-email.perl index f5bbf1647e..bda5211f0d 100755 --- a/git-send-email.perl +++ b/git-send-email.perl @@ -495,6 +495,9 @@ sub read_config { # 'default' encryption is none -- this only prevents a warning $smtp_encryption = '' unless (defined $smtp_encryption); +if ($smtp_encryption ne "" && $smtp_encryption ne "ssl" && $smtp_encryption ne "tls") { + die __("Invalid smtp_encryption configuration: expected 'ssl', 'tls', or nothing.\n"); +} # Set CC suppressions my(%suppress_cc);
Signed-off-by: Drew DeVault <sir@cmpwn.com> --- Documentation/git-send-email.txt | 4 ++-- git-send-email.perl | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-)