Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,24 @@ public static void verifyResourceName(String... resourceNames) {
}
}

/**
* verifies that key name is a valid name.
*
* @param keyName key name to be validated
*
* @throws IllegalArgumentException
*/
public static void verifyKeyName(String keyName) {
if (keyName == null) {
throw new IllegalArgumentException("Key name is null");
}
if(!OzoneConsts.KEYNAME_ILLEGAL_CHARACTER_CHECK_REGEX
.matcher(keyName).matches()){
throw new IllegalArgumentException("key name contains " +
"illegal characters.");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would like to propose making this message consistent with the one in OmUtils, so that ozone fs -put and ozone sh key put both print the same error. I think it can be most simply done by moving the message from OmUtils here and letting the OMException propagate the underlying exception's message.

Suggested change
throw new IllegalArgumentException("key name contains " +
"illegal characters.");
throw new IllegalArgumentException("Invalid key name: " + keyName);

If you agree, please feel free to wait for others' reviews before making this change, to reduce the number of rounds.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I agree with you.
I'll wait for others' reviews and suggestions before I make this change.

}
}

/**
* Checks that object parameters passed as reference is not null.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.apache.ratis.thirdparty.io.grpc.Context;
import org.apache.ratis.thirdparty.io.grpc.Metadata;

import java.util.regex.Pattern;

import static org.apache.ratis.thirdparty.io.grpc.Metadata.ASCII_STRING_MARSHALLER;

/**
Expand Down Expand Up @@ -324,5 +326,19 @@ private OzoneConsts() {
public static final String GDPR_SECRET = "secret";
public static final String GDPR_ALGORITHM = "algorithm";

/**
* Block key name as illegal characters
*
* This regular expression is used to check if key name
* contains illegal characters when creating/renaming key.
*
* Avoid the following characters in a key name:
* "\", "{", "}", "^", "<", ">", "#", "|", "%", "`", "[", "]", "~", "?"
* and Non-printable ASCII characters (128–255 decimal characters).
* https://docs.aws.amazon.com/AmazonS3/latest/dev/UsingMetadata.html
*/
public static final Pattern KEYNAME_ILLEGAL_CHARACTER_CHECK_REGEX =
Pattern.compile("^[^^{}<>^?%~#`\\[\\]\\|\\\\(\\x80-\\xff)]+$");

