1195856 - Don't require LDAP account to push to MozReview

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Gregory Szorc [:gps]]

Currently, MozReview (specifically ssh://reviewboard-hg.mozilla.org) requires users have an LDAP account with registered SSH public keys to push changesets into it. This is not good for contributors, as it prevents them from having nice things.

We need to enable contributors / anonymous users to push things to MozReview without jumping over the hurdle of obtaining an LDAP account first.

(This is listed as one of my Q3 deliverables.)

Comment 1

[Gregory Szorc [:gps]]

I was originally thinking we'd allow anonymous pushes via SSH. Now that Mercurial has bundle2 (an extensible wire protocol) and now that we have a full Python environment in MozillaBuild (previously we had to assume Windows users didn't have access to Python modules like "json"), we have a number of options available to us. We could potentially deploy a web service that accepts a bundle[2] payload along with other metadata and it would do everything that `hg push` is doing today. Food for thought.

Comment 2

[Gregory Szorc [:gps]]

Having thought about things some more, I don't think reimplementing `hg push` should be a blocker for this. So ignore comment #1.

Comment 3

[Jeff Bryner [:jeff] (use NEEDINFO 😉)]

kang; what was the RRA result for this service?

Comment 4

[Gregory Szorc [:gps]]

My recollection from my conversion with kang is that the big risks are when pushed code is executed, not from the push / version control interaction itself. As long as we establish limits on who can push code to Try and cause execution of pushed code, we are OK to proceed with allowing anyone on the internets to push code to a review repository.

I believe we discussed establishing a non SSH key method for authenticating pushes. e.g. we could use Bugzilla usernames and {passwords/API keys} to validate the SSH connection. This would be safer than a password-less guest account since we could ban accounts easily. The critical goal we need to accomplish is lower the barrier to first-time contributors submitting code for review.

Comment 5

[Gregory Szorc [:gps]]

OpenSSH can use PAM for authenticating. However, the PAM service is derived from the name of the binary (sshd). On the Mercurial servers today, we don't currently set "UsePAM" in sshd_config, so the sshd PAM service can be used. However, if we ever use PAM for port 22, we'll need to copy the sshd binary so we can use a separate PAM service.

https://bugzilla.mindrot.org/show_bug.cgi?id=2102 tracks making the PAM service configurable in sshd_config. There is a reference to a RedHat bug where it looks like they may be putting this in RHEL 7.1, even if it doesn't make it upstream. Of course, we still run RHEL/CentOS 6, so this doesn't help us much. But long term...

Comment 6

[Gregory Szorc [:gps]]

Also, OpenSSH doesn't appear to support UsePAM in Match conditional block. Since we're using LPK for port 22, I'm pretty sure enabling UsePAM would cause all kind of pain on port 22. This probably means we have no choice but to run a separate sshd service.

Comment 7

[Jeff Bryner [:jeff] (use NEEDINFO 😉)]

Also consider that SSH attacks are the number one attack we experience. If first-time contributors are the target, won't they already have ssh keys?

Comment 8

[Gregory Szorc [:gps]]

I'm not sure how many contributors will have SSH keys. Although anyone pushing to GitHub will have SSH keys. And given how popular GitHub is, the percentage of users with SSH keys may be very high.

But you need to solve the problem of giving Mozilla's systems access to users' SSH keys. LDAP is our solution today and the overhead for creating LDAP accounts is too high. We want to enable people to write a patch and "throw it over a wall." We want the MozReview world to be no worse than today. (Today you have to create a Bugzilla account so you can upload an attachment.) So requiring Bugzilla accounts is acceptable.

Bugzilla now allows sign-in through GitHub. Perhaps Bugzilla could gain the ability to store SSH keys. Then, we could have Bugzilla obtain SSH keys from GitHub. Modern versions of OpenSSH allow you to specify a command to look up SSH public keys for a user (AuthorizedKeysCommand). Presumably we could use Bugzilla usernames for SSH usernames and have OpenSSH query a Bugzilla API with a privileged account to retrieve their SSH keys. I /think/ we could even use Bugzilla user IDs as the system UID. It is a bit more work than a passwordless guest account, but I think the barriers are low enough.

I grok the security concerns here. Out of curiosity, what are your specific concerns about SSH attacks?

Comment 9

[Jeff Bryner [:jeff] (use NEEDINFO 😉)]

