Repository navigation
Show a license key only by its last 4 characters in logs and error messages - #85
Draft
jwrosewell wants to merge 6 commits into
Draft
jwrosewell wants to merge 6 commits into
jwrosewell wants to merge 6 commits into
Conversation
A license key in a log entry or an error message is shown only by its last 4 characters, with every character before them replaced by '*', so a reader can tell two keys apart but cannot use either. A key of 4 characters or fewer is replaced in full, because its last 4 characters would be all of it. maskIn masks each key in a text, as written and as encoded in a URL, and gives a throwable to log in place of one whose message, causes or suppressed throwables hold a key, keeping the class names and stack traces.
The data update service is tested for a 403, a 429, another failing status, a 304, no response and an exception that names the URL, with two keys in the update URL. Everything it passes to the logger, at every level and with each exception's stack trace, holds each key only by its last 4 characters. The cloud request engine is tested with an error from the cloud that names the license key, and the 51Did client with a 401 body and a 400 errors list that name the licence, and with a key of 4 characters, which is replaced in full.
…errors The data update service logged the formatted update URL, whose query carries the license keys, at debug and error level, and logged the exception an HTTP library raised, whose message often names that URL. Every message now names the URL with each configured key masked by LicenseKeyMask, and the exception is logged as a copy whose messages are masked. The cloud answers a refused license key with an error that names it, and the cloud request engine put that text in the exception it raised, which can reach the log and the JSON builder's errors list. Its messages now mask the license key, and each key in it when it holds several separated by '|'. The 51Did client puts the start of the body in the message of a redeem error, and the errors list of a 400 answer in the message of the IllegalArgumentException it raises. Both now mask the licence key the redeem request sent. The 51Did package depends on nothing in the Pipeline, so it masks with a private method that gives the same result as LicenseKeyMask. The exception's body is kept as the service sent it.
…e URL maskIn_EncodedKeyShowsThePlainLast4 expects a key whose last characters are percent-encoded in a URL to show the encoded form of its plain last 4 characters. maskIn_MasksAKeyEncodedInLowerCase expects a key encoded with lower case hexadecimal digits to be masked too. KeyTypedIntoTheUrl_LogsMaskedKey gives the data update service a URL whose LicenseKeys parameter holds a key that is not among the data update license keys, and expects the key masked in every log entry. All three are written to fail on the previous commit.
… URL LicenseKeyMask.maskIn now replaces a URL-encoded key with the encoded form of its masked plain form, so it shows the same last 4 characters as the plain key, and also masks the key encoded with lower case hexadecimal digits. Its Javadoc now says that every occurrence of a key's text is masked, so a key of only a few characters masks those characters elsewhere too, as the specification asks for keys of 4 characters or fewer. The specification link now points at rc, where the rule is. The data update service now also masks any key in the LicenseKeys parameter of the data update URL, such as one typed straight into the DataUpdateUrl setting rather than given as a data update license key. DidHttpException.getBody now says the body is not masked and can hold the license key sent, so a caller that logs it must mask it first.
licenseKeysToMask was added between downloadFile and its Javadoc, so the Javadoc described the wrong method. It is now above downloadFile again.
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.
Closes #10. Follows the License keys section of the logging specification, which 51Degrees/specifications#55 added and #123 reworded, both now on
rc.The problem
A license key is a paid credential, and log entries and error messages travel further than the server, to log services, to support tickets and, through the JSON builder's
errorslist, to the browser. Onrcat 7299688 three places wrote license keys in full.FiftyOneUrlFormatterputs the license keys in the query of the update URL.DataUpdateServiceDefault.downloadFilelogged that URL at debug level for every download and for a 304, at error level for no response, a 429, a 403 and any other failing status, and logged any exception with its stack trace. The exception an HTTP library raises often names the URL it requested, for exampleServer returned HTTP response code: 403 for URL: ....CloudRequestEngineDefault.validateResponseput the cloud's error text, or the whole response for a failing status, in the message of theCloudRequestExceptionit raised.DidClient.readRedeemput the first 200 characters of the body in the message of a redeem error, and theerrorslist of a 400 answer in the message of theIllegalArgumentExceptionit raised. The redeem request is the one that sends the licence key.A search of every
logger.call and exception message inpipeline,device-detection,ip-intelligence,locationandexamplesfor license key values and for URLs or response bodies that can hold one found no other place.What the specification says
The License keys section of
pipeline-specification/features/logging.md, onrcat 5a25065, says the following.#55 also added to the logging section of
data-updates.mdthat the<url>in its messages "is the URL with each license key masked to its last 4 characters".What changed
fiftyone.pipeline.util.LicenseKeyMaskinpipeline.core.mask(key)replaces every character before the last 4 with*, soTESTKEYNOTREALABCDEFGHIJ1234becomes************************1234. A key of 4 characters or fewer becomes all*.maskIn(text, keys)masks each key wherever the text holds it, as written and as encoded in a URL, longest key first.maskIn(throwable, keys)gives the throwable to log in place of one whose message, causes or suppressed throwables hold a key. Each message in the copy starts with the name of the class it stands for, and the stack traces are the originals. A throwable that holds no key is returned as it is, so logs that never held a key do not change.downloadFilenames the URL in every message with each configured key masked, and logs the exception throughmaskIn. The request itself still uses the unmasked URL.validateResponsemasks the license key in every message before it raises the exception, and so does each key in it when the value holds several separated by|(getLicenseKeys).readRedeemmasks the licence key in both messages, before the body is cut to 200 characters, so a key cannot be left half shown.DidHttpException.getBody()still returns the body as the service sent it. The 51Did package depends on nothing in the Pipeline butorg.json, so it cannot useLicenseKeyMaskwithout bringingpipeline.coreand its dependencies to every 51Did user. It masks with a private method of eight lines that gives the same result, and its tests pin the same masked value as the helper's tests.Resource keys are left as they are, because the section covers license keys only.
For the release notes
*.fiftyone.pipeline.util.LicenseKeyMaskmasks license keys in text and in throwables, for code of your own that logs them.How to verify
No test calls a live service.
DidClientTestsneeds thepipeline/owid-javasubmodule checked out, as the 51Did package compiles its source.rcLicenseKeyMaskTests(pipeline.core)DataUpdateServiceLicenseKeyTests(pipeline.engines)CloudRequestEngineTests(pipeline.cloudrequestengine)DidClientTests(pipeline.did)CloudRequestEngineTestshas four parameterised methods, so a run reports more cases than methods.LicenseKeyMaskTests, which pass.rc. Ten are written to fail there: the six inDataUpdateServiceLicenseKeyTests(Forbidden,TooManyRequests,OtherStatus,NotModified,NoResponseandRequestException, each_LogsMaskedKeys),CloudRequestEngineTests.Process_LicenseKeyInCloudError_Masked, and inDidClientTestsredeem_RefusedLicenceIsMaskedInTheMessage,redeem_LicenceInErrorsIsMaskedInTheMessageandredeem_ShortLicenceIsMaskedInFull. The data update tests capture everything passed to a mocked logger, at every level, with each throwable's printed stack trace.Other drafts that touch the same lines
#79 rewrites the logging of
DataUpdateServiceDefault.downloadFile. It names the URL unmasked in its debug line, in the "Checking for update", "Downloaded new data" and "No data newer" info lines, and throughconnectFailure(url, ...)anddownloadFailure(url, ...), and in both catch blocks it copiese.getMessage()into the message text before passing the throwable. Whichever of the two merges second must do four things.licenseKeys = dataFile.getConfiguration().getDataUpdateLicenseKeys()andloggedUrl = LicenseKeyMask.maskIn(url, licenseKeys)once at the top ofdownloadFile, and passloggedUrl, noturl, to every message and helper that names the URL. The realurlis used only fornew URI(url.trim()).toURL().Throwable masked = LicenseKeyMask.maskIn(e, licenseKeys)and usemasked.getMessage()for the error detail text as well asmaskedas the logged throwable, because an HTTP library's message names the URL.DataUpdateServiceLicenseKeyTests. It captures every logger call at every level, so it fails for any message that is missed.errortowarnand drops the URL from them, so those tests' expected level changes on Make the data update follow the specification #79's side, not in the masking.Full build
The full build and test matrix passed every job other than macOS on head cb0b80e in run 42696617119 on 9 November 2026, 10 jobs succeeded, run as a dry run so nothing merged. Every macOS job was cancelled under the 9 November decision. The rc head is built with macOS before it merges into main.