imprv: Add config to toggle ACL between public_read and private on PutObject when using S3 with FileUploader #8778

ToshihitoKon · 2024-04-30T16:45:10Z

概要

FileUploaderのバックエンドにS3を利用する際にPutObjectのオプションでACLをpublic_read固定からprivateに切り替えられるよう設定を追加しました。
crowi:aws:S3PutObjectを設定することで切り替えられます、
デフォルトは後方互換をもつためpublic_readです。

動作確認

Growi,mongoを手元で建て、AWS S3 BucketとIAMアクセスキーを指定して以下の動作を確認しました

public_read指定時、ACL有効のS3 Bucketをバックエンドで画像アップロードに成功すること
public_read指定時、ACL無効のprivateなS3 Bucketに画像をアップロードしようとすると失敗すること
private指定時、ACL無効のS3 Bucketに画像のアップロードに成功すること

このPRで対応しないこと

オプションの変更方法に関して、自分の理解が浅いこと、PRを小さくしたいことからこのPRでは対応していません

確認してほしい点

オプションの変更方法の追加に関しての対応要望をコードにコメントで追加したほうがよいか
Configcrowi:aws:S3PutObjectの命名は適切か
- Crowiから引き継いだと思われるConfigに新規で追加するのは不適切かもしれない

yuki-takei · 2024-05-01T08:17:48Z

@ToshihitoKon PR ありがとうございます！

Config `crowi:aws:S3PutObject` の命名は適切か

ここのキーはなんでもいいのですが(そもそも歴史的経緯もあってそんなに整合性も取れてない…)
ObjectCannedACL という型の値になるので、aws:s3ObjectCannedACL がいいかなと思います。

オプションの変更方法について

app/src/server/service/config-loader.ts というファイルがあるのですが、そちらへ記載することで環境変数から値を変更できるようになります。そちらの差分を本 PR にも含めていただけますでしょうか？

このような感じのものになると思います。S3_ prefix がついているものがいくつかあると思いますのでその付近に。

  S3_OBJECT_CANNED_ACL: {
    ns:      'crowi',
    key:     'aws:s3ObjectCannedACL',
    type:    ValueType.STRING,
    default: 'public_read',
  },

その他

aws.ts 中にまだまだ type safe といえない箇所があったので、さきほど以下の PR を作って master にマージしました。

#8780

getAwsConfig メソッドを消したりしています。
お手数おかけして申し訳ありませんがこちらを取り込んで再度 PR いただけますでしょうか？

…public-read

ToshihitoKon

フィードバック対応しました！

動作確認

環境変数なしでpublic_readで処理されること（private bucketで弾かれる)
S3_BUCKET_ACLS_DISABLE=trueでprivate bucketにPutできること
S3_BUCKET_ACLS_DISABLE=falseでprivate bucketへのPutが弾かれること

ToshihitoKon · 2024-05-03T15:37:33Z

apps/app/src/server/service/config-loader.ts

