fix(metrics): Remove/reject nul-bytes from metric strings [ingest-1204] #1235

untitaker · 2022-04-21T18:55:58Z

We are observing release names which are UTF-16 strings reinterpreted as
UTF-8. This is causing issues downstream and does not line up with the
releases that end up in the event payload, which for some reason are
totally correct.

In order to counteract this, we strip NUL-bytes from release values.

In other string types (tag keys, metrics), there are "fewer excuses" for
not getting those strings right, so we just drop them.

We are observing release names which are UTF-16 strings reinterpreted as UTF-8. This is causing issues downstream and does not line up with the releases that end up in the event payload, which for some reason are totally correct. In order to counteract this, we strip NUL-bytes from release values. In other string types (tag keys, metrics), there are "fewer excuses" for not getting those strings right, so we just drop them.

relay-metrics/src/aggregation.rs

See getsentry/relay#1235, we want to ensure we don't accept release names with NUL-bytes in them. We might want to strip NUL-bytes instead of dropping the entire release but that'd be too much effort for now.

untitaker · 2022-04-21T19:06:23Z

relay-general/src/protocol/event.rs

@@ -343,7 +343,7 @@ pub struct Event {
    /// can be the git SHA for the given project, or a product identifier with a semantic version.
    #[metastructure(
        max_chars = "tag_value",  // release ends in tag
-        deny_chars = "\r\n\x0c\t/\\",
+        // release allowed chars are validated in the sentry-release-parser crate!


this change does mean that the release name is only validated in the store normalizer. I think we will need to follow up with moving this part of store normalization to run before metrics extraction as well

create ticket

Does it even make sense to remove the deny_chars at this point? Or should we do that as part of the follow up?

A separate PR that moves this in one go would've been slightly more semantic, but let's keep it this way in light of moving fast.

https://getsentry.atlassian.net/browse/INGEST-1285

untitaker · 2022-04-22T15:28:35Z

relay-metrics/src/aggregation.rs

@@ -1055,6 +1080,8 @@ impl Aggregator {
        let timestamp = key.timestamp;
        let project_key = key.project_key;

+        let key = key.validate_strings().map_err(|()| AggregateMetricsError)?;


this will fail the entire batch. we have two options:

map the error to return Ok(())

make the error type an enum with multiple variants, and have matching logic in merge_all

@jjbayer @jan-auer @iker-barriocanal I believe we should change merge_all. there is a related case involving get_bucket_timestamps where the entire batch fails as well, for no discernible reason

untitaker · 2022-04-22T15:39:20Z

asking for additional review to make sure

jjbayer · 2022-04-25T05:51:14Z

relay-metrics/src/aggregation.rs

@@ -1140,7 +1167,7 @@ impl Aggregator {
        I: IntoIterator<Item = Bucket>,
    {
        for bucket in buckets.into_iter() {
-            self.merge(project_key, bucket)?;
+            let _result = self.merge(project_key, bucket);


I think this makes sense. Should we log to sentry or datadog when _result is not Ok though?

i considered this but there's no information on the error type. I can make this a simple error enum. I know that @jan-auer would have opinions on how we do error handlig

I'd definitely not silently drop the error. An enum works for me, but not opinionated here.

jan-auer · 2022-04-25T11:22:49Z

relay-metrics/src/aggregation.rs

@@ -716,8 +716,17 @@ impl Bucket {

 /// Any error that may occur during aggregation.
 #[derive(Debug, Fail)]
-#[fail(display = "failed to aggregate metrics")]
-pub struct AggregateMetricsError;
+pub enum AggregateMetricsError {


Thanks for introducing this, it makes sense at this point.

Please move this to an internal enum and leave the metrics aggregation error opaque to the public interface of relay-metrics. I would not expose the enum at all, and rely on Display outside of the crate. We should not match on the enum variant.

Introducing this enum variant could've been a good standalone PR to go before this one.

jan-auer · 2022-04-25T11:24:16Z

relay-metrics/src/aggregation.rs

@@ -745,6 +754,31 @@ impl BucketKey {
        std::hash::Hash::hash(self, &mut hasher);
        hasher.finalize() as i64
    }
+
+    /// Remove invalid characters from tags and metric names.


Metric names and metric tags have different character allow lists, let's have separate validator functions. Especially, there is already is_valid_name. parse_tags doesn't have such logic yet.

More of a code style suggestion: BucketKey is a plain data object used for bucketing. I would suggest to put such validator functions
on types that specify the public protocol or directly in the aggregator if it's easily possible.

jan-auer · 2022-04-25T11:24:35Z

relay-metrics/src/aggregation.rs

+    ///
+    /// Returns `Err` if the metric should be dropped.
+    fn validate_strings(mut self) -> Result<Self, ()> {
+        if self.metric_name.contains('\0') {


Can you instead reject all control characters here?

jan-auer · 2022-04-25T11:25:00Z

relay-metrics/src/aggregation.rs

+    fn validate_strings(mut self) -> Result<Self, ()> {
+        if self.metric_name.contains('\0') {
+            relay_log::error!("invalid metric name {:?}", self.metric_name);
+            return Err(());


Let's not log here but rather return AggregateMetricsError that can be logged somewhere else.

I didn't go for that because it would bloat the size of AggregateMetricsError but I can do it

Whether you return () or the error enum shouldn't affect the result size here since Self is larger anyway.

Ah I mean if I were to store the metric name in the error. and yeah even then it's not struct size but the extra allocation

Ah, I see. Maybe log this on debug! then, we certainly don't want that in a production instance.

I have removed it entirely now :/

jan-auer · 2022-04-25T11:26:02Z

relay-metrics/src/aggregation.rs

+            return Err(());
+        }
+
+        self.metric_name.retain(|c| c != '\0');


Isn't this redundant given that we're rejecting the name just before?

…acefully

In order for #1235 to land, we need the latest release parser that actually rejects control characters in release names.

jan-auer

Thanks!

* master: fix: Add bucket key to extra (#1243) fix(metrics): Remove/reject nul-bytes from metric strings (#1235) chore: Bump sentry-release-parser (#1242) fix(metrics): Fix MRI validation in statsd protocol (#1241) ref(metrics): Refactor aggregation error, recover from errors more gracefully (#1240)

untitaker requested a review from a team April 21, 2022 18:55

untitaker commented Apr 21, 2022

View reviewed changes

relay-metrics/src/aggregation.rs Show resolved Hide resolved

Update relay-metrics/src/aggregation.rs

8e69309

fix wrapping

ddf33c8

untitaker mentioned this pull request Apr 21, 2022

fix: Disallow NUL-bytes [ingest-1204] getsentry/sentry-release-parser#30

Merged

remove release/environment character validation

48455d0

untitaker commented Apr 21, 2022

View reviewed changes

add changelog

4539d6d

iker-barriocanal approved these changes Apr 22, 2022

View reviewed changes

jjbayer approved these changes Apr 22, 2022

View reviewed changes

untitaker commented Apr 22, 2022

View reviewed changes

ignore errors in merge_all

708928c

untitaker requested review from jan-auer and mitsuhiko April 22, 2022 15:39

jjbayer reviewed Apr 25, 2022

View reviewed changes

iker-barriocanal mentioned this pull request Apr 25, 2022

fix(metrics): Enforce metric name length limit [INGEST-1205] #1238

Merged

make aggregatemetricserror an enum

5461474

jan-auer reviewed Apr 25, 2022

View reviewed changes

ref(metrics): Refactor aggregation error, recover from errors more gr…

b2d8f18

…acefully

untitaker mentioned this pull request Apr 25, 2022

ref(metrics): Refactor aggregation error, recover from errors more gracefully [ingest-1204] #1240

Merged

untitaker added 7 commits April 25, 2022 13:54

add changelog

3ef65b3

Merge branch 'ref/aggregation-error' into fix/reject-null-values

df08cc2

address review feedback

dd7369f

Merge remote-tracking branch 'origin/master' into fix/reject-null-values

75a463c

fix lint

5a74087

restore debug stmts

3a6559a

fix mri validation

88e3bf9

fix mri validation

8ab8097

This was referenced Apr 25, 2022

fix(metrics): Fix MRI validation in statsd protocol [ingest-1204] #1241

Merged

fix: Actually disallow all ASCII control characters [ingest-1204] getsentry/sentry-release-parser#31

Merged

untitaker added 2 commits April 25, 2022 20:17

fix formatting

ef658b9

fix tests

02f0b08

untitaker changed the base branch from master to fix/mri-validation April 25, 2022 18:20

untitaker added 4 commits April 25, 2022 20:21

Merge branch 'fix/mri-validation' into fix/reject-null-values

2d695c8

fix tests

c526013

fix docstring

3618181

Merge branch 'fix/mri-validation' into fix/reject-null-values

ce8f57f

Base automatically changed from fix/mri-validation to master April 25, 2022 18:39

fix tests

df4f9f3

untitaker requested a review from jan-auer April 25, 2022 18:40

untitaker added a commit that referenced this pull request Apr 25, 2022

chore: Bump sentry-release-parser [ingest-1204]

0607100

In order for #1235 to land, we need the latest release parser that actually rejects control characters in release names.

untitaker mentioned this pull request Apr 25, 2022

chore: Bump sentry-release-parser [ingest-1204] #1242

Merged

fix docs

1b4b7ed

jan-auer approved these changes Apr 26, 2022

View reviewed changes

untitaker merged commit a66c0eb into master Apr 26, 2022

untitaker deleted the fix/reject-null-values branch April 26, 2022 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(metrics): Remove/reject nul-bytes from metric strings [ingest-1204] #1235

fix(metrics): Remove/reject nul-bytes from metric strings [ingest-1204] #1235

untitaker commented Apr 21, 2022

untitaker Apr 21, 2022 •

edited

Loading

jjbayer Apr 22, 2022

jan-auer Apr 25, 2022

untitaker Apr 25, 2022

untitaker Apr 22, 2022

untitaker Apr 22, 2022

untitaker commented Apr 22, 2022

jjbayer Apr 25, 2022

untitaker Apr 25, 2022

iker-barriocanal Apr 25, 2022

jan-auer Apr 25, 2022

jan-auer Apr 25, 2022

jan-auer Apr 25, 2022

jan-auer Apr 25, 2022

untitaker Apr 25, 2022

jan-auer Apr 25, 2022

untitaker Apr 25, 2022

jan-auer Apr 25, 2022

untitaker Apr 25, 2022

jan-auer Apr 25, 2022

jan-auer left a comment

fix(metrics): Remove/reject nul-bytes from metric strings [ingest-1204] #1235

fix(metrics): Remove/reject nul-bytes from metric strings [ingest-1204] #1235

Conversation

untitaker commented Apr 21, 2022

untitaker Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker commented Apr 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-auer left a comment

Choose a reason for hiding this comment

untitaker Apr 21, 2022 •

edited

Loading