[Optimizations] Extract query parameters only once. Verify signature with the proper requested algorithm.

[cherrador]

On version 4.3.0, I optimized two functions on the Utils class to avoid redundant processes.

The first one is at Utils::extractOriginalQueryParam
Optimized code:

    /**
     * Extract a query param - as it was sent - from $_SERVER[QUERY_STRING]
     *
     * @param string|null $name The param value to-be return. If null, it returns all the parameters.
     *
     * @return string|array
     */
    public static function extractOriginalQueryParam($name = null)
    {
        static $queryArray = [];
        if (empty($queryArray)) {
            if(empty($_SERVER['QUERY_STRING'])) {
                throw new Error(
                    "No query string provided",
                    Error::INVALID_PARAMETER
                );
            }
            $keys = ["SAMLRequest", "SAMLResponse", "RelayState", "SigAlg", "Signature"];
            $params = explode('&', $_SERVER['QUERY_STRING']);
            foreach ($params as $param) {
                $val = explode('=', $param);
                if(isset($queryArray[$val[0]]) && in_array($val[0], $keys) ) {
                    throw new Error(
                        "Duplicate parameter in query string",
                        Error::INVALID_PARAMETER
                    );
                }
                $queryArray[$val[0]] = $val[1] ?? '';
            }
        }
        if(is_null($name)) {
            $return = $queryArray;
        } else {
            $return = $queryArray[$name] ?? '';
        }
        return $return;
    }

This process extract the query parameters only once, the next time the function is called for another parameter, it should be already available on the static array. At the extraction time, it also checks if there is a QUERY_STRING defined and if there are not duplicated parameters. I am not sure if it's required to check only those specific parameters duplication (keys). To optimize even more, it could avoid this check and trigger an error if any parameter is duplicated.

One final optimization is when the function is called without the $name parameter. It returns the full parameters array, which will be used on a next optimization.

The next optimizations are at the Utils::validateBinarySign
Optimized code:

    public static function validateBinarySign($messageType, $getData, $idpData, $retrieveParametersFromServer = false)
    {
        if (!isset($getData['SigAlg'])) {
            $signAlg = XMLSecurityKey::RSA_SHA1;
        } else {
            $signAlg = $getData['SigAlg'];
        }
        if ($retrieveParametersFromServer) {
            $params = Utils::extractOriginalQueryParam();
            if(isset($params['SAMLRequest']) && isset($params['SAMLResponse'])) {
                throw new Error(
                    "Both SAMLRequest and SAMLResponse provided",
                    Error::INVALID_PARAMETER
                );
            }
            $signedQuery = $messageType.'='.$params[$messageType];
            if (isset($getData['RelayState'])) {
                $signedQuery .= '&RelayState='.$params['RelayState'];
            }
            $signedQuery .= '&SigAlg='.$params['SigAlg'];
        } else {
            if (isset($getData['SAMLRequest']) && isset($getData['SAMLResponse'])) {
                throw new Error(
                    "Both SAMLRequest and SAMLResponse provided",
                    Error::INVALID_PARAMETER
                );
            }
            $signedQuery = $messageType.'='.urlencode($getData[$messageType]);
            if (isset($getData['RelayState'])) {
                $signedQuery .= '&RelayState='.urlencode($getData['RelayState']);
            }
            $signedQuery .= '&SigAlg='.urlencode($signAlg);
        }
        if ($messageType == "SAMLRequest") {
            $strMessageType = "Logout Request";
        } else {
            $strMessageType = "Logout Response";
        }
        $existsMultiX509Sign = isset($idpData['x509certMulti']) && isset($idpData['x509certMulti']['signing']) && !empty($idpData['x509certMulti']['signing']);
        if ((!isset($idpData['x509cert']) || empty($idpData['x509cert'])) && !$existsMultiX509Sign) {
            throw new Error(
                "In order to validate the sign on the ".$strMessageType.", the x509cert of the IdP is required",
                Error::CERT_NOT_FOUND
            );
        }
        if ($existsMultiX509Sign) {
            $multiCerts = $idpData['x509certMulti']['signing'];
        } else {
            $multiCerts = array($idpData['x509cert']);
        }
        $signatureValid = false;
        foreach ($multiCerts as $cert) {
            try{
                $objKey = new XMLSecurityKey($signAlg, array('type' => 'public'));
                $objKey->loadKey($cert, false, true);
                if ($objKey->verifySignature($signedQuery, base64_decode($getData['Signature'])) === 1) {
                    $signatureValid = true;
                    break;
                }
            } catch (Exception $e) {
                if (count($multiCerts) == 1) {
                    throw new ValidationError(
                        $e->getMessage().' in the received '.$strMessageType,
                        ValidationError::KEY_ALGORITHM_ERROR
                    );
                }
            }
        }
        return $signatureValid;
    }

Two areas were optimized:

1. When the $retrieveParametersFromServer is true. Now it use the optimized query parameters extraction.
2. At the end, it simplifies the XMLSecurityKey creation.

The second optimization is because the original code is forcing to start using RSA_SHA1 algorithm even if the signature is using a different one. Which, if this is the case, it will have to go through the extra process of Utils::castKey (recreate the XMLSecurityKey with the correct algorithm). On these days RSA-SHA256 or stronger are more common. Instead of that wasted process, go directly and create the XMLSecurityKey with the correct algorithm on the first iteration.

As side effect, now I found that the function Utils::castKey is not used any more by the library and it could be deprecated.

Not sure if this workaround was done previously for cases when the algorithm is not known, but that should be irrelevant now, as at the beginning of the function the algorithm is defined as RSA-SHA1 if it's not specified anyways.

I have implemented these changes on this and previous versions of the library at production servers with Azure Active Directory (Entra ID) SAML for more than 1 year without any problem. I am not sure what is the process to collaborate in this project. If you agree with these changes, I could create a PR.

[pitbulk]

Yes please, create the PR and I will review deeper, thanks!