LogStash::Event should not allow nil keys!

[andrewvc]

Currently, it is possible to create an event with nil keys using the following code LogStash::Event.new(nil => 123). This should fail! It really screws with things like the Elasticsearch output.

The accessor [] does not have this error, but should raise a better error message (it just happens to fail by trying to invoke nil.starts_with?.

[jordansissel]

There's a bunch of places this can happen. Many plugins probably use Event#to_hash and then modify it directly (event.to_hash.merge!(nil => 123)). I helped someone last week with exactly this issue, where they had a filter that was injecting a nil key using to_hash.merge - oops!

I'm OK if we take an iterative approach to closing off the areas where nil keys are entering the event, though, and close them one at a time (constructor, to_hash, accessor, etc)

[andrewvc]

@jordansissel looking at the code, I see what you mean. When we call to_hash we're exposing the internal state of the object. I assumed it was a copy (I wish we were using clojure about now ;) ).

Given that I see two ways forward:

1. Clone the entire object when executing to_hash
2. Run a nil check when executing to_hash.

What are your thoughts?

[jordansissel]

@andrewvc I think the problem is slightly beyond API weakness. Some filters rely on to_hash being a modifiable object (historically, pre Logstash 1.2.0, field references weren't even available in Logstash, so this was the only way to really do much with the event).

We could tighten up our contracts and make to_hash provide something immutable probably, but if we do only that, we'll abandon (by accident) all folks who want to, say, take one hash and smush it into an existing event.

What if we allow a way for plugins to merge complex data structures into an event? It's funky because you can technically do this in Logstash/Ruby: event["[some][field]"] = { "my" => "hash" } and it all works out. I don't know if we want to accept this in the future, but it's what works today.

Hopefully I'm making sense - the issue is that folks are using an means to add things into an event that isn't safe and we don't currently provide a safe way to do it.

[andrewvc]

@jordansissel given that it sounds like we just need to filter data on the way out of to_hash, by invoking null check.

So, the principle is garbage in, shiny null safe hashes out.

Sound good?

[jordansissel]

@andrewvc I think we need to filter things on the way in, not out.

In a filter, if I have:

event.to_hash.merge!(nil => 123)

The "out" of to_hash could have produced a hash with no nils, but I Just forced a nil key into it with merge!. I don't know if we can reasonably protect against this unless we override all means by which to add new keys to our hash.

[jordansissel]

It's also worth noting that this would also bypass any protections we put directly on the Event:

event["[some][thing]"] = { nil => 123 }

At some point, validation feels like it will become this recursive descent searching the key space (and any nested objects) for nil keys. Feels expensive. I'm hoping we can come up with an API that allows free-ish assignment of values to fields but allows validation at-assignment-time (cheaper than walking the entire object each time)

[andrewvc]

@jordansissel yes, I don't think we can protect against this unless we subclass Hash but that seems unwise. Additionally, it doesn't protect us against nested values!

That's why I'm thinking the only guarantees we can really make are on the way out.

I would vastly prefer to protect against this stuff on the way in, I'm just struggling to find a way.

[jordansissel]

@andrewvc let's spend some time thinking about how to provide an API on Event that may allow us to do this on the way in - I hope we can figure something out ;P

[colinsurprenant]

+1 on avoiding expensive recursive descent searching the key space (and any nested objects) for nil keys

[andrewvc]

@jordansissel hmmm, I don't think its possible within the confines of ruby, especially given that you can nest arbitrary ruby objects in there. A further question is, what happens if an event does this? event['foo'] = {'blah' => {Thread.new => 'hello!'}}.

I think if we really want to tighten stuff up we'll have to use recursive sanitizing on the way in, and make sure we only emit immutable objects on the way out. This would require breaking the current contract and would need to coincide with a major version change.

[andrewvc]

We could also just label these interfaces as 'dangerous', and put the responsibility on the plugin authors. The issue here is that when it blows up in weird ways (as in the Elasticsearch output bug I just debugged), it will look like logstash's fault, not the plugins.

That may be an acceptable cost however, given the relative infrequency of these occurrences.

[colinsurprenant]

AFAIK this hasn't been a very common problem, so as a pragmatic short-term solution I suggest we simply add an event cleaner plugin or a new mutate function to remove nil keys?

also, could we better trap these nil keys errors/exceptions and reject+log the offending event?