authentication manager feature addition

[JacobBarthelmeh]

The authentication manager feature adds support for a user login and checking a users permissions for performing a group+action. The API was designed with PKCS11 in mind.

Some things of note:

• I added a callback function framework for checking authorization of key use based on key ID and user permissions but did not tie in that check yet. I would like to tie that in later when/if needed. This currently checks for authorization of user for a group/action that they can do. Which ties a user ID to crypto actions done.
• The user list in port/posix/posix_auth.c is a simple list not yet in NVM. This initial simplicity is deliberate.
• There is a TODO listed for logging of authentication events. Login failures, success, crypto actions should have logging additions in the future.

[Copilot]

Pull request overview

This pull request adds a comprehensive authentication and authorization manager to wolfHSM, enabling user management, login/logout functionality, and permission-based access control for HSM operations.

Changes:

• New authentication manager with PIN and certificate-based authentication support
• Authorization system with group and action-level permission checks
• User management APIs for adding, deleting, and modifying users and their credentials
• Complete client and server implementation with message translation support

Reviewed changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
wolfhsm/wh_auth.h	Core auth manager types, structures, and API definitions
wolfhsm/wh_message_auth.h	Message structures and translation functions for auth operations
wolfhsm/wh_server_auth.h	Server-side auth request handler declaration
wolfhsm/wh_client.h	Client-side auth API function declarations
wolfhsm/wh_server.h	Server context updated with auth context pointer
wolfhsm/wh_message.h	New auth message group and action enums
wolfhsm/wh_error.h	New auth-specific error codes
src/wh_auth.c	Core auth manager implementation with callback wrappers
src/wh_message_auth.c	Message translation implementations for auth messages
src/wh_server_auth.c	Server-side request handler for auth operations
src/wh_client_auth.c	Client-side auth API implementations
src/wh_server.c	Server integration with authorization checks
src/wh_client.c	Minor formatting fixes
port/posix/posix_auth.h	POSIX auth backend declarations
port/posix/posix_auth.c	POSIX auth backend implementation with in-memory user storage
test/wh_test_auth.h	Auth test suite declarations
test/wh_test_auth.c	Comprehensive auth test suite implementation
test/wh_test.c	Test integration for auth tests
examples/posix/wh_posix_server/*	Server configuration with auth context setup
examples/demo/client/wh_demo_client_all.c	Demo integration for auth

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 29 out of 30 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bigbrett]

@JacobBarthelmeh merge conflicts

[JacobBarthelmeh]

Force pushed to resolve merge conflict.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 17 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bigbrett]

Partial review - will pick back up later. But think I found some juicy things to chew on.

[JacobBarthelmeh]

Force pushed to resolve wh_test_she.c merge conflict.

[JacobBarthelmeh]

Rebased to resolve merge conflicts

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

test/wh_test_crypto.c:1

• The TEST_ADMIN_USERNAME / TEST_ADMIN_PIN definitions are duplicated across multiple test files (crypto/keywrap/she/clientserver/auth). Centralizing these in a shared header (e.g., test/wh_test_common.h) reduces the chance of inconsistencies and keeps test configuration in one place.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

Pull request overview

Copilot reviewed 41 out of 43 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #270

Scan targets checked: wolfhsm-consttime, wolfhsm-core-bugs, wolfhsm-defaults, wolfhsm-mutation, wolfhsm-proptest, wolfhsm-src, wolfhsm-zeroize

Findings: 7
7 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[Copilot]

Pull request overview

Copilot reviewed 41 out of 43 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

wolfhsm/wh_auth.h:1

• The comment says this macro enables the group and only the given action bit, but the implementation ORs the bit into the existing mask (it does not clear other action bits). Either update the comment to match behavior ("enables the given action bit") or change the macro to clear all action words for the group before setting the single bit.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bigbrett]

As discussed offline, one blocking bugfix needed, as well as a documentation update to tag this feature as experimental. I will post a list of the other issues found in a subsequent review that can then be addressed with follow-up PRs

[bigbrett]

Approved given that the feature is now tagged as "experimental" and individual fixes for the remaining security related issues can be introduced in subsequent PRs.

# Security Review — Distilled

---

## Vuln 1 — Cert auth has no proof-of-possession (`src/wh_auth_base.c:147-184`)

**The bug:** Certificate login only runs `CertManagerVerifyBuffer` against the stored CA. The server never checks that the client actually holds the matching private key.

**Why it matters:** Certificates are public documents. Verifying the chain proves "this cert was issued by that CA" — not "the sender owns the private key."

**The attack:**
1. Admin provisions Alice with `method=CERTIFICATE`, credential = Alice's CA.
2. Attacker grabs *any* valid cert signed by that CA (off the wire, from a public directory, from Alice's public leaf cert).
3. Attacker sends `AuthLogin(method=CERTIFICATE, username="alice", auth_data=<cert>)`.
4. Chain verifies → `is_active=true`, full permissions → attacker is now Alice.

**Key insight:** Presenting a cert should not equal proving identity. Every real cert-auth protocol pairs the cert with a signature over a server-chosen nonce.

**Fix:** Add a challenge-response action — server issues a random nonce, client returns `(cert, signature(nonce))`, server verifies *both*. Alternatively have client obtain nonce out-of-band or via earlier message.

---

## Vuln 2 — Cert login does not bind subject to username (`src/wh_auth_base.c:147-167`)

**The bug:** After the cert chain verifies, nothing compares the presented cert's subject/CN/SAN/pubkey to the user being logged into. `whAuthBase_User` doesn't even have a field to store an expected subject.

**Why it matters:** The intended model is "CA-as-credential" — multiple users share the same enterprise CA. Any legitimately-issued leaf from that CA will chain-verify against any user trusting it.

**The attack:**
1. Deployment has Alice (admin) and Bob (non-admin), both stored with `credential = EnterpriseCA`.
2. Bob holds his own genuine leaf cert signed by that CA.
3. Bob sends `LOGIN(username="alice", cert=bob's_leaf)`.
4. Server finds alice's record, loads her CA, calls `VerifyBuffer(bob's_leaf)` → chain is valid → server assigns Bob the session with Alice's `user_id` and admin permissions.

**Key insight:** Distinct from Vuln 1. Vuln 1 = *anyone with a valid signed cert wins*. Vuln 2 = *even a legitimate private-key holder can log in as a different user who trusts the same CA*. Both must be fixed.

**Fix:** Store an `expected_subject` or `expected_pubkey_hash` per user when storing credentials, OR operate on subject/CN/SAN/pubkey fields. Then on login, after chain verification, parse the leaf and compare. Fail closed on mismatch.

---

## Vuln 3 — `UserSetCredentials` missing caller authorization (`src/wh_auth.c:407-430`, `src/wh_auth_base.c:455-573`)

**The bug:** Three reinforcing defects that together let anyone with the `USER_SET_CREDENTIALS` action bit rewrite any other user's credentials:

1. **API layer:** `wh_Auth_UserSetCredentials` forwards the client-supplied `user_id` to the backend with no check that `user_id == caller` or caller is admin. Its siblings (`UserDelete`, `UserSetPermissions`) *do* pass the caller's id.
2. **Callback signature:** `UserSetCredentials`'s backend signature is missing the `current_user_id` parameter that peer callbacks include. So even if the backend wanted to check the caller, it can't — it only sees the target.
3. **Null-credential shortcut:** If the target user has `credentials_len == 0`, the backend accepts `current_credentials = NULL` without any proof-of-knowledge. This is reachable because `UserAdd` permits creating users with no credential.

**The attack (pre-provisioned admin takeover):**
1. Admin pre-provisions Bob (admin, credentials pending) via `whnvmtool` with `credentials_len=0`.
2. Non-admin Alice has only `(AUTH, USER_GET)` and `(AUTH, USER_SET_CREDENTIALS)`.
3. Alice calls `UserGet("bob")` → gets `user_id=2, admin=1`.
4. Alice calls `UserSetCredentials(user_id=2, current_len=0, new="1234")` → accepted because Bob has no credential set.
5. Alice logs in as "bob" with PIN "1234" → full admin.

**Key insight:** This is especially dangerous with bulk provisioning — any pre-created account without a credential is a landmine until an admin sets its PIN.

**Fix:** Add `current_user_id` to the callback; pass `context->user.user_id` from the API; require `caller == target` OR `WH_AUTH_IS_ADMIN(caller)`; forbid the null-credential shortcut on cross-user calls by non-admins.
**Alternative Fix:** Restrict ALL user modification to admin only

---

## Vuln 4 — Session carry-over on abrupt disconnect (`src/wh_server.c:160-173`)

**The bug:** `auth_ctx->user` is only cleared by the explicit `COMM_CLOSE` handler. A TCP RST, a client crash, or a malicious client that just drops the socket does not clear it. The disconnect callback only flips `server->connected` — it never touches `server->auth`.

**Why it matters:** The reference POSIX server uses a `Cleanup → Init → re-accept` pattern. `Cleanup` memsets the server context but does not call `wh_Auth_Cleanup` on the externally-owned auth context, and `Init` re-attaches the same auth pointer without re-zeroing `user`. So the prior session's identity survives across reconnects.

**The attack:**
1. Admin connects over TCP, authenticates with a PIN.
2. The socket closes without a `COMM_CLOSE` (RST, crash, or malicious).
3. `auth_ctx.user` still holds `{user_id=admin, is_active=true, permissions=admin}`.
4. Attacker's new TCP connection is accepted. Attacker sends no `AUTH_LOGIN`.
5. Attacker issues a privileged request. `CheckRequestAuthorization` sees `user_id != INVALID`, evaluates the stale admin permissions → authorized.

**Key insight:** A logout that only runs on the graceful path is not a logout — it's a courtesy. Abrupt disconnect is the common case on real networks.

**Fix:** On transition to `WH_COMM_DISCONNECTED`, call `wh_Auth_Logout` for any non-invalid user_id. Additionally, have `wh_Server_Init` re-zero `auth->user` so reconnect-reuse is safe by default.

---

## Vuln 5 [FIXED]  — Duplicate-username check terminates at first empty slot (`src/wh_auth_base.c:286-295`)

**The bug:** The uniqueness loop in `UserAdd` breaks at the first empty slot instead of scanning the whole array. `UserDelete` zeroes slots in place without compacting, so any delete creates a gap that blinds the check to everything past it.

**The attack (identity confusion / lockout):**
1. State: `[0]=bob, [1]=admin`.
2. Admin deletes bob → `[0]=empty, [1]=admin`.
3. Attacker with `USER_ADD` perm calls `UserAdd(username="admin", ...)` with their own PIN.
4. Loop breaks at slot 0 (empty), never scans slot 1 → duplicate admin is inserted.
5. `FindUser("admin")` returns the first match → always the impostor.
6. Real admin's login fails (PIN doesn't match impostor's hash) = **locked out**. Attacker logs in as "admin" at their own (non-admin) permissions = **identity confusion**.

**Key insight:** The attacker doesn't gain admin rights — they gain the *name* "admin", which is enough to shadow the real account in every name-based lookup.

**Fix:** Two-pass the loop — scan all slots for duplicates, then find a free slot. Or compact the array on delete. Restricting ALL modification to admin only would eliminate the possibility of a client-driven exploit, but still leave the erroneous code in place.

---

## Vuln 6 — `UserAdd` does not require caller permissions to be a superset (`src/wh_auth.c:329-338`)

**The bug:** `UserAdd` only checks one thing: non-admins can't mint admins. It never checks that the new user's requested permission bitmap is a subset of the caller's own permissions. The backend writes whatever bitmap the caller supplied straight into the new slot.

**The attack (verified end-to-end):**
1. Admin gives Alice a tiny role: `USER_ADD` + `USER_SET_CREDENTIALS` only. No crypto, NVM, key, or image perms.
2. Alice calls `UserAdd(username="victim", permissions={groupPermissions=0xFF…, admin=0}, credentials=NULL)` → server accepts the bitmap as-is.
3. Alice calls `UserSetCredentials(victim, current=NULL, new="victimpin")` → accepted via Vuln 3's null-credential shortcut.
4. Alice logs out, logs back in as "victim" → session now carries crypto/NVM/key/image permissions Alice was never granted.

**Key insight:** Without a subset check, `USER_ADD` is effectively "grant yourself any non-admin permission you want." Vulns 3 and 6 compose: Vuln 6 mints the elevated account, Vuln 3 lets the attacker log into it.

**Fix:** In `wh_Auth_UserAdd`, require the requested `groupPermissions`, `actionPermissions`, and `keyIds` be bitwise subsets of `context->user.permissions` unless the caller is admin. Reject with `WH_AUTH_PERMISSION_ERROR` otherwise. Additionally, forbid `UserAdd` with `credentials_len == 0` for non-admin callers. Fixing Vuln 3 is a prerequisite.
**Alternative Fix:** Restrict ALL user modification to admin only


## Relationships Between Vulns

- Vulns 1 & 2 are independent cert-auth defects — fixing one doesn't fix the other.
- Vulns 3 & 6 compose — Vuln 6 mints an over-privileged account, Vuln 3 lets the attacker install credentials for it.
- Vuln 4 makes any reconnect-reuse server pattern unsafe regardless of the auth method.
- Restricting ALL user modification to admin only would eliminate Vulns 3 and 6, and eliminate an exploit path for 5 (though the bug would remain).

[billphipps]

Great start! Let's continue to refine this model so we can make it part of the mainline.