Extra policies for sensible topics (contributions, reviews, merging)

[moul]

I recently approved a pull request for merging on GitHub without conducting a thorough analysis (#670). However, upon reflection, I realize that this was not the best approach, and I should have taken the time to conduct a more thorough review. Moving forward, I suggest defining additional policies and identifying areas that require extra attention to ensure that we achieve both efficiency and security.

When adding new features or extending the surface of our programs, we need to justify their necessity, and we must ensure that they are future-proof and minimalist. In particular, when working with cryptography, we should perform entropy and security benchmarks and document our findings. I welcome suggestions on the rules that we could add to ensure that we maintain our security standards.

To implement these changes, I propose adding a new label to identify pull requests that require extra attention and updating the CONTRIBUTING.md file. We may also need to reconsider (revert) two recent pull requests and conduct a thorough analysis before merging (#670 and #580).

Edit: related with #687.

[grepsuzette]

Cryptography is both small in terms of volume of code and critical in terms of impact. That being said, it's almost the same about some of our stdlibs or the VM itself for example.

When it comes to those areas we need to:

• discourage changes, as a default stance (it's the opposite for non-critical areas),
• document problems with external documentation or references,
• have more than 1 reviewer for those PR/projects,
• the reviewers need to study the doc, the references if not familiar yet,
• if no consensus, reach out to Jae, or relevant experts,

I think more than 1 label here.

[moul]

CODEOWNERS combined with stronger protected branch configurations enables the creation of designated patterns for sensitive areas, ensuring authorized access and establishing a clear audit trail.

[thehowl]

I'll defend my case on sha256: it is no less secure than std.Hash, it was reviewed and approved (in the scope of Sum256) by Jae and it uses the same calling code as std.Hash.

But with this in mind, I do agree with you and think that we should probably add as a requirement for every PR that:

• At least one person from the core team reviews it
• The person merging the PR is not the author

Additionally, I think that the first reviewer may request either an additional review from another core-tech or from a "security" group of sorts (with you and Jae for now, I would imagine).

PRs touching crypto (-related) directories require automatically a security approval through codeowners I would say.

[sbibek086]

Yes, I totally agree with @moul . I will have more to say on this on weekend.