Page MenuHomePhabricator
Create Task
Maniphest T13625

Daemons may make commit-related API calls as users without privileges
Closed, ResolvedPublic
Actions

Description

Most likely due to recent refactoring, daemons sometimes attempt to interact with the API while acting as a user who does not have permission. I believe these cases most likely arise when the author of a commit is invalid (not a user, a mailing list, a disabled user, etc).

  • See PHI1908, which is likely "not a user":
[2020-10-15 07:37:57] EXCEPTION: (PhutilProxyException) Error while executing Task ID 21488284. {>} (PhabricatorPolicyException) [You Shall Not Pass: Restricted Repository] (Can View) You do not have permission to view this object. // This object has a custom policy controlling who can take this action. at [<phabricator>/src/applications/policy/filter/PhabricatorPolicyFilter.php:713]
arcanist(head=stable, ref.master=d54cb072facd, ref.stable=a798e740719b), phabricator(head=stable, ref.stable=0e4d62847cfc, custom=3)
#0 <#2> PhabricatorPolicyFilter::rejectObject(PhabricatorRepository, string, string) called at [<phabricator>/src/applications/policy/filter/PhabricatorPolicyFilter.php:590]
#1 <#2> PhabricatorPolicyFilter::checkCapability(PhabricatorRepository, string) called at [<phabricator>/src/applications/policy/filter/PhabricatorPolicyFilter.php:268]
#2 <#2> PhabricatorPolicyFilter::apply(array) called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:273]
#3 <#2> PhabricatorPolicyAwareQuery::execute() called at [<phabricator>/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php:180]
#4 <#2> PhabricatorPolicyAwareQuery::executeOne() called at [<phabricator>/src/applications/diffusion/request/DiffusionRequest.php:160]
#5 <#2> DiffusionRequest::newFromIdentifier(string, PhabricatorUser, NULL) called at [<phabricator>/src/applications/diffusion/request/DiffusionRequest.php:109]
#6 <#2> DiffusionRequest::newFromDictionary(array) called at [<phabricator>/src/applications/diffusion/conduit/DiffusionQueryConduitAPIMethod.php:99]
#7 <#2> DiffusionQueryConduitAPIMethod::execute(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/method/ConduitAPIMethod.php:123]
#8 <#2> ConduitAPIMethod::executeMethod(ConduitAPIRequest) called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:131]
#9 <#2> ConduitCall::executeMethod() called at [<phabricator>/src/applications/conduit/call/ConduitCall.php:81]
#10 <#2> ConduitCall::execute() called at [<phabricator>/src/applications/diffusion/query/DiffusionQuery.php:82]
#11 <#2> DiffusionQuery::callConduitWithDiffusionRequest(PhabricatorUser, DiffusionMercurialRequest, string, array) called at [<phabricator>/src/applications/differential/engine/DifferentialDiffExtractionEngine.php:57]
#12 <#2> DifferentialDiffExtractionEngine::newDiffFromCommit(PhabricatorRepositoryCommit) called at [<phabricator>/src/applications/differential/engine/DifferentialDiffExtractionEngine.php:246]
#13 <#2> DifferentialDiffExtractionEngine::updateRevisionWithCommit(DifferentialRevision, PhabricatorRepositoryCommit, array, PhabricatorDaemonContentSource) called at [<phabricator>/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php:186]
#14 <#2> DiffusionUpdateObjectAfterCommitWorker::updateRevision(PhabricatorRepositoryCommit, DifferentialRevision) called at [<phabricator>/src/applications/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php:57]
#15 <#2> DiffusionUpdateObjectAfterCommitWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:124]
#16 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:159]
#17 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:22]
#18 PhabricatorTaskmasterDaemon::run() called at [<phabricator>/src/infrastructure/daemon/PhutilDaemon.php:219]
#19 PhutilDaemon::execute() called at [<phabricator>/scripts/daemon/exec/exec_daemon.php:131]
  • Observed in production, this looks like it might be a mailing list or disabled user.
