Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fail fast and loud for invalid GELF messages #3972

Merged
merged 5 commits into from Jul 5, 2017
Merged

Fail fast and loud for invalid GELF messages #3972

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1293a6e
Fail fast and loud for invalid GELF messages
Jul 3, 2017
ec26295
Don't check "version" field for backward-compatibility
Jul 3, 2017
cb2597e
Make GELF validation more lenient
Jul 5, 2017
4ca36de
Add test for minimal GELF messages
Jul 5, 2017
7505d0b
Fix logic for validating "host" message field
Jul 5, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 45 additions & 0 deletions graylog2-server/src/main/java/org/graylog2/inputs/codecs/GelfCodec.java
View file
Open in desktop
Expand Up @@ -16,6 +16,7 @@
*/
package org.graylog2.inputs.codecs;

import com.eaio.uuid.UUID;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
Expand All @@ -24,6 +25,7 @@
import org.graylog2.inputs.codecs.gelf.GELFMessage;
import org.graylog2.inputs.transports.TcpTransport;
import org.graylog2.plugin.Message;
import org.graylog2.plugin.ResolvableInetSocketAddress;
import org.graylog2.plugin.Tools;
import org.graylog2.plugin.configuration.Configuration;
import org.graylog2.plugin.configuration.ConfigurationRequest;
Expand Down Expand Up @@ -124,6 +126,8 @@ public Message decode(@Nonnull final RawMessage rawMessage) {
throw new IllegalStateException("JSON is null/could not be parsed (invalid JSON)", e);
}

validateGELFMessage(node, rawMessage.getId(), rawMessage.getRemoteAddress());

// Timestamp.
final double messageTimestamp = doubleValue(node, Message.FIELD_TIMESTAMP);
final DateTime timestamp;
Expand Down Expand Up @@ -218,6 +222,47 @@ public Message decode(@Nonnull final RawMessage rawMessage) {
return message;
}

private void validateGELFMessage(JsonNode jsonNode, UUID id, ResolvableInetSocketAddress remoteAddress) {
final String prefix = "GELF message <" + id + "> " + (remoteAddress == null ? "" : "(received from <" + remoteAddress + ">) ");

final JsonNode hostNode = jsonNode.path("host");
if (hostNode.isMissingNode()) {
log.warn(prefix + "is missing mandatory \"host\" field.");
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check for the short_message / message fields first before returning? As it is now, a missing host field would skip the short_message / message field checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this is missing the alternative case.

}
if (!hostNode.isTextual()) {
throw new IllegalArgumentException(prefix + "has invalid \"host\": " + hostNode.asText());
}
if (StringUtils.isBlank(hostNode.asText())) {
throw new IllegalArgumentException(prefix + "has empty mandatory \"host\" field.");
}

final JsonNode shortMessageNode = jsonNode.path("short_message");
final JsonNode messageNode = jsonNode.path("message");
if (!shortMessageNode.isMissingNode()) {
if (!shortMessageNode.isTextual()) {
throw new IllegalArgumentException(prefix + "has invalid \"short_message\": " + shortMessageNode.asText());
}
if (StringUtils.isBlank(shortMessageNode.asText())) {
throw new IllegalArgumentException(prefix + "has empty mandatory \"short_message\" field.");
}
} else if (!messageNode.isMissingNode()) {
if (!messageNode.isTextual()) {
throw new IllegalArgumentException(prefix + "has invalid \"message\": " + messageNode.asText());
}
if (StringUtils.isBlank(messageNode.asText())) {
throw new IllegalArgumentException(prefix + "has empty mandatory \"message\" field.");
}
} else {
throw new IllegalArgumentException(prefix + "is missing mandatory \"short_message\" or \"message\" field.");
}

final JsonNode timestampNode = jsonNode.path("timestamp");
if (timestampNode.isValueNode() && !timestampNode.isNumber()) {
throw new IllegalArgumentException(prefix + "has invalid \"timestamp\": " + timestampNode.asText());
}
}