Every day we see dozens of ssh attacks against anything we leave open to the internet, especially non-ssh key-based implementations. 

So for sure if we allow ssh with no credentials necessary, it will be abused. I'm just trying to leverage whatever these folks may have to secure it.

Comment 10

[Gregory Szorc [:gps]]

For the record, we have SSH on these machines using ForceCommand to execute a Python script that will exec() to `hg` once a sanity check of the environment and requested command arguments has been performed. The entry point is https://hg.mozilla.org/hgcustom/version-control-tools/file/35edcee4c734/hgserver/pash/pash.py#l74. You'll also be interested in https://hg.mozilla.org/hgcustom/version-control-tools/file/35edcee4c734/hgserver/pash/hg_helper.py#l477. Yes, that regexp scares me. I've been wanting to rewrite that. I just got this code into what I consider a maintainable state *this week*. It's on my short list of things to look at.

I do worry about credential-less auth being abused to cause excessive process invocations, etc. I now think requiring and validating a Bugzilla account should be required.

Comment 11

[Mark Côté [:mcote]]

I'm not sure if we've done an RRA on this specifically.  If not, we should.  I want to make sure security is on board for whatever approach we settle on.

Comment 12

[Guillaume Destuynder [:kang] [this account is no longer active]]

No my call but I suspect we can assume that contributors have SSH keys. Everyone doing dev these days have at least one on Github as previously mentioned.

Here's the RRA for MozReview: https://docs.google.com/spreadsheets/d/1csA2Nt6BE0akwprFbrLmUl-nW6JpaG6vNDO4qAvWZTw/edit#gid=0 (bug https://bugzilla.mozilla.org/show_bug.cgi?id=1177574)

This RRA was ran (as per notes) with the design that uses LDAP as trust backend and matching bugzilla  accounts to LDAP accounts.

We can always re-run, discuss, etc.

Comment 13

[Gregory Szorc [:gps]]

kang, mcote, myself, and members of the BMO team just met to discuss this.

BMO wasn't too keen on storing SSH keys in BMO. They don't want to be a generic database and they have no use for storing SSH keys, so not much benefit to them. Those are legitimate concerns, so we decided to not add support for storing SSH keys in BMO.

We discussed better identity management solutions at Mozilla. If ldap.mozilla.org accounts could be created turnkey (say by a user logging into Bugzilla), that would satisfy my use case since users can go to https://login.mozilla.com/ and upload SSH keys. However, we assumed this would be a big policy change, open us up to more security considerations, and it wouldn't be implemented for a while - longer than we'd like. So we decided to not worry about this. kang still wishes we had a better identity management solution at Mozilla and it sounds like he'll push for something somehow. But that's not relevant to this bug.

We discussed teaching MozReview to store SSH keys. We'd implement a web interface for defining SSH keys. The SSH server on MozReview would fetch SSH keys from MozReview and we'd have some additional PAM magic to map MozReview accounts to system accounts. This would almost certainly work. But it seemed like a lot of work. There are all kinds of "fun" problems, such as determining what the system UID should be and managing two versions of OpenSSH on the server.

After the BMO people disconnected, kang and I talked some more. I suggested allowing people to push over HTTP. Mercurial supports HTTP authentication with usernames/passwords or X509 certificates. And it is trivial to configure HTTP servers to use any system your want for username/password authnz. We determined that using Bugzilla usernames and API keys as passwords would be sufficient. Users already need to configure a Bugzilla API key to use MozReview. Credential re-use should be easy. A downside is the HTTP server process will need write access to all repos. Another is we'll lose file-level granularity on who wrote which files (today files are owned by users like "[e-mail]"). But, the pushlog should still record the authenticated user, so the losses should be minor. Also, there are some load balancer considerations. Pushes need to hit the master server (not a real-only mirror). Fortunately, reviewboard-hg1.dmz.scl3 is our only server and there is no replication. In the distant future, we /might/ merge reviewboard-hg1.dmz.scl3 into hg.mozilla.org, which has separate load balancer services for ports 22 and 80/44 (22 hits master, 80/443 hits mirrors). We'll cross that bridge when we need to: it shouldn't matter today.

tl;dr we're going to enable pushes via HTTPS using Bugzilla usernames and API keys as the credentials. The MozReview client should "just work" and this should only be a matter of configuring the/a HTTP server for auth against Bugzilla/MozReview.

Comment 14

