wolfsshd: bind FPKI certificate UPN realm to AuthorizedUPNDomains

[yosuke-wolfssl]

wolfsshd: bind FPKI certificate UPN realm to AuthorizedUPNDomains

Summary

When WOLFSSL_FPKI is enabled, wolfsshd validated a client certificate's UPN
SAN by comparing only the local part (the text before @) to the requested SSH
username and discarding the realm/domain. A client holding a valid certificate
from a trusted CA for alice@other-domain.example could therefore
authenticate as local user alice. This is most dangerous on the CA-only path
(no AuthorizedKeysFile set), where the CA-verified chain plus the local-part
match are the only gates.

This PR adds an opt-in allowlist of permitted UPN realms, AuthorizedUPNDomains.
When set, a certificate's UPN realm must exactly match one of the configured
domains (case-insensitive) in addition to the existing local-part match. When
unset, behavior is unchanged for backward compatibility and a warning is logged
so operators know the domain is not being checked.

Addressed by f_5580.

Problem

RequestAuthentication (apps/wolfsshd/auth.c) scanned the UPN SAN up to @
and matched only the local part:

for (idx = 0; idx < current->len; idx++) {
    if (current->name[idx] == '@') break;
}
if ((int)XSTRLEN(usr) == idx && XSTRNCMP(usr, current->name, idx) == 0) {
    usrMatch = 1;
}

The realm after @ was explicitly ignored, so any trusted-CA certificate whose
local part equalled the target username was accepted regardless of its domain.

Solution

• New config option AuthorizedUPNDomains — a whitespace/comma separated
  list of permitted UPN realms. Plumbed like the other WOLFSSHD_CONFIG string
  options (struct field, option table, copy/free, getter) and scoped to Match
  blocks via the existing config copy.
• New helper MatchUPNToUser() that performs the local-part match and, when
  an allowlist is configured, requires the certificate's realm to be present and
  to match a listed domain exactly (case-insensitive). It is length-bounded
  (nameSz) because certificate altName buffers are not guaranteed to be
  NUL-terminated.
• The multi-value ForceCommand/AuthorizedUPNDomains storage was de-duplicated
  into a shared SetListString() helper instead of a per-option handler.

Example configuration:

# /etc/ssh/sshd_config
TrustedUserCAKeys /etc/ssh/ca-cert-ecc.pem
HostCertificate   /etc/ssh/server-cert.pem

# only accept certificate UPNs in these realms
AuthorizedUPNDomains corp.example partner.example

Behavior:

AuthorizedUPNDomains	Certificate UPN	Result
unset	alice@anything	accepted (local part only) + warning logged
corp.example	alice@corp.example	accepted
corp.example	alice@other.example	rejected
corp.example	alice (no realm)	rejected

Backward compatibility

Unset is the default and preserves the prior local-part-only behavior, so
existing FPKI deployments are unaffected until they opt in. A WS_LOG_WARN is
emitted on each authentication attempt in which a certificate UPN matches while
the option is unset, making the unchecked-domain gap visible in logs.

The warning is deliberately per-attempt rather than one-time-per-process: a
one-time flag would require shared mutable state, which is racy in threaded
builds (Windows), and the log volume is bounded by the rate of successful
certificate logins on a deployment that has chosen not to opt in.

Security guidance

AuthorizedUPNDomains is opt-in, so the default remains fail-open with respect
to the certificate realm. FPKI deployments that rely on the CA-only path (no
AuthorizedKeysFile) should always set AuthorizedUPNDomains, since the
CA-verified chain and the local-part match are otherwise the only gates on
identity. The per-attempt warning exists to make an un-opted-in deployment
visible in the logs.

Files changed

• apps/wolfsshd/configuration.c, apps/wolfsshd/configuration.h — new option,
  getter, and shared SetListString() helper.
• apps/wolfsshd/auth.c, apps/wolfsshd/auth.h — MatchUPNToUser() and its
  use in RequestAuthentication; helper exported under WOLFSSHD_UNIT_TEST
  via WOLFSSHD_STATIC.
• apps/wolfsshd/wolfsshd.c — wolfsshd -? advertises a Build features: FPKI certificate UPN domain checking line so test scripts can detect the feature
  deterministically.
