4

In a company of about ~100 developers, and ~2 pen-testers, a product is being developed. There's a good amount of diverse code coming in via github PR requests every day.

The goal is to review as much new code as possible before it gets merged.

On the one end of the extreme, it'd be something like 'review every PR and don't allow merge before a pen-tester vets". Clearly this is very resource consuming. The other extreme would be 'let everything pass through and do per-module pen-testing on releases'.

Are there any meaningful PR coverage strategies laying in between these two extremes?

Note: not asking about integrating automated code analysis into the build pipeline, the goal is to have the manual audit for as much code as possible in a meaningful way. Also, not asking about code review methods (such as methodology to identify certain classes of bugs).

  • 2
    This might be better suited for Software Engineering. (Also "~2 pen-testers"? Like 1 1/2? ;) – Tom K. Jan 23 '18 at 15:14
  • Despite the testing being security in nature, it's simply a test. SW Eng is likely a better place. – schroeder Jan 23 '18 at 15:16
  • 2
    It depends on your devops approach, resource, and just how much code you can really review in a day. If your devs are doing code reviews on pull requests tech them how to look for security issues - empower them to do it themselves. Try not to be a blocker to development, a pen test must review every PR or "no merge until I've looked at it" creates enormous work for you, obstructs development and will lead to dev hating the security process. – iainpb Jan 23 '18 at 16:15
  • 2
    What's the cost of failure here? If you are working on healthcare, credit card or other sensitive data, failing to pen-test something could be disastrous. If, on the other hand, you are just dealing with someone's list of favorite recipes, you can probably get away with something lighter. Also, what kinds of regulations / oversight do you have to deal with? – Becuzz Jan 23 '18 at 17:44
  • How big are the PRs, how long does it take the review them, and how many developers are knowledgeable enough about the code-in-question to review it? – mmathis Jan 26 '18 at 15:50
5

I would suggest making a list of types of changes that require pen tester sign-off on pull requests, such as:

  • Adding a new SQL query.
  • Using a new library.
  • Changing anything to do with passwords, credit cards, or other sensitive information.
  • Making or receiving new network connections.
  • Fixes for known vulnerabilities.
  • Urgent hotfixes that will go into production before full pen testing can be performed.

This is just an example. Your experts should be able to make a better list for your specific domain.

Also, you can make a policy to leave pull requests open long enough for pen testers to opt to review other types of changes if they desire. I follow a pomodoro schedule, so I only check my email every half hour. It's super annoying to have something important to say about a PR that was opened and merged within that time frame.

| improve this answer | |
  • I believe this wouldn't work well in practice: attempting to come up with a blacklist of changes that need pen-testers sign-off basically means deciding in advance where security flaws may happen, which is a task doomed to fail. Side note: even if such classes are somehow reasonably identified, it'd be hard to parse code to reliably spot those changes in the code (not sure if what's github support for this). – bgd223 Jan 25 '18 at 3:00
  • 1
    @bgd223 This answer is not advocating an automated solution but a policy. Before a change is merged, devs are supposed to go through such a checklist and actively get the pen-testers involved where necessary. While it's not possible to predict every security flaw, some parts of the code will have higher risk. So it's sensible to focus on those. Don't let perfect be the enemy of good. – amon Jan 25 '18 at 10:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.