-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add metrics for the checksums of the HTTP body #1354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
e041036
to
4821d61
Compare
Hi there, thanks for this handy improvement. We are really looking forward for this check. Is there a timeline when this is going to be merged and possibly released? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I see the attraction of having a cryptographic checksum I wonder what use cases are solved by that which having a simple 32-bit checksum doesn't solve? I can see cases where people accidentally turn this on for dynamic content and get tons of cardinality.
(There is also another option here, if we used a larger hash function we could truncate it to 2^53 per https://stackoverflow.com/a/3793950 and still store that in a float64, meaning we'd have more than 32-bits of checksum but still use a simple integer. I would be surprised if anyone actually needs cryptographically secure hashing from their probes.)
prober/http.go
Outdated
} | ||
|
||
func hashContent(src io.Reader, hashBody bool, useHash string) (hashStr string, crc uint32, err error) { | ||
crcHash := crc32.New(crc32.MakeTable(crc32.IEEE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already use of FNV-1a for IP hashing in blackbox_exporter. I think we should stick to one hash algorithm rather than picking a random one. (No 32-bit hash function is going to be perfect, but FNV-1a does have fewer collisions than crc32.)
prober/http.go
Outdated
_, err = io.Copy(io.Discard, byteCounter) | ||
enableHash := len(httpConfig.FailIfBodyNotMatchesHash) > 0 || httpConfig.ExportHash | ||
|
||
hashStr, crc, err := hashContent(byteCounter, enableHash, httpConfig.HashAlgorithm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
byteCounter
wraps a http.MaxBytesReader
above. This is maybe fine as that is on the failure path anyway, but we probably should at least mention in the config docs that if the probe fails due to body_size_limit the returned checksums won't be on the complete content, but the part that was read up to the limit.
fcfa359
to
7442c05
Compare
Add metrics to monitor the integrity of an HTTP resource. These are configured using: - `fail_if_body_not_matches_hash` configures hash-based probe failures. - `hash_algorithm` (`sha256` by default) configures the hash used. - `export_hash` enables exporting the hashed body as a label. This results in the following new metrics: - `probe_http_content_checksum` contains the CRC32 of the page as a value. This is not cryptographically secure, but should work sufficiently well for monitoring changes in normal situations. - `probe_http_content_hash` contains a configurable hash of the page in a label. This *is* cryptographically secure, but may lead to high cardinality when enabled. The hash is configurable. - `probe_failed_due_to_hash` contains a metric that indicates if the probe failed because the content hash did not match the expected value. Signed-off-by: Silke Hofstra <[email protected]>
7442c05
to
70feb34
Compare
I included it because there were people that had a use case for this in #351. For example:
These files rarely change, so cardinality shouldn't be an issue.
That idea has crossed my mind, but having a value that can be converted to hex to verify seems more useful to me. Especially since the 53 bytes is still not enough to be cryptographically secure. |
Add metrics to monitor the integrity of an HTTP resource. These are configured using:
fail_if_body_not_matches_hash
configures hash-based probe failures.hash_algorithm
(sha256
by default) configures the hash used.export_hash
enables exporting the hashed body as a label.This results in the following new metrics:
probe_http_content_checksum
contains the CRC32 of the page as a value. This is not cryptographically secure, but should work sufficiently well for monitoring changes in normal situations.probe_http_content_hash
contains a configurable hash of the page in a label. This is cryptographically secure, but may lead to high cardinality when enabled. The hash is configurable.probe_failed_due_to_hash
contains a metric that indicates if the probe failed because the content hash did not match the expected value.Resolves #351