Fix auth when both (token, secret) modes are enabled

[deep9]

Fix for #2580

[lminiero]

Thanks, added some notes inline.

[lminiero]

I'm not sure that's needed. It's easier to change the defaults above, e.g.:

gboolean secret_authorized = (api_secret == NULL), token_authorized = !janus_auth_is_enabled();

This way the checks are simplified, because what's disabled automatically resolves to TRUE. This would also allow us to remove the Nothing to check branch entirely and only do something if either mechanism is needed.

[deep9]

I think this will not work, because the problem is in the function is_api_secret_valid - if it is disabled, it will return TRUE and we are back where we were before.

So the code might look like this:

	gboolean secret_authorized = (api_secret == NULL), token_authorized = !janus_auth_is_enabled();
	if(secret_authorized && token_authorized) {
		/* Nothing to check */
	} else {
		json_t *root = request->message;
		if(!secret_authorized) {
			/* There's an API secret, check that the client provided it */
			json_t *secret = json_object_get(root, "apisecret");
			if(secret && json_is_string(secret) && janus_strcmp_const_time(json_string_value(secret), api_secret)) {
				secret_authorized = TRUE;
			}
		}
		if(!token_authorized) {
			/* The token based authentication mechanism is enabled, check that the client provided it */
			json_t *token = json_object_get(root, "token");
			if(token && json_is_string(token) && janus_auth_check_token(json_string_value(token))) {
				token_authorized = TRUE;
			}
		}
		/* We consider a request authorized if either the proper API secret or a valid token has been provided */
		if(!secret_authorized && !token_authorized)
			return JANUS_ERROR_UNAUTHORIZED;
	}

[lminiero]

Good point, makes sense 👍

[lminiero]

Actually, thinking about it my notes were just wrong... the defaults should NOT be TRUE, because it means that if only one mechanism is enabled, it will always be bypassed by the TRUE on the other, disabled, mechanism (e.g., tokens in use but no API secret, wrong token still passes the check thanks to secret_authorized being TRUE).

I think that was the key reason why we did that change in #2524.

[deep9]

You are right, I did not think about that much when implementing the notes :) The previous solution works fine, even though it is a bit more complicated.

[lminiero]

Same here: if defaults for secret_authorized and token_authorized are set to TRUE if the related mechanism is disabled, then the checks are simpler. In this case, I agree the check below must get back to an && rather than an || as in the core.

[deep9]

So the code based on the suggestion might look like

		gboolean secret_authorized = !gateway->is_api_secret_needed(&janus_http_transport), token_authorized = gateway->is_auth_token_needed(&janus_http_transport);
		if(secret_authorized && token_authorized) {
			/* Nothing to check */
		} else {
			if(!secret_authorized && gateway->is_api_secret_valid(&janus_http_transport, secret)) {
				/* API secret is valid or disabled */
				secret_authorized = TRUE;
			}
			if(!token_authorized && gateway->is_auth_token_valid(&janus_http_transport, token)) {
				/* Token is valid or disabled */
				token_authorized = TRUE;
			}
			/* We consider a request authorized if either the proper API secret or a valid token has been provided */
			if(!secret_authorized && !token_authorized) {
				response = MHD_create_response_from_buffer(0, NULL, MHD_RESPMEM_PERSISTENT);
				janus_http_add_cors_headers(msg, response);
				ret = MHD_queue_response(connection, MHD_HTTP_FORBIDDEN, response);
				MHD_destroy_response(response);
				goto done;
			}
		}

[lminiero]

Thanks for the additional fixes! I just restarted the automated checks, as they failed for your build due to issues unrelated to the code. I'll make a few tests to see if they work in the different configurations.

[lminiero]

Seems to be working fine 👍
@deep9, can you sign our CLA? Apparently the bot didn't ask you for some reason (hope it didn't break), but I don't see your ID in the list. That's a prerequisite for merging, thanks!

[lminiero]

Ok, I see the signature now: merging then, thanks!