fix(server): log a fingerprint of rejected API keys instead of the raw key#1751
Merged
Merged
Conversation
verify_api_key logged the rejected key verbatim at WARNING, writing the client's secret into server.log on every failed auth (a mistyped or non-ASCII key from jundot#1719 leaks the credential). Add fingerprint_key to admin/auth.py (truncated SHA-256, surrogatepass encoding matching compare_keys) and log the fingerprint instead, so repeated rejections of the same key stay correlatable without exposing the secret. Item 4 of jundot#1440
Owner
|
Thanks, this addresses item 4 of #1440 cleanly. The change stays on the common rejected-key path, preserves auth behavior, and logs only a stable fingerprint instead of the submitted secret. CI is green and I do not see a regression risk here. Since this is still marked draft, I'll wait until you mark it ready before merging. |
Owner
|
Thanks for marking this ready. I rechecked the PR, and the scope still looks right: the rejected-key path now logs only a stable fingerprint, while auth behavior and both Bearer/x-api-key handling stay unchanged. The added tests cover the fingerprint helper and the dependency-level log behavior, including non-ASCII and lone-surrogate inputs, and CI is green. This looks good to me, and I am going to merge it. |
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
2 participants
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.
Addresses item 4 of #1440 (the rejected-key logging item).
Problem
When a request fails API key auth,
server.pylogs the rejected key verbatim at WARNING:That writes the client's secret into
server.logon every failed auth, at an always-on level. A user who mistypes a key, or whose key carries the non-ASCII problem fixed in #1719, leaks their credential straight into the logs.Fix
Log a short, non-reversible fingerprint instead of the raw value: the first 8 hex characters of the SHA-256 of the key. Operators can still correlate repeated rejections of the same key (a misconfigured client hammering the endpoint shows one stable
fp=) without the secret ever landing in the log.Added
fingerprint_keytoomlx/admin/auth.py, next tocompare_keys, using the sameencode("utf-8", "surrogatepass"). That keeps it tolerant of anystrthe auth path accepts, including the non-ASCII and lone-surrogate keys that #1719 already handles.Testing
tests/test_api_auth.py: fingerprint format (8 hex chars), determinism, that the fingerprint does not contain the raw secret, that distinct keys differ, that non-ASCII / empty / lone-surrogate inputs do not raise, and an end-to-end check that the auth dependency logs the fingerprint and not the raw rejected key (viacaplog).OMLX_API_KEYset, sent a wrongBearerkey to/v1/models, and confirmed the server log showsRejected API key (fp=...), does not contain the raw key, and that the logged fingerprint matchesfingerprint_keyof the key I sent. A correct key returned 200.Scope note
This is a logging-only change; auth behavior is unchanged (a bad key still gets a 401). The other items in #1440 are design questions (open setup when no key is configured, the key being readable in the frontend, the session cookie's
Secureflag), so I left those alone. Happy to take any of them as separate follow-ups if you confirm the intended behavior.