Implementation of VCS passwords against user.
ClosedPublic
Actions

Authored by epriestley on Oct 30 2013, 9:43 PM.

Details

Reviewers

hach-que
btrahan

Group Reviewers

Blessed Reviewers

Maniphest Tasks

Restricted Maniphest Task

Commits

Restricted Diffusion Commit
rP0278b15ceb5d: Implementation of VCS passwords against user.

Summary

This allows users to set their HTTP access passwords via Diffusion interface.

Test Plan

Clicked the "Set HTTP Access Password" link, set a password and saw it appear in the DB.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

A couple minor things -- fix whatever makes sense and I'll pull this?

resources/sql/patches/20131031.vcspassword.sql
5	Oh, I actually didn't intend for this to become per-repo. I'm fine with that if you think it's useful, but we could also just drop it. We'll need to make the auth logic a little more sophisticated to keep it. It seems maybe-better to me overall.
11	ENGINE=InnoDB, CHARSET utf8;
src/applications/diffusion/controller/DiffusionRepositoryController.php
353–361 ↗	(On Diff #16814)	Ah, nice! I want to improve the hosting messaging in general, but this is a pretty clean start.
src/applications/diffusion/controller/DiffusionSetPasswordController.php
14–18 ↗	(On Diff #16814)	You shouldn't need EDIT to be able to set a VCS password -- I think you can remove this (VIEW is default).
43 ↗	(On Diff #16814)	At some point we should check if this is your current password. We should also let you remove the password entirely. We can deal with that later, though.
46 ↗	(On Diff #16814)	Make the hashing part a method on the object, and expose a `setPassword()` or similar? We should do HMAC / salt / iterations / etc before making this live, but that's very minor.
95 ↗	(On Diff #16814)	"Encoding" copy/paste?

Unknown Object (User) added inline comments.Oct 31 2013, 3:37 AM

resources/sql/patches/20131031.vcspassword.sql
5	I don't think it's particularly useful, but you did say you didn't want it on the user table. Where should we put it if it's per-user but not on the user table?
src/applications/diffusion/controller/DiffusionSetPasswordController.php
14–18 ↗	(On Diff #16814)	Will fix.
43 ↗	(On Diff #16814)	Definitely think these things can be dealt with later. Not really sure to go about checking if the entered password is the same as the user's password, because of all the auth system.
46 ↗	(On Diff #16814)	If you want to fix the hashing when you land this I think that'll be better since I have no experience with how Phabricator's hashing / salting / etc. is structured
95 ↗	(On Diff #16814)	Will fix.

Made changes requested in code review.

My interest in keeping it out of the user table is mostly a desire to keep the object simple. At Facebook, a lot of stuff that didn't really need to be on the "user" object got added there (like "Dorm Mailbox Number"), and the combined weight of all that stuff eventually added up and became overwhelming, so "user" was split into "user fields we actually use all over the place" and "user fields which probably shouldn't be there because they're not really part of the user". I'd like to head this issue off at the pass by resisting the urge to stick anything which can reasonably fit on the user on it. VCS Passwords seem clearly-not-fundamental-to-the-user.

Prior to hashing, putting the field on User also risks it leaking in stack traces in plain text, which is a huge amount of surface area. We can mitigate this with load/save hooks and PhutilOpaqueEnvelope, but moving it off seems cleaner.

I don't think "password" should actually be on the user, either, for the same reasons: it doesn't need to be, and we slightly increase risk by having it there. But I don't think fixing it is a huge priority.

This looks good to me. I'll pull this once I dig through the rest of my email.

I'll poke this a bit.

pokey pokey

Move back to Settings since it's no longer per-repository.
Use "Password" input type.
Add "Confirm Password".
Show hinting if the user has a password.
Suggest random password if the user does not have a password.
Allow password removal.
Prevent empty password.
Prevent password identical to login password.
Hash password using SHA1+HMAC and 1000 iterations.
Use PhutilOpaqueEnvelope to pass passwords.
Add diffusion.allow-http-auth setting to lock this mechanism down globally.

The major thing missing here usability-wise is hinting in the Diffusion UI ("go set this setting", "go set a password", etc.). I'm going to tackle that soon, but we need similar hinting around clone URIs too, so I'll do them both at once.

epriestley added a reviewer: btrahan.Oct 31 2013, 4:16 PM

hach-que added inline comments.Oct 31 2013, 9:12 PM

src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php
87	I think this option should disable plain HTTP by default, but perhaps HTTPS should be allowed?

Went through and set up a VCS password, removed it and also use the generated one. It all looks to be working.

The code looks reasonably good, but I'd like to consider having HTTPS enabled by default (with the allow-http-auth simply toggling whether you can auth over plain HTTP).

I want to try this first -- I like being able to completely knock out the panel when the option is off, and it's now possible for "Create Repository" permission to be given to arbitrary users. I'm more worried about the characteristics of HTTP password auth (it's transmitted in the clear on every request, it's often cached in places users may not be familiar with, it's relatively short compared to an SSH key, it doesn't have a standard, secure approach to managing it, etc.). Individually, none of these are a real problem, but all the barriers to something bad happening are lower than with SSH.

I'd generally like to discourage authenticated HTTP for other reasons, too -- for example, it needs to buffer the entire change into RAM during a push, and I don't think there's anything we can do about that with PHP as the server software. From SSH, we can stream things correctly.

I will add more callouts and hinting in the UI, though ("This won't work until you go enable option X.", "go set a VCS password here" and such). If this ends up being cumbersome we can reduce the strength or get rid of it, but I think we're way better off in the long run if we can get all installs which can reasonably use SSH using it.