Making Code Reviews Easier
Why you might want to keep using them — with some changes
Free Content on Jobs in Cybersecurity | Sign up for the Email List
One of my posts on secure code and application security.
Free Content on Jobs in Cybersecurity | Sign up for the Email List
Code reviews may cause people to quit. I did.
Code reviews are painful. I get it. At one company where I worked I hated the code reviews — but not because of the code review itself. I hated the fact that we could not take appropriate steps to speed them up. Also the person doing the code reviews was not reasonable in and kind of mean.
It reminds me of that saying — “Hate the player, not the game.” :-D
Except I said it backwards. Hate is a strong word but here’s an example of why I had some disdain for this person. He seemed to antagonize me more than other people. Perhaps because I wasn’t afraid of him, busted through his fast-talking logic when others couldn’t, and got around his blocking issues more than once.
One time he caused all the technical managers to come to a meeting to argue against a certain approach. I knew he was going to say the approach wouldn’t work. I worked on a back office team. I went to the corresponding person on the front office team and asked them to test the approach prior to the meeting, which he did. When the reviewer started trying to use his scare tactics on management about how it wouldn’t work, the front office person said, “I already tested it and it worked fine.”
Meeting adjourned.
I don’t know exactly why this guy was so cantankerous. I didn’t do anything to him personally or say the kind of things he said to me in front of other people. I just wanted to get my projects done in a timely manner and yes, I did argue (with logic and solutions, not emotions) against unreasonable delays and requests. He once made fun of me in a meeting — which did not actually phase me. But someone else said, “He treats you like your his sister or something.”
I had to make a strong case to get some secure and robust code in place to prevent errors in a particular application for a back office banking system which he was trying to block for whatever reason. When I went on vacation this person sent me an email. “Enjoy your vacation. :)” He did not mean that in a nice way. I just sighed. I knew I had had enough.
When I got back, no one on my team had the energy or guts to argue with this person. They just made whatever changes he told them to make, even though everyone, including the managers involved, where in the room when we agreed to use the code as is. The managers couldn’t even stand up to him because he had been around longer than them and controlled everything about the database.
He was a piece of work. My code was destroyed and had a bunch of problems in it. I was continually finding problems in this code base that caused weird discrepancies and I didn’t want my name anywhere near that code. It was at that moment I knew I was going to leave.
As I have written about before, I was learning AWS at that time and running the Seattle AWS Architects and Engineers meetup and reached out to the executives at Capital One to see if there was any chance they would ever have an AWS team and if there was any chance I could be on it. If not there, I was going anywhere I could use AWS.
I had just paid my own way to re:Invent and little did I know, the Capital One team was there, trying to reach me via my work email. But I didn’t bring my work laptop. I was going on a road trip after the conference and did not want to bring it on that portion of the vacation. I moved to the cloud team a short time later and escaped the bane of my existence at the time — code reviews preventing me from getting my code to production in a timely manner.
So there you have it — draconian code reviews run by the wrong people will cause people to quit. Period.
Code reviews with good intentions
Now you would think I despised code reviews at this point. But that is not the case. I know this person was the problem, not the review itself. Reviews are a good thing. I love a second set of eyes on my code and especially QA teams who help me find bugs before they find me in production! I had almost all amazing QA team members throughout my career. Love your QA team and help them find your bugs before your name is associated with them in a production application.
The same should be true of code reviews — if the right person is performing the code review. The findings should be reasonable, matter, and help you ensure your code aligns with the company’s policies. The reviewers need to not only tell you to follow the policies but why they exist.
Part of the problem I had with this draconian person reviewing my code is that he could not clearly articulate why I had to follow a certain pattern. Another reviewer said something to me and it finally clicked. The pattern they were using was trusted because it had been widely used over time. Deviate from the pattern and the reviewers have to take more time to review the code.
Now, I didn’t particularly like the code in that pattern. I found it overly complex and had reduced it to something much cleaner, clearer, and simpler. But if someone had told me that if I followed that pattern my code would get to production faster, I would have done it in a heartbeat.
That was a communication issue that could have saved a lot of angst. The person that told me didn’t even realize he told me this. He was just telling me that he was having to review everything in more detail because it was different. That’s when I figured out what I could do to speed up reviews in the future.
The reviewers should also be able to articulate to you how you can get through the reviews faster. What steps can you take to limit the findings in a review and ensure it follows all the necessary components to quickly get through the review? When I worked at Capital One, the person reviewing network rules for production applications going to AWS was overloaded. If he found an issue, you would have to fix the problem and go back into the queue.
To help people ensure their networking was correct before they got to the review, we set up office hours, which I ran for a time. People all over the company would come and I would help them with their network documentation to help ensure it passed the review. If they were trying to do something non-compliant I would warn them that the review might need to be escalated because likely that wasn’t going to pass his review.
Things a code review will find that automated tools won’t
I’ve started thinking of things that an automated code review won’t find. Yes, you absolutely should use automated tools. They are faster, more efficient, and will find things missed by the human eye. But they won’t find nuanced flaws, some logic flaws, and some organization-specific requirements.
Here are some examples:
- I created a naming convention for parameters in CloudFormation templates. When I got back to my code I realized I hadn’t followed my own naming convention. That is not going to cause a security flaw but naming conventions may exist for a reason.
- If your code requires a certain header or footer, some automated tools will find that, but not all.
- I wrote about a piece of questionable code in my book. Someone had a line at the end of a stored procedure that skipped the transaction if it was less than a dollar. Perhaps the QA team would test for $.01. But may be they only tested whole dollar amounts. That logic flaw would be missed — and it was. I caught it during code review but the draconian reviewer — who told my junior dev to add that code — would not remove it. A year later when we were working on that code again I alerted the business manager and she pretty much freaked out and said there were a bunch of transactions they needed to fix that were affected by that logic. No automated or manual testing found that problem — but it was easily spotted by me in a code review.
- In my code in this blog post, I am using naming conventions with particular prefix. All the IAM roles I’ve created limit the roles from modifying any CloudFormation stacks except for those with a prefix associated with that role. If someone leaves out that condition in an IAM Policy — tell me what automated tool is going to find that problem? It won’t. Perhaps your QA team will find it — if they have a test case for it. Do you even have a QA team testing your IAM policies? I’ve seen organizations where the person that writes the policy tests it. No automation is going to help you in that case.
These are just a few examples. I can think of many, many others based on 25 years of experience writing code — especially the code for back office banking systems processing billions of dollars of assets under management. I’ve also discovered many issues with review of code samples from cloud providers, for example. I also find issues in penetration tests and assessments when I manually review policies that might have been discovered in a code review but not automated tools or testing alone due to the number of test cases required to find the problem.
I have always liked how this person at AWS re:Invent described code reviews for a critical piece of security code. After Heartbleed, AWS extracted the code they needed from an open source library to minimize their code to only what they needed. You can read more about heartbleed here. It created a huge security risk for many companies.
AWS created their own open source TLS library. You can watch this presentation to learn about the reviews. I, for one, am glad to hear they reviewed that code.
Making code reviews easier
Sure you don’t like code reviews and you cheer when someone says you don’t need them anymore. But is that prudent? Do you want your name on that code that brings down production or causes the latest data breach?
Would you accept an inconvenience to prevent a data breach?
Maybe?
OK, but what can we do to make code reviews less painful?
The process should be as efficient as possible. In the words of Einstein things should be as simple as possible — but not too simple. Too simple leads to errors and inaccuracies. The rush to the cloud is why security is currently plagued with misconfigurations as one of the leading cause of data breaches. This should be a lesson to anyone in a rush to get their code into production without appropriate checks for security and accuracy.
However, I am 100% in favor of automating absolutely everything possible and working with people on both sides of the review to reduce the bottlenecks. What is the underlying cause of the bottleneck and how can we speed it up?
In the case of the draconian reviewer, the problem is that he was claiming he had to review all the code for performance reasons. The way you write your database queries for a relational database can have a significant impact on performance. That said, he could not articulate the rules that would create the best performance. I did a lot of research on that and made sure I provided queries that offered the best possible performance — to a point.
Someone who is familiar with database transactions, query logic, and other database tuning options in stored procedures can look at one before you ever test and and tell you it’s not going to work well. This is faster than testing. But in some cases you cannot be sure what performance is best because it depends on how the data is laid out on disk. The performance of the same query may vary based on the structure of data across an array of drives.
The DBA in that case had access to a production-like environment where he could test the code. This one reviewer was holding up multiple teams by testing all the performance for each stored procedure himself. Not real scalable right? In addition, ironically, even after his testing and making changes a stored procedure had performance issues in production.
My idea was perhaps the developers should be able to execute queries and tests in that environment or a similar one. The initial problem was that yet another mirrored production environment would cost too much. However, the developers could use his environment. He wasn’t using it at all times.
The other problem was that developers were not allowed to see production data (except the person on call and something broke occasionally.) Developers didn’t need access to the data to run performance tests. Provide a way to drop the code, run the queries, and spit out the performance without having developers look at the data itself.
In other words, could we somehow automate those tests? Then the DBA could simply review the performance stats. The developers could also see and fix any issues in advance of the review. The problem would be if a bunch of developers were executing the code at once so you’d need some kind of test queue for performance jobs.
Perhaps the DBA shouldn’t be running the performance tests at all but creating a mechanism for the developers to do it themselves. The tests would need to be run in a secure and repeatable manner so they could be trusted. He could review the outputs of the performance tests and recommend changes instead of doing all the work himself.
I’m just throwing out ideas, because I left before pursuing that further, sensing that the company had some issues. If a company cannot produce results in a timely manner, it’s not going to be successful. In the end, that division of the company was sold off to their arch rival in the industry. Due to code reviews? I don’t know but perhaps inability to produce new features in a timely manner had something to do with it. And don’t get me started on the scrum masters either. I wrote about scrum here:
If you have a bottleneck related to code reviews, try to work with the reviewers and the developers to determine how you might be able to speed up certain aspects of the test that can be automated — without trashing the review completely or making developers just bend over backwards for reviewers.
Something that is very helpful when it comes to code reviews is to create common patterns wherever possible. Then if developers are using the common patterns or standard templates, the reviews go quickly. If the code is not changing at all and the developers are simply passing in parameters — then you can skip the review altogether. The code hasn’t changed!
To see what I mean, check out this post on how a common S3 bucket CloudFormation template can help security.
Also consider the priority of code. IF your network segments off marketing applications that display static content from critical business applications hosting PII and financial data, perhaps the code reviews for those applications only require a peer review by someone on the same team, for example. Or no review if you really think there’s no way an attacker can traverse the network from those applications, if compromised, and get to another more sensitive application on your network.
For a team with limited resources, I simply required the lead on each team to review other’s code prior to going to QA. We didn’t have a separate security team that could perform that function. It was at least a second set of eyes on the code by a more experienced person. It was in the best interest of the team lead to get that code to production but they would not want their name on something that would cause a problem.
I trusted my team leads in that case and knew they were experienced. The problem arises when team leads are not actually experienced enough to perform proper reviews. Then your reviews might not be very helpful. Maybe you have excellent QA team members — or maybe they are not quite experienced enough to understand how to create test cases that provide full coverage. Understanding the capabilities of your team may influence your review process as well.
But code reviews miss things
Sure, code reviews miss things. Automation is more efficient. These statements do not negate the need for code reviews. You should use code reviews and automation to ensure your code is secure, performant, and meets the company’s standards.
I saw the argument about the log4j vulnerability not being discovered in any code review. I used log4j for years in different companies. I have a question for everyone on this subject.
***************************************************
How many of you actually reviewed the log4j code?
***************************************************
If you and no one at your company reviewed every line of log4j before using it, then it is not logical to use this vulnerability as a case against code reviews.
Perhaps the problem was actually that:
*************************************************
NO ONE REVIEWED THE CODE.
***************************************************
They just used it and assume it was ok because everyone else was using it.
…without considering removing or disabling features in the code that they didn’t need:
By the way, anyone running that code in a private network would not be affected because the code would not be able to reach the Internet. More on networking here because this is something else some people think they can do without.
If you are not familiar with log4j or want to read my assessment of the problem, you can find it here.
In summary — fix your code reviews, don’t ditch them entirely
The argument that something slows you down is never a reason to stop doing it. That’s like saying:
- Brushing my teeth takes too long. I go to the dentist so why bother.
- Locking my front door slows me down so I’m going to skip it.
- I don’t have time to change the oil in my car.
I mean, you can skip those things. You might get lucky. But it’s a risk I wouldn’t take. It may cost you a lot in the long run if you’re on the wrong side of the luck equation.
The reason you should stop doing it is if it provides no value whatsoever. Code reviews can find things other reviews cannot. That said, code reviews alone are not very effective, and code reviews need to be as efficient as possible. Dive into the reasons why code reviews are a bottleneck and try to find ways to alleviate the bottleneck, while retraining the parts of the review that provide value.
Even though I’ve told you about the draconian code reviewer in my stories above and feel that some parts of the reviews were completely unnecessary, I did learn a lot through that review process. I became much better at tuning database queries and using database transactions properly. Typically, things are not “all bad” or “all good.” You have to dive into the details a bit more to find solutions that solve the problem in a way that does not create bigger problems in the process.
Follow for updates.
Teri Radichel | © 2nd Sight Lab 2023
About Teri Radichel:
~~~~~~~~~~~~~~~~~~~~
⭐️ Author: Cybersecurity Books
⭐️ Presentations: Presentations by Teri Radichel
⭐️ Recognition: SANS Award, AWS Security Hero, IANS Faculty
⭐️ Certifications: SANS ~ GSE 240
⭐️ Education: BA Business, Master of Software Engineering, Master of Infosec
⭐️ Company: Penetration Tests, Assessments, Phone Consulting ~ 2nd Sight LabNeed Help With Cybersecurity, Cloud, or Application Security?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
🔒 Request a penetration test or security assessment
🔒 Schedule a consulting call
🔒 Cybersecurity Speaker for PresentationFollow for more stories like this:
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
❤️ Sign Up my Medium Email List
❤️ Twitter: @teriradichel
❤️ LinkedIn: https://www.linkedin.com/in/teriradichel
❤️ Mastodon: @teriradichel@infosec.exchange
❤️ Facebook: 2nd Sight Lab
❤️ YouTube: @2ndsightlab