This allows users to set their HTTP access passwords via Diffusion interface.
Details
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.
Clicked the "Set HTTP Access Password" link, set a password and saw it appear in the DB.
Diff Detail
Diff Detail
- Branch
- vcspassword
- Lint
Lint Passed - Unit
Tests Passed
Event Timeline
Comment Actions
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? |
| 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. |
Comment Actions
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.
Comment Actions
- 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.
| src/applications/diffusion/config/PhabricatorDiffusionConfigOptions.php | ||
|---|---|---|
| 87 | I think this option should disable plain HTTP by default, but perhaps HTTPS should be allowed? | |
Comment Actions
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).
Comment Actions
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.