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.

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

DNM support client crypto multipart upload #157

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

yangwanyuan
Copy link
Contributor

Support client crypto multipart upload, but now use x-oss-meta-* as crypto header. Will replace with x-oss-client-side-crypto-* after OSS server code is ready

@coveralls
Copy link

coveralls commented Jan 23, 2019

Coverage Status

Coverage decreased (-35.3%) to 61.99% when pulling 0c6782b on yangwanyuan:ywy-multipart-crypto-dev-20190123 into df1e8fa on aliyun:master.

@huiguangjun huiguangjun requested review from hangzws, ailan-gl and coderall and removed request for ailan-gl January 24, 2019 05:51
oss2/api.py Outdated Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
oss2/api.py Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
get_result = self.rsa_crypto_bucket.get_object(key, byte_range=(32, 103))
self.assertEqual(get_result.read(), content[32:103+1])

self.assertRaises(oss2.exceptions.ClientError, self.rsa_crypto_bucket.get_object, key, byte_range=(31, None))
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个返回的异常需要再具体下,返回clienterror无法知道错误是什么

Copy link
Contributor Author

Choose a reason for hiding this comment

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

在api.py中是会返回具体message的。这里只捕获ClientError异常是因为参考了其他测试用例里的使用方法,保持代码风格一致。

tests/test_object.py Show resolved Hide resolved
tests/test_object.py Show resolved Hide resolved
tests/test_object.py Show resolved Hide resolved
oss2/headers.py Outdated Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
if 'range' in headers:
raise ClientError('Crypto bucket do not support range get')
if byte_range and (not utils.is_multiple_sizeof_encrypt_block(byte_range[0])):
raise ClientError('Crypto bucket get range start must align to encrypt block')
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里把出错的byte_range打出来,方便debug

Copy link
Collaborator

Choose a reason for hiding this comment

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

恩,这个版本先把byte_range打印出来,debug的时候可以看的比较清楚,另外range只支持对齐要在文档的说明。如果后续用户有不对齐的需求,后续版本可能还要考虑是否可以兼容?

oss2/api.py Outdated Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
oss2/api.py Outdated Show resolved Hide resolved
except:
return None

def check_plain_key_valid(self, plain_key, plain_key_hmac):
if str(hash(plain_key)) != plain_key_hmac:
raise ClientError("The decrypted key is inconsistent, make sure use right RSA key pair")
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里检查的应该是私钥的正确性,在错误提示中可以更明确点指出

tests/test_multipart.py Outdated Show resolved Hide resolved
oss2/api.py Outdated
:return: :class:`ListPartsResult <oss2.models.ListPartsResult>`
"""
logger.info("Start list parts securely, upload_id = {0}".format(upload_id))
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

缺少uploadPartCopy接口?

if not isinstance(headers, CaseInsensitiveDict):
headers = CaseInsensitiveDict(headers)

if 'content-md5' in headers:
headers['x-oss-meta-unencrypted-content-md5'] = headers['content-md5']
headers[OSS_CLIENT_SIDE_ENCRYPTION_UNENCRYPTED_CONTENT_MD5] = headers['content-md5']
Copy link
Collaborator

Choose a reason for hiding this comment

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

之前的逻辑是用户如果传入MD5,那么后端回校验,现在如果传入未加密数据的MD5,后端的逻辑怎么处理?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

后端对此不做任何处理,这里把MD5去掉的原因就是防止后端发现加密数据和明文数据的MD5不一样而报错,可以考虑重新计算加密数据的MD5填入。

# parts.append(oss2.models.PartInfo(i+1, result.etag, size = part_size, part_crc = result.crc))
#
#res = bucket.list_parts(multi_key, upload_id)
#crypto_multipart_context_new = res.crypto_multipart_context
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的示例能不能加上提示,提醒用户在中断后,获取上一次的context后,再次校验下以符合预期

Copy link
Contributor Author

Choose a reason for hiding this comment

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

好的,可以加上一些文字提示。由于现在对part的合法性判断全部移到服务端做了,所以用户是否校验符合预期已经不重要了,因为就算不符合,下一次上传时服务端也会报错。

return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里为什么不把异常抛出去

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里我没有做特殊更改,保留了原始开发者的代码处理逻辑。

Copy link
Collaborator

@hangzws hangzws Apr 17, 2019

Choose a reason for hiding this comment

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

这个地方有异常肯定要抛出来啊,原始开发的逻辑是不对的。


crypto_key = self.crypto_provider.decrypt_from_str(OSS_CLIENT_SIDE_ENCRYPTION_KEY, res.crypto_key)
crypto_start = int(self.crypto_provider.decrypt_from_str(OSS_CLIENT_SIDE_ENCRYPTION_START, res.crypto_start))
context = CryptoMultipartContext(crypto_key, crypto_start, res.client_encryption_data_size, res.client_encryption_part_size)
Copy link
Collaborator

Choose a reason for hiding this comment

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

结合语雀文档上的说明,在list接口中现在没有返回 key-hmac ?是否考虑加上,并添加校验逻辑?

count_start = self.__crypto_provider.decrypt_oss_meta_data(resp.headers, OSS_CLIENT_SIDE_ENCRYPTION_START)

# if content range , adjust the decrypt adapter
count_offset = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

这一行多了一个分号

headers = self.crypto_provider.build_header(headers, context)

resp = self.bucket.init_multipart_upload(key, headers)
resp.crypto_multipart_context = context;
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个地方有多余的分号

@@ -1768,6 +1791,150 @@ def get_object_to_file(self, key, filename,

return result

def init_multipart_upload(self, key, data_size, part_size = None, headers=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个地方data_size的值如果跟真实文件不一致会导致什么问题?complete的时候回校验这个data_size

logger.info("Start upload part by crypto bucket, upload_id = {0}, part_number = {1}".format(upload_id, part_number))

headers = http.CaseInsensitiveDict(headers)
headers[FLAG_CLIENT_SIDE_ENCRYPTION_MULTIPART_FILE] = "true"
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个header的命名为啥跟其他的命名风格不一致?

Copy link
Collaborator

Choose a reason for hiding this comment

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

为啥需要客户端传入FLAG_CLIENT_SIDE_ENCRYPTION_MULTIPART_FILE,有没有其他的解决方案?


return res

def list_parts(self, key, upload_id,
Copy link
Collaborator

Choose a reason for hiding this comment

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

uploadPartCopy接口的设计呢?

@@ -621,6 +623,8 @@ def get_object(self, key,
resp = self.__do_object('GET', key, headers=headers, params=params)
logger.debug("Get object done, req_id: {0}, status_code: {1}".format(resp.request_id, resp.status))

if models._hget(resp.headers, OSS_CLIENT_SIDE_ENCRYPTION_KEY):
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个地方在get_object这里做了限制,其他操作如symlink或者uploadpartcopy可以做类似的限制么?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants