Closed Bug 1518164 Opened 1 year ago Closed 9 months ago

Import the libotr, libgpg-error and libgcrypt library code into the comm-central tree

Categories

(Chat Core :: Security: OTR, task)

task
Not set

Tracking

(thunderbird_esr68 fixed, thunderbird69 wontfix, thunderbird70 fixed)

RESOLVED FIXED
Instantbird 70
Tracking Status
thunderbird_esr68 --- fixed
thunderbird69 --- wontfix
thunderbird70 --- fixed

People

(Reporter: KaiE, Assigned: KaiE)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

This is part of the work for bug 954310, pending confirmation in bug 1518091.

Copies of the (unmodified) libotr, libgpg-error and libgcrypt libraries should be checked in to the comm-central tree.
Blocks: 1518166
See Also: → 1519804
Depends on: 1519803
Type: defect → enhancement
Depends on: 1519804

Which subdirectory of the comm tree should contain the new libraries?

I suggest
comm/other-licenses/liggpg-error
comm/other-licenses/libgcrypt
comm/other-licenses/libotr

Some thoughts and questions regarding licenses in source files.

In my understanding, it's allowed if some source files use licenses that are stricter than MPL.
For the libraries we're importing here, it's acceptable if the files use the LGPL license, because we'll distribute the respective binary code under the terms of the LGPL license.

I'm uncertain what to do about source files and text documents that are GPL-only, without exceptions.

For libotr, the subdirectories named "tests" (test suite) and "toolkit" (helper tools) contain GPL-only files. They probably are unnecessary for building the library. If true, we shouldn't import those files.

For libgpg-error, several files in the "doc" subdirectory seem to be GPL-only. If we're uncertain, I suggest to exclude the whole directory.

In libgpg-error, I'm not sure if rules in file m4/gettext.m4 allow us to use it. Could someone please help me understand?