@Nullable
@Override
public CodecAggregator getAggregator() {
Expand Down
213 changes: 213 additions & 0 deletions graylog2-server/src/test/java/org/graylog2/inputs/codecs/GelfCodecTest.java
View file
Open in desktop
Expand Up @@ -29,10 +29,12 @@
import org.mockito.junit.MockitoJUnit;
import org.mockito.junit.MockitoRule;

import java.net.InetSocketAddress;
import java.nio.charset.StandardCharsets;
import java.util.Collections;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
import static org.hamcrest.Matchers.isA;
import static org.junit.Assume.assumeTrue;

Expand Down Expand Up @@ -148,4 +150,215 @@ public void decodeLargeCompressedMessageFails() throws Exception {
public void getAggregatorReturnsGelfChunkAggregator() throws Exception {
assertThat(codec.getAggregator()).isSameAs(aggregator);
}

@Test
public void decodeSucceedsWithoutHost() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"short_message\": \"A short message that helps you identify what is going on\""
+ "}";
final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

final Message message = codec.decode(rawMessage);
assertThat(message).isNotNull();
}

@Test
public void decodeFailsWithWrongTypeForHost() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": 42,"
+ "\"short_message\": \"A short message that helps you identify what is going on\""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has invalid \"host\": 42");
}

@Test
public void decodeFailsWithEmptyHost() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"\","
+ "\"short_message\": \"A short message that helps you identify what is going on\""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has empty mandatory \"host\" field.");
}

@Test
public void decodeFailsWithBlankHost() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \" \","
+ "\"short_message\": \"A short message that helps you identify what is going on\""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has empty mandatory \"host\" field.");
}

@Test
public void decodeFailsWithoutShortMessage() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> is missing mandatory \"short_message\" or \"message\" field.");
}

@Test
public void decodeFSucceedsWithoutShortMessageButWithMessage() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\","
+ "\"message\": \"A short message that helps you identify what is going on\""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

final Message message = codec.decode(rawMessage);
assertThat(message).isNotNull();
}

@Test
public void decodeFailsWithWrongTypeForShortMessage() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\","
+ "\"short_message\": 42"
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has invalid \"short_message\": 42");
}

@Test
public void decodeFailsWithWrongTypeForMessage() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\","
+ "\"message\": 42"
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has invalid \"message\": 42");
}

@Test
public void decodeFailsWithEmptyShortMessage() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\","
+ "\"short_message\": \"\""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has empty mandatory \"short_message\" field.");
}

@Test
public void decodeFailsWithEmptyMessage() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\","
+ "\"message\": \"\""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has empty mandatory \"message\" field.");
}

@Test
public void decodeFailsWithBlankShortMessage() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\","
+ "\"short_message\": \" \""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has empty mandatory \"short_message\" field.");
}

@Test
public void decodeFailsWithBlankMessage() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\","
+ "\"message\": \" \""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has empty mandatory \"message\" field.");
}

@Test
public void decodeFailsWithWrongTypeForTimestamp() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\","
+ "\"short_message\": \"A short message that helps you identify what is going on\","
+ "\"timestamp\": \"Foobar\""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> has invalid \"timestamp\": Foobar");
}

@Test
public void decodeIncludesSourceAddressIfItFails() throws Exception {
final String json = "{"
+ "\"version\": \"1.1\","
+ "\"host\": \"example.org\""
+ "}";

final RawMessage rawMessage = new RawMessage(json.getBytes(StandardCharsets.UTF_8), new InetSocketAddress("198.51.100.42", 24783));

assertThatIllegalArgumentException().isThrownBy(() -> codec.decode(rawMessage))
.withNoCause()
.withMessageMatching("GELF message <[0-9a-f-]+> \\(received from <198\\.51\\.100\\.42:24783>\\) is missing mandatory \"short_message\" or \"message\" field.");
}

@Test
public void decodeSucceedsWithMinimalMessages() throws Exception {
assertThat(codec.decode(new RawMessage("{\"short_message\":\"0\"}".getBytes(StandardCharsets.UTF_8)))).isNotNull();
assertThat(codec.decode(new RawMessage("{\"message\":\"0\"}".getBytes(StandardCharsets.UTF_8)))).isNotNull();
}
}