refactor: `frame/` のリファクタリングと、使い方解説ドキュメント by laysakura · Pull Request #641 · LayerXcom/confidential-computing-modules

laysakura · 2021-06-08T18:49:05Z

Issueへのリンク

Fixes: #610
Fixes: #611

（e2e テストを走らせたく、ブランチ名を変えるために #638 を閉じて立て直しました）

やったこと

frame/ 以下のフレームワークはとても良いもの（特に ecall_entry_point() を中心とした設計）だと思うが、実際に使ってみて慣れるまでに時間がかかったのと、名称や構造に一部違和感があったので、リファクタリングをする。
またフレームワークの使い方ドキュメントがあると新feature実装や改修の際に役立つと感じたので追加する。

使い方解説ドキュメント追加
ドキュメントの TO-BE に合わせてリファクタリング
その他のリファクタリング
- trait {Basic,StateRuntime}EnclaveUseCase の関連関数にデフォルト実装がついていたのを外した。デフォルト実装に気づかず全部 impl したつもりになって、ランタイムでデバッグする必要に迫られるのを避けるため。
- anonify-enclave のほうでHostInputやHostOutputに signer, gas を持たせていたが、本来どちらの持ち物でもない（use case計算の入出力どちらでもない）ので外に出した

やらないこと

誰か今後書いてくれたら嬉しいな

動作検証

CI pass (含む e2e）
テストのカバレッジを知らないので、これでどの程度自信を持ってよいのか...

特に、 cmd: u32 の付け間違えなどに一抹の不安... → この箇所はセルフレビューでだいぶ自信持てた

参考

frame を使っている箇所が多いので、どうしても差分が大きくなっています。意味単位でcommitしているので、commitごとに見たほうがやや見やすいかもしれません。
元の author である osuketh さん、別リポジトリで frame をメインで使っている cipepser さんのお二人にレビューもらいたいです。
- おすすめの読み方: 最初にcommitごとにメッセージと差分をチラ見していき、どんな変更を加えていったのか意図を頭に入れる。その後 Files changed で全差分を確認。
- レビュアーが2人なこともあり、私の方でペアレビュー会を開催するのが一番効率いいかもしれません。

… runtime behaivior

… weired runtime behaivior

… weired runtime behaivior (and explicitly impl it)

…to avoid weired runtime behaivior (and explicitly impl it)

… runtime behaivior (and explicitly impl it)

…EnclaveUseCase)

…e param

modules/anonify-eth-driver/src/controller.rs

frame/enclave/src/enclave_use_case/state_runtime_enclave_use_case.rs

laysakura · 2021-06-09T02:15:04Z

modules/anonify-enclave/src/commands/treekem.rs

        Ok(())
    }

-    fn run<C>(self, enclave_context: &C, max_mem_size: usize) -> anyhow::Result<Self::EO>


max_mem_size はアプリ側で決める必要ありなケースがある模様

max_cipher_text_size　とかいい名前はありそうだが？

モチベーションは、block chainへのtxから、何のデータが送られたか推測されないように、全てのサイズを整えるpaddingくん

@osuketh @cipepser 改めてコードを読んだところ、 max_mem_size は「全ての StateRuntimeEnclaveUseCase の impl で使うもの」ではなく、「CommandByEnclaveKeySender が使うもの」と見受けられました。

だとすると、CommandByEnclaveKeySender の HostInput である input::Command のフィールドとして max_mem_size: usize をもたせるのが妥当だと思います。

方針合ってるでしょうか？ 👀

そうですね！ EnclaveからBCに向けてデータ吐き出す操作の時しか使われないはずなので :yosage: です！

f9f0680

CommandByEnclaveKeySender の HostInput である input::Command のフィールドとして max_mem_size: usize をもたせる

をやる上で、環境変数にしました。
example層からnode層を経由してmodule層に渡す方法にちょっと悩み、request parameterで渡すよりも（クライアントサイドの後方互換性ぶっ壊したりとか考えると）環境変数のほうが良いよなと判断しまして。
環境変数だとそれはそれで便利な認識です。

環境変数だとRemote Attestationで検証されるEnclaveコードハッシュ(MRENCLAVE)を変えずにAnonify運営者が型が許す任意の値をCMD_CIPHER_PADDING_SIZE に入れられてしまうことにより、クライアントごとのアプリケーション依存の命令Aと命令Bを識別しうるサーフェスが増えてしまう、と感じています。（原則、Enclaveコードの挙動はRemote Attestationで検証される用に実装すべき）
ですが、現状そこまでの脅威モデルは必要としていない＆クライアント側の暗号化paddingなど未対応ということで、一旦このままでも良いかと思いますー。

うわああ勉強になります...
環境変数にした部分はそこまで手間取ってもなかったので、修正できるならしちゃいたいなという気持ちです。

このPRの前の状況みたいに、register_*! マクロ引数にサイズを書く形なら、example層から一気にmodule層にサイズを渡すことができます。
一方で、本来サイズを使わないで良い構造にまでサイズが行き渡るのが違和感あり、できれば避けたいです。

サイズの良い渡し方、思いつくでしょうか...？

今サイズの渡し方に悩んでいるのは、サイズを渡す口がHostInputにあるからな気がしてきました。
enclaveの挙動はenclaveコードの中でstaticに決まるべきという至言に基づくと、Host側でサイズが決められちゃうこの方針は最初から破綻してる。
寝入る前の脳トレとしてちょっと考えてみます！

https://github.com/LayerXcom/anonify/blob/79cc1ff1a27303eb285e61622c89340c6a7477ce/example/erc20/enclave/src/lib.rs#L29

このあたりで定数値としてコンテキストにぶち込むのは一つの解決策。
上述の「本来サイズを使わないで良い構造にまでサイズが行き渡るのが違和感」な状況から構造的には何も変わってないが、コンテキストの使い方としては悪くもない気も。
ちゃんとやるなら、構造ごとに別個のコンテキストを取れるようにマクロを拡張するとよさそう。

ということで色々と修正しました。

436fcff で環境変数にしちゃったやつをrevertし、
10e4812 でそれぞれの UseCase が異なるコンテキストを取れるようにマクロを修正し、
0a833d4 で、 UseCase の trait 側においては new で渡される enclave_context の型を何でもOKにし、
cee5884 で、 CommandByEnclaveKeySender, CommandByTreeKemSender においては AnonicyContext を一層ラップして cmd_cipher_padding_size も一緒に持った struct ContextWithCmdCipherPaddingSize をコンテキストとし受け取る

ようになりました。
最後の 73a16ee は名前合わせです。

よっさそうです 👍

laysakura · 2021-06-09T02:19:05Z

example/erc20/enclave/src/ecalls.rs

    (
        SEND_COMMAND_ENCLAVE_KEY_CMD,
-        CommandByEnclaveKeySender<NoAuth>
+        CommandByEnclaveKeySender<AnonifyEnclaveContext, Runtime<AnonifyEnclaveContext>,NoAuth>


これ、 AnonifyEnclaveContext が modules 以下だったら、 CommandByEnclaveKeySender の定義側で埋めることができる

12ec8b6
1eafabd
5cb1ea9

f90e85c ← 漏れ修正

…`. `host_input::Command` takes it.

…MEM_SIZE`. `host_input::Command` takes it." This reverts commit f9f0680.

… use case type

… defined from example/**/enclave in order to be included in MRENCLAVE

osuketh

👍

cipepser · 2021-06-10T06:37:59Z

example/erc20/enclave/src/lib.rs

+    pub static ref ENCLAVE_CONTEXT_WITH_CMD_CIPHER_PADDING_SIZE: ContextWithCmdCipherPaddingSize<'static> = {
+        ContextWithCmdCipherPaddingSize {
+            ctx: &*ENCLAVE_CONTEXT,
+            cmd_cipher_padding_size: 100,


アプリケーション開発者がこの値をいじる必要があることを認識できるようにしたいですね。
（元々なってなかったですがmm）
docsに残すのがいいのかな

一番良さそうだなって思うことを雑に書きます。

Enclaveの設定値は、ここの教え通り、ライブラリのバイナリにstaticに書き込まれているべきです。
こういうのは本来（今みたいに）ハードコードもすべきじゃないし、ましてやコード中で散逸すべきでもない。

ビルドパラメータであるのが一番ですね。
build.rs が build.toml (特に規格があるわけではないと思うけど) みたいなの読む形式になっているかのが一番キレイそうだなって思いました。
(Makefileでも良い)

雑にissue化 :done:
#642

あざます！
staticに書き込まれるべきは完全同意です！
アプリケーション開発者がなんじゃそりゃとならないようにだけフォローできるといいなと思った次第です〜
Makefileも良さげですが、またmakeの変数増えてしまう心理的障壁との戦い。悩ましい
（一番よさそうなとこで大丈夫です）

若干オフトピですが、Makefileは結構古代の技術だし苦しいですよね... $@ とかいい加減にしてほしい

できる限り build.rs で頑張れるといいなという思いがあるが、あんまりrustで巨大プロジェクトのビルドを頑張ったことないからMakefileをどこまで代替できるかは自信なしです。Makefileの依存グラフ記述力は馬鹿にできないですしね。

更にオフトピを重ねると、cargo-make っていうのはタスクランナーとして好んで使っています。依存関係記述力がどこまであるかはよくわからない 🙄

cargo-makeめちゃ気になりますね
$@
$^
$<
$?
=
:=
?=
から脱却したい

ここら辺とかはmakefileなくしているです
https://github.com/mobilecoinfoundation/mobilecoin/tree/master/sgx/build

:sasuga: の調査力 👏

cipepser · 2021-06-10T09:20:42Z

modules/anonify-enclave/src/commands/enclave_key.rs

    command_plaintext: CommandPlaintext<AP>,
+    enclave_context: &'c ContextWithCmdCipherPaddingSize<'c>,
    user_id: Option<AccountId>,
+    _p: PhantomData<R>,


_pってなんの頭文字です？:chimpan:

Phantomです 🙄
使うフィールドじゃないしくっそ適当でいいかなって...

_rで固定観念に縛られてました
:kanzenrikai:
_p で 👍

laysakura · 2021-06-10T09:33:34Z

レビューありがとうございましたﾏｰｰｰｰｰｰｰｰｰｰｰｰｰｰｰｰｰｰｰｼﾞします！

laysakura added 30 commits June 4, 2021 06:38

docs: link to docs/ from top-level README

c94452d

docs: adds framework.md and link from dir_structure.md

5dee35a

refactor: consistent type parameter name

dfa803a

todo comment

cd62d0f

docs: writes to-be

c1e5722

docs: AA alignment

331f4d6

docs: fix

6362321

refactor: BasicEnclave{Engine -> UseCase}

7411687

refactor: StateRuntimeEnclave{Engine -> UseCase}

7f8bb84

refactor: rename files

83af7bc

refactor: HostEngine -> EcallController

2bcc431

refactor: {Ecall -> Enclave}{In,Out}put

685db0e

refactor: {Ecall -> Enclave}OutputNotSet

b4bbd18

refactor: {ECALL -> EI}_MAX_SIZE and writes doc comment

16da948

refactor: register_{ecall -> enclave_use_case}

002dd18

*UseCase::{handle -> run}

55e20f9

refactor: rm default impl of BasicEnclaveUseCase::run to avoid weired…

c5cb4be

… runtime behaivior

refactor: rm default impl of StateRuntimeEnclaveUseCase::run to avoid…

7fb5334

… weired runtime behaivior

refactor: rm default impl of StateRuntimeEnclaveUseCase::new to avoid…

255ee43

… weired runtime behaivior (and explicitly impl it)

refactor: rm default impl of StateRuntimeEnclaveUseCase::eval_policy …

c7816e8

…to avoid weired runtime behaivior (and explicitly impl it)

refactor: rm default impl of BasicEnclaveUseCase::new to avoid weired…

489e92e

… runtime behaivior (and explicitly impl it)

refactor: EcallController::{exec -> run}

a6ad36c

docs: MyEcallController::run takes eid as it is (this seems simpler)

ba42a20

refactor: BasicEnclaveUseCase does not need Default boundary

b2d557d

refactor: rm BasicEnclaveUseCase::new()

30a392d

refactor: register_enclave_use_case! made smaller

1ec435b

refactor: $handler -> $use_case

73f329b

refactor: ecall_entry_point made simpler (to be DRY with StateRuntime…

e8fc964

…EnclaveUseCase)

refactor: mod {engine -> enclave_use_case}

faa35bf

refactor: StateRuntimeEnclaveUseCase::run does not have to take R typ…

36aaaa4

…e param

laysakura commented Jun 8, 2021

View reviewed changes

modules/anonify-eth-driver/src/controller.rs Show resolved Hide resolved

laysakura commented Jun 8, 2021

View reviewed changes

frame/enclave/src/enclave_use_case/state_runtime_enclave_use_case.rs Outdated Show resolved Hide resolved

laysakura added 3 commits June 9, 2021 00:24

refactor: avoid .and_then nest hell

3777200

fix: call eval_policy()

605603f

fix: signature

c262745

laysakura commented Jun 9, 2021

View reviewed changes

laysakura added 5 commits June 9, 2021 13:47

refactor: Eliminate EncryptedSqlOpsEnclaveContext type parameter

12ec8b6

refactor: Eliminate KeyVaultEnclaveContext type parameter

1eafabd

refactor: Eliminate AnonifyEnclaveContext type parameter

5cb1ea9

feat: CMD_CIPHER_PADDING_SIZE envvar instead of `const MAX_MEM_SIZE…

f9f0680

…`. `host_input::Command` takes it.

fix: JoinGroupWithTreeKem type param

f90e85c

laysakura requested review from cipepser and osuketh June 9, 2021 07:47

laysakura added 6 commits June 9, 2021 23:18

Revert "feat: CMD_CIPHER_PADDING_SIZE envvar instead of `const MAX_…

436fcff

…MEM_SIZE`. `host_input::Command` takes it." This reverts commit f9f0680.

refactor: register_enclave_use_case! takes different context for each…

10e4812

… use case type

refactor: loosen trait *EnclaveUseCase type parameter bounds

0a833d4

fix: cmd_cipher_padding_size (formerly called MAX_MEM_SIZE) should be…

cee5884

… defined from example/**/enclave in order to be included in MRENCLAVE

refactor: max_mem_size -> cmd_cipher_padding_size

73a16ee

fmt

1a4e728

laysakura requested review from cipepser and osuketh and removed request for cipepser and osuketh June 10, 2021 01:11

osuketh approved these changes Jun 10, 2021

View reviewed changes

cipepser approved these changes Jun 10, 2021

View reviewed changes

laysakura merged commit 427599f into main Jun 10, 2021

laysakura deleted the refactor/framework-e2e branch June 10, 2021 09:33

laysakura mentioned this pull request Jun 10, 2021

[refactor] enclaveの設定値はビルドパラメータとして集約されていたい #642

Closed

Conversation

laysakura commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issueへのリンク

やったこと

やらないこと

動作検証

参考

Uh oh!

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!

osuketh Jun 9, 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!

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!

laysakura Jun 9, 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!

osuketh left a comment

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!

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!

laysakura commented Jun 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

laysakura commented Jun 8, 2021 •

edited

Loading

osuketh Jun 9, 2021 •

edited

Loading

laysakura Jun 9, 2021 •

edited

Loading