(In reply to Kai Engert (:kaie:) from comment #2)

In libgpg-error, I'm not sure if rules in file m4/gettext.m4 allow us to use it. Could someone please help me understand?

dnl Copyright (C) 1995-2014 Free Software Foundation, Inc.
dnl This file is free software; the Free Software Foundation
dnl gives unlimited permission to copy and/or distribute it,
dnl with or without modifications, as long as this notice is preserved.
dnl
dnl This file can can be used in projects which are not available under
dnl the GNU General Public License or the GNU Library General Public
dnl License but which still want to provide support for the GNU gettext
dnl functionality.
dnl Please note that the actual code of the GNU gettext library is covered
dnl by the GNU Library General Public License, and the rest of the GNU
dnl gettext package package is covered by the GNU General Public License.
dnl They are not in the public domain.

Reading it again, the text from comment 3 is probably fine. I was a little worried about the final sentence, but the related code files inside the libgpg-error tree are LGPL, so they are fine.
The important part for this file is probably the top section, which says there aren't restrictions for distributing it, besides including the notice.

For the location, seems mozilla in general put it under third_party? https://searchfox.org/comm-central/source/mozilla/third_party

You don't suggest that we add to Mozilla's third_party directory, right?

IIUC you prefer to create a new comm/third_party directory (not reuse comm/other-licenses)?

Rob, can the build approach that you're exploring in bug 1518166 use sources that we import into hg?

I'm not sure how we should deal with portions that are GPL-only, see above comments.

Option 1:
Is it allowed to import GPL-only code into the tree, if we don't distribute any binary code that is based on the GPL-only sources?

Option 2:
Or are we required to omit any GPL-only sources when importing into the tree? Then we'll have to ensure that our stripped code still builds.

Option 3:
Or should we completely avoid importing the sources into the tree, and trigger the build for all target platforms separately, like you explore in bug 1518166?

(In reply to Kai Engert (:kaie:) from comment #6)

IIUC you prefer to create a new comm/third_party directory (not reuse
comm/other-licenses)?

That's what I'm suggesting yes.
But going back checking, I'm not sure which it should be. The readme at https://searchfox.org/comm-central/source/mozilla/other-licenses/README is vague. Maybe we want to ask licensing@mozilla.org which one is correct?

I had sent email to licensing on June 18, pinged again yesterday. We're still waiting for advice from licensing.

Assignee: nobody → kaie

I have received email from Mike Hoye.

Mike suggests to use a subdirectory named third_party, which is the same as the suggestion from Magnus.

Also, regarding comment 7, my understanding of Mike's explanation is, that Option 1 is permissible, assuming we include the proper license files, and we don't have an overall license that restricts the rights of the subdirectories.

I've also had a chat with Rob, having the sources in tree works for him. (For the work he intends to do in bug 1518166.)

(In reply to Kai Engert (:kaie:) from comment #10)

I've also had a chat with Rob, having the sources in tree works for him. (For the work he intends to do in bug 1518166.)

This is also good if we need to patch the source at all (e.g. the work in bug 1562900 if we can't upstream it for some reason).

I have submitted three patches to phabricator, one for each of the libraries, see the attached links.

Is anyone interested to have a look?

I don't ask for a review of the library contents, however, it would be good to get an r+ for:

  • libraries have been added in the correct place
  • overall confirmation that there's agreement with me performing this change.
  • I've included README files, see the first file in each attached phabricator request,
    so you could confirm it looks good.

Any volunteers?

Flags: needinfo?(mkmelin+mozilla)

(In reply to Patrick Cloke [:clokep] from comment #11)

This is also good if we need to patch the source at all (e.g. the work in bug 1562900 if we can't upstream it for some reason).

I'd like to avoid patching, because that would be the equivalent of forking.

We don't have the resources to maintain a fork and keep it secure. With forking, we'd always have to merge in our changes into new releases. Ensuring that a fork of a security library doesn't degrade security is difficult.

Another argument is expectation of distributors. Linux distributors commonly have only one copy of a library version on their system, because that simplifies their response to security incidents. Linux distributions already include all these libraries. The package maintainers of Linux distributions don't need to rebuild these libraries, but can use their existing packages. If we change them, it's no longer possible, and it would create confusion.

Should we ever have a good reason to change those libraries, we should fork them, rename them, publish them, and maintain them. But I hope very much this will never be necessary.

... if we require changes to those libraries, the correct way is to work with the upstream developers to get changes into their official code.

We talked about this on IRC a bit.

First -- I'm not suggesting we want to patch the source, but (while working with libpurple) we've found this necessary in order to:

  1. Pull in changes from releases faster.
  2. Fix bugs in upstream (with the assumption that the patches are upstream, but to get them faster).
  3. Fix compilation issues for Windows.

Unfortunately it seems like we won't really know what version of the library is used on Linux (I suspect this is less of a realistic issue on Windows) since Linux will be able to just have their Thunderbird package depend on their current libotr package (i.e. they won't be using our compiled source).

This does make me somewhat concerned that we might not be able to depend on any upstream changes we might want to make, but I'm sure there are technical solutions to that.

(In reply to Patrick Cloke [:clokep] from comment #18)

Unfortunately it seems like we won't really know what version of the library is used on Linux (I suspect this is less of a realistic issue on Windows) since Linux will be able to just have their Thunderbird package depend on their current libotr package (i.e. they won't be using our compiled source).

Because we use the jstypes mechanism to use the OTR library, we are indeed limited in what we can require. If someone uses tools like "ldd" to look at the library dependencies of Thunderbird, none of these libraries will be obvious as dependencies. This is because the libraries are opened dynamically at runtime.

If a Linux user doesn't use the binaries that we provide, but uses the Thunderbird binary provided by the Linux distribution, then it will depend on the package maintainer's diligence. Thunderbird will start up fine if the libraries are missing on the system, but the OTR feature will be unavailable. It's necessary that the package maintainer notices the dependency on the OTR library, and adds an appropricate dependency to the package, to ensure that the distribution's OTR library gets installed.

We should document in our release notes that Linux distributions should add a dependency to the OTR library, and we should list the minimum version number that we expect.

I think that's fine for now, because we're declaring the feature as experimental anyway. In the future, once we consider the feature as stable and want to enabled it by default, we could add a runtime check, that informs the user if the required libraries cannot be found.

This does make me somewhat concerned that we might not be able to depend on any upstream changes we might want to make, but I'm sure there are technical solutions to that.

At load time, we check the version of the OTR library we're loading. If the library is too old, we can skip loading, and the OTR feature will be disabled. We could have a runtime check and user feedback, or quit.

We cannot enforce that a Linux distribution installs the required minimum version of the OTR library, because we use dynamic loading of the OTR library. We'll have to document a need for a newer library version in release notes, and ask the Linux package maintainers to bump the dependent library versions.

Status: NEW → ASSIGNED
Type: enhancement → task
Flags: needinfo?(mkmelin+mozilla)
Attachment #9076652 - Flags: approval-comm-beta?
Attachment #9076654 - Flags: approval-comm-beta?
Attachment #9076655 - Flags: approval-comm-beta?
Keywords: checkin-needed
Attachment #9076652 - Flags: approval-comm-beta? → approval-comm-esr68?
Attachment #9076654 - Flags: approval-comm-beta? → approval-comm-esr68?
Attachment #9076655 - Flags: approval-comm-beta? → approval-comm-esr68?

Jörg, these patches from phabricator are needed for license compliance, and to enable Rob's work in bug 1518166.
This code isn't part of the default build.

We can fix this in comm-central.

We can fix this in comm-esr68.

Situation for comm-beta 69: Because we don't have a beta release branch of mozilla for comm-beta, we cannot uplift bug 1519804 and bug 1559900 ourselves. Without those license declaration in about:license, we must not import the related code. Maybe we can simply exclude it from comm-beta 69. Or, I filed bug 1566397, to request uplift to mozilla-beta. If approved, then we could uplift bug 1519804 and this bug, too.

Kai, can you please establish dependencies between the three parts, so they will land all at once. I didn't find the button to push. In general, Phab is a great productivity enhancer (not!).

Flags: needinfo?(kaie)

You don't need to worry about deps, because that code isn't build yet. Land in any order.

In each of the phab links, there's a link on the upper right "Download Raw Diff".
Please check in each of those, unmodified.

Please use the respective summary line:
Bug 1518164 - Import the libgpg-error library code into the comm-central tree. r=mkmelin
Bug 1518164 - Import the libgcrypt library code into the comm-central tree. r=mkmelin
Bug 1518164 - Import the libotr library code into the comm-central tree. r=mkmelin

Flags: needinfo?(kaie)

FYI, the patches are all new files. All are below a new top-evel directory in comm-central named "third_party".

I have hg phabread set up, but I wanted to try landing this directly from Lando, but not in three pushes, but one, that's why the dependencies are important. So can you please establish those dependencies and let me know how it's done.

I don't know how to setup "dependencies" between patches in phabricator.

There is already a parent/child relationship configured between the three patches:

Sorry Kai, I beat you to it.
Over on the right of the screen when viewing a revision is "Edit related revisions". Then for this one I had to search "All Objects" to find the other revisions. moz-phab does make doing this a lot easier btw. :)

Perfect, many thanks. I'll land it at midnight ;-)

Real productivity enhancer:
Could not Communicate with Lando API
There was an error when trying to communicate with Lando API. Please try your request again later.

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bda21df4a789
Import the libgpg-error library code into the comm-central tree. r=mkmelin
https://hg.mozilla.org/comm-central/rev/51aa7c327951
Import the libgcrypt library code into the comm-central tree. r=mkmelin
https://hg.mozilla.org/comm-central/rev/7f110fc4973f
Import the libotr library code into the comm-central tree. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Keywords: checkin-needed
Resolution: --- → FIXED

FYI, there is a loose Phag-HG integration described here:
https://www.mercurial-scm.org/wiki/Phabricator#Setting_up_hg
I have that set up to avoid having to construct HG headers by hand.

Target Milestone: --- → Instantbird 70

Comment on attachment 9076652 [details]
Bug 1518164 - Import the libgpg-error library code into the comm-central tree.

Usually we apply the approval to all patches, so you don't need to ask three times.

Attachment #9076652 - Flags: approval-comm-esr68? → approval-comm-esr68+
Attachment #9076654 - Flags: approval-comm-esr68? → approval-comm-esr68+
Attachment #9076655 - Flags: approval-comm-esr68? → approval-comm-esr68+
Blocks: 1577646
Component: General → Security: OTR
You need to log in before you can comment on or make changes to this bug.