fix(security): mask credentials in Entra ID debug logs - #3213
Open
marevol wants to merge 1 commit into
Open
Conversation
PR #3077 truncated the id token and the refresh token, but three call sites were left writing credentials verbatim when debug logging is on: - getAccessToken() logged the authorization code in full - processAuthenticationData() logged the whole parameter map, which carries code and id_token, plus the request URL including its query string - containsAuthenticationData() logged the same parameter map again Add maskSecret(), maskParams() and maskQueryString(), and route every one of these through them. Credential values are truncated to an 8-character prefix so they can still be correlated across log lines; state, error and error_description stay verbatim, and the parameter key set is preserved, so a failed login is still diagnosable. containsAuthenticationData() only reads the key set, so it now logs only that. The two values PR #3077 masked inline now reuse maskSecret(). That also removes an NPE: validateNonce() called idToken.substring() outside its try block, so a response without an id token failed with a NullPointerException instead of the intended SsoLoginException.
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.
Problem
#3077 truncated
idTokenandrefreshTokeninEntraIdAuthenticator's debug logs, butthree other call sites were left writing credentials verbatim:
getAccessToken(AuthorizationCode, String)logger.debug("authCode={}, ...", authCode, ...)processAuthenticationData()logger.debug("process authentication: url: {}, params: {}", urlBuf, params)code/id_tokenfrom the POST body, plus the request URL including its query stringcontainsAuthenticationData()logger.debug("params={}", params)parseAuthenticationResponse()logged the full callback URL as well.Fix
Three helpers, and every one of those sites routed through them:
maskSecret(String)- keeps an 8-character prefix plus***, so two different codes arestill distinguishable in a log without the value being usable. Null/empty pass through.
maskParams(Map<String, List<String>>)- masks only the values ofcode,id_token,access_token,refresh_tokenandclient_secret(case-insensitive).state,erroranderror_descriptionstay verbatim, and the key set is preserved, so a failed login is stilldiagnosable from the log.
maskQueryString(String)- drops the query string from a logged URL. Every parameter itholds is already logged separately through
maskParams.containsAuthenticationData()only ever reads the key set, so it now logs onlyparams.keySet().The two values #3077 masked inline now reuse
maskSecret(). That also removes an NPE:validateNonce()calledidToken.substring(...)outside itstryblock, so a responsewithout an id token failed with a
NullPointerExceptioninstead of the intendedSsoLoginException.Tests
EntraIdAuthenticator's unit test class, 3 new tests:test_maskSecret- long / short / exactly-prefix-length / null / emptytest_maskParams_masksCredentialsAndKeepsDiagnostics- credentials masked, diagnostics kept,key set preserved, caller's map not mutated
test_maskParams_isCaseInsensitiveOnKeysVerified falsifiable: with
maskSecret()stubbed to return its input, all 3 fail.