(dev/core#3416) authx - If Authorization: header is disabled, then ignore it.

[totten]

Overview

Suppose a deployment has two layers of authorization:

• (1) Some generic/site-wide HTTP basic check (perhaps to prevent search-engines from finding the site; perhaps enforced by a reverse proxy)
• (2) Some Civi or CMS credential (eg session-cookie, ?_authx, or X-Civi-Auth:)

You would want to configure authx to ignore layer (1)'s Authorization: header -- while still checking some other layer (2) credential (eg ?_authx=Bearer+ASDF). In this case, you would configure authx to disable header support (eg authx_header_cred=[]).

This PR is based on the problem+solution reported by @davejenx via Mattermost. It adds test-coverage.

Before

You can configure authx_header_cred=[], but requests are rejected because it still evaluates the Authorization: header.

After

It only evaluates Authorization: if it's enabled. If it's disabled, then it's ignored.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[totten]

Relatedly (but not necessarily a blocker)... if someone has these 2 layers of authorization, and if they enable authx through the web UI, then they would initially get the default configuration (authx_header_cred=[jwt,api_key]). So they would still hit errors - and it could only be resolved by modifying authx_header_cred=[] (eg with cv / wp / drush or civicrm.settings.php)

On the flip side, if we change the default to authx_header_cred=[], then it wouldn't fail out-of-the-box for 2-layer deployments. But then the "HTTP standard way" wouldn't work by default (for regular 1-layer), and it also would be a bit too quiet in the (likely scenario) that a sysadmin has merely neglected to configure it.

I have maybe an emotional attachment to the simplicity of supporting standard HTTP auth by default, but realistically -- we don't actually support it out-of-the-box right now (user/pass support is opt-in b/c the security community's general enthusiasm for it is a bit weak). So maybe it's not really worth trying to support the standard as the default mode...

In which case it would make sense to change the default to authx_header_cred=[]?

[davejenx]

Hi @totten ,

Many thanks for this PR. Re the default for authx_header_cred, if your preference is to keep this as [jwt,api_key], which seems reasonable given that the 2-layer case is less common, then perhaps we could forestall the danger in the 2-layer case with some messaging to the user. Some text in info.xml would be a simple approach but not the most visible. Not sure if it's feasible to run a check for the presence of the Authorization: header in the /civicrm/admin/extensions request and if present, display a warning - tricky perhaps, as the extension would still be disabled at that point.

Thinking aloud now... Or perhaps when the extension is enabled, it could check for the presence of the Authorization: header and set the value of authx_header_cred accordingly, if it's not already set?

[totten]

Messaging would be great - but we'd probably have to add some new mechanism (akin to hook_requirements) for pre-install checks.

The idea of setting authx_header_cred during installation sounds pretty doable. Pushing up a revision to do that. (The commit note describes some r-run testing.)

[davejenx]

Hi Tim,

The code for setting authx_header_cred during installation worked well in my testing, where I enabled the extension through the UI with an Authorization header vs through cv.

A possible small edge case that doesn't seem very important: if the extension has previously been installed & configured and then uninstalled, its settings remain. If authx_header_cred had been explicitly set to an empty array and then the extension is re-enabled from cli (e.g. cv) where no Authorization header is provided, then authx_header_cred gets changed upon installation to ['jwt', 'api_key']. I'd have thought it would preferably stay as an empty array here. Maybe the check on authx_header_cred in authx_civicrm_install() could be for an absent value rather than empty?

While testing, I ran across an issue with the default for authx_guards which I suspect is spurious. When installing authx (with no authx* settings already in db), either via UI with Authentication header or via cv without, the default for authx_guards seemed to be empty, as evidenced both by behaviour (no guards applied) and by Civi::settings()->get("authx_guards") - whereas the default should be ['site_key', 'perm']. The test site is Civi 5.39.0 so I'm wondering if there's an incompatibility in the settings code with authx from master + this PR. I'll see if I can re-test on a vanilla instance.

[MattTrim1]

I've also ran into this scenario with some work on a site behind basic auth. Just wanted to confirm that applying the patch has resolved this issue, thanks for the patch!

[totten]

civibot, test this please

[MegaphoneJon]

I opened a Gitlab issue for this before this PR was pointed out to me: https://lab.civicrm.org/dev/core/-/issues/3416

[totten]