• apps/wolfsshd/test/test_configuration.c — unit tests for option parsing,
  Match-block copy, and MatchUPNToUser (match/mismatch, missing/empty realm,
  multi/comma/tab/CR-LF separators, case-insensitivity, and length-bounded
  buffers).
• apps/wolfsshd/test/create_sshd_config.sh,
  apps/wolfsshd/test/sshd_x509_upn_fail.sh,
  apps/wolfsshd/test/run_all_sshd_tests.sh — end-to-end negative test: a cert
  with realm example is rejected by a daemon configured with
  AuthorizedUPNDomains other.example.

Testing

• Unit tests (apps/wolfsshd/test/test_configuration): all pass. New
  test_MatchUPNToUser covers the separator set, case-insensitivity, and the
  length-bounded (non-NUL-terminated) buffer path. Negative controls confirm the
  vectors fail when enforcement is removed or when the helper reads past
  nameSz.
• Integration test: sshd_x509_upn_fail.sh gates on FPKI by grepping the
  daemon's help output, then asserts the daemon-side rejection reason
  (incorrect user cert sent) from the log with a before/after match count, so
  neither a non-FPKI build, a stale log line, nor an unrelated client failure
  can produce a false pass. Skips cleanly (exit 77) when FPKI is not compiled in.
• The existing positive test sshd_config_test_x509 now also sets
  AuthorizedUPNDomains example, matching the fixture certificate's UPN realm
  (keys/renewcerts.cnf: otherName = msUPN;UTF8:fred@example). This gives the
  allowlist-accept path end-to-end coverage, which it otherwise lacked.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1079

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[Copilot]

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1079

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[aidangarske]

🐺 Skoll Code Review

Overall recommendation: COMMENT
Findings: 5 total — 5 posted, 0 skipped

Posted findings

• [Low] AuthorizedUPNDomains opt-in default keeps realm-unchecked (fail-open) UPN matching and warns on every cert auth attempt — apps/wolfsshd/auth.c:1619-1632
• [Medium] Existing positive x509 UPN test now depends on fixture cert realm being exactly 'example' — apps/wolfsshd/test/create_sshd_config.sh:32
• [Low] Unquoted variable expansions in new test script reduce robustness — apps/wolfsshd/test/sshd_x509_upn_fail.sh:6
• [Info] Multi-line function header comment on MatchUPNToUser — apps/wolfsshd/auth.c:1390-1392
• [Info] WSTRNCASECMP degrades to case-sensitive strncmp on MICROCHIP targets — apps/wolfsshd/auth.c:1441

Review generated by Skoll.

[aidangarske]

🔵 [Low] AuthorizedUPNDomains opt-in default keeps realm-unchecked (fail-open) UPN matching and warns on every cert auth attempt
💡 SUGGEST

When AuthorizedUPNDomains is unset (the default), MatchUPNToUser returns 1 on a local-part-only match (allowList == NULL || *allowList == '\0' branch), so a trusted-CA certificate for alice@any-realm still authenticates as local user alice — the original cross-realm behavior. The PR fixes the cross-domain bypass only when an operator opts in. This is a deliberate, documented backward-compatibility decision (security view: Info, CWE-1188, fail-open but mitigated). It is mitigated by a per-attempt WS_LOG_WARN; the code comment correctly documents this as per-attempt with no shared state, but the PR description claims a one-time (per process) warning — the description is stale relative to the code (review view: Low). On a busy FPKI deployment that has intentionally not opted in, this produces one WARN line per successful cert login. Confirmed against the code at auth.c:1616-1632.

Recommendation: Behavior is acceptable for backward compatibility. Document prominently that FPKI CA-only deployments should always set AuthorizedUPNDomains, or provide a build/config option to make realm checking fail-closed for security-sensitive deployments. Separately, reconcile the code and PR description: either confirm per-attempt logging is intended (visibility vs. log volume) or gate the warning with a static flag to make it one-time-per-process as the PR text implies.

[yosuke-wolfssl]

