[KODO-13155] 添加服务端文件签名校验 #532

yinxulai · 2021-10-15T10:12:54Z

jira-issue

https://jira.qiniu.io/browse/KODO-13155

改动

直传时添加 crc32 与服务端对文件进行校验
分片上传使用 MD5 与服务端对分片进行校验
添加 checkByServer 启用上述两个功能（默认关闭）

yinxulai · 2021-10-15T10:13:33Z

test/demo1/main.js

+      checkCrc: true,
+      checkByMD5: true,
+      forceDirect: true,


codecov · 2021-10-15T10:14:50Z

Codecov Report

Merging #532 (63e20de) into master (b65d484) will increase coverage by 0.43%.
The diff coverage is 81.03%.

❗ Current head 63e20de differs from pull request most recent head 3fd2786. Consider uploading reports for the commit 3fd2786 to get more accurate results

@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
+ Coverage   73.96%   74.39%   +0.43%     
==========================================
  Files          20       21       +1     
  Lines        1060     1117      +57     
  Branches      197      210      +13     
==========================================
+ Hits          784      831      +47     
- Misses        276      286      +10

Impacted Files	Coverage Δ
src/api/index.ts	`48.83% <0.00%> (-2.39%)`	⬇️
src/upload/base.ts	`82.72% <ø> (ø)`
src/utils/config.ts	`95.00% <ø> (ø)`
src/upload/direct.ts	`63.41% <50.00%> (-1.46%)`	⬇️
src/utils/crc32.ts	`88.00% <88.00%> (ø)`
src/upload/resume.ts	`94.33% <100.00%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b65d484...3fd2786. Read the comment docs.

yinxulai · 2021-10-18T02:17:37Z

个人倾向将新添加的 checkByServer 与原来的 checkByMD5 配置合并。

简单说一下两个配置代表的实际功能

checkByServer 在请求时将本地计算的签名作为参数传送给服务端、服务端将接收到的数据与接收到的签名进行校验比对
checkByMD5 在分片上传时、将当前的分片与缓存(上次分片上传成功时的服务端返回的 MD5)进行比对来决定是缓存是否有效。

合并的理由

两种手段都是通过与服务端进行签名比对来确保数据有效性的一种手段，唯一的差别可能是比对行为的发生主体不同
checkByMD5 这个功能作为配置略显鸡肋、关闭的好处可能只有减少了一些MD5计算，在数据安全上、我认为在使用缓存时校验 MD5 还是很有必要的、不应该为了减少计算、放弃这个安全验证。

yinxulai · 2021-10-18T03:13:16Z

跟产品讨论了一下，产品支持移除 checkByMD5 参数、内部实现为缓存固定验证 md5

lzfee0227 · 2021-10-18T03:04:22Z

src/utils/crc32.ts

@@ -0,0 +1,60 @@
+/* eslint-disable no-bitwise */


这文件参考哪里搞出来的，贴个出处

并且写一下
现成第三方库为什么不满足需求
以及相对于被参考的第三方库来说，修改了什么东西（后续被参考的第三方库如果修复了什么 bug 这里就能溯源跟进）

lzfee0227 · 2021-10-18T03:21:17Z

src/utils/crc32.ts

+    if (file.size <= MB) {
+      // arrayBuffer 方法 jest(jsdom) 暂时不支持、所以不对该方法添加单测
+      // https://github.com/jsdom/jsdom/issues/3206 => https://github.com/jsdom/jsdom/issues/2555
+      const block = await file.arrayBuffer()


现在兼容到 IE 什么版本的？支持这个吗？

ie 不支持

这个兼容性已经处理

所以到底是兼容到什么版本？ie 9?

src/utils/crc32.ts

lzfee0227 · 2021-10-18T03:23:59Z

src/utils/crc32.ts

+      // https://github.com/jsdom/jsdom/issues/3206 => https://github.com/jsdom/jsdom/issues/2555
+      const block = await file.arrayBuffer()
+      this.append(new Uint8Array(block))
+      return (this.crc ^ -1) >>> 0


^ -1) >>> 0 的过程应该封装起来
下同

为啥没必要？我看别的库这个步骤也是封装在内部的

一个基础运算、没必要、别人做了不等于我们要做

如果别人都做了，你应该证明的是为什么唯独你不用做…
比如这个 -1 是运算过程中的内部概念，为什么要消费者去处理
这跟代码长短无关，如果因为代码短事情就简单，你就不会花了这么久都找不到问题出在 >>> 0 上

lzfee0227 · 2021-10-18T03:37:37Z

README.md

@@ -215,6 +215,7 @@ qiniu.compressImage(file, options).then(data => {
  * config.region: 选择上传域名区域；当为 `null` 或 `undefined` 时，自动分析上传域名区域，当指定了 `uphost` 时，此设置项无效。
  * config.retryCount: 上传自动重试次数（整体重试次数，而不是某个分片的重试次数）；默认 3 次（即上传失败后最多重试两次）。
  * config.concurrentRequestLimit: 分片上传的并发请求量，`number`，默认为3；因为浏览器本身也会限制最大并发量，所以最大并发量与浏览器有关。
+  * config.checkByServer: 是否开启服务端文件签名校验，为布尔值；开启后在文件上传时会计算本地的文件签名、上传时服务端会根据本地的签名与接收到的数据的签名进行比对，如果不相同、则说明文件可能存在问题，此时会返回错误，默认为 false，不开启。


NIP：在语文里，顿号真的不是这么用的… 而且这里是可以用句号的，没必要强行搞顿号

是不是应该说 上传前 上传后 而不是 上传时
另外 返回错误 的错误信息封装了吗？用户会拿到一个什么 error

是不是应该说上传前上传后而不是上传时

上传时 泛指整个上传过程
之所以不写是上传前还是上传后是因为分片和直传的实现不同，
同时也没必要暴露这么精确的概念给用户。

另外返回错误的错误信息封装了吗？用户会拿到一个什么 error

没有针对这个错误单独封装 Error，用户会拿到 QiniuRequestError，通过 code 消费错误信息

我只是觉得这两个 上传时 很难理解而已

你这里至少提一下这个错误对应的 code 是多少吧

lzfee0227 · 2021-10-18T03:46:08Z

README.md

+  * config.checkByServer: 是否开启服务端文件签名校验，为布尔值；开启后在文件上传时会计算本地的文件签名、上传时服务端会根据本地的签名与接收到的数据的签名进行比对，如果不相同、则说明文件可能存在问题，此时会返回错误，默认为 false，不开启。
  * config.checkByMD5: 是否开启 MD5 校验，为布尔值；在断点续传时，开启 MD5 校验会将已上传的分片与当前分片进行 MD5 值比对，若不一致，则重传该分片，避免使用错误的分片。读取分片内容并计算 MD5 需要花费一定的时间，因此会稍微增加断点续传时的耗时，默认为 false，不开启。


这两个 false 应该是

lzfee0227 · 2021-10-18T07:16:10Z

checkByMD5 这个功能作为配置略显鸡肋、关闭的好处可能只有减少了一些MD5计算，在数据安全上、我认为在使用缓存时校验 MD5 还是很有必要的、不应该为了减少计算、放弃这个安全验证。

这个不能想当然的
当年如果不是有人提需求，是不会有人主动做这种功能的
在我的印象中，只有减少一些 是能够明显感受出来的，不仅仅是 只有一些
很有必要 也不见得，文件内容往往不会随意变化的

另外如果你要删掉这个，那就发大版本 breaking change

lzfee0227 · 2021-10-18T07:23:37Z

两种手段都是通过与服务端进行签名比对来确保数据有效性的一种手段，唯一的差别可能是比对行为的发生主体不同

这不是从技术实现手段上看的，而是从需求上看的
从需求上看，这就是两个完全不一样的需求
一个是，每个分片或文件上传完之后立马校验数据传输内容是否正确，针对的是 上传过程的数据安全性或完整性，比如是否请求被劫持之类的，是个持续时间很短的行为
另外一个是，断点续传的时候是否校验本地内容是否发生变化，针对的是 客户端文件内容的时效性，有可能是个长时间的行为比如过了两三天之后重新上传但是忘记已经改了文件内容了
而且做这个需求的时候，现在的产品经理估计都还没来

nighca · 2021-10-18T08:03:50Z

checkByMD5 这个功能作为配置略显鸡肋、关闭的好处可能只有减少了一些MD5计算，在数据安全上、我认为在使用缓存时校验 MD5 还是很有必要的、不应该为了减少计算、放弃这个安全验证。

跟产品讨论了一下，产品支持移除 checkByMD5 参数、内部实现为缓存固定验证 md5

我也感觉这个结论有点草率；如果内容失效的可能性很小（或者失效导致的后果可以接受），而设备对性能敏感（比如移动设备等），去掉本地的 md5 验证可能是合理的

除了 @lzfee0227 提的几个问题，其他我没有问题

yinxulai · 2021-10-18T08:20:30Z

checkByMD5 这个功能作为配置略显鸡肋、关闭的好处可能只有减少了一些MD5计算，在数据安全上、我认为在使用缓存时校验 MD5 还是很有必要的、不应该为了减少计算、放弃这个安全验证。

跟产品讨论了一下，产品支持移除 checkByMD5 参数、内部实现为缓存固定验证 md5

我也感觉这个结论有点草率；如果内容失效的可能性很小（或者失效导致的后果可以接受），而设备对性能敏感（比如移动设备等），去掉本地的 md5 验证可能是合理的

除了 @lzfee0227 提的几个问题，其他我没有问题

实际上、现在读分片、如果关闭 md5(默认关闭)，针对缓存是否有效的校验就只有文件名和 size、感觉风险还是蛮大的

lzfee0227

lgtm

lzfee0227 · 2021-10-18T09:15:11Z

README.md

@@ -215,8 +215,8 @@ qiniu.compressImage(file, options).then(data => {
  * config.region: 选择上传域名区域；当为 `null` 或 `undefined` 时，自动分析上传域名区域，当指定了 `uphost` 时，此设置项无效。
  * config.retryCount: 上传自动重试次数（整体重试次数，而不是某个分片的重试次数）；默认 3 次（即上传失败后最多重试两次）。
  * config.concurrentRequestLimit: 分片上传的并发请求量，`number`，默认为3；因为浏览器本身也会限制最大并发量，所以最大并发量与浏览器有关。
-  * config.checkByServer: 是否开启服务端文件签名校验，为布尔值；开启后在文件上传时会计算本地的文件签名、上传时服务端会根据本地的签名与接收到的数据的签名进行比对，如果不相同、则说明文件可能存在问题，此时会返回错误，默认为 false，不开启。
-  * config.checkByMD5: 是否开启 MD5 校验，为布尔值；在断点续传时，开启 MD5 校验会将已上传的分片与当前分片进行 MD5 值比对，若不一致，则重传该分片，避免使用错误的分片。读取分片内容并计算 MD5 需要花费一定的时间，因此会稍微增加断点续传时的耗时，默认为 false，不开启。
+  * config.checkByServer: 是否开启服务端文件签名校验，为布尔值；开启后在文件上传时会计算本地的文件签名，服务端会根据本地的签名与接收到的数据的签名进行比对，如果不相同、则说明文件可能存在问题，此时会返回错误（`code`: 406），默认为 `false`，不开启。


406 -> 406

添加服务端文件校验

a87224d

qiniu-bot added the size/L label Oct 15, 2021

yinxulai commented Oct 15, 2021

View reviewed changes

test/demo1/main.js Outdated

Comment on lines 5 to 7

checkCrc: true,

checkByMD5: true,

forceDirect: true,

Copy link

Collaborator Author

yinxulai Oct 15, 2021

Choose a reason for hiding this comment

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

mark

yinxulai added 3 commits October 18, 2021 09:21

补充 config test

e14e688

优化 README

a27c2a3

添加一些注释、说明、以及部分实现调整

b2cc3d9

yinxulai force-pushed the KODO-13155 branch from e653e57 to f5f4545 Compare October 18, 2021 02:03

添加 0 size 的文件测试

e602ce3

yinxulai force-pushed the KODO-13155 branch from f5f4545 to e602ce3 Compare October 18, 2021 02:03

yinxulai changed the title ~~[KODO-13155] 添加服务端文件校验~~ [KODO-13155] 添加服务端文件签名校验 Oct 18, 2021

lzfee0227 reviewed Oct 18, 2021

View reviewed changes

提升兼容性、优化 README

8785bf0

yinxulai added 3 commits October 18, 2021 16:38

更新注释文档

3d16252

更新版本号

3e03f1f

update

3fd2786

This comment has been minimized.

Sign in to view

lzfee0227 approved these changes Oct 18, 2021

View reviewed changes

yinxulai merged commit c6fea03 into qiniu:master Oct 18, 2021

		* config.checkByServer: 是否开启服务端文件签名校验，为布尔值；开启后在文件上传时会计算本地的文件签名、上传时服务端会根据本地的签名与接收到的数据的签名进行比对，如果不相同、则说明文件可能存在问题，此时会返回错误，默认为 false，不开启。
		* config.checkByMD5: 是否开启 MD5 校验，为布尔值；在断点续传时，开启 MD5 校验会将已上传的分片与当前分片进行 MD5 值比对，若不一致，则重传该分片，避免使用错误的分片。读取分片内容并计算 MD5 需要花费一定的时间，因此会稍微增加断点续传时的耗时，默认为 false，不开启。

[KODO-13155] 添加服务端文件签名校验 #532

[KODO-13155] 添加服务端文件签名校验 #532

Uh oh!

Conversation

yinxulai commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jira-issue

改动

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yinxulai commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

简单说一下两个配置代表的实际功能

合并的理由

Uh oh!

yinxulai commented Oct 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzfee0227 Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzfee0227 Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lzfee0227 commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lzfee0227 commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nighca commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yinxulai commented Oct 18, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

lzfee0227 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yinxulai commented Oct 15, 2021 •

edited

Loading

codecov bot commented Oct 15, 2021 •

edited

Loading

yinxulai commented Oct 18, 2021 •

edited

Loading

lzfee0227 Oct 18, 2021 •

edited

Loading

lzfee0227 Oct 18, 2021 •

edited

Loading

lzfee0227 commented Oct 18, 2021 •

edited

Loading

lzfee0227 commented Oct 18, 2021 •

edited

Loading

nighca commented Oct 18, 2021 •

edited

Loading