[Gregory Szorc [:gps]]

Attachment: MozReview Request: pushlog: support obtaining user from alternate sources; r=smacleod

pushlog: support obtaining user from alternate sources; r?smacleod

We are enabling MozReview to accept pushes over HTTP. HTTP servers often
define authenticated users via the REMOTE_USER environment variable. So,
we need to teach the pushlog to obtain users from this variable.

Since MozReview's HTTP authentication will use Bugzilla's user database
and SSH will use LDAP - two separate databases - we add the ability to
prefix users in the pushlog with a static string denoting where the user
value came from and thus which user database it belongs to. This will
enable us to distinguish LDAP users from Bugzilla users in the pushlog.

Comment 15

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r?smacleod, kang

We're going to configure the existing MozReview Mercurial server to
allow pushes over HTTP. We'll be using standard HTTP Basic
authentication using Bugzilla usernames and API Keys. In order to
authenticate these credentials, we're going to use mod_authnz_external
to invoke a script that determines whether the credentials are valid.
This is that script.

Comment 16

[Gregory Szorc [:gps]]

Attachment: MozReview Request: mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

Currently, only people with an LDAP account can push to MozReview (over
SSH). The barrier to obtaining an LDAP account is quite high and is not
suitable for first-time contributors.

This commit enables pushing to MozReview via HTTP using Bugzilla
usernames and API keys as credentials.

The Apache httpd server has been configured to authenticate non {GET,
HEAD} requests using the Bugzilla API Key authentication script
previously written using mod_authnz_external.

We had to modify the hgrc on the hgrb server to both accept pushes and
- in the case of the Docker environment - accept pushes via HTTP (as
opposed to HTTPS). This /might/ be incorrect because in production we
may have a load balancer sending the origin server plaintext.

Because the hgweb server now needs to talk to Review Board, we had to
add various Python packages to the hgweb virtualenv.

Because the LDAP and Bugzilla user databases are separate, we added
prefixing of HTTP/Bugzilla users in the pushlog so we can distinguish
which database each user comes from. We retain unprefixed LDAP entries
for backwards compatibility. No machines should be looking at the
pushlog on MozReview, so we could probably rewrite the pushlogs at some
later date to contain prefixes. Or we could just add them now and deal
with both formats for LDAP pushers until we rewrite...

Comprehensive tests of the functionality have been added.

There are a few areas in the implementation as of this commit. They will
be addressed in subsequent commits.

Comment 17

[Steven MacLeod [:smacleod]]

Comment on attachment 8663205 [details]
MozReview Request: pushlog: support obtaining user from alternate sources; r=smacleod

https://reviewboard.mozilla.org/r/19739/#review18429

Comment 18

[Steven MacLeod [:smacleod]]

Comment on attachment 8663206 [details]
MozReview Request: mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

https://reviewboard.mozilla.org/r/19741/#review18433

LGTM

::: scripts/httpd-authn-bugzilla-key:40
(Diff revision 1)
> +                                       assert_fingerprint=self.fingerprint,

In case kang looks into this part as well:

I was unsure this was actually verifying the fingerprint due to the comment at [1]. But, looking at [2] it does appear the fingerprint is verified even without assert_hostname.

So this looks fine to me and it appears the comment[1] is just misleading.

[1] https://github.com/kennethreitz/requests/blob/8b5e457b756b2ab4c02473f7a42c2e0201ecc7e9/requests/packages/urllib3/connectionpool.py#L661-L663

[2] https://github.com/kennethreitz/requests/blob/8b5e457b756b2ab4c02473f7a42c2e0201ecc7e9/requests/packages/urllib3/connection.py#L240-L242

Comment 19

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 8663206 [details]
MozReview Request: mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

https://reviewboard.mozilla.org/r/19741/#review18459

It's shippable regardless of my 2 comments as they're safety measures (one for security, one for http protocol handling), that said - up to you! :)

::: scripts/httpd-authn-bugzilla-key:75
(Diff revision 1)
> +    if r.status_code != 200:

from the http point of view, >200 is valid, so while it's safe, just something to know about in case you get unexpected replies (keep-alive's continue, redirects, etc.) 
If that's expected then its fine though :)

::: scripts/httpd-authn-bugzilla-key:95
(Diff revision 1)
> +    if not bugzilla_url.startswith(('http://', 'https://')):

Since an API key travels over the write, I would recommend only allowing https, or for convenience, only allow non-https (or invalid https as well) with an explicit program flag such as:

-k --insecure Allow connecting to servers using invalid x509 ("TLS") certificates
-p --plaintext Allow connecting in plain-text (http)

Comment 20

[Steven MacLeod [:smacleod]]

Comment on attachment 8663207 [details]
MozReview Request: mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

https://reviewboard.mozilla.org/r/19743/#review18463

Comment 21

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/19741/#review18459

> from the http point of view, >200 is valid, so while it's safe, just something to know about in case you get unexpected replies (keep-alive's continue, redirects, etc.) 
> If that's expected then its fine though :)

I don't expect non-200 responses. So I'm fine with it erroring when something unexpected happens. I'll add a test for this case.

> Since an API key travels over the write, I would recommend only allowing https, or for convenience, only allow non-https (or invalid https as well) with an explicit program flag such as:
> 
> -k --insecure Allow connecting to servers using invalid x509 ("TLS") certificates
> -p --plaintext Allow connecting in plain-text (http)

I'll add support for refusing http:// unless an argument overrides it. I shouldn't need an insecure TLS mode.

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8663205 [details]
MozReview Request: pushlog: support obtaining user from alternate sources; r=smacleod

pushlog: support obtaining user from alternate sources; r=smacleod

We are enabling MozReview to accept pushes over HTTP. HTTP servers often
define authenticated users via the REMOTE_USER environment variable. So,
we need to teach the pushlog to obtain users from this variable.

Since MozReview's HTTP authentication will use Bugzilla's user database
and SSH will use LDAP - two separate databases - we add the ability to
prefix users in the pushlog with a static string denoting where the user
value came from and thus which user database it belongs to. This will
enable us to distinguish LDAP users from Bugzilla users in the pushlog.

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8663206 [details]
MozReview Request: mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

We're going to configure the existing MozReview Mercurial server to
allow pushes over HTTP. We'll be using standard HTTP Basic
authentication using Bugzilla usernames and API Keys. In order to
authenticate these credentials, we're going to use mod_authnz_external
to invoke a script that determines whether the credentials are valid.
This is that script.

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8663207 [details]
MozReview Request: mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

Currently, only people with an LDAP account can push to MozReview (over
SSH). The barrier to obtaining an LDAP account is quite high and is not
suitable for first-time contributors.

This commit enables pushing to MozReview via HTTP using Bugzilla
usernames and API keys as credentials.

The Apache httpd server has been configured to authenticate non {GET,
HEAD} requests using the Bugzilla API Key authentication script
previously written using mod_authnz_external.

We had to modify the hgrc on the hgrb server to both accept pushes and
- in the case of the Docker environment - accept pushes via HTTP (as
opposed to HTTPS). This /might/ be incorrect because in production we
may have a load balancer sending the origin server plaintext.

Because the hgweb server now needs to talk to Review Board, we had to
add various Python packages to the hgweb virtualenv.

Because the LDAP and Bugzilla user databases are separate, we added
prefixing of HTTP/Bugzilla users in the pushlog so we can distinguish
which database each user comes from. We retain unprefixed LDAP entries
for backwards compatibility. No machines should be looking at the
pushlog on MozReview, so we could probably rewrite the pushlogs at some
later date to contain prefixes. Or we could just add them now and deal
with both formats for LDAP pushers until we rewrite...

Comprehensive tests of the functionality have been added.

There are a few areas in the implementation as of this commit. They will
be addressed in subsequent commits.

Comment 25

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: automatically use Bugzilla credentials from hgrc; r?smacleod, kang

reviewboard: automatically use Bugzilla credentials from hgrc; r?smacleod, kang

We already have users define Bugzilla usernames and API Keys in their
hgrc file under the "bugzilla.username" and "bugzilla.apikey" options.

Mercurial has its own method for storing credentials for URLs. To make
initial setup easier and so Bugzilla API Keys aren't duplicated in the
config files, we add integration into Mercurial's password manager that
will automatically use [bugzilla] defined credentials when talking to a
known Mozilla HTTP server.

We establish a list of trusted URL endpoints that we release the
credentials to. Unless an attacker convinces someone to change the
trusted list and connect to a rogue server (it's hard to prevent
phishing), the current implementation can only be exploited by
someone conducting a MitM, with a x509 certificate for a mozilla.org
hostname, and with that certificate signed by a CA trusted by the
client. This should be very unlikely. Keep in mind that if someone
is in a position to carry out this attack, they are probably in position
to intercept Bugzilla and other Mozilla traffic as well, so the Bugzilla
credentials can already be considered compromised. This is a long winded
way of saying I believe the current implementation provides sufficient
security.

Comment 26

[Kendall Libby [:fubar] (he/him)]

Comment on attachment 8663207 [details]
MozReview Request: mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

https://reviewboard.mozilla.org/r/19743/#review18837

lgtm

Comment 27

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 8668190 [details]
MozReview Request: reviewboard: automatically use Bugzilla credentials from hgrc; r?smacleod, kang

https://reviewboard.mozilla.org/r/20877/#review19043

Comment 28

[Gregory Szorc [:gps]]

Comment on attachment 8663205 [details]
MozReview Request: pushlog: support obtaining user from alternate sources; r=smacleod

pushlog: support obtaining user from alternate sources; r=smacleod

We are enabling MozReview to accept pushes over HTTP. HTTP servers often
define authenticated users via the REMOTE_USER environment variable. So,
we need to teach the pushlog to obtain users from this variable.

Since MozReview's HTTP authentication will use Bugzilla's user database
and SSH will use LDAP - two separate databases - we add the ability to
prefix users in the pushlog with a static string denoting where the user
value came from and thus which user database it belongs to. This will
enable us to distinguish LDAP users from Bugzilla users in the pushlog.

Comment 29

[Gregory Szorc [:gps]]

Comment on attachment 8663206 [details]
MozReview Request: mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

We're going to configure the existing MozReview Mercurial server to
allow pushes over HTTP. We'll be using standard HTTP Basic
authentication using Bugzilla usernames and API Keys. In order to
authenticate these credentials, we're going to use mod_authnz_external
to invoke a script that determines whether the credentials are valid.
This is that script.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 8663207 [details]
MozReview Request: mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

Currently, only people with an LDAP account can push to MozReview (over
SSH). The barrier to obtaining an LDAP account is quite high and is not
suitable for first-time contributors.

This commit enables pushing to MozReview via HTTP using Bugzilla
usernames and API keys as credentials.

The Apache httpd server has been configured to authenticate non {GET,
HEAD} requests using the previously written Bugzilla API Key authentication
script using mod_authnz_external.

We had to modify the hgrc on the hgrb server to both accept pushes over
HTTP and not require TLS for those pushes.

Because the hgweb server now needs to talk to Review Board, we had to
add various Python packages to the hgweb virtualenv.

Because the LDAP and Bugzilla user databases are separate, we added
prefixing of HTTP/Bugzilla users in the pushlog so we can distinguish
which database each user comes from. We retain unprefixed LDAP entries
for backwards compatibility. No machines should be looking at the
pushlog on MozReview, so we could probably rewrite the pushlogs at some
later date to contain prefixes. Or we could just add them now and deal
with both formats for LDAP pushers until we rewrite...

Comprehensive tests of the functionality have been added.

There are a few minor areas where HTTP pushing is sub-par. They will be
addressed in subsequent commits.

Comment 31

[Gregory Szorc [:gps]]

Comment on attachment 8668190 [details]
MozReview Request: reviewboard: automatically use Bugzilla credentials from hgrc; r?smacleod, kang

reviewboard: automatically use Bugzilla credentials from hgrc; r?smacleod, kang

We already have users define Bugzilla usernames and API Keys in their
hgrc file under the "bugzilla.username" and "bugzilla.apikey" options.

Mercurial has its own method for storing credentials for URLs. To make
initial setup easier and so Bugzilla API Keys aren't duplicated in the
config files, we add integration into Mercurial's password manager that
will automatically use [bugzilla] defined credentials when talking to a
known Mozilla HTTP server.

We establish a list of trusted URL endpoints that we release the
credentials to. Unless an attacker convinces someone to change the
trusted list and connect to a rogue server (it's hard to prevent
phishing), the current implementation can only be exploited by
someone conducting a MitM, with a x509 certificate for a mozilla.org
hostname, and with that certificate signed by a CA trusted by the
client. This should be very unlikely. Keep in mind that if someone
is in a position to carry out this attack, they are probably in position
to intercept Bugzilla and other Mozilla traffic as well, so the Bugzilla
credentials can already be considered compromised. This is a long winded
way of saying I believe the current implementation provides sufficient
security.

Comment 32

[Gregory Szorc [:gps]]

Attachment: MozReview Request: reviewboard: refactor repository discovery to facilitate HTTP pushing; r?smacleod

reviewboard: refactor repository discovery to facilitate HTTP pushing; r?smacleod

When we implemented review repository discovery, we assumed there would
only ever be a single URL we'd push to - the SSH URL. Now that we allow
pushes over HTTP, there are multiple URLs for each review repo.

We refactor how the server exposes review repos so it advertises both an
HTTP and SSH URL for each repo.

Since we had to create a new client capability so old clients wouldn't
choke on the new format, I also took the time to change how the data is
exposed to clients. Before, we were using the "pushkey" protocol. I'm no
longer a fan of shoving everything inside the pushkey protocol, so I
established a dedicated wire protocol command for exposing the raw file
contents. I dropped the old capability because the listkeys command to
retrieve data from it will return an empty set. It shouldn't be called
by modern clients since modern clients will require an extension upgrade
before we get to this code (due to the introduced client-side
capability).

A test for discovery over HTTP has been added. It discovered that
capabilities were being cached across repo redirects. This has also been
fixed.

Comment 33

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/20877/#review19347

I rewrote this patch. So please re-review.

Comment 34

[Gregory Szorc [:gps]]

Comment on attachment 8663205 [details]
MozReview Request: pushlog: support obtaining user from alternate sources; r=smacleod

pushlog: support obtaining user from alternate sources; r=smacleod

We are enabling MozReview to accept pushes over HTTP. HTTP servers often
define authenticated users via the REMOTE_USER environment variable. So,
we need to teach the pushlog to obtain users from this variable.

Since MozReview's HTTP authentication will use Bugzilla's user database
and SSH will use LDAP - two separate databases - we add the ability to
prefix users in the pushlog with a static string denoting where the user
value came from and thus which user database it belongs to. This will
enable us to distinguish LDAP users from Bugzilla users in the pushlog.

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8663206 [details]
MozReview Request: mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

We're going to configure the existing MozReview Mercurial server to
allow pushes over HTTP. We'll be using standard HTTP Basic
authentication using Bugzilla usernames and API Keys. In order to
authenticate these credentials, we're going to use mod_authnz_external
to invoke a script that determines whether the credentials are valid.
This is that script.

Comment 36

[Gregory Szorc [:gps]]

Comment on attachment 8663207 [details]
MozReview Request: mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r?fubar, smacleod

Currently, only people with an LDAP account can push to MozReview (over
SSH). The barrier to obtaining an LDAP account is quite high and is not
suitable for first-time contributors.

This commit enables pushing to MozReview via HTTP using Bugzilla
usernames and API keys as credentials.

The Apache httpd server has been configured to authenticate non {GET,
HEAD} requests using the previously written Bugzilla API Key authentication
script using mod_authnz_external.

We had to modify the hgrc on the hgrb server to both accept pushes over
HTTP and not require TLS for those pushes.

Because the hgweb server now needs to talk to Review Board, we had to
add various Python packages to the hgweb virtualenv.

Because the LDAP and Bugzilla user databases are separate, we added
prefixing of HTTP/Bugzilla users in the pushlog so we can distinguish
which database each user comes from. We retain unprefixed LDAP entries
for backwards compatibility. No machines should be looking at the
pushlog on MozReview, so we could probably rewrite the pushlogs at some
later date to contain prefixes. Or we could just add them now and deal
with both formats for LDAP pushers until we rewrite...

Comprehensive tests of the functionality have been added.

There are a few minor areas where HTTP pushing is sub-par. They will be
addressed in subsequent commits.

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 8668190 [details]
MozReview Request: reviewboard: automatically use Bugzilla credentials from hgrc; r?smacleod, kang

reviewboard: automatically use Bugzilla credentials from hgrc; r?smacleod, kang

We already have users define Bugzilla usernames and API Keys in their
hgrc file under the "bugzilla.username" and "bugzilla.apikey" options.

Mercurial has its own method for storing credentials for URLs. To make
initial setup easier and so Bugzilla API Keys aren't duplicated in the
config files, we add integration into Mercurial's password manager that
will automatically use [bugzilla] defined credentials when talking to a
known Mozilla HTTP server.

