Status Check - Raise severity of the check for signing keys

[totten]

Overview

The CIVICRM_SIGN_KEYS was introduced as an optional feature. It's becoming important as more components (like afform) rely on JWTs.

Before

If CIVICRM_SIGN_KEYS is missing, then it gives a NOTICE.

After

If CIVICRM_SIGN_KEYS is missing, then it gives a WARNING an ERROR.

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

Suggested change

	ts('Signing Key Strongly Recommended'),
	ts('Signing Key Missing'),

[totten]

Right, "Strongly Recommended" is verbose. "Missing" reads a little weird to me, because none of the other headings end with "-ing", and "Signing Key Missing" has two "-ing"s.

How about "Signing Key Undefined" or just plain "Signing Key" (or go back to just "Signing Key Recommended")?

or

[totten]

Aside: Trimming those titles ("Database Upgrade Required", "Debug Mode Enabled", "Signing Key Recommended", "PHP") would probably make it cleaner/easier to read/easier to compose. It would be harder to google on on the message-title, but then I think people would google for the text instead...

[colemanw]

@totten the docs are rather long and complex, here's an addition to offer admins a quick fix for the error: https://lab.civicrm.org/documentation/docs/sysadmin/-/merge_requests/345

[demeritcowboy]

Is it a known short list of features that require this key? Can it look up if you have those features enabled and then make it an ERROR and say "Signing key required", otherwise not even display the check if you don't have those features enabled?

Some of the awkwardness here I think is because it's not clear whether you need the key or not, and so trying to come up with wording that is short and strong but has wiggle room is difficult.

[colemanw]

I agree with @demeritcowboy - why not move the status check into the AuthX extension itself, and make it an error not a warning.

[totten]

(demerit) Is it a known short list of features that require this key?

Circumstantially, yes. Design-wise, no.

For example, within civicrm-core.git, we can search around and find our current circumstances:

• authx's Authenticator has an option to login via JWT, so it uses crypto.jwt. That, in turn, uses the SIGN key.
• afform has a mechanism to upload files. That uses a JWT token, so it uses crypto.jwt. That, in turn, uses the SIGN key.
• afform has an option to make authenticated hyperlinks to forms. (Hey {contact.first_name}. Please update your <a href="{afform.profileUrl}">profile</a>.) That relies on authx for JWT logins (which in turn relies on crypto.jwt and thus SIGN).

That's 3 things which transitively depend on the SIGN key.

However, in my mind, the arrangement is part of balancing act:

• It's bad for everything to rely on one secret. You get forced into some awkward situations. ("Bob just left the organization, but he knew the SITE_KEY, because his mobile client needed it for REST calls. But if we change the SITE_KEY, then it breaks the encryption on our SMTP password, and it causes all the case-ids to change. Is it worthwhile to update SITE_KEY?")
• It's bad for everything to define separate secrets. You have more stuff to administer. ("I installed authx, and now it needs a system-level key to sign login-tokens. I installed afform, and now it needs a system-level key to sign upload-tokens. I installed inlay, and now it needs a system-level token to something else.")

The current middle-ground is to define a few reusable keys (SIGN, CRED) with different purposes/qualities. It is an affirmative feature that the SIGN key can be used in multiple ways (e.g. authx logins; afform uploads; afform mail-tokens) as long as they are qualitatively similar (i.e. they all sign tokens on behalf of the system; tokens that will be sent back to us later).

I think it's important to have some intentional design like that. (Of course, that doesn't mean the current middle-ground is perfect or permanent.) But I wouldn't want to change it just to make an obscure status-check look nicer. (The message doesn't show up on new sites; and as soon as you fix it, it goes away forever.)

(coleman) why not move the status check into the AuthX extension itself,

Overall complexity...

Right now, it doesn't matter who (exactly) uses the SIGN key. We don't have to think about the dependency-graph. The rule is simple. ( If you use CiviCRM, then you should have a SIGNing key.)

For the enhanced messaging ("You need ABC for X+Y+Z"), then that requires thinking about the dependency-graph (ie more complexity).

[demeritcowboy]

Ok then I'd suggest either warning or even error, but mainly remove any vagueness from the wording, so:

Signing key required
Please configure <a %1>CIVICRM_SIGN_KEYS</a>

[demeritcowboy]

I'm good with this version.