Page MenuHomePhabricator

Refactor Conduit auth to be stateless, token-based, and support wire encodings
Open, NormalPublic

Description

Conduit authentication currently supports two mechanisms:

  • A session-based mechanism, where you use conduit.connect to establish a session.
  • An undocumented stateless mechanism likely used only by Facebook.

The first mechanism is unnecessarily complex and makes Conduit slow (extra round trips) and hard to use (can't use CURL, pain to write clients). We should move away from it and deprecate it.

The actual wire token is needlessly complicated. We transmit a proof-of-token, not the token itself. The proof we transmit is not a request signature, so all this does is make replay attacks moderately more difficult. In practice, it just causes a bunch of issues for users with bad client or server timestamps or goofy environmental problems. No one has ever expressed interest in upgrading to a request signature scheme.

Because the wire token is needlessly complicated, the token/handshake/certificate UIs are also needlessly complicated and users hit a bunch of issues using them. These UIs should just be "generate session", which gives you a durable token, with attendant session management/review capabilities.

Upshot:

  • Deprecate conduit sessions and conduit.connect.
  • Support direct token-based auth (?token=abdef123) and make this the standard.
  • Leave room for a proof-of-token + request-signature flavor of this eventually.
  • Support SSH auth.
  • Support multiple request encodings (likely BSON, protobuf, or messagepack). Leave JSON as the default, but in cases where messages can not be represented in JSON this gives us a plausible way forward.
  • Fix the UIs to make handshakes and session management straightforward.

Related Objects

StatusAssignedTask
Resolvedepriestley
Resolvedepriestley
ResolvedNone
Wontfixepriestley
OpenNone
DuplicateNone
Resolvedepriestley
OpenNone
OpenNone
Resolvedepriestley
OpenNone
OpenNone
OpenNone
Openepriestley
Resolvedepriestley
OpenNone
DuplicateNone
DuplicateNone
OpenNone
Resolvedbtrahan

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
epriestley added a project: Phacility.

T2783 is at least partially blocked on this, since I need to make intra-cluster calls as the viewing user and don't want to do handshakes. I also don't really want to ship session keys across in the raw, although that wouldn't be the end of the world. I'll probably push this about halfway forward and then drop it again once T2783 is unblocked.

epriestley moved this task from Backlog to v0 Closed Beta on the Phacility board.Dec 12 2014, 6:05 PM
epriestley removed epriestley as the assignee of this task.Dec 13 2014, 6:27 PM
epriestley removed a project: Phacility.

At least for now, this no longer blocks Phacility or T2783.

The utf8 stuff hasn't been coming up much; the token-based stuff is just blocked on docs and also hasn't come up much; I touched the protocol enough that I'm pretty confident we can move forward without backward compatibility breaks.

Is the above commit able to be used or are there a few steps to go? Gave it a quick try and it still wanted my bot user to install a certificate despite providing an API token with the above parameter.

I'd expect it to work on relatively up-to-date Phabricator. You may still need to have some valid-ish ~/.arcrc file, which we might be reading and then ignoring. I'll check that and fix it if it's the case -- we shouldn't require a ~/.arcrc if you specify --conduit-token.

Great, thanks. It appears that it does still need the certificate in a .arcrc file somewhere, as we removed that file as part of the above test and received the certificate prompt.

As a goofy workaround you can probably write some dummy file

dummy.arcrc
{
  "hosts": {
    "http://blah.yourcompany.com": {}
  }
}

...and specify that with --arcrc-file, but this is stupid; I'll just fix the actual problem.

Yeah that's what we were doing actually, but given our environment and arcanist's checks regarding the file permissions (that it's 600, etc.) it was messy due to shell restrictions and other stuff where our scripts are running. This will be great for build agents posting back Harbormaster statuses.

@epriestley Is there an ETA on this? It's been 7 days since I cleared out failing Git parser tasks and there's another 20 I need to clear out now. This issue has a significant impact on our background processing because commits without UTF8 data in them effectively block the queue.

commits without UTF8 data in them effectively block the queue.

This is unexpected. What are you seeing that makes you think they're blocking the queue?

Tasks with at least any failures automatically de-prioritize and are executed after tasks with no failures. The expectation is that the screenshot in T8857 does not show a block queue, just a queue with some cruft at the end of it.

Generally we find that the commit / message parser tasks tend to get done first, and take up taskmasters that could be processing Harbormaster builds. This delays builds because we have less taskmasters available.

If it were the case where the commit / message parser tasks become deprioritized, I would expect to see the currently leased tasks quickly replaced entirely with Harbormaster tasks when a build with 50+ targets starts. But we don't see that; instead we see the same commit / message parser tasks being attempted (with failure counts >2000) being leased, with the remaining slots being used for builds (and search and other tasks).

Okay, can you file a separate issue for that and I'll try to reproduce it? That's bad and not the expected queue behavior.

epriestley moved this task from Backlog to vNext on the Conduit board.Dec 8 2015, 10:53 PM
eadler added a project: Restricted Project.Jan 8 2016, 10:27 PM
eadler moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Jan 8 2016, 10:42 PM

How can i use DiffusionBrowseController.php to brow my files with non-latin character names correctly?

nchammas removed a subscriber: nchammas.Mar 10 2016, 3:45 PM
erich added a subscriber: erich.May 20 2016, 9:29 PM
scode added a subscriber: scode.Jun 3 2016, 4:27 PM
fcoelho added a subscriber: fcoelho.Jun 8 2016, 5:39 PM
urzds added a subscriber: urzds.Jul 12 2017, 11:13 AM
scp added a subscriber: scp.Jun 20 2018, 7:05 PM