public static final String FS_FILE_COPYING_TEMP_SUFFIX= "._COPYING_";
}
10 changes: 10 additions & 0 deletions hadoop-hdds/common/src/main/resources/ozone-default.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2581,4 +2581,14 @@
Timeout for the request submitted directly to Ratis in datanode.
</description>
</property>
<property>
<name>ozone.om.keyname.character.check.enabled</name>
<tag>OZONE, OM</tag>
<value>false</value>
<description>If true, then enable to check if the key name
contains illegal characters when creating/renaming key.
For the definition of illegal characters, follow the
rules in Amazon S3's object key naming guide.
</description>
</property>
</configuration>
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
import org.apache.hadoop.ozone.client.io.OzoneInputStream;
import org.apache.hadoop.ozone.client.io.OzoneOutputStream;
import org.apache.hadoop.ozone.client.protocol.ClientProtocol;
import org.apache.hadoop.ozone.om.OMConfigKeys;
import org.apache.hadoop.ozone.om.ha.OMFailoverProxyProvider;
import org.apache.hadoop.ozone.om.helpers.BucketEncryptionKeyInfo;
import org.apache.hadoop.ozone.om.helpers.OmBucketArgs;
Expand Down Expand Up @@ -144,6 +145,7 @@ public class RpcClient implements ClientProtocol {
private final long retryInterval;
private Text dtService;
private final boolean topologyAwareReadEnabled;
private final boolean checkKeyNameEnabled;

/**
* Creates RpcClient instance with the given configuration.
Expand Down Expand Up @@ -240,6 +242,9 @@ public RpcClient(ConfigurationSource conf, String omServiceId)
topologyAwareReadEnabled = conf.getBoolean(
OzoneConfigKeys.OZONE_NETWORK_TOPOLOGY_AWARE_READ_KEY,
OzoneConfigKeys.OZONE_NETWORK_TOPOLOGY_AWARE_READ_DEFAULT);
checkKeyNameEnabled = conf.getBoolean(
OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_KEY,
OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_DEFAULT);
}

@Override
Expand Down Expand Up @@ -618,6 +623,9 @@ public OzoneOutputStream createKey(
Map<String, String> metadata)
throws IOException {
HddsClientUtils.verifyResourceName(volumeName, bucketName);
if(checkKeyNameEnabled) {
HddsClientUtils.verifyKeyName(keyName);
}
HddsClientUtils.checkNotNull(keyName, type, factor);
String requestId = UUID.randomUUID().toString();

Expand Down Expand Up @@ -695,6 +703,9 @@ public void deleteKey(
public void renameKey(String volumeName, String bucketName,
String fromKeyName, String toKeyName) throws IOException {
HddsClientUtils.verifyResourceName(volumeName, bucketName);
if(checkKeyNameEnabled){
HddsClientUtils.verifyKeyName(toKeyName);
}
HddsClientUtils.checkNotNull(fromKeyName, toKeyName);
OmKeyArgs keyArgs = new OmKeyArgs.Builder()
.setVolumeName(volumeName)
Expand Down Expand Up @@ -870,6 +881,9 @@ public OzoneOutputStream createMultipartKey(String volumeName,
String uploadID)
throws IOException {
HddsClientUtils.verifyResourceName(volumeName, bucketName);
if(checkKeyNameEnabled) {
HddsClientUtils.verifyKeyName(keyName);
}
HddsClientUtils.checkNotNull(keyName, uploadID);
Preconditions.checkArgument(partNumber > 0 && partNumber <=10000, "Part " +
"number should be greater than zero and less than or equal to 10000");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,4 +262,26 @@ public void testVerifyResourceName() {
}
}
}

@Test
public void testVerifyKeyName() {
List<String> invalidNames = new ArrayList<>();
invalidNames.add("#");
invalidNames.add("ab^cd");
invalidNames.add("test|name~");
invalidNames.add("~hi!ozone");
invalidNames.add("test<string>");
invalidNames.add("10%3=1");
invalidNames.add("photo[0201]");
invalidNames.add("what?");

for (String name : invalidNames) {
try {
HddsClientUtils.verifyKeyName(name);
fail("Did not reject invalid string [" + name + "] as a name");
} catch (IllegalArgumentException e) {
// throwing up on an invalid name. it's working.
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -506,4 +506,17 @@ public static long getOMClientRpcTimeOut(Configuration configuration) {
return OzoneConfiguration.of(configuration)
.getObject(OMClientConfig.class).getRpcTimeOut();
}

/**
* Verify key name is a valid name.
*/
public static void validateKeyName(String keyName)
throws OMException {
try {
HddsClientUtils.verifyKeyName(keyName);
} catch (IllegalArgumentException e) {
throw new OMException("Invalid key name: " + keyName,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Corresponding to the suggestion for HddsClientUtils:

Suggested change
throw new OMException("Invalid key name: " + keyName,
throw new OMException(e.getMessage(),

OMException.ResultCodes.INVALID_KEY_NAME);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -231,4 +231,9 @@ private OMConfigKeys() {
// hadoop-policy.xml, "*" allows all users/groups to access.
public static final String OZONE_OM_SECURITY_CLIENT_PROTOCOL_ACL =
"ozone.om.security.client.protocol.acl";

public static final String OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_KEY =
"ozone.om.keyname.character.check.enabled";
public static final boolean OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_DEFAULT =
false;
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,11 @@

import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.ozone.OmUtils;
import org.apache.hadoop.ozone.OzoneAcl;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.om.OMConfigKeys;
import org.apache.hadoop.ozone.om.exceptions.OMReplayException;
import org.apache.hadoop.ozone.om.response.file.OMFileCreateResponse;
import org.slf4j.Logger;
Expand Down Expand Up @@ -90,6 +94,15 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {

KeyArgs keyArgs = createFileRequest.getKeyArgs();

// Verify key name
final boolean checkKeyNameEnabled = ozoneManager.getConfiguration()
.getBoolean(OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_KEY,
OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_DEFAULT);
if(checkKeyNameEnabled){
OmUtils.validateKeyName(StringUtils.removeEnd(keyArgs.getKeyName(),
OzoneConsts.FS_FILE_COPYING_TEMP_SUFFIX));
}

if (keyArgs.getKeyName().length() == 0) {
// Check if this is the root of the filesystem.
// Not throwing exception here, as need to throw exception after
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@

import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.ozone.OmUtils;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.om.OMConfigKeys;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer;
import org.slf4j.Logger;
Expand Down Expand Up @@ -83,6 +87,15 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {

KeyArgs keyArgs = commitKeyRequest.getKeyArgs();

// Verify key name
final boolean checkKeyNameEnabled = ozoneManager.getConfiguration()
.getBoolean(OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_KEY,
OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_DEFAULT);
if(checkKeyNameEnabled){
OmUtils.validateKeyName(StringUtils.removeEnd(keyArgs.getKeyName(),
OzoneConsts.FS_FILE_COPYING_TEMP_SUFFIX));
}

KeyArgs.Builder newKeyArgs =
keyArgs.toBuilder().setModificationTime(Time.now());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import org.apache.hadoop.ozone.OmUtils;
import org.apache.hadoop.ozone.om.OMConfigKeys;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -84,6 +86,13 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {

KeyArgs keyArgs = createKeyRequest.getKeyArgs();

// Verify key name
final boolean checkKeyNameEnabled = ozoneManager.getConfiguration()
.getBoolean(OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_KEY,
OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_DEFAULT);
if(checkKeyNameEnabled){
OmUtils.validateKeyName(keyArgs.getKeyName());
}
// We cannot allocate block for multipart upload part when
// createMultipartKey is called, as we will not know type and factor with
// which initiateMultipartUpload has started for this key. When
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,9 @@

import com.google.common.base.Optional;
import com.google.common.base.Preconditions;
import org.apache.hadoop.ozone.OmUtils;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.om.OMConfigKeys;
import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
import org.apache.hadoop.ozone.security.acl.IAccessAuthorizer;
import org.apache.hadoop.ozone.security.acl.OzoneObj;
Expand Down Expand Up @@ -83,6 +85,14 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
RenameKeyRequest renameKeyRequest = getOmRequest().getRenameKeyRequest();
Preconditions.checkNotNull(renameKeyRequest);

// Verify key name
final boolean checkKeyNameEnabled = ozoneManager.getConfiguration()
.getBoolean(OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_KEY,
OMConfigKeys.OZONE_OM_KEYNAME_CHARACTER_CHECK_ENABLED_DEFAULT);
if(checkKeyNameEnabled){
OmUtils.validateKeyName(renameKeyRequest.getToKeyName());
}

// Set modification time.
KeyArgs.Builder newKeyArgs = renameKeyRequest.getKeyArgs().toBuilder()
.setModificationTime(Time.now());
Expand Down