(@davejenx) If authx_header_cred had been explicitly set to an empty array and then the extension is re-enabled from cli (

Ah, I see what you're saying. getExplicit('authx_header_cred') could return NULL or [] (ie "not explicitly set" or "empty leftover from previous installation*). Both are FALSE-ish.

I'm a little torn:

• The intent of this line is "if it's not explicitly set, then set it". That would be better reflected by checking NULL === Civi::settings()->getExplict(...).
• There's a bigger issue around disable/uninstall/settings. Comparing disable/uninstall, a major goal of uninstall is to wipe the slate clean -- uninstall should wipe the settings. But I think it's quite rare for anyone to implement hook_uninstall and cleanup old settings.

If there was a general fix for uninstall/settings, then this conditional wouldn't matter.

I think maybe I erred in putting this under hook_install. Rather, hook_enable is probably a better fit. (To wit: This check should run for both new/clean-installs and existing/dirty re-activations. And it should be a NULL check.)

[totten]

I'll rebase/revise/r-run a couple of those edge-cases for the install-logic. After that, I'll add merge ready (since the main logic worked for @davejenx and @MattTrim1)

[totten]

Rebased. Retested. merge ready.

[demeritcowboy]

jenkins retest this please. Disk issue?

Warning: file_get_contents(/home/jenkins/bknix-dfl/build/core-22837-1fyax/web/sites/all/modules/civicrm/sql/test_data.mysql): Failed to open stream

[totten]

Warning: file_get_contents(/home/jenkins/bknix-dfl/build/core-22837-1fyax/web/sites/all/modules/civicrm/sql/test_data.mysql): Failed to open stream

I think the cleanup job kicked in a bit too early and deleted the job while it was running.

[composerjk]

It looks like this may still be an issue in 5.57.0. Using Cloudways with Digital Ocean, when creating a Staging instance, they set a Basic Auth username/password by default for the site. After authentication to the site through the Basic Auth piece, the main WordPress site worked but the CiviCRM part returned 401 Invalid Credential. Disabling the extra auth layer to the staging site allows access to Civi, again.

Since AuthX is required by civiimport, org.civicrm.afform, and org.civicrm.afform_admin and Form Builder and Form Core seem to be the forward direction for Civi, it makes it a little difficult to simply disable the AuthX extension.

[totten]

@composerjk Yeah, I agree that disabling authx probably isn't a long-term resolution. But within authx, the Basic Auth should still be optional.

I'm not familiar with Cloudways, but you mention "creating a staging instance". Is the issue like this?

1. Setup live instance.
2. Install authx (etal) on the live instance.
   • At this point, it's the live site. The web-host has not added an extra tier of "Basic Auth". So the authx installer figures that it's safe to enable "Basic Auth" (ie authx_header_cred=["jwt","api_key"]). And in that environment, it's fine.
3. Copy the DB from the live instance to a new staging instance
   • The staging inherits the settings which enable "Basic Auth" for authix (ie authx_header_cred=["jwt","api_key"]).
4. Go the staging instance. But now the web-host adds another tier of "Basic Auth".
   • Here we get the conflict -- where both authx and the web-host are trying to use "Basic Auth" for their own purposes.

FWIW, for a specific site/one-off fix, you might address by either:

• On the live site (before making a copy), go to "Administer => System => Authentication". Delete all the options for "Acceptable credentials (HTTP Header) ". (This updates the setting authx_header_cred to []. And it should fix any future copies of the site.)
• On the staging site (after making a copy), use CLI to update the setting, e.g.

   cv vset authx_header_cred=[]
  

Of course, it's better if we can find a more general resolution...

[composerjk]

Thanks, @totten! That's helpful.

I'm just testing Cloudways, at the moment, for our nonprofit to see if it might be a reasonable hosting option. I believe your description of the "creating a staging instance" is probably similar to what they do behind the scenes for the 1-click create staging feature. I can choose to disable the Basic Auth option, but seemed like it was worth reporting. The cv vset authx_header_cred=[] on the staging site does allow access to CiviCRM even with the Basic Auth layer in front.

I hadn't yet configured any aspect of authx on the live instance.

If we go with Cloudways, I will likely suggest having an option to add some notes when the staging server is created to remind folk to access appropriate URLs to reset caches and paths. Or an option to have some commands run on the server after creation to do so. Cloudways is mostly a layer on top of other cloud services; in this case, managing a Digital Ocean droplet, I believe.

[stesi561]

Just flagging I'm still seeing this as well and the cv vset authx_header_cred=[] works.

Note error message observed on CiviCRM screens is "HTTP 401 Invalid credential" - just to make finding this again easier!