Conversation
b201372 to
777b8ba
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces hash-based redaction as an opt-in feature that allows correlating log entries while preserving privacy. When enabled, sensitive values marked with the HashValue interface are hashed (using SHA-1/HMAC-SHA1) instead of being fully redacted, with the hash output denoted by a dagger (†) prefix in redaction markers.
Key Changes:
- Adds
EnableHashing(salt)API to enable hash-based redaction with optional HMAC salt - Introduces
HashValueinterface and wrapper types (HashString,HashInt, etc.) for marking values as hashable - Implements hash computation logic using SHA-1/HMAC-SHA1 with truncation to 8 hex characters
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| api.go | Exports new HashValue types and EnableHashing function to public API |
| interfaces/interfaces.go | Defines HashValue marker interface and concrete wrapper types for hashable values |
| internal/markers/constants.go | Adds HashPrefix constant (†) and updates regex patterns to handle hash markers |
| internal/markers/hash.go | Implements hash computation logic with SHA-1/HMAC-SHA1 and global state management |
| internal/markers/hash_test.go | Adds unit tests for low-level hash functions with various inputs and salt configurations |
| internal/markers/markers.go | Updates Redact() methods to detect and process hash markers (‹†value›) |
| internal/rfmt/print.go | Adds HashValue interface detection in three locations to wrap values with hash markers during formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
53b3995 to
365e09b
Compare
dhartunian
left a comment
There was a problem hiding this comment.
This is great! I will review more thoroughly next week. Thanks for working on this.
This commit introduces the ability to hash sensitive information instead
of fully redacting it. This let's the user correlate entries while still
preserving privacy. This is an opt-in feature and requires the user to explicitly mark
values as "hashable".
Changes:
* EnableHashing(salt) is used to turn on hash-based redaction. If this is
not called, redaction behaves as before (full redaction).
* If both SafeValue and HashValue interfaces are implemented on a type,
SafeValue takes precedence.
* Uses SHA-1 / HMAC-SHA1 (if salt is provided) to hash the sensitive
parts of the string.
* A dagger (†) prefix is added within the redaction markers to denote
that it should be hashed instead of fully redacted.
* There are 2 ways to mark a value "hashable" (similar to redaction):
1) Implementing the HashValue interface directly on a type.
2) Wrapping a value with the HashString/HashBytes/Hash* functions
* Hashes are calculated at formatting time and not during string
creation. Just like redaction.
**Example usage**
* Implementing the HashValue interface:
```go
type UserID string
func (u UserID) HashValue() string {} // makes the type "hashable"
fmt.Println(redact.Sprintf("%s", UserID("alice")).Redact()) // Output: ‹522b276a›
```
* Inline with helper functions:
```go
fmt.Println(redact.Sprintf("%s", redact.HashString("alice")).Redact()) // Output: ‹522b276a›
```
365e09b to
74df909
Compare
|
Ack. Will address the comments end of the week. |
dhartunian
left a comment
There was a problem hiding this comment.
This PR also contains no tests for any redactable and hashed strings and mixed hashed redactable stuff, etc. Huge test coverage gap.
aa-joshi
left a comment
There was a problem hiding this comment.
@aa-joshi reviewed 4 files and made 3 comments.
Reviewable status: 4 of 9 files reviewed, 17 unresolved discussions (waiting on Abhinav1299, arjunmahishi, and dhartunian).
internal/markers/hash.go line 1 at r4 (raw file):
// Copyright 2025 The Cockroach Authors.
nit: 2026
internal/markers/markers.go line 94 at r4 (raw file):
// they are redacted like regular markers. func (s RedactableBytes) Redact() RedactableBytes { result := ReStripSensitive.ReplaceAllFunc([]byte(s), func(match []byte) []byte {
same as here: https://reviewable.io/reviews/cockroachdb/redact/33#gh-2823788809
eae80f4 to
12de922
Compare
12de922 to
576761f
Compare
dhartunian
left a comment
There was a problem hiding this comment.
Nice work. Just a few more edge cases and then we should be good go. Thanks!
| // TestHashRedact_RedactableBytesDisabled verifies bytes-path behavior when hashing is disabled. | ||
| func TestHashRedact_RedactableBytesDisabled(t *testing.T) { | ||
| defer DisableHashing() | ||
| DisableHashing() |
There was a problem hiding this comment.
Why do we disable hashing twice?
There was a problem hiding this comment.
There should be only one.
the defer DisableHashing() was a boilerplate I used for all testcases and then applied the hashing state which was needed as per test. In this test it was specifically testing a disbabled state, so the defer is not needed.
hash_redact_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // TestHashRedact_WithSalt verifies that salted hashing produces different output than unsalted. |
There was a problem hiding this comment.
nit: These comments on top of each test are noise.
There was a problem hiding this comment.
I have removed them and kept only the ones required.
| } | ||
|
|
||
| // TestHashRedact_ToggleTransitions verifies Enable -> Disable -> Enable transitions. | ||
| func TestHashRedact_ToggleTransitions(t *testing.T) { |
There was a problem hiding this comment.
can you add a test that concurrently enables/disables hashing from multiple goroutines? that will help test the concurrency model.
| EnableHashing(nil) | ||
| defer DisableHashing() | ||
|
|
||
| s := Sprintf("user=%s action=%s", HashString("alice"), "login") |
There was a problem hiding this comment.
If you use a string like %s%s with first a hashed and then a non-hashed value, they'll get combined together because of an optimization that combines adjacent redaction zones, that will hash the combined value because the hash marker will apply to both. See if we can avoid that.
There was a problem hiding this comment.
Modified startRedactable() to support this and added test cases as well for multiple scenarios under this.
| EscapeMark = '?' | ||
| EscapeMarkS = string(EscapeMark) | ||
| RedactedS = StartS + "×" + EndS | ||
| HashPrefix = '†' |
There was a problem hiding this comment.
I think this marker needs to be escaped in InternalEscapeBytes so that it doesn't force hash behavior in customer-controlled text. EscapeMarkers does this correctly because it uses ReStripMarkers, but InternalEscapeBytes is different.
There was a problem hiding this comment.
Agreed, InternalEscapeBytes should also escape leading † for consistency with EscapeMarkers. I have added the changes for it along with test cases.
… concurrency test
This commit introduces the ability to hash sensitive information instead of fully redacting it. This let's the user correlate entries while still preserving privacy. This is an opt-in feature and requires the user to explicitly mark values as "hashable".
Changes:
EnableHashing(salt)is used to turn on hash-based redaction. If this isnot called, redaction behaves as before (full redaction).
SafeValueandHashValueinterfaces are implemented on a type,SafeValuetakes precedence.SHA-256/HMAC-SHA256(if salt is provided) to hash the sensitiveparts of the string.
†) prefix is added within the redaction markers to denotethat it should be hashed instead of fully redacted.
HashValueinterface directly on a type.HashString/HashBytes/Hash*functionscreation. Just like redaction.
Example usage
Implementing the
HashValueinterface:Inline with helper functions:
Benchmark of hashing enabled vs disabled
This change is