Hi Remi, Willy,
Is the length check at the start of `jwt_parse_alg()` actually useful? I would
expect that the vast majority of strings passed are valid algorithms that are
*not* `none`. Thus I expect this `if()` to almost never be `true`.
Should the `if()` be removed and a new `case 'n'` be added to the switch? Or
should an `unlikely()` be added around the condition?
Best regards
Tim Düsterhus
Apply with `git am --scissors` to automatically cut the commit message.
-- >8 --
`jwt_parse_alg()` previously incorrectly returned `JWS_ALG_NONE` for inputs
`""`, `"n"`, `"no"`, and `"non"` due to an incorrect check with `strncmp` that
also matches prefixes.
This bug did not affect the matching of the other known variants, because of
the special cased length check at the start of the function. Nonetheless these
variants are also affected and this bug might've been exposed during
refactoring.
I did not see an small fix for `strncmp`, so I used the opportunity to migrate
this function to the ist API, which avoids the issue altogether. The overall
structure of this function was preserved.
A config like:
http-response set-header bearer %[str(),jwt_verify(,)]
Now correctly returns:
> [ALERT] (109770) : config : parsing [./haproxy.cfg:6] : error detected in
> proxy 'test' while parsing 'http-response set-header' rule : failed to parse
> sample expression <str(),jwt_verify(,)]> : invalid args in converter
> 'jwt_verify' : unknown JWT algorithm: .
JWT support is new in 2.5, no backport needed.
---
include/haproxy/jwt.h | 2 +-
src/jwt.c | 34 +++++++++++++++++-----------------
src/sample.c | 2 +-
3 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/include/haproxy/jwt.h b/include/haproxy/jwt.h
index a343ffaf7..84421530d 100644
--- a/include/haproxy/jwt.h
+++ b/include/haproxy/jwt.h
@@ -26,7 +26,7 @@
#include <haproxy/buf-t.h>
#ifdef USE_OPENSSL
-enum jwt_alg jwt_parse_alg(const char *alg_str, unsigned int alg_len);
+enum jwt_alg jwt_parse_alg(struct ist);
int jwt_tokenize(const struct buffer *jwt, struct jwt_item *items, unsigned
int *item_num);
int jwt_tree_load_cert(char *path, int pathlen, char **err);
diff --git a/src/jwt.c b/src/jwt.c
index 94bfa5adb..590b18c3b 100644
--- a/src/jwt.c
+++ b/src/jwt.c
@@ -28,49 +28,49 @@ static struct eb_root jwt_cert_tree = EB_ROOT_UNIQUE;
* The possible algorithm strings that can be found in a JWS's JOSE header are
* defined in section 3.1 of RFC7518.
*/
-enum jwt_alg jwt_parse_alg(const char *alg_str, unsigned int alg_len)
+enum jwt_alg jwt_parse_alg(struct ist str)
{
enum jwt_alg alg = JWT_ALG_DEFAULT;
/* Algorithms are all 5 characters long apart from "none". */
- if (alg_len < sizeof("HS256")-1) {
- if (strncmp("none", alg_str, alg_len) == 0)
+ if (istlen(str) < sizeof("HS256")-1) {
+ if (isteq(str, ist("none")))
alg = JWS_ALG_NONE;
return alg;
}
if (alg == JWT_ALG_DEFAULT) {
- switch(*alg_str++) {
+ switch(istshift(&str)) {
case 'H':
- if (strncmp(alg_str, "S256", alg_len-1) == 0)
+ if (isteq(str, ist("S256")))
alg = JWS_ALG_HS256;
- else if (strncmp(alg_str, "S384", alg_len-1) == 0)
+ else if (isteq(str, ist("S384")))
alg = JWS_ALG_HS384;
- else if (strncmp(alg_str, "S512", alg_len-1) == 0)
+ else if (isteq(str, ist("S512")))
alg = JWS_ALG_HS512;
break;
case 'R':
- if (strncmp(alg_str, "S256", alg_len-1) == 0)
+ if (isteq(str, ist("S256")))
alg = JWS_ALG_RS256;
- else if (strncmp(alg_str, "S384", alg_len-1) == 0)
+ else if (isteq(str, ist("S384")))
alg = JWS_ALG_RS384;
- else if (strncmp(alg_str, "S512", alg_len-1) == 0)
+ else if (isteq(str, ist("S512")))
alg = JWS_ALG_RS512;
break;
case 'E':
- if (strncmp(alg_str, "S256", alg_len-1) == 0)
+ if (isteq(str, ist("S256")))
alg = JWS_ALG_ES256;
- else if (strncmp(alg_str, "S384", alg_len-1) == 0)
+ else if (isteq(str, ist("S384")))
alg = JWS_ALG_ES384;
- else if (strncmp(alg_str, "S512", alg_len-1) == 0)
+ else if (isteq(str, ist("S512")))
alg = JWS_ALG_ES512;
break;
case 'P':
- if (strncmp(alg_str, "S256", alg_len-1) == 0)
+ if (isteq(str, ist("S256")))
alg = JWS_ALG_PS256;
- else if (strncmp(alg_str, "S384", alg_len-1) == 0)
+ else if (isteq(str, ist("S384")))
alg = JWS_ALG_PS384;
- else if (strncmp(alg_str, "S512", alg_len-1) == 0)
+ else if (isteq(str, ist("S512")))
alg = JWS_ALG_PS512;
break;
default:
@@ -279,7 +279,7 @@ enum jwt_vrfy_status jwt_verify(const struct buffer *token,
const struct buffer
enum jwt_vrfy_status retval = JWT_VRFY_KO;
int ret;
- ctx.alg = jwt_parse_alg(alg->area, alg->data);
+ ctx.alg = jwt_parse_alg(ist2(alg->area, alg->data));
if (ctx.alg == JWT_ALG_DEFAULT)
return JWT_VRFY_UNKNOWN_ALG;
diff --git a/src/sample.c b/src/sample.c
index 5abf4712a..008ce04ba 100644
--- a/src/sample.c
+++ b/src/sample.c
@@ -3518,7 +3518,7 @@ static int sample_conv_jwt_verify_check(struct arg *args,
struct sample_conv *co
vars_check_arg(&args[1], NULL);
if (args[0].type == ARGT_STR) {
- enum jwt_alg alg = jwt_parse_alg(args[0].data.str.area,
args[0].data.str.data);
+ enum jwt_alg alg = jwt_parse_alg(ist2(args[0].data.str.area,
args[0].data.str.data));
switch(alg) {
case JWT_ALG_DEFAULT:
--
2.33.1
