wolfsshd: mark AuthorizedKeysFile as explicitly set in public setter#1044
Open
yosuke-wolfssl wants to merge 1 commit into
Open
wolfsshd: mark AuthorizedKeysFile as explicitly set in public setter#1044yosuke-wolfssl wants to merge 1 commit into
yosuke-wolfssl wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a security-sensitive configuration inconsistency in wolfsshd by ensuring AuthorizedKeysFile configured through the public API is treated the same as when configured via the config-file parser.
Changes:
- Update
wolfSSHD_ConfigSetAuthKeysFile()so it sets/clearsauthKeysFileSetwhen the public setter is used. - Remove the redundant (and failure-prone)
authKeysFileSetassignment from the config parser path. - Add a unit test to validate the
authKeysFileSetflag behavior when using the public setter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/wolfsshd/configuration.c | Moves authKeysFileSet ownership into the public setter and removes the parser-side pre-set. |
| apps/wolfsshd/test/test_configuration.c | Adds a unit test covering authKeysFileSet transitions via the public setter and updates related comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5da86d4 to
2cb2edb
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1044
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
wolfsshd: mark AuthorizedKeysFile as explicitly set in public setter
Summary
wolfSSHD_ConfigSetAuthKeysFile()updatedconf->authKeysFilebut never setconf->authKeysFileSet. Applications that configureAuthorizedKeysFilethrough the public setter (instead of the config-file parser) left the flag
clear, and
RequestAuthenticationthen skipped the authorized-keys check forcertificate public-key logins, relying on CA validation alone. The setter now
owns the flag so both the parser and public-API paths behave identically.
Addressed by 5581.
Background
RequestAuthenticationcallswolfSSHD_ConfigGetAuthKeysFileSet()to decidewhether a certificate public-key login must still be matched against the
configured authorized-keys file. Only the parser path
(
HandleConfigOption/OPT_AUTH_KEYS_FILE) setauthKeysFileSet; the publicsetter did not. An embedder configuring both
TrustedUserCAKeysandAuthorizedKeysFilevia the public setters could therefore unintentionally letany otherwise-valid trusted certificate for the account identity bypass the
authorized-keys restriction.
Changes
apps/wolfsshd/configuration.cwolfSSHD_ConfigSetAuthKeysFile()now setsauthKeysFileSet = 1only aftera non-NULL file is configured successfully, and clears it (
= 0) when thefile is removed (
file == NULL). The flag stays consistent withauthKeysFileon every path.(*conf)->authKeysFileSet = 1;assignment in theparser path. It previously set the flag before the setter ran, which would
have wrongly left it set if
CreateString()failed; the setter is now thesingle source of truth.
apps/wolfsshd/test/test_configuration.ctest_ConfigSetAuthKeysFile: confirms the flag is0on a freshconfig, becomes set after the public setter succeeds, and clears again when
the file is set to
NULL. Registered intestCases[].test_ConfigCopyand aroundtest_ConfigFree.Testing
apps/wolfsshd/test/test_configurationpasses, including the new case.confirming it actually guards the behavior.