Various panics fixes

[zc-devs]

Closes #67 and #56.

• refactored message parsing
• introduced custom message, request and response
• fixed both panics
• fixed response body processing

[jcchavezs]

I'd rather do something functions to get values from the map as string/int/ip.

func getString(args map[string]interface{}, key string) (string, error) {
   ...
}

Adding new struct to fetch values from a map looks overkill unless there are really important logic.

cc @anuraaga

[jcchavezs]

First please check if the RuleEngine is on/off, check https://github.com/corazawaf/coraza/blob/main/http/middleware.go#L132

[jcchavezs]

Could you please add some unit tests to functions?

[jcchavezs]

Do we really need to execute all the previous code if ruleEngine is off?

[zc-devs]

We need application at least. Then we could get rule engine status from WAF, if it would be public interface. In public interface there are only transactions constructors. So, I think we need to execute all previous code.

[anuraaga]

IIUC, this error should be returned if not ok above, while here it is the not exists case, so not an error

[zc-devs]

How to signal caller that there is no such key in the map/message? Is below code OK?

func getString(args map[string]interface{}, key string) (string, bool, error) {
	argVal, exist := args[key]
	if !exist {
		return "", false, nil
	}
	if argVal == nil {
		return "", true, nil
	}
	val, ok := argVal.(string)
	if !ok {
		return "", true, fmt.Errorf("invalid argument for %s, string expected, got %v", key, argVal)
	}
	return val, true, nil
}

[zc-devs]

coraza-spoa requires method field in the message.

1. message app=simple_app method= path... - variable value set up issue in HAProxy (probably haproxy.cfg;
2. message app=simple_app path... - message definition issue in HAProxy SPOE config.

[anuraaga]

Returning a bool seems fine - from what it looks like some fields are required and some aren't so the caller needs to determine whether it's an error or not, not this utility. Or otherwise we would need two versions of each, getFoo and getFooRequired or similar, but maybe too much

[jcchavezs]

Still we might need to learn about the error on retrieval. How about

var errNotFound = errors.New("key not found")

func getString(args map[string]interface{}, key string) (string, error) {
	argVal, exist := args[key]
	if !exist {
		return "", errNotFound
	}
	if argVal == nil {
		return "", nil
	}
	val, ok := argVal.(string)
	if !ok {
		return "", fmt.Errorf("invalid argument for %s, string expected, got %v", key, argVal)
	}
	return val, nil
}

so that we can flag when the key isn't in the map and return a ErrNotFound to the upstream function letting them decide what happens if not found and finally another error in case the data isn't correct (hence programatic error).

[zc-devs]

the caller needs to determine whether it's an error or not

Already does:

	req.method, err = getString(args, "method")
	if err != nil {
		return nil, err
	}
	req.path, err = getString(args, "path")
	if err != nil {
		fmt.Println(err.Error())
	}
	req.body, _ = getByteArray(args, "body")

[jcchavezs]

I would get the app from the spoeMsg and get the request right before we would use it.

[zc-devs]

I don't get it. What problem do you try to solve?

From spoeMsg only application name can be obtained. Application != application name.

Also in request and respone application name is already converted to string and 'validated'.

[jcchavezs]

What I am trying to avoid is parsing the entire message if the engine is off. If the engine is off we should cause minimum impact.

[zc-devs]

Does c490c9f look OK?

[jcchavezs]

Awesome work!