Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Add a rule for SRLabs audits #136

Closed
the-right-joyce opened this issue Aug 3, 2023 · 7 comments · Fixed by paritytech/polkadot-sdk#1673
Closed

Add a rule for SRLabs audits #136

the-right-joyce opened this issue Aug 3, 2023 · 7 comments · Fixed by paritytech/polkadot-sdk#1673
Assignees
@mordamax

Comments

@the-right-joyce
Copy link

@0xJayPi, @serhanwbahar and I had a call today with SRLabs where we discussed how the current labels-based auditing process should be replaced in our beautiful monorepo.

Currently, the CI enforces users to add a D* label to their PRs in case these files are touched:
polkadot: ^runtime/polkadot
polkadot : ^runtime/kusama
polkadot : ^primitives/src/
polkadot : ^runtime/common
substrate : ^frame/
substrate : ^primitives/

We aligned that we want to keep the enforcement of the auditing process for the same files (the runtime files will be obsolete, as we won't have them in the monorepo), but the process should now look like this:

  1. When a PR is opened and one of these files are touched (see above substrate) the tool should check if this PR was created by an external user (= non-member of @paritytech)
  2. In case that's true this PR will be added to the board https://github.com/orgs/paritytech/projects/103 (Status: Backlog)
  3. And can't be merged until at least one member of @paritytech/srlabs has approved the PR on the review process
@mordamax
Copy link
Contributor

mordamax commented Aug 4, 2023

@the-right-joyce few questions:

  1. is this change actual only in scope of polkadot-sdk (monorepo) or needs to be implemented Before we move to monorepo? (like ~now-ish?)
  2. can we change a bit the 1st requirement:

instead of:

the tool should check if this PR was created by an external user (= non-member of https://github.com/paritytech)

this:

the tool should check if PR was created by non paritytech/core-dev member?

https://github.com/orgs/paritytech/teams/core-devs/members i think it includes almost everyone of "knowers-of-the-code", and if I (not a core-dev) change any of the listed files, that would require srlabs to approve too?

this change would:

  1. let main contributors to be not blocked by srlabs if they touch those files
  2. allow us not to implement a check for contributor membership in any organization, and integrate this easier

@Bullrich Bullrich mentioned this issue Aug 8, 2023
Closed
@redzsina
Copy link

redzsina commented Aug 9, 2023

Thank you for driving this and our call last week @the-right-joyce ! We had some further discussions with the team what might make sense to have in place for the new auditing process:

  • time traceability: it would be great if in the dashboard we could have an overview when someone moved a PR to the audited column, just to keep track of the time a feature was audited
  • priorities: as discussed last week, we could implement some kind of tagging that would enable everyone to understand the current priorities within the queued PRs (like P0, P1, etc)
  • last week one of the options proposed was to have a column for in progress PRs. This would be helpful for us, but we should also have a label in place for the ones where we did find and report some vulnerabilities (for example: "fixinprogress"), since it could take some time to move them to the "Audited" column.

@the-right-joyce
Copy link
Author

  1. is this change actual only in scope of polkadot-sdk (monorepo) or needs to be implemented Before we move to monorepo? (like ~now-ish?)

it's for the polkadot-sdk, so once we're live with it it would be good to have this implemented asap (currently we're using the D* labels rule set)

https://github.com/orgs/paritytech/teams/core-devs/members i think it includes almost everyone of "knowers-of-the-code", and if I (not a core-dev) change any of the listed files, that would require srlabs to approve too?

sounds good!

@redzsina
I have an issue tracking all of the changes we discussed here, including the fields and status
I'm not sure if there's a way to track on gh when an issue was moved from one column to another, but I will see what we can do here

@mordamax mordamax self-assigned this Aug 14, 2023
@Bullrich
Copy link
Contributor

Hi! I'm Javier and I'm currently working on the automatic review system!

Given the requirements of this ticket, I believe it would be better for it to become its own project.

Flow

To make it clear, the project requirements would work the following way:

  1. When a Pull Request is created the system checks if the user does not belong to the team core-devs
  2. The system then checks if the file belongs to one the mentioned files
  3. If rule 1 & 2 are completed it add the PR to Security Audit board with the status Backlog.

For the next step:

And can't be merged until at least one member of @paritytech/srlabs has approved the PR on the review process

This should be easy to achieve by adding a new rule to the PRCR bot (soon to be replaced by https://github.com/paritytech/review-bot).

Regarding @redzsina comments:

time traceability: it would be great if in the dashboard we could have an overview when someone moved a PR to the audited column, just to keep track of the time a feature was audited

I just looked into it and you can see the changes inside the PR/Issue itself:
image

My questions

last week one of the options proposed was to have a column for in progress PRs

Could we have "draft" PRs instead for this situation?

priorities: Should we keep the labeling system for the priorities? Is this "used" by any system?

Let me know if this would satisfy all your requirements!

@the-right-joyce
Copy link
Author

Could we have "draft" PRs instead for this situation?

This was solved, I've added the status "in progress" on the board

priorities: Should we keep the labeling system for the priorities? Is this "used" by any system?

Also here, we have a field on the project where the priorities are given. For the monorepo we don't want to use priority-labels any longer as a PR/issue can have different priorities on each project.

@louismerlin
Copy link

As discussed in this issue, some PRs in the polkadot-sdk repository are now required to have a review by a member of @paritytech/SRLabs. Unfortunately there is a bug with the CI bot automating this workflow.

I reviewed one of these PRs and marked it as such. The CI bot then re-requested a review from us, in effect nullifying my action.

paritytech/polkadot-sdk#1194 (comment)

Screenshot from 2023-08-28 10-13-49

@Bullrich
Copy link
Contributor

Hi @louismerlin, this seems to be intentional.

From the latests changes:

      - min_approvals: 2
        teams:
          - srlabs

Changing that number to one would fix this problem

Bullrich added a commit to paritytech/polkadot-sdk that referenced this issue Aug 28, 2023
@Bullrich
PRCR - Reduced required reviews for srlabs
f42b1c5 
As requested by @the-right-joyce and @louismerlin, the amount of required reviews from the `srlabs` team has been reduced to 1.

See related discussion: paritytech/pr-custom-review#136 (comment)
@Bullrich Bullrich mentioned this issue Aug 28, 2023
Merged
ggwpez pushed a commit to paritytech/polkadot-sdk that referenced this issue Aug 28, 2023
@Bullrich
PRCR - Reduced required reviews for srlabs (#1203)
70ab64b 
As requested by @the-right-joyce and @louismerlin, the amount of required reviews from the `srlabs` team has been reduced to 1.

See related discussion: paritytech/pr-custom-review#136 (comment)
@mordamax mordamax mentioned this issue Aug 29, 2023
Merged
Daanvdplas pushed a commit to paritytech/polkadot-sdk that referenced this issue Sep 11, 2023
@Bullrich @Daanvdplas
PRCR - Reduced required reviews for srlabs (#1203)
5165a0d 
As requested by @the-right-joyce and @louismerlin, the amount of required reviews from the `srlabs` team has been reduced to 1.

See related discussion: paritytech/pr-custom-review#136 (comment)
Bullrich added a commit to paritytech/polkadot-sdk that referenced this issue Sep 19, 2023
@Bullrich
added custom rule for audit team
7a33492 
Added rule which allows the audit team to lock particular important changes when they were created by someone who does not belong to the core-devs team

This resolves paritytech/pr-custom-review#136
Bullrich added a commit to paritytech/polkadot-sdk that referenced this issue Sep 22, 2023
@Bullrich
added custom rule for audit team
2b4e3cb 
Added rule which allows the audit team to lock particular important changes when they were created by someone who does not belong to the core-devs team

This resolves paritytech/pr-custom-review#136
@Bullrich Bullrich mentioned this issue Sep 22, 2023
Merged
Bullrich added a commit to paritytech/polkadot-sdk that referenced this issue Sep 28, 2023
@Bullrich
Added review-bot to fine tune review requirements (#1673)
4384c61 
Created a Github Action that uses the [Review-Bot
app](https://github.com/paritytech/review-bot) to require more fine
tuned requirements to review pull requests before allowing the PR to be
merged.

This uses
[`pull_request_target`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target)
for the event, not `pull_request`. This is a security measure so that an
attacker doesn’t have access to the secrets.

All the rules have been copied from the original
`.github/pr-custom-review.yml` file.

I want to clarify, this particular commit is **not intended to replace
PRCR yet**.

# Advantages it brings over `PRCR`

Most of the features available in `PRCR` have been duplicated and
enhanced. For a complete detailed write up, please see:
- paritytech/pr-custom-review#114 -> Proposal for the rewrite
- [Review Bot
Documentation](https://github.com/paritytech/review-bot/blob/main/README.md)

The most important features are:
- `include` and `exclude` fields now accept an array, making it easier
to read the regular expressions.
- Ability to skip a rule
- We can set that PRs coming from a particular user or team will cause
the rule to be skipped.
- This is used in the `Audit rule`, which was requested by
@the-right-joyce.
  - This resolves paritytech/pr-custom-review#136
- Ability to request fellows instead of teams
- As requested in polkadot-fellows/runtimes#7, this bot has the ability
to request fellows by rank instead of users.
- We currently have polkadot-fellows/runtimes#31 which is using that
feature.

Aside from all the rules available in `PRCR` I have added a particular
rule to lock the review-bot files and require a review from the
`locks-review` team, the @paritytech/ci team and the
@paritytech/opstooling team to ensure that the file has been written
correctly.

## Next steps

The next steps will consist on paritytech/review-bot#53, once this issue
has been resolved, and `review-bot` has worked without any issues on
this repository for a while, we will upgrade it to be able to fully
replace `PRCR`.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mordamax mordamax

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Added review-bot to fine tune review requirements paritytech/polkadot-sdk
5 participants
@mordamax @Bullrich @louismerlin @redzsina @the-right-joyce