We establish a list of trusted URL endpoints that we release the
credentials to. Unless an attacker convinces someone to change the
trusted list and connect to a rogue server (it's hard to prevent
phishing), the current implementation can only be exploited by
someone conducting a MitM, with a x509 certificate for a mozilla.org
hostname, and with that certificate signed by a CA trusted by the
client. This should be very unlikely. Keep in mind that if someone
is in a position to carry out this attack, they are probably in position
to intercept Bugzilla and other Mozilla traffic as well, so the Bugzilla
credentials can already be considered compromised. This is a long winded
way of saying I believe the current implementation provides sufficient
security.

Comment 38

[Gregory Szorc [:gps]]

Comment on attachment 8671008 [details]
MozReview Request: reviewboard: refactor repository discovery to facilitate HTTP pushing; r?smacleod

reviewboard: refactor repository discovery to facilitate HTTP pushing; r?smacleod

When we implemented review repository discovery, we assumed there would
only ever be a single URL we'd push to - the SSH URL. Now that we allow
pushes over HTTP, there are multiple URLs for each review repo.

We refactor how the server exposes review repos so it advertises both an
HTTP and SSH URL for each repo.

Since we had to create a new client capability so old clients wouldn't
choke on the new format, I also took the time to change how the data is
exposed to clients. Before, we were using the "pushkey" protocol. I'm no
longer a fan of shoving everything inside the pushkey protocol, so I
established a dedicated wire protocol command for exposing the raw file
contents. I dropped the old capability because the listkeys command to
retrieve data from it will return an empty set. It shouldn't be called
by modern clients since modern clients will require an extension upgrade
before we get to this code (due to the introduced client-side
capability).

A test for discovery over HTTP has been added. It discovered that
capabilities were being cached across repo redirects. This has also been
fixed.

Comment 39

[Gregory Szorc [:gps]]

Attachment: MozReview Request: docs/mozreview: document HTTP push configuration (bug 1195856); r?smacleod

docs/mozreview: document HTTP push configuration (bug 1195856); r?smacleod

Comment 40

[Guillaume Destuynder [:kang] [this account is no longer active]]

https://reviewboard.mozilla.org/r/20877/#review19447

Comment 41

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 8668190 [details]
MozReview Request: reviewboard: automatically use Bugzilla credentials from hgrc; r?smacleod, kang

hmm not sure how to mark r+ in reviewboard, i thought I did but the new one doesn't carry. Might have not done it correctly ;) doing it manually here at the risk of breaking more things :)

Patch looks fine to me though!

Comment 42

[Steven MacLeod [:smacleod]]

Comment on attachment 8668190 [details]
MozReview Request: reviewboard: automatically use Bugzilla credentials from hgrc; r?smacleod, kang

https://reviewboard.mozilla.org/r/20877/#review19695

Comment 43

[Steven MacLeod [:smacleod]]

Comment on attachment 8671008 [details]
MozReview Request: reviewboard: refactor repository discovery to facilitate HTTP pushing; r?smacleod

https://reviewboard.mozilla.org/r/21497/#review19697

Comment 44

[Steven MacLeod [:smacleod]]

Comment on attachment 8671487 [details]
MozReview Request: docs/mozreview: document HTTP push configuration (bug 1195856); r?smacleod

https://reviewboard.mozilla.org/r/21583/#review19699

::: docs/mozreview/install.rst:49
(Diff revision 1)
> +   and have never pushed via SSH before, you will need someone else to
> +   trigger *Try* jobs for you.

Right now only the author or those with a special permission can trigger try jobs. We should just open up try triggering to non-author's though so I think it's fine to leave this.

Comment 45

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/cd54cad68e086a35707be5a072009562c08f6d24
mozreview: add a script that validates Bugzilla API Keys (bug 1195856); r=smacleod, kang

https://hg.mozilla.org/hgcustom/version-control-tools/rev/23ab0c6249777671f739d5e9e65a69b1be27dca1
mozreview: support pushing via HTTP using Bugzilla credentials (bug 1195856); r=fubar, smacleod

https://hg.mozilla.org/hgcustom/version-control-tools/rev/8772dc578d8868a011f9ba828d7ff346995418bc
docs/mozreview: document HTTP push configuration (bug 1195856); r=smacleod

Comment 46

[Gregory Szorc [:gps]]

This is deployed and appears to be working (I was able to push over HTTPS using a BMO API Key).