[27-Feb-2021 19:28:02 UTC] [2021-02-27 19:28:02] EXCEPTION: (PhutilProxyException) Error while executing Task ID 9283371. {>} (ConduitClientException) ERR-INVALID-AUTH: <diffusion.rawdiffquery> User account is not permitted to use the API. at [<arcanist>/src/conduit/ConduitFuture.php:70]
[27-Feb-2021 19:28:02 UTC] arcanist(head=stable, ref.master=ade25facfdf2, ref.stable=b177e489b4c8), libcore(), phabricator(head=stable, ref.stable=5c8f0362f8f9), services(head=stable, ref.master=b5cef1ac31ff, ref.stable=2f820c62
d504)
[27-Feb-2021 19:28:02 UTC]   #0 <#2> ConduitFuture::didReceiveResult(array) called at [<arcanist>/src/future/FutureProxy.php:40]
[27-Feb-2021 19:28:02 UTC]   #1 <#2> FutureProxy::isReady() called at [<arcanist>/src/future/Future.php:63]
[27-Feb-2021 19:28:02 UTC]   #2 <#2> Future::updateFuture() called at [<arcanist>/src/future/FutureIterator.php:224]
[27-Feb-2021 19:28:02 UTC]   #3 <#2> FutureIterator::next() called at [<arcanist>/src/future/FutureIterator.php:190]
[27-Feb-2021 19:28:02 UTC]   #4 <#2> FutureIterator::rewind()
[27-Feb-2021 19:28:02 UTC]   #5 <#2> iterator_to_array(FutureIterator) called at [<arcanist>/src/future/FutureIterator.php:84]
[27-Feb-2021 19:28:02 UTC]   #6 <#2> FutureIterator::resolveAll() called at [<arcanist>/src/future/Future.php:47]
[27-Feb-2021 19:28:02 UTC]   #7 <#2> Future::resolve() called at [<phabricator>/src/applications/diffusion/query/DiffusionQuery.php:83]
[27-Feb-2021 19:28:02 UTC]   #8 <#2> DiffusionQuery::callConduitWithDiffusionRequest(PhabricatorUser, DiffusionGitRequest, string, array) called at [<phabricator>/src/applications/differential/engine/DifferentialDiffExtractionEn
gine.php:58]
[27-Feb-2021 19:28:02 UTC]   #9 <#2> DifferentialDiffExtractionEngine::newDiffFromCommit(PhabricatorRepositoryCommit) called at [<phabricator>/src/applications/differential/engine/DifferentialDiffExtractionEngine.php:246]
[27-Feb-2021 19:28:02 UTC]   #10 <#2> DifferentialDiffExtractionEngine::updateRevisionWithCommit(DifferentialRevision, PhabricatorRepositoryCommit, array, PhabricatorDaemonContentSource) called at [<phabricator>/src/applications
/diffusion/worker/DiffusionUpdateObjectAfterCommitWorker.php:189]
[27-Feb-2021 19:28:02 UTC]   #11 <#2> DiffusionUpdateObjectAfterCommitWorker::updateRevision(PhabricatorRepositoryCommit, DifferentialRevision) called at [<phabricator>/src/applications/diffusion/worker/DiffusionUpdateObjectAfte
rCommitWorker.php:57]
[27-Feb-2021 19:28:02 UTC]   #12 <#2> DiffusionUpdateObjectAfterCommitWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:124]
[27-Feb-2021 19:28:02 UTC]   #13 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:160]
[27-Feb-2021 19:28:02 UTC]   #14 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:22]
[27-Feb-2021 19:28:02 UTC]   #15 PhabricatorTaskmasterDaemon::run() called at [<phabricator>/src/infrastructure/daemon/PhutilDaemon.php:219]
[27-Feb-2021 19:28:02 UTC]   #16 PhutilDaemon::execute() called at [<phabricator>/scripts/daemon/exec/exec_daemon.php:131]
  • See PHI1926, likely the same as above.
[03-Nov-2020 19:09:42 UTC] [2020-11-03 19:09:42] EXCEPTION: (PhutilProxyException) Error while executing Task ID 4251440. {>} (ConduitClientException) ERR-CONDUIT-CORE: <diffusion.rawdiffquery> [You Shall Not Pass: Restricted Repository] (Can View) You do not have permission to view this object. // This object has a custom policy controlling who can take this action. at [<arcanist>/src/conduit/ConduitFuture.php:70]
[03-Nov-2020 19:09:42 UTC] arcanist(head=stable, ref.master=e3030ebcad53, ref.stable=ac54d61d7af2), libcore(), phabricator(head=stable, ref.master=87fb35abb72e, ref.stable=86ad69863930), services(head=stable, ref.master=35f9c1919842, ref.stable=a2c67587b5df)

Revisions and Commits

rP Phabricator
D21582rP55f4a258d2de When updating revisions in responset to commits, use the omnipotent viewer to…

Related Objects

Event Timeline

epriestley triaged this task as Normal priority.Feb 27 2021, 7:38 PM
epriestley created this task.
epriestley updated the task description. (Show Details)Feb 27 2021, 7:41 PM
epriestley added a comment.Mar 1 2021, 6:15 PM

All three of these are almost certainly diffusion.rawdiffquery, I think the first one just predated D21463 reaching production so it didn't show the method.

epriestley added a revision: D21582: When updating revisions in responset to commits, use the omnipotent viewer to pull diffs.Mar 1 2021, 7:00 PM
epriestley added a comment.Mar 1 2021, 7:09 PM

When we are importing a commit, we often don't have any legitimate user to act as: the commit may come from an observed repository (so we never authenticated a pusher), and the "Author" and "Committer" strings are arbitrary and untrusted in Git and Mercurial.

Although they're untrusted (and may not exist), we currently try to act as the Author/Committer if possible when publishing commits we import, since this is best aligned with user expectation in the overwhelming majority of cases.

We get into a bit of trouble if we're trying to publish a commit that has language or references (like Fixes T123 or Differential Revision: D456) pointing at other objects. The user we're acting as may not have permission to view those objects.

  • For tasks, we currently don't update those objects.
  • For revisions, after D21582, we currently act as the omnipotent user and do update those objects.

Both "use the author" and "use the omnipotent user" generally have the same security/policy implications: an attacker can just make a commit using a privileged user's author string anyway, so the less-powerful behavior here is just a guard rail, not a security feature.

Neither case represents a policy violation per se. In both cases, you can attach a commit to a revision or task you can't see (and possibly cause a status change) but you can't read any information or affect any other fields. This is possibly surprising (and, when it occurs, likely either an attack or unintended) but not really a problem.

I think a better behavior in both cases would be:

  1. Don't make the update.
  2. Clearly show on the commit why we didn't make the update.

Implementing (2) is a bit complex, though, since there are a wide range of possible failures which could lead to (1).

epriestley added a commit: rP55f4a258d2de: When updating revisions in responset to commits, use the omnipotent viewer to….Mar 1 2021, 7:11 PM
epriestley updated the task description. (Show Details)Mar 2 2021, 11:29 PM
epriestley mentioned this in 2021 Week 10 (Early March).Mar 5 2021, 9:29 PM
epriestley closed this task as Resolved.Mar 10 2021, 6:26 PM

Although I didn't look particularly hard, I can't immediately find any more evidence that this is occurring in production.

The general "nice-to-have" improvement to the approach outlined above remains desirable, but I'm going to leave that for another time.

Phabricator · Built by Phacility