util/log: fix redactability of logging tags#72992
util/log: fix redactability of logging tags#72992craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
|
This change is |
|
the two bugs:
probably a 3rd bug as well: redaction markers inside sensitive values should be escaped. Need to check whether the test catches it. |
273f5ae to
e5ca0f3
Compare
TestFormatRedaction to demonstrate two bugse5ca0f3 to
6051cd5
Compare
abarganier
left a comment
There was a problem hiding this comment.
Although this is my first time really digging into our logEntry/redaction code, I found the formattableTags API to be readable and straightforward - nice work.
It seems like a test or two is causing issues with the iterator, although I'm unsure if it's just the test or if we truly need to account for the case where bytes.IndexByte returns -1 🤔
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @abarganier and @knz)
pkg/util/log/formattable_tags.go, line 79 at r3 (raw file):
} // Advance the cursor to the beginning of the first next value. nv := bytes.IndexByte(i.tags, 0)
should we code defensively against a case where bytes.IndexByte() returns -1?
Code quote:
nv := bytes.IndexByte(i.tags, 0)pkg/util/log/formattable_tags.go, line 136 at r3 (raw file):
if len(val) > 0 { if len(key) != 1 { w.SafeRune('=')
I'm curious, what does a key with a length of 1 represent/imply?
Code quote:
if len(key) != 1 {
w.SafeRune('=')
}6051cd5 to
608a9a9
Compare
knz
left a comment
There was a problem hiding this comment.
It seems like a test or two is causing issues with the iterator,
it was just a linter error. Fixed.
Reviewable status:
pkg/util/log/formattable_tags.go, line 79 at r3 (raw file):
Previously, abarganier (Alex Barganier) wrote…
should we code defensively against a case where
bytes.IndexByte()returns-1?
No need - it's an invariant of the formattableTags that there's always a value for every key, possibly empty.
pkg/util/log/formattable_tags.go, line 136 at r3 (raw file):
Previously, abarganier (Alex Barganier) wrote…
I'm curious, what does a
keywith a length of 1 represent/imply?
We skip the = sign for 1-letter keys: we write "n1" in logs, not "n=1".
If `ApplyConfig` encountered certain kinds of errors, specifically malformed arguments generated by erroneous unit tests, it would fail with a go panic instead of a regular error. This is because `return nil, err` would assign `nil` to the cleanup callback just before it would be called in a defer. This patch fixes it. Release note: None
Release note: None
|
oh the test in TestStoreRangeMergeWithData reveals there may be nul bytes in keys or values. Looking into it now. |
Some time in the v21.2 cycle, the log entry preparation logic was refactored and a mistake was introduced: the logging tags were not any more subject to the redaction logic. The result was that redaction markers were missing in log tag values, and if a value had contained unbalanced redaction markers in a value string (say, as part of a SQL table key), it would have caused log file corruption and possibly a confidential data leak. This patch fixes that, by preparing the logging tags in the same way as the main message for each entry. Additionally, this patch addresses a long-standing shortcoming: it was previously possible to see raw ascii nul bytes in the tag output. This is not possible any more. Release note (cli change): A bug affecting the redactability of logging tags in output log entries has been fixed. This bug had been introduced in the v21.2 release.
608a9a9 to
e57ace1
Compare
Fixed. |
abarganier
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/util/log/formattable_tags.go, line 79 at r3 (raw file):
Previously, knz (kena) wrote…
No need - it's an invariant of the
formattableTagsthat there's always a value for every key, possibly empty.
Ack, with the addition of escapeNulBytes it seems like we're good here 👍
pkg/util/log/formattable_tags.go, line 136 at r3 (raw file):
Previously, knz (kena) wrote…
We skip the
=sign for 1-letter keys: we write "n1" in logs, not "n=1".
Ack.
|
TFYR! bors r=abarganier |
|
blathers backport 21.2 |
|
Build succeeded: |
Fixes #72905.
Some time in the v21.2 cycle, the log entry preparation logic was
refactored and a mistake was introduced: the logging tags were not any
more subject to the redaction logic. The result was that redaction
markers were missing in log tag values, and if a value had contained
unbalanced redaction markers in a value string (say, as part of a SQL
table key), it would have caused log file corruption and possibly a
confidential data leak.
This patch fixes that, by preparing the logging tags in the same way
as the main message for each entry.
Release note (cli change): A bug affecting the redactability of
logging tags in output log entries has been fixed. This bug had
been introduced in the v21.2 release.