[shreyansh-nexthop]

What this does

Decides Redfish access from the client certificate rather than from a local Unix account, and validates the certificate's common name against a trusted list in CONFIG_DB.

UserMgr previously looked the common name up in /etc/passwd and /etc/shadow, which meant the redfish container had to ship a bmcweb account with a password. It now holds no accounts: bmcweb authenticates the client by mTLS, and the bridge answers priv-admin for a trusted common name, and no privileges, no groups and not enabled for an untrusted one.

Common name matching

RedisAdapter::getRedfishClientCnames() reads REDFISH|certs field client_crt_cname, a comma separated list. Entries are exact names, or wildcards of the form *.example.com matching any name ending in .example.com but not example.com itself. Matching is case sensitive, and entries beginning with * that are not usable wildcards are skipped with a warning.

This mirrors CommonNameMatch in the SONiC REST API server. The two were compared over exact matches, subdomain and multi level subdomain matches, the domain itself, a suffix that merely ends with the domain, malformed wildcard entries and a case mismatch, and they agree on every case.

An unset or empty list refuses every common name, as the REST API server does: its trusted list, split from an empty value, matches no certificate. Being issued by the staged CA is not enough on its own. The list is read from CONFIG_DB on every GetUserInfo call, which bmcweb already makes per request, so adding or removing a name takes effect on the next request without restarting the service.

Failure behaviour

The read returns a tri-state rather than a string, because hget() cannot distinguish a field that is not set from a read that failed. A string reply is the configured list and a nil reply is an unset field; any other reply, such as -LOADING while Redis starts, is a failed read and is logged. Every failure refuses the request. After an I/O error the CONFIG_DB context is freed and the next read reconnects, as it does when CONFIG_DB was not reachable at startup.

An untrusted common name is answered with a role carrying no privileges, empty groups and UserEnabled false, not a D-Bus error: bmcweb maps an unexpected error to 500, which would report an untrusted client as a server fault. An empty role fails the privilege check and is answered with 403. The keys stay present, because bmcweb answers 500 to a local user reply without UserGroups. A refused common name is logged at info, since any client holding a certificate from the CA can trigger it on every request.

The bmcweb user object the bridge publishes is for the AccountService listing only; its privilege and enabled flag grant nothing.

Redfish defines ServiceRoot as requiring no privileges, so /redfish/v1 still answers 200 for a refused common name. The difference shows on privileged paths such as /redfish/v1/Managers.

Verification

Builds clean. Behaviour was exercised on a BMC before the per request read: a trusted common name reaches privileged paths, an untrusted one is refused with 403 while the service root still answers. The per request read, error reply handling, reconnect and the refusal for an unset or empty list have been built and not yet exercised on a BMC.

No unit test exercises the matcher; the repository's unit tests cover field mapping and the inventory model only.

Related

The CA these certificates are validated against is staged by sonic-net/sonic-buildimage#28276

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[judyjoseph]

LGTM

[yxieca]

Reviewed at 582a53c6. One blocking item; the authorization decision itself I checked carefully and it holds up.

Starting with what's right, since it's the part that mattered most to verify. Every path out of isCommonNameTrusted returns false — null adapter, failed load, empty list, no match — and getUserInfo emits a privilege only under trusted ? std::string("priv-admin") : std::string(""). The wildcard matching is also correct: suffix = entry.substr(1) keeps the leading dot, the commonName.size() > suffix.size() guard rejects example.com against *.example.com, and the suffix compare rejects evil-example.com (ends in -example.com) and example.com.attacker.net. Entry parsing trims each token and skips empties. I went looking for a fail-open and didn't find one.

Blocking — a Redis error reply is misclassified as "field not set", and then cached forever.

The function states its own reason for existing:

// Read directly rather than through hget(), which cannot distinguish a
// field that is not set from a read that failed. That difference decides
// whether client certificates are accepted, so it must not be lost.

But after the null check, the only classification left is:

std::optional<std::string> result = std::string();
if (reply->type == REDIS_REPLY_STRING)
{
    result = std::string(reply->str, reply->len);
}

A REDIS_REPLY_ERROR — -LOADING while Redis is still coming up, -MISCONF, WRONGTYPE — is non-NULL and is not REDIS_REPLY_STRING, so it falls through as an empty string: a successful read of an unset field. That's precisely the distinction the comment says must not be lost, and it contradicts the contract added to redis_adapter.hpp in this same PR ("no value: CONFIG_DB could not be read").

What turns it from a classification bug into an availability defect is the caching. loadTrustedCnames() latches any non-nullopt result:

cachedCnames_ = *read;
haveCachedCnames_ = true;

and the retry in isCommonNameTrusted is guarded by if (!haveCachedCnames_ && !loadTrustedCnames()). Once the flag is set it never reloads. So a transient Redis error during startup — the -LOADING window is the realistic one, since the bridge and Redis come up together — permanently refuses every client certificate until someone restarts sonic-dbus-bridge. The PR's stated behaviour, that a failed read "is retried on the next request until it succeeds", doesn't hold for this case.

