Should the credential settings you already enumerate be encrypted at rest?

[timujinne]

We were about to encrypt an existing oauth_google_client_secret value at rest
and stopped, because the read path would not have decrypted it. Before
proposing anything we would like to know the intended position, since the code
suggests this was already thought about.

What we measured on live installs

PhoenixKit.Integrations.Encryption lists client_secret among
@sensitive_fields (encryption.ex:66), and integration reads decrypt —
Encryption.decrypt_fields/1 is called throughout integrations.ex
(117, 128, 162, 317, 707) and in key_rotation.ex.

The settings subtree contains no encryption at all: Encryption, encrypt
and enc:v1 occur zero times across settings.ex, setting.ex,
queries.ex and events.ex. get_google_oauth_credentials/0
(settings.ex:934) reads the secret as a plain map lookup through
get_settings_cached/2, so an encrypted value would be handed to the provider
verbatim.

So the same conceptual field — an OAuth client secret — is encrypted at rest in
one store and not in the other, and the two stores are in the same library.

The list of affected keys already exists

settings.ex:91 defines @restricted_setting_keys:

oauth_google_client_secret
oauth_github_client_secret
oauth_facebook_app_secret
aws_access_key_id
aws_secret_access_key

Its comment says these "hold live credential material", and explains that
oauth_*_client_id is deliberately excluded because it is public by OAuth's
own design. So the library already knows exactly which setting keys are
credentials. That list is used to restrict reads; it does not affect how the
values are stored.

Note this is wider than OAuth — the AWS keys sit in the same list.

The mechanism already exists, and is already keyed

This is why we think the question is narrower than it first looks:

• Encryption.encrypt_value/1 and decrypt_value/1 work on single values,
  not only on integration maps, so no new primitive is needed.
• On both installs we checked, Encryption.status/0 already returns
  :dedicated — a dedicated encryption key is configured and in use.

So this is not "where would a key come from"; the key is already there and
already protecting the integration store. The question is whether you want the
same treatment applied to the settings this list already names.

The part that reads as deliberate

insert_setting/1 (queries.ex:181) explains at length that this table keeps
every value in the same two generic columns, that redact: never reaches
Ecto's SQL logger, and that log: false is therefore cheaper and safer than
enumerating sensitive keys — noting it was found printing a literal secret on a
live install. That is a considered answer to secrets in this table, which is
why we are asking rather than assuming a gap.

Our questions

1. Is plaintext at rest the intended position for these settings, with log
   suppression as the chosen mitigation? A short note near
   @restricted_setting_keys would save the next person this investigation.

2. If not, would you accept encrypting exactly the keys that list already
   names, with a decrypting read path for them? We can see one alternative —
   moving OAuth provider credentials into the integrations store that already
   encrypts them — but that looks like a larger change for the same result.

   One tension we may be misreading: insert_setting/1's comment says
   enumerating which keys are sensitive is not worth doing there, while
   @restricted_setting_keys is exactly such an enumeration a few hundred
   lines away. If those are deliberately separate concerns, saying so would
   settle it.

3. Either way there are values already written in plaintext on existing
   installs, and get_settings_cached/2 holds decrypted values in memory.
   Do you have a preference for how those are handled?

We have not changed anything. Happy to open a PR once we know which direction
you would accept — this did not look like a decision to make from the outside.

---

Unrelated to the above, and in a different repository — mentioning it here to
avoid a separate issue. In phoenix_kit_ecommerce (0.3.0), the Shopify
provider's moduledoc still says under "Known limitation":

This provider deliberately omits :validation, so "Test Connection" always
reports success without actually checking anything.

Since do_validate/2's catch-all now returns :unverified rather than :ok,
Test Connection reports "Not verified — this provider has no connection check"
instead of success. The limitation itself still holds — there is no check — but
the sentence describing what the button does is no longer accurate, and it is
the kind of sentence someone auditing connection status would rely on. Happy to
send a one-line PR there if you would like it.