Missing error return?

[afshinpir]

Hi

I was going through the code and I noticed this line:

nginx-auth-jwt/src/ngx_http_auth_jwt_module.c

Lines 2052 to 2062 in 72548c2

	if (key) {
	if (jwt_verify_sig(ctx->jwt, (char *) ctx->token, ctx->payload_len,
	(unsigned char *) key, strlen(key)) == 0) {
	ctx->verified = 1;
	return ngx_http_auth_jwt_validate_variable(r, cf, ctx);
	}
	ngx_log_error(NGX_LOG_INFO, r->connection->log, 0,
	"auth_jwt: rejected due to signature validate failure"
	": kid=\"%s\"", kid);
	}

Shouldn't be a return NGX_ERROR; after ngx_log_error here?

Best Regards

[kjdev]

I did not return here because I wanted to process log messages in the event of an error all together.

kid = jwt_get_header(ctx->jwt, "kid");
if (kid) {
  key = ngx_http_auth_jwt_key_get(ctx->keys, kid);
}
if (key) { .. }
json_object_foreach(ctx->keys, kid, var) {
  if (!kid || !json_is_string(var)) { // kid (key) is present, go through
    continue;
  }
  ..
}

ngx_log_error(NGX_LOG_INFO, .. "auth_jwt: rejected due to missing signature key or signature validate failure");
ngx_log_debug(NGX_LOG_DEBUG_HTTP, .. "auth_jwt: token: \"%s\"", (char *) ctx->token);

return NGX_ERROR;

[afshinpir]

The problem that I see is the following scenario:

1. JWT has a valid kid and it can be found in the keys list.
2. Validation of the specific key with that kid fails.
3. Now in the json_object_foreach loop, all keys are checked. If the JWT is verified against another key, this method will succeed.

It means that you have verified JWT with a signature from a key that was different from the key that was specified with kid. I assume this is a buggy situation. If we had a kid in JWT, it means that only that key should verify JWT.

So we need either return inside if (key){} block, or put whole json_object_foreach block inside else statement of if (key){}else{}.

[afshinpir]

After checking code more, I believe code should be like this here:

  if (kid) {
    key = ngx_http_auth_jwt_key_get(ctx->keys, kid);

    if (key) {
      if (jwt_verify_sig(ctx->jwt, (char *) ctx->token, ctx->payload_len,
                         (unsigned char *) key, strlen(key)) == 0) {
        ctx->verified = 1;
        return ngx_http_auth_jwt_validate_variable(r, cf, ctx);
      }

      ngx_log_error(NGX_LOG_INFO, r->connection->log, 0,
                    "auth_jwt: rejected due to signature validate failure"
                    ": kid=\"%s\"", kid);
    }
  }
  else {
    json_object_foreach(ctx->keys, kid, var) {
      if (!kid || !json_is_string(var)) {
        continue;
      }

      key = json_string_value(var);

      if (jwt_verify_sig(ctx->jwt, (char *) ctx->token, ctx->payload_len,
                         (unsigned char *) key, strlen(key)) == 0) {
        ctx->verified = 1;
        return ngx_http_auth_jwt_validate_variable(r, cf, ctx);
      }
    }
  }

[kjdev]

I will consider “valid overwrite with same kid on same directives” test, etc. as it causes an error.

How to handle duplicate kids in the configuration?

[afshinpir]

If you mean duplicate kids in the JWK file, they should be be distinct unless some other parameter makes it distinct:

4.5. "kid" (Key ID) Parameter

The "kid" (key ID) parameter is used to match a specific key. This
is used, for instance, to choose among a set of keys within a JWK Set
during key rollover. The structure of the "kid" value is
unspecified. When "kid" values are used within a JWK Set, different
keys within the JWK Set SHOULD use distinct "kid" values. (One
example in which different keys might use the same "kid" value is if
they have different "kty" (key type) values but are considered to be
equivalent alternatives by the application using them.) The "kid"
value is a case-sensitive string. Use of this member is OPTIONAL.
When used with JWS or JWE, the "kid" value is used to match a JWS or
JWE "kid" Header Parameter value.

But if you just want to check for duplicate kids anyway, you can write a code like this probably which handles with/without kid in JWT:

requestkid = kid;
json_object_foreach(ctx->keys, kid, var) {
  if (!kid || !json_is_string(var)) {
     continue;
  }

  if (requestkid && strcmp(kid, requestkid)!=0) {
    continue;
  }

  key = json_string_value(var);

  if (jwt_verify_sig(ctx->jwt, (char *) ctx->token, ctx->payload_len,
                         (unsigned char *) key, strlen(key)) == 0) {
    ctx->verified = 1;
    return ngx_http_auth_jwt_validate_variable(r, cf, ctx);
  }
}
if (requestkid) {
  ngx_log_error(NGX_LOG_INFO, r->connection->log, 0,
                "auth_jwt: rejected due to signature validate failure"
                ": kid=\"%s\"", requestkid);
}

BTW, I didn't get what you mean exactly by:

I will consider “valid overwrite with same kid on same directives” test, etc. as it causes an error.

[kjdev]

Not checking for kid on JWT request.

How to handle the process when a key (ctx->keys) as a validation target is captured (auth_jwt_key_file or auth_jwt_key_request)
Overwritten when imported with keyval specified (later setting has priority).
Include all when imported with jwks specified.

I wonder if it would be possible to override the jwks specification as well, so that it would be handled as expected.

[kjdev]

For example, if the configuration is as follows

auth_jwt_key_file test1.jwks; # {kid:test1, k:secret1}
auth_jwt_key_file test2.jwks; # {kid:test1, k:secret2}

The ctx->keys data would look like this

ctx->keys # {test1: secret2, thumbprint1: secret1, thumbprint2: secret2}

[kjdev]

I'm trying to allow duplicate verification key (jwks) kid so I think it should be unique

I wonder if it would be better to be able to switch between allowing duplicates and uniqueness in the configuration file.