+  S3_BUCKET_ACLS_DISABLE: {
+    ns:      'crowi',
+    key:     'aws:s3BucketAclsDisable',


環境変数名はユーザーが見える範囲なので、Canned ACLの指定ではなくて"ACL無効のS3 Bucketを利用する場合”の意味合いでS3_BUCKET_ACL_DISABLEのboolにしてみました

@ToshihitoKon 修正ありがとうございます。

環境変数の命名に関してのフィードバックですが、ここは 'public_read' という値がそのまま入るようなものの方が望ましいと思います。

理由は少なくとも2つあって、

一つは現状の命名では「バケットに対する設定である」というミスリードを誘います。(実際は、Put する Object に対する挙動の変更)

二つ目は概念の理解の容易さに関する部分で、
ただでさえ AWS の ACL の概念が複雑なので、利用者は追加の概念を学びたくはないはずです(たぶん)。
この仕様だとここの T/F がどのような挙動にマッピングされるのかを気にしないといけないので、それよりは S3 の概念をそのまま採用し、オブジェクトの設定を透過的に変更できるよ、という説明にした方が利用者にとって不安がないのではないかと。

とはいえ一度決めてしまった後で再度変更するのはあまりやりたくはないので、命名は慎重になっていいと思います。上記の観点以外に S3_BUCKET_ACLS_DISABLE の方が望ましいと思える要素があれば検討したいので挙げていただけると嬉しいです。またはもし第三の命名のアイデアを思いついた場合はそちらも引き続き検討したいと考えています。

お手数ですがよろしくお願いいたします。

@yuki-takei フィードバックありがとうございます。

自分がS3_BUCKET_ACL_DISABLEを採用した理由としては

ユーザーが意識する点として、設定するS3 BucketをACL無効でも使用できるかどうかの1点に絞るため

環境変数の設定値としてpublic_read|privateを期待すると、ユーザーは「内部でS3 PutObjectをする際にデフォルトでACLpublic_readを使うため、CannedACLの一つであるprivateを指定しないといけない」という文脈を汲むことになると思っています。
これは上げて頂いた理由の2つ目についての解釈の違いなので、最終的な判断はお任せしようかなと思っています。

今後S3 ACLs disabledなBucketを利用した際の設定フラグとしての汎用性のため

今回自分が気づいたのはPutObjectのみでしたが、他の箇所で同じような対応が必要になった際に別の変数を追加 or こっそり今回追加する変数を見て挙動を変えるのはちょっと綺麗ではないな、と思いました。

現状の命名では「バケットに対する設定である」というミスリードを誘います。(実際は、Put する Object に対する挙動の変更)

これはたしかにそうかもしれないです
S3BackendBucketACLsDisableMode…？(長い)
UseACLsDisableBucketとかもアリでしょうか

1でも書きましたが、最終的な判断はメンテナの方にお任せしようと思っております。

@ToshihitoKon 見解の共有ありがとうございました。一旦マージしてこちらで環境変数名と取り得る値を変更するかどうか検討しようと思います。リリースは今月中旬を予定しています。

追加で1点お願いがあります。

v7.0.3 以前の GROWI では、初期状態のまま運用すると ACL Enabled な Bucket に対し ACL: public_read なオブジェクトを Put することになると思います。この時、

Bucket や GROWI のその他の設定を変えずに ACL: private 部分だけを変更して Object を Put できるか？

上記で正常に Put できるとして、正常にコンテンツを閲覧できるか？

Relay Mode / Redirect Mode 問わず正常にコンテンツを閲覧できるか？

ref: GROWI Docs | ファイルアップロード設定

を検証いただくことは可能でしょうか？

お手隙の際にお願いできれば幸いです。

@yuki-takei ありがとうございます

Bucket や GROWI のその他の設定を変えずに ACL: private 部分だけを変更して Object を Put できるか？

こちらの認識合わせなのですが

S3 Bucket: ACL有効 (Public Bucket)

GROWI: PutObjectで ACL: private を指定(public_readと比較検証)

の状態での挙動を確認したい、ということで合っていますか？
合っていれば検証結果は以下になります

1. public_read固定の場合(現行、デフォルト挙動)

S3: ACL有効

--- a/apps/app/src/server/service/file-uploader/aws.ts +++ b/apps/app/src/server/service/file-uploader/aws.ts @@ -283,7 +283,7 @@ module.exports = (crowi) => { Bucket: getS3Bucket(), Key: filePath, Body: fileStream, - ACL: getS3PutObjectCannedAcl(), + ACL: ObjectCannedACL.public_read, // put type and the file name for reference information when uploading ContentType: contentHeaders.contentType?.value.toString(), ContentDisposition: contentHeaders.contentDisposition?.value.toString(), @@ -298,7 +298,7 @@ module.exports = (crowi) => { ContentType: contentType, Key: filePath, Body: data, - ACL: getS3PutObjectCannedAcl(), + ACL: ObjectCannedACL.public_read, })); };

アップロード: OK
ObjectACL: Public Access Read

2. private固定の場合(本件対応後、S3BucketACLsDisable=true時の挙動)

S3: ACL有効

--- a/apps/app/src/server/service/file-uploader/aws.ts +++ b/apps/app/src/server/service/file-uploader/aws.ts @@ -283,7 +283,7 @@ module.exports = (crowi) => { Bucket: getS3Bucket(), Key: filePath, Body: fileStream, - ACL: getS3PutObjectCannedAcl(), + ACL: ObjectCannedACL.private, // put type and the file name for reference information when uploading ContentType: contentHeaders.contentType?.value.toString(), ContentDisposition: contentHeaders.contentDisposition?.value.toString(), @@ -298,7 +298,7 @@ module.exports = (crowi) => { ContentType: contentType, Key: filePath, Body: data, - ACL: getS3PutObjectCannedAcl(), + ACL: ObjectCannedACL.private, })); };

Redirect mode

アップロード： OK
表示: OK
ObjectACL: Public Access 許可なし

Relay mode

アップロード： OK
表示: Redirect modeでアップロードした画像共にOK

合ってます！ありがとうございます。

この挙動になるのであれば、後方互換を保っていると言ってよさそうですね。PutObjectで ACL: private をデフォルト挙動としていい気がしてきました。

privateをデフォルト挙動にしてしまうとS3のファイルが今までPublicだったものが以降privateになってしまうので、後方互換は保ててないかと思いました
Growiから見たときは一時認証URLを発行しているため表示は出来ますが、場合によってはS3にアップロードされたオブジェクトのURLを直接参照していた場合に挙動が変わってしまうので

growi/apps/app/src/server/service/file-uploader/aws.ts

Line 192 in 55b2024

const signedUrl = await getSignedUrl(s3, new GetObjectCommand(params), {

レアケースな感じもありますが、一旦デフォルトはpublic_readのままで今後のアップデートでデフォルトが変更されますというアナウンスをしてからデフォルト変更でもいいかなと思います

一旦デフォルトはpublic_readのままで今後のアップデートでデフォルトが変更されますというアナウンスをしてからデフォルト変更でもいいかなと思います

一旦そうしますね

場合によってはS3にアップロードされたオブジェクトのURLを直接参照していた場合に

この挙動はかなり前、v3とかの頃のまだ /attachment route がない時代にはそうしていたのでその頃の URL が markdown 中に残っていればたしかに breaking changes になってしまうのですが、さすがにそこは今後切り落としてもいいのかなと思っています。(もちろんアップグレードガイドには記述する予定ですが)

ToshihitoKon · 2024-05-03T15:38:24Z

apps/app/src/server/service/file-uploader/aws.ts

@@ -48,6 +48,14 @@ const isFileExists = async(s3: S3Client, params: HeadObjectCommandInput) => {
  return true;
 };

+const getS3PutObjectCannedAcl = (): ObjectCannedACL => {


こちらの関数名にCannedACLを入れてみました

ToshihitoKon added 2 commits May 1, 2024 00:01

add: crowi:s3PutObjectAcl config

edeae39

fix: whitespace

bea3545

ToshihitoKon added 2 commits May 1, 2024 22:34

Merge remote-tracking branch 'origin' into feature/option-s3-without-…

10a2487

…public-read

add: S3_BUCKET_ACLS_DIABLE environment variable

16974f7

ToshihitoKon commented May 3, 2024

View reviewed changes

ToshihitoKon mentioned this pull request May 4, 2024

Add AWS IAM best practices and S3_BUCKET_ACLS_DISABLE. growilabs/growi-docs#465

Merged

ToshihitoKon had a problem deploying to VRT May 7, 2024 06:27 — with GitHub Actions Failure

yuki-takei merged commit 7d9a391 into weseek:master May 14, 2024
17 of 21 checks passed

This was referenced May 14, 2024

support: Modify env var for S3 Object-ACL #8805

Merged

Release v7.0.5 #8804

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imprv: Add config to toggle ACL between public_read and private on PutObject when using S3 with FileUploader #8778

imprv: Add config to toggle ACL between public_read and private on PutObject when using S3 with FileUploader #8778

ToshihitoKon commented Apr 30, 2024

yuki-takei commented May 1, 2024

ToshihitoKon left a comment

ToshihitoKon May 3, 2024

yuki-takei May 7, 2024 •

edited

Loading

ToshihitoKon May 7, 2024 •

edited

Loading

yuki-takei May 8, 2024 •

edited

Loading

ToshihitoKon May 9, 2024

yuki-takei May 10, 2024

ToshihitoKon May 11, 2024

yuki-takei May 14, 2024

ToshihitoKon May 3, 2024

imprv: Add config to toggle ACL between public_read and private on PutObject when using S3 with FileUploader #8778

imprv: Add config to toggle ACL between public_read and private on PutObject when using S3 with FileUploader #8778

Conversation

ToshihitoKon commented Apr 30, 2024

概要

動作確認

このPRで対応しないこと

確認してほしい点

yuki-takei commented May 1, 2024

Config crowi:aws:S3PutObject の命名は適切か

オプションの変更方法について

その他

ToshihitoKon left a comment

Choose a reason for hiding this comment

ToshihitoKon May 3, 2024

Choose a reason for hiding this comment

yuki-takei May 7, 2024 • edited Loading

Choose a reason for hiding this comment

ToshihitoKon May 7, 2024 • edited Loading

Choose a reason for hiding this comment

yuki-takei May 8, 2024 • edited Loading

Choose a reason for hiding this comment

ToshihitoKon May 9, 2024

Choose a reason for hiding this comment

1. public_read固定の場合(現行、デフォルト挙動)

2. private固定の場合(本件対応後、S3BucketACLsDisable=true時の挙動)

Redirect mode

Relay mode

yuki-takei May 10, 2024

Choose a reason for hiding this comment

ToshihitoKon May 11, 2024

Choose a reason for hiding this comment

yuki-takei May 14, 2024

Choose a reason for hiding this comment

ToshihitoKon May 3, 2024

Choose a reason for hiding this comment

Config `crowi:aws:S3PutObject` の命名は適切か

yuki-takei May 7, 2024 •

edited

Loading

ToshihitoKon May 7, 2024 •

edited

Loading

yuki-takei May 8, 2024 •

edited

Loading

2. private固定の場合(本件対応後、`S3BucketACLsDisable=true`時の挙動)