Skip to content

Enforce LoginGraceTime in wolfsshd on Windows and make the grace flag per connection#1028

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_5577
Open

Enforce LoginGraceTime in wolfsshd on Windows and make the grace flag per connection#1028
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_5577

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Enforce LoginGraceTime in wolfsshd on Windows and make the grace flag per-connection

Summary

Two related fixes to LoginGraceTime handling in wolfsshd:

  1. Close a fail-open default on Windows. The Windows build never armed a
    grace timer. LoginGraceTime was accepted from the config but only logged a
    "not enforced on this platform" warning, so an unauthenticated connection
    could stay open indefinitely. Windows now arms a one-shot threadpool timer
    that drops the connection at the deadline, matching the POSIX SIGALRM
    behavior.

  2. Fix a latent shared-flag bug. The grace timeout was tracked in a single
    file-scope timeOut variable (static volatile int on Windows,
    __thread int elsewhere). POSIX always fork()s per connection so the flag
    was effectively per-process there, but Windows daemon connections run as
    threads via CreateThread, so one connection timing out could be observed by
    another. The flag now lives in WOLFSSHD_CONNECTION, making it
    per-connection on every platform.

Details

  • Added timeOut (and, on Windows, a PTP_TIMER loginTimer) to
    WOLFSSHD_CONNECTION, initialized per connection in StartSSHD.
  • POSIX: the SIGALRM handler sets a file-scope volatile sig_atomic_t loginGraceTimedOut (a signal handler may only touch a sig_atomic_t). The
    flag is cleared before alarm() is armed to avoid stale state from a reused
    process.
  • Windows: CreateThreadpoolTimer schedules GraceTimeoutCb, which marks
    the connection timed out at the deadline. CancelLoginTimer does
    SetThreadpoolTimer(NULL) -> WaitForThreadpoolTimerCallbacks(timer, TRUE) ->
    CloseThreadpoolTimer, and is NULL-guarded and idempotent. It is reached
    unconditionally when graceTime > 0, plus early on auth success.
  • Thread-safe flag on Windows. GraceTimeoutCb runs on a threadpool thread
    while the accept loop reads the flag on the connection thread. To avoid a
    cross-thread data race on conn->timeOut, the callback publishes it with
    InterlockedExchange8 and the accept loop reads it with InterlockedOr8
    (the field stays a single byte; no LONG widening). The POSIX
    sig_atomic_t flag is left as-is, which is the correct primitive for a signal
    handler.
  • LoginGraceExpired() abstracts the read of the timeout state for the accept
    loop across both platforms (interlocked read of conn->timeOut on Windows,
    the loginGraceTimedOut global on POSIX). The logging and grace-period checks
    go through this one accessor, so there is a single source of truth on each
    platform.
  • UserAuthResult now receives the connection via wolfSSH_SetUserAuthResultCtx
    and cancels the grace timer (CancelLoginTimer on Windows, alarm(0) on
    POSIX) as soon as authentication succeeds.
  • The grace-failure log line is only emitted when authentication did not also
    complete in the same accept iteration, avoiding a misleading "Failed login
    within grace period" message on a timer-vs-success race.
  • Logging callback now flushes after each line so a consumer reading the log
    while the daemon runs sees output immediately.

Tests

  • Reworked the grace-time test from stalling on an interactive password prompt
    (which never stalled without a TTY in CI) to opening a raw TCP connection and
    measuring, behaviorally, when the daemon drops it relative to the grace
    deadline.
  • The .sh test installs an EXIT trap right after starting the daemon, so
    wolfsshd is stopped on every exit path (including the early failure exits)
    and cannot leak into later tests.
  • Added a matching PowerShell test (sshd_login_grace_test.ps1) for the Windows
    path, wired into windows-check.yml. It cleans up the daemon in a finally
    block and decides "connection dropped" from the measured elapsed time, so a
    server-side reset (which surfaces as an IOException rather than a graceful
    EOF) near the deadline is still counted as enforcement rather than a false
    failure.
  • windows-check.yml now genuinely enables the sshd/sftp settings so the
    Windows build path is exercised.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 15, 2026
Copilot AI review requested due to automatic review settings June 15, 2026 06:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a fail-open behavior in wolfsshd where LoginGraceTime was not enforced on Windows, and corrects the timeout bookkeeping so it is safe under the Windows thread-per-connection model. It also updates logging behavior and adds/reenables tests to prevent regressions across Unix and Windows CI.

Changes:

  • Enforce LoginGraceTime on Windows using a per-connection threadpool timer and move the timeout flag into WOLFSSHD_CONNECTION.
  • Adjust authentication result handling to cancel grace timers/alarms on successful authentication.
  • Rework and expand tests (Bash + PowerShell) and extend Windows CI to exercise the Windows enforcement path.

Reviewed changes

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

Show a summary per file
File Description
apps/wolfsshd/wolfsshd.c Adds per-connection timeout state and Windows threadpool timer enforcement; logging flush behavior updated.
apps/wolfsshd/test/sshd_login_grace_test.sh Makes grace-time test deterministic by stalling with a raw TCP connection; reorders logic accordingly.
apps/wolfsshd/test/sshd_login_grace_test.ps1 Adds Windows regression test for grace-time enforcement.
apps/wolfsshd/test/run_all_sshd_tests.sh Re-enables the grace-time test in the SSHD test suite and updates skipped count.
.github/workflows/windows-check.yml Adds a workflow step to locate wolfsshd.exe, stage wolfssl.dll, and run the new Windows grace-time test.

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

Comment thread apps/wolfsshd/wolfsshd.c Outdated
Comment thread apps/wolfsshd/wolfsshd.c
Comment thread apps/wolfsshd/wolfsshd.c Outdated
Comment thread apps/wolfsshd/wolfsshd.c Outdated
Comment thread apps/wolfsshd/wolfsshd.c Outdated
Comment thread apps/wolfsshd/test/sshd_login_grace_test.ps1
@yosuke-wolfssl yosuke-wolfssl marked this pull request as draft June 15, 2026 06:32
@yosuke-wolfssl yosuke-wolfssl force-pushed the fix/f_5577 branch 2 times, most recently from de58d1d to 14e248f Compare June 15, 2026 07:35

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1028

Scan targets checked: none
Failed targets: wolfssh-bugs, wolfssh-src

⚠️ Review incomplete — one or more scan targets failed before findings could be produced. See the Fenrir PR review detail page for logs.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1028

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

@yosuke-wolfssl yosuke-wolfssl marked this pull request as ready for review June 15, 2026 23:43
@yosuke-wolfssl yosuke-wolfssl force-pushed the fix/f_5577 branch 3 times, most recently from e0befb8 to 15b6718 Compare June 24, 2026 00:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comments suppressed due to low confidence (1)

apps/wolfsshd/wolfsshd.c:2046

  • conn->timeOut can be set asynchronously by the Windows threadpool timer, but the accept loop only checks LoginGraceExpired(conn) in the while condition. If the grace timer expires while blocked in tcp_select(), the loop body can still call wolfSSH_accept() once more (without re-checking the timeout) and potentially accept a connection after the grace deadline.
        while (LoginGraceExpired(conn) == 0 && (ret != WS_SUCCESS
                && ret != WS_SCP_INIT && ret != WS_SFTP_COMPLETE)
                && (error == WS_WANT_READ || error == WS_WANT_WRITE)) {

            select_ret = tcp_select(conn->fd, 1);

Comment thread apps/wolfsshd/wolfsshd.c
Comment thread apps/wolfsshd/wolfsshd.c
Comment thread apps/wolfsshd/test/sshd_login_grace_test.sh

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1028

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants