Skip to content

fix(auth): harden multi-user sessions and proxy authentication - #880

Open
SiYue-ZO wants to merge 1 commit into
sipeed:mainfrom
SiYue-ZO:fix/followup-876
Open

fix(auth): harden multi-user sessions and proxy authentication#880
SiYue-ZO wants to merge 1 commit into
sipeed:mainfrom
SiYue-ZO:fix/followup-876

Conversation

@SiYue-ZO

@SiYue-ZO SiYue-ZO commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

This follow-up hardens the multi-user authentication work introduced in #876 and restores the affected compatibility paths.

  • allow the device owner to rename their own account and revoke stale HTTP/realtime sessions only when account state actually changes
  • keep browser login JWTs in an HttpOnly cookie, while allowing automation clients to explicitly request a response token with X-NanoKVM-Return-Token: true
  • give explicit Bearer credentials precedence over ambient cookies, including fail-closed handling for malformed Authorization headers
  • trust forwarded host/scheme headers only from configured proxy IPs/CIDRs and require exact WebSocket origin matching
  • derive the session cookie Secure attribute from the validated external request scheme
  • safely decode CryptoJS/OpenSSL encrypted passwords from raw Base64 and legacy percent-encoded requests without malformed-ciphertext panics
  • cache the default-account bcrypt hash while retaining randomized token versions when the account file is absent
  • document API migration and reverse-proxy configuration in English, Chinese, and Japanese

Allow device-owner account renames while revoking stale sessions, keep browser logins cookie-only by default, and add explicit Bearer-token opt-in for automation clients. Validate forwarded proxy headers against configured peers, enforce exact WebSocket origins, and derive secure cookie attributes from the external request scheme. Make password decryption compatible with CryptoJS/OpenSSL form submissions, cache the boot-reset default hash, and document the migration and reverse-proxy requirements. Add regression coverage for account lifecycle, token precedence, cookie security, origin validation, and encrypted form passwords.
yuzi-co added a commit to yuzi-co/IronKVM that referenced this pull request Aug 28, 2026
…S goes

Two ways to hand the browser a session cookie it cannot use, both of
which end at a login page that reappears with nothing logged.

The first is the Secure attribute. It was derived from conf.Proto, which
says what the server offers rather than how this request arrived. A board
configured for HTTPS that answers a plain HTTP request marked the cookie
Secure, and a browser stores such a cookie and then never sends it back
over that scheme. RequestIsSecure asks the request instead: the TLS state
if the server terminated it, or X-Forwarded-Proto if a proxy did. Anyone
can send that header, and the worst it buys them is a cookie of their own
they cannot use.

The second is switching HTTPS off. The cookie was set Secure while HTTPS
was on, and a browser holding a Secure cookie will not send it over plain
HTTP, will not let a plain HTTP response replace it, and will not accept
a deletion that arrives without the attribute either. Every route to
clearing it runs over the scheme that cannot reach it, so the operator
logs in, the form posts, the server sets a cookie the browser refuses to
store over the old one, and the login page comes back. The board looks
broken and the log says nothing.

The response to POST /api/vm/tls is the last one that travels over
HTTPS, so SetTls deletes the cookie there while the connection is still
encrypted. That costs one deliberate log in. Enabling HTTPS leaves the
session alone: the cookie set over plain HTTP carries no Secure
attribute, so it still travels once the listener moves.

The cookie helpers move to middleware, which already owns CookieName, so
service/vm can reach them without importing service/auth.

enableTls, disableTls and the init-script restart become variables for
the same reason ensureTlsCert already is: all three reach outside the
process, and what SetTls does around them is worth testing without that.

The scheme derivation is the shape upstream PR sipeed#880 uses.
yuzi-co added a commit to yuzi-co/IronKVM that referenced this pull request Aug 28, 2026
POST /api/auth/login runs utils.Decrypt on the request body before it
checks any credential, so whatever an anonymous caller puts in the
password field reaches the cipher. The library underneath,
github.com/mervick/aes-everywhere, checks the "Salted__" magic and the
total length and nothing else, so two shapes of garbage panic it:

  - A cipher text that is not a whole number of blocks reaches
    CryptBlocks, which panics with "input not full blocks".
  - A whole number of blocks decrypts to noise, and PKCS7 trimming then
    slices from the last byte of that noise. Above the block size it runs
    off the front of the plaintext; sixteen bytes of "Salted__" and a
    salt and no blocks at all indexes [-1].

The second one needs no special input. The last byte of a wrong
decryption is above 16 about fifteen times in sixteen, so nearly any
value of the right length does it.

gin.Recovery() is registered, so this answered 500 rather than stopping
the server. It is still an unauthenticated fault, and there are six call
sites.

Read the format here instead. The salt, the block alignment and the
padding are all checked before the plaintext is returned, and the
EVP_BytesToKey derivation is thirteen lines. The wire format does not
move: the fixtures in the test were produced by the old library, and
service/auth still encrypts with it, so the round trip stays covered
from both ends.

DecodeDecrypt now takes the raw base64 as well as the percent-encoded
form. The web UI sends encodeURIComponent(...) and a client that posts a
form sends neither, because gin has already decoded it. The raw value is
tried first, because a second unescape turns a base64 '+' into a space.

The shape is taken from upstream PR sipeed#880, so this does not diverge from
what sipeed/NanoKVM will carry.
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.

1 participant