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

chore: add the new policy.json review process documentation and config #29383

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# audit these changes on their own, and leave their analysis in a comment.
# These codeowners will review this analysis, and review the policy changes in
# further detail if warranted.
lavamoat/ @MetaMask/extension-devs @MetaMask/supply-chain
lavamoat/ @MetaMask/extension-devs @MetaMask/policy-reviewers @MetaMask/supply-chain

# The offscreen.ts script file that is included in the offscreen document html
# file is responsible, at present, for loading the snaps execution environment
Expand Down
29 changes: 29 additions & 0 deletions docs/lavamoat-policy-review-process.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# LavaMoat Policy Review process in metamask-extension

When there's a need to change policy (because of new or updated packages that require a different set of capabilities), please follow these steps:

> In the initial soft-launch of the process the approval from policy reviewers is not mandatory, but it will be in the near future.

### Engineer on the dev team:
1. Notice the `validate-lavamoat-policy-*` circleci job fail because dependency updates or changes need a policy update.
2. (optional) Generate an updated policy and give it a cursory look in local development whenever you’re testing the change.
- If you're confident your update is complete, you can push it to the PR branch.
3. To generate a complete set of new policies, call `metamaskbot` for help:
- put `@metamaskbot update-policies` in a comment on the PR. When it produces changes, they need to be reviewed. The following steps assume update-policies produced changes.
- *Note the response from the bot points to instructions on how to review the policy for your convenience. https://lavamoat.github.io/guides/policy-diff/*
4. Analyze the diff of policy.json and use the understanding of the codebase and change being made to decide whether the capabilities added make sense in that context. Leave a comment listing any doubts you have with brief explanations or if everything is in order \- saying so and explaining why the most powerful capabilities are there (like access to child_process.exec in node or network access in the browser)
*Remember the Security Reviewer comes with more security expertise but less intimate knowledge of the codebase and the change you’ve built, so you are the most qualified to know whether the dependency needs the powers detected by LavaMoat or not.*
- You can use these questions to guide your analysis:
1. What new powers (globals and builtins) do you see? Why should the package be allowed to use these new powers? Explain if possible
2. What new packages do you see? Did you intend to introduce them? If you didn’t, which package did? (can you see them in `packages` field in policy of any other package that you updated or introduced?)
- The comment is mandatory even if you don’t understand the policy change, in which case you’re expected to state so (it’s ok to not understand)
- Note: this could be enforced by a job that is only passing if the comment was made
- Note: we’d introduce more tooling to summarize and analyze policy and post that as a comment on the PR
5. Mention `policy-reviewers` group in your comment.
policy-reviewers group includes security liaisons and their involvement is not limited to (but likely focused more around) their respective teams’ PRs.

### L1 Security Reviewer:
1. Look at the policy and the comment from the Developer. Approve the PR if they match and the policy change seems safe. Address questions the Developer had and discuss if the policy change doesn’t seem right.
2. If changes are hard to explain or seem dangerous, escalate to a review of the dependency and its powers by mentioning `supply-chain`
### (optional) L2 Security Reviewer:
1. Review the dependency in question and inform the PR reviewers whether it’s deemed malicious or safe.
Loading