Add a rule for SRLabs audits

[the-right-joyce]

@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]

@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

[redzsina]

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]

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

[Bullrich]

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:

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]

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]

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.

https://github.com/paritytech/polkadot-sdk/pull/1194#event-10206066965

[Bullrich]

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