From 08b26269d85c5a01b0ebcfeeb07e9c455a4c6681 Mon Sep 17 00:00:00 2001 From: Nadav Har'El Date: Mon, 22 Jan 2024 10:14:55 +0200 Subject: [PATCH 1/2] alternator: allow empty tag value The existing code incorrectly forbid setting a tag on a table to an empty string value, but this is allowed by DynamoDB and is useful, so we fix it in this patch. While at it, improve the error-checking code for tag parameters to cleanly detect more cases (like missing or non-string keys or values). The following patch is a test that fails before this patch (because it fails to insert a tag with an empty value) and passes after it. Fixes #16904. Signed-off-by: Nadav Har'El --- alternator/executor.cc | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/alternator/executor.cc b/alternator/executor.cc index a7812c7a48..90bb570913 100644 --- a/alternator/executor.cc +++ b/alternator/executor.cc @@ -763,15 +763,33 @@ enum class update_tags_action { add_tags, delete_tags }; static void update_tags_map(const rjson::value& tags, std::map& tags_map, update_tags_action action) { if (action == update_tags_action::add_tags) { for (auto it = tags.Begin(); it != tags.End(); ++it) { - const rjson::value& key = (*it)["Key"]; - const rjson::value& value = (*it)["Value"]; - auto tag_key = rjson::to_string_view(key); - if (tag_key.empty() || tag_key.size() > 128 || !validate_legal_tag_chars(tag_key)) { - throw api_error::validation("The Tag Key provided is invalid string"); + if (!it->IsObject()) { + throw api_error::validation("invalid tag object"); } - auto tag_value = rjson::to_string_view(value); - if (tag_value.empty() || tag_value.size() > 256 || !validate_legal_tag_chars(tag_value)) { - throw api_error::validation("The Tag Value provided is invalid string"); + const rjson::value* key = rjson::find(*it, "Key"); + const rjson::value* value = rjson::find(*it, "Value"); + if (!key || !key->IsString() || !value || !value->IsString()) { + throw api_error::validation("string Key and Value required"); + } + auto tag_key = rjson::to_string_view(*key); + auto tag_value = rjson::to_string_view(*value); + + if (tag_key.empty()) { + throw api_error::validation("A tag Key cannot be empty"); + } + if (tag_key.size() > 128) { + throw api_error::validation("A tag Key is limited to 128 characters"); + } + if (!validate_legal_tag_chars(tag_key)) { + throw api_error::validation("A tag Key can only contain letters, spaces, and [+-=._:/]"); + } + // Note tag values are limited similarly to tag keys, but have a + // longer length limit, and *can* be empty. + if (tag_value.size() > 256) { + throw api_error::validation("A tag Value is limited to 256 characters"); + } + if (!validate_legal_tag_chars(tag_value)) { + throw api_error::validation("A tag Value can only contain letters, spaces, and [+-=._:/]"); } tags_map[sstring(tag_key)] = sstring(tag_value); } From 830e52008d40f38bcfbf16aa325ef485139d7b1c Mon Sep 17 00:00:00 2001 From: Nadav Har'El Date: Sun, 21 Jan 2024 20:16:12 +0200 Subject: [PATCH 2/2] test/alternator: add more tests for TagResource Issue #16904 discovered that Alternator refuses to allow an empty tag value while it's useful (and DynamoDB allows it). This brought to my attention that our test coverage of the TagResource operation was lacking. So this patch adds more tests for some corner cases of TagResource which we missed, including the allowed lengths of tag keys and values. These tests reproduce #16904 (the case of empty tag value) and also #16908 (allowing and correctly counting unicode letters), and also add regression testing to cases which we already handled correctly. As usual, all the new tests also pass on DynamoDB. Refs #16904 Refs #16908 Signed-off-by: Nadav Har'El --- test/alternator/test_tag.py | 103 ++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/test/alternator/test_tag.py b/test/alternator/test_tag.py index 5ad7fae120..2a037a5b1c 100644 --- a/test/alternator/test_tag.py +++ b/test/alternator/test_tag.py @@ -342,3 +342,106 @@ def test_concurrent_tag(dynamodb, test_table): t2.start() t1.join() t2.join() + +# An empty string is allowed as a tag's value +# Reproduces #16904. +def test_empty_tag_value(dynamodb, test_table): + client = dynamodb.meta.client + arn = client.describe_table(TableName=test_table.name)['Table']['TableArn'] + tag = random_string() + client.tag_resource(ResourceArn=arn, Tags=[{'Key': tag, 'Value': ''}]) + # Verify that the tag with the empty value was correctly saved: + tags = client.list_tags_of_resource(ResourceArn=arn)['Tags'] + assert {'Key': tag, 'Value': ''} in tags + # Clean up the tag we just added + client.untag_resource(ResourceArn=arn, TagKeys=[tag]) + +# However, an empty string is NOT allowed as a tag's key +def test_empty_tag_key(dynamodb, test_table): + client = dynamodb.meta.client + arn = client.describe_table(TableName=test_table.name)['Table']['TableArn'] + with pytest.raises(ClientError, match='ValidationException'): + client.tag_resource(ResourceArn=arn, Tags=[{'Key': '', 'Value': 'dog'}]) + +# Although an empty Value is allowed for a tag, a *missing* Value is not +# allowed: +def test_missing_tag_value(dynamodb, test_table): + client = dynamodb.meta.client + arn = client.describe_table(TableName=test_table.name)['Table']['TableArn'] + with pytest.raises(ClientError, match='ValidationException'): + client.tag_resource(ResourceArn=arn, Tags=[{'Key': 'dog'}]) + +# According to the DynamoDB documentation, the maximum tag key length allowed +# is 128 characters. Actually, it's 128 *unicode* characters which are +# allowed, not 128 bytes. +# Reproduces #16908 +@pytest.mark.parametrize("is_ascii", [ + True, + pytest.param(False, marks=pytest.mark.xfail(reason="#16908"))]) +def test_tag_key_length_128_allowed(dynamodb, test_table, is_ascii): + client = dynamodb.meta.client + arn = client.describe_table(TableName=test_table.name)['Table']['TableArn'] + tag = ('x' if is_ascii else 'א') * 128 + client.tag_resource(ResourceArn=arn, Tags=[{'Key': tag, 'Value': 'dog'}]) + tags = client.list_tags_of_resource(ResourceArn=arn)['Tags'] + assert {'Key': tag, 'Value': 'dog'} in tags + client.untag_resource(ResourceArn=arn, TagKeys=[tag]) + +def test_tag_key_length_129_forbidden(dynamodb, test_table): + client = dynamodb.meta.client + arn = client.describe_table(TableName=test_table.name)['Table']['TableArn'] + tag = 'x'*129 + with pytest.raises(ClientError, match='ValidationException'): + client.tag_resource(ResourceArn=arn, Tags=[{'Key': tag, 'Value': 'dog'}]) + +# According to the DynamoDB documentation, the maximum tag value length +# allowed is 256 characters. Actually, it's 256 *unicode* characters which +# are allowed, not 256 bytes. +# Reproduces #16908 +@pytest.mark.parametrize("is_ascii", [ + True, + pytest.param(False, marks=pytest.mark.xfail(reason="#16908"))]) +def test_tag_value_length_256_allowed(dynamodb, test_table, is_ascii): + client = dynamodb.meta.client + arn = client.describe_table(TableName=test_table.name)['Table']['TableArn'] + tag = random_string() + value = ('x' if is_ascii else 'א') * 256 + client.tag_resource(ResourceArn=arn, Tags=[{'Key': tag, 'Value': value}]) + tags = client.list_tags_of_resource(ResourceArn=arn)['Tags'] + assert {'Key': tag, 'Value': value} in tags + client.untag_resource(ResourceArn=arn, TagKeys=[tag]) + +def test_tag_value_length_257_forbidden(dynamodb, test_table): + client = dynamodb.meta.client + arn = client.describe_table(TableName=test_table.name)['Table']['TableArn'] + value = 'x'*257 + with pytest.raises(ClientError, match='ValidationException'): + client.tag_resource(ResourceArn=arn, Tags=[{'Key': 'dog', 'Value': value}]) + +# According to the DynamoDB documentation, only letters, whitespace, numbers, +# and the characters [+-=._:/] are allowed in both tag keys or values. +# Let's check that other non-letter characters are not allowed in either +# key or value. +def test_tag_forbidden_chars(dynamodb, test_table): + client = dynamodb.meta.client + arn = client.describe_table(TableName=test_table.name)['Table']['TableArn'] + for x in ['hi!', 'd%g', '"hello"']: + with pytest.raises(ClientError, match='ValidationException'): + client.tag_resource(ResourceArn=arn, Tags=[{'Key': x, 'Value': 'dog'}]) + with pytest.raises(ClientError, match='ValidationException'): + client.tag_resource(ResourceArn=arn, Tags=[{'Key': 'dog', 'Value': x}]) + +# Check that's it's allowed to reassign a new value to an existing tag. +def test_tag_reassign(dynamodb, test_table): + client = dynamodb.meta.client + arn = client.describe_table(TableName=test_table.name)['Table']['TableArn'] + tag = random_string() + value1 = random_string() + value2 = random_string() + client.tag_resource(ResourceArn=arn, Tags=[{'Key': tag, 'Value': value1}]) + tags = client.list_tags_of_resource(ResourceArn=arn)['Tags'] + assert {'Key': tag, 'Value': value1} in tags + client.tag_resource(ResourceArn=arn, Tags=[{'Key': tag, 'Value': value2}]) + tags = client.list_tags_of_resource(ResourceArn=arn)['Tags'] + assert {'Key': tag, 'Value': value2} in tags + client.untag_resource(ResourceArn=arn, TagKeys=[tag])