It fails closed, so this isn't an escalation path. But the operator symptom is bad: all Redfish access denied, a log line saying no trusted common names are configured, and a CONFIG_DB that looks correctly populated when you go and check it.

Treating REDIS_REPLY_ERROR (and any type that isn't REDIS_REPLY_STRING or REDIS_REPLY_NIL) as std::nullopt, and logging reply->str, is a few lines and restores the tri-state the header promises.

Non-blocking:

1. There's no revocation path. Once haveCachedCnames_ is set, removing a common name from CONFIG_DB has no effect until the service restarts. The comment is honest about it and says it mirrors the REST API server, which is a defensible choice — but for an authorization boundary, "revoke access by knowing to restart a particular container" should live in operator-facing documentation rather than only in a source comment. A CONFIG_DB subscription or a bounded TTL would make it operational.

2. *.com is accepted as a wildcard. The only rejection is entry.size() < 3, so a single-label wildcard trusts every common name in that TLD. Requiring at least two labels after the * would bound a plausible operator typo.

3. An untrusted client still gets UserGroups: {"redfish"} and UserEnabled: true. The whole denial rests on bmcweb mapping an empty UserPrivilege to no access. That's probably true today, but gating the group list and enabled flag on trusted as well would make the refusal independent of one downstream consumer's behaviour.

4. initUserObjects() hardcodes the bmcweb object to priv-admin and enabled = true regardless of whether any common name is trusted, and it runs before loadTrustedCnames() in the constructor. Harmless if every consumer goes through GetUserInfo, but anything reading UserPrivilege off /xyz/openbmc_project/user/bmcweb directly sees priv-admin unconditionally. Worth either deriving it from the trusted-list state or noting that the object is display-only.

5. A stale comment now sits directly above the changed constructor — // (scans /etc/passwd on construction) in bridge_app.cpp, while this PR removes the getpwnam_r lookup and the <pwd.h>/<shadow.h> includes.

6. Refusals log at LOG_ERROR per request, so any client holding a CA-issued certificate can generate unbounded error volume, and a malformed config entry is re-warned on every request rather than once at load. Validating entries in loadTrustedCnames() and demoting the per-request refusal would keep the signal useful.

Two questions I couldn't settle from the diff:

• getRedfishClientCnames() is the only accessor that reaches into configDbContext_ directly; hgetall() and hget() both take a caller-supplied redisContext*. If configDbContext_ is non-NULL but in a sticky error state after a failed redisCommand, does anything reconnect it? If not, the retry path can't recover even once the blocking issue above is fixed.
• isUserExist and isUserEnabled are removed from the header while Users is still constructed with a UserMgr& back-reference. The PR says it builds clean, which likely settles it, but users.cpp isn't in the diff.

One cross-PR note: sonic-mgmt #28424 is the test suite for this feature, and its test_wrong_ca_rejected has an except (requests.exceptions.SSLError, ssl.SSLError) branch that asserts nothing. Any TLS-layer failure satisfies it — so that test would still pass if this code stopped rejecting untrusted certificates entirely. The implementation and its test currently have gaps on the same boundary, which is worth closing from both ends.

---

AI-assisted review, reviewed by Ying Xie (@yxieca).

[mssonicbld]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[shreyansh-nexthop]

Thanks @yxieca, I have addressed your reviews comments:

Blocking: Error replies like -LOADING are now treated as a failed read instead of an unset field, and we log the error string. I am not caching it anymore.

1: I dropped the cache entirely. The list is now read from CONFIG_DB on every GetUserInfo call. bmcweb already calls that per request.
2: I kept this as is so it behaves the same as the REST API server. Its CommonNameMatch accepts the same wildcard entries, so client_crt_cname means the same thing in both places.
3: Correct, an untrusted CN now gets empty groups and UserEnabled false, not just an empty role.
4: Updated the comment to make clear the object is only there for the AccountService listing and doesn't grant anything. Access is still decided per request in GetUserInfo.
5: Removed the stale comment.
6: Refusals for an untrusted CN now log at info, so a client can't flood the error log. I left the warnings for a malformed or empty client_crt_cname as they are, since they only repeat while the config is wrong and that's something we'd want to notice.

On your questions: you were right that nothing reconnected the context before, and that's fixed now. isUserExist and isUserEnabled aren't used anywhere, and everything builds clean.

[yxieca]

Re-reviewed at 1348c3b. The blocking item is fixed: getRedfishClientCnames() now classifies REDIS_REPLY_STRING as a value, REDIS_REPLY_NIL as an unset field, and everything else — including REDIS_REPLY_ERROR — as std::nullopt, restoring the tri-state the header documents. Dropping the cache resolves it more thoroughly than correcting the classification alone would have, since revocation now takes effect immediately rather than at restart, and the reconnect on a dropped context closes the sticky-error question from the last round. Fail-closed re-verified on all four paths, and an untrusted common name now gets empty groups and UserEnabled false rather than relying on one downstream mapping.

---

AI-assisted review, reviewed and approved by Ying Xie (@yxieca).