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

Status: RESOLVED FIXED

(Chat Core :: Security: OTR, task)

Description

[Kai Engert (:KaiE:)]

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.

Comment 1

[Kai Engert (:KaiE:)]

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

Comment 2

[Kai Engert (:KaiE:)]

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?

Comment 3

[Kai Engert (:KaiE:)]

(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.

Comment 4

[Kai Engert (:KaiE:)]

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.

Comment 5

[Magnus Melin [:mkmelin]]

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

Comment 6

[Kai Engert (:KaiE:)]

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)?

Comment 7

[Kai Engert (:KaiE:)]

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?

Comment 8

[Magnus Melin [:mkmelin]]

(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 [e-mail] which one is correct?

Comment 9

[Kai Engert (:KaiE:)]

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

Comment 10

[Kai Engert (: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.)

Comment 11

[Patrick Cloke [:clokep]]

(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).

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Bug 1518164 - Import the libgpg-error library code into the comm-central tree.

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Bug 1518164 - Import the libgcrypt library code into the comm-central tree.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Bug 1518164 - Import the libotr library code into the comm-central tree.

Comment 15

[Kai Engert (:KaiE:)]

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?

Comment 16

[Kai Engert (:KaiE:)]

(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.

Comment 17

[Kai Engert (:KaiE:)]

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

Comment 18

[Patrick Cloke [:clokep]]

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.

Comment 19

[Kai Engert (:KaiE:)]

(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.

Comment 20

[Kai Engert (:KaiE:)]

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.

Comment 21

[Kai Engert (:KaiE:)]

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.

Comment 22

[Jorg K (CEST = GMT+2)]

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!).

Comment 23

[Kai Engert (: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

Comment 24

[Kai Engert (:KaiE:)]

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

Comment 25

[Jorg K (CEST = GMT+2)]

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.

Comment 26

[Kai Engert (:KaiE:)]

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

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

Comment 27

[Rob Lemley [:rjl]]

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. :)

Comment 28

[Jorg K (CEST = GMT+2)]

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

Comment 29

[Jorg K (CEST = GMT+2)]

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.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

Comment 31

[Jorg K (CEST = GMT+2)]

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.

Comment 32

[Jorg K (CEST = GMT+2)]

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.

Comment 33

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr68/rev/2a1b7b88e66a1b2b081b6eed7c6a17e5e22f6b9a
https://hg.mozilla.org/releases/comm-esr68/rev/5effd2dba901c5d573963c81b27f64b82aeb576e
https://hg.mozilla.org/releases/comm-esr68/rev/8c34fc693509a149c5e8d490725c3b82a69acef1