Thanks — you're right that the description was stale. The per-attempt warning is intentional: the original one-time-per-process version used a function-local static int, which Copilot correctly flagged as a data race in threaded (Windows) builds, so I moved to a stack-local upnRealmUnchecked set only when a UPN actually matches. I've updated the PR description to describe the per-attempt behavior and explain why, and added a "Security guidance" section stating that FPKI CA-only deployments should always set AuthorizedUPNDomains. Keeping the default opt-in for backward compatibility as you suggested; log volume is bounded by the rate of successful cert logins on a deployment that has chosen not to opt in.

[aidangarske]

🟡 [Medium] Existing positive x509 UPN test now depends on fixture cert realm being exactly 'example'
💡 SUGGEST

The pre-existing positive config sshd_config_test_x509 now sets AuthorizedUPNDomains example. This converts a previously domain-agnostic passing test into one that requires the client fixture certificate's UPN realm to be exactly example (case-insensitive). If the fixture cert generated by renewcerts.sh carries any other realm (or none), the existing positive x509 test will start failing as a side effect of this PR. The new negative test (sshd_config_test_x509_upn_bad with other.example) is internally consistent with the assumption that the cert realm is example, but that assumption is now load-bearing for the positive path too.

Recommendation: Confirm the generated client cert's UPN realm is example (grep the cert / renewcerts output). If it is not guaranteed, either align the config value with the actual fixture realm or drop AuthorizedUPNDomains from the positive config to keep that test testing only the local-part path.

[yosuke-wolfssl]

Confirmed, the realm is exactly example and it's pinned rather than incidental. keys/renewcerts.cnf:47 has otherName = msUPN;UTF8:fred@example, and the checked-in keys/fred-cert.der decodes to othername: UPN:fred@example.

Worth noting the positive x509 test was already load-bearing on this fixture before the change: MatchUPNToUser requires the local part to equal the requested username, so the cert already had to be fred@... for that test to pass. Adding AuthorizedUPNDomains example extends an existing dependency rather than introducing a new one, and it gives the allowlist-accept path end-to-end coverage that nothing else exercised. Leaving as-is.

[aidangarske]

🔵 [Low] Unquoted variable expansions in new test script reduce robustness
🔧 NIT

PWD=pwd and the later cd $PWD are unquoted; a working directory containing spaces would break the cd. This matches the existing sibling scripts' style so it is not new-in-kind, but the new file could quote for safety.

Recommendation: Quote the expansions (cd "$PWD"). Non-blocking; consistent with existing scripts otherwise.

[yosuke-wolfssl]

Fixed — quoted the cd "$PWD" on line 41. The PWD=\pwd`` assignment on line 8 doesn't word-split (assignments aren't subject to it), so I left it matching the sibling scripts' style.

[aidangarske]

⚪ [Info] Multi-line function header comment on MatchUPNToUser
🔧 NIT

The three-line block comment above the helper describes parameters/return in the style the project's comment guidance discourages for new functions. The function name and the inline comments inside already make the intent clear. Minor; several existing functions in this file use similar blocks, so this is consistent with the file if not with the stricter guidance.

Recommendation: Optionally collapse to a single-line WHY comment. Take it or leave it.

[yosuke-wolfssl]

Leaving as-is. The block is exactly three lines, but it's consistent with the other function headers in this file as you mentioned.

[aidangarske]

⚪ [Info] WSTRNCASECMP degrades to case-sensitive strncmp on MICROCHIP targets
🔧 NIT

The realm comparison relies on WSTRNCASECMP for case-insensitive matching. On MICROCHIP_MPLAB_HARMONY/PIC32 builds (wolfssh/port.h:599) this macro maps to plain strncmp, so realm matching would be case-sensitive there. wolfsshd realistically only builds on Unix/Windows where the case-insensitive mapping applies, so this is a theoretical note rather than a real defect for this app.

Recommendation: No action needed unless wolfsshd is ever targeted at MICROCHIP builds; noted for completeness.

[yosuke-wolfssl]

Agreed, no action. wolfsshd isn't targeted at MICROCHIP_MPLAB_HARMONY/PIC32 builds, so the case-sensitive strncmp mapping in wolfssh/port.h isn't reachable here.