Juan Pablo Tosso

Cybersecurity Research Engineer

Backend Developer

Penetration Tester

Open-Source Developer

Juan Pablo Tosso
Juan Pablo Tosso
Juan Pablo Tosso
Juan Pablo Tosso

Cybersecurity Research Engineer

Backend Developer

Penetration Tester

Open-Source Developer

Blog Post

Golang “quality frameworks”?

November 14, 2021 Coraza

Once a open-source project gets some attention, people begin to question more than the test coverage and functionality, but the code quality itself. There are predefined rules about golang’s good practices, like using camelcase, but once the community asks for certain standards and PRs are coming with an “unwanted” style, you must stablish what are the code good practices and design guidelines. In this post we are going to talk about how I chose the quality framework for Coraza v2.

So one day my friend @felipe sent me his .pre-commit-config.yaml file for go-ftw and I tried to use it on Coraza, I got hundreds of errors and I said this is a future Coraza problem, not now. Eventually the day came and I wanted Coraza to get a highlight on the awesome-go project, which required better goreportcard.

So what are the challenges to implement this .pre-commit-config.yaml?

Go fmt or unused code (go vet) were not issues because my vscode plugins handled both, but let’s talk about go-lint: Go lint rules are quite strict, you are forced to use camel-case, you must replace some keywords like Id with ID, it just hates your variables, types, functions and everything you create. In order to comply with go-lint I had to change hundreds of names and add a lot of documentation.

Go-critic was harder to comply with, because it required me to transform if-elses into switch statements and it alerted some “redundant” type assertion that I’m sure was not redundant at all.

Transforming if-elses into switches

I’m not a fan of go labels, I think labels are hacks for bad algorithm designs but If I wanted to migrate an if-else into switch I will have to use continue and break with the parent’s label, otherwise I would just break the switch.

//sample from seclang/rule_parser.go
for i, c := range actions {
	if iskey && c == ' ' {
		// skip whitespaces in key
		continue
	} else if !quoted && c == ',' {
		f, err := actionsmod.GetAction(ckey)
		if err != nil {
			return nil, err
		}
		res = append(res, ruleAction{
			Key:   ckey,
			Value: cval,
			F:     f,
			Atype: f.Type(),
		})
		ckey = ""
		cval = ""
		iskey = true
	} else if iskey && c == ':' {

In order to transform this code into a proper switch, I added the label actionLoop:

actionLoop:
	for i, c := range actions {
		switch {
		case iskey && c == ' ':
			// skip whitespaces in key
			continue actionLoop
		case !quoted && c == ',':
			f, err := actionsmod.GetAction(ckey)
			if err != nil {
				return nil, err
			}
			res = append(res, ruleAction{
				Key:   ckey,
				Value: cval,
				F:     f,
				Atype: f.Type(),
			})
			ckey = ""
			cval = ""
			iskey = true
		case iskey && c == ':':

Finally after thousands of modified lines of code I could comply with go-critic.

The last plugin check was go-cyclo, it is a hard one because some Coraza code is transcribed from Modsecurity’s C++ code, making it really hard to remove complex conditions with more than 5 levels of nesting and over 80 ifs in a single function. I had made my best effort to fix this but the plugin is going to be disabled for a while, until I can migrate the modsecurity functions to native golan code.

Conclusion

It’s really hard to implement code quality frameworks for big codes like Coraza (20k+ lines), I should have started the project using quality frameworks. My next projects will start with high quality standards and I will enforce high quality standards for Coraza too.

Taggs:
Write a comment