Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combined tokens / password access #2580

Closed
deep9 opened this issue Mar 15, 2021 · 5 comments
Closed

Combined tokens / password access #2580

deep9 opened this issue Mar 15, 2021 · 5 comments

Comments

@deep9
Copy link
Contributor

deep9 commented Mar 15, 2021

After update to 0.10.10 the streaming connection is not working.

I have api_secret enabled as it allows for token issue, which is then used for user connection.

Before the update it was working fine, now it seems that if the api_secret is set, it needs to be used with the token.

Strange behavior is that the POST request will work, but the GET will return 403.

I believe the error is located here (

if(!secret_authorized || !token_authorized) {
), because similar logic is used in here (
if(!secret_authorized && !token_authorized)
) and the logic is different (I believe working one, according to the description).

Also, the POST request will eventually end up here (

static int janus_request_check_secret(janus_request *request, guint64 session_id, const gchar *transaction_text) {
) where the logic is different.

It was "fixed" in this commit (16173af)

I've read the issues and I don't think the fix is valid.

The proper one should be to still check if the secret/token is needed before setting the value to true.

I don't know C that much to create a PR, but if that is still needed, I can try to do so.

@deep9
Copy link
Contributor Author

deep9 commented Mar 15, 2021

Something like this

if(!(secret_authorized && gateway->is_api_secret_needed(&janus_http_transport)) && !(token_authorized && gateway->is_auth_token_needed(&janus_http_transport))) {
				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;
			}

@deep9
Copy link
Contributor Author

deep9 commented Mar 15, 2021

So, based on the findings, I think it would maybe make sense to create one validation function in the gateway (janus.c) and call it from those 2 places to prevent errors :)

@deep9
Copy link
Contributor Author

deep9 commented Mar 15, 2021

So, I wanted to test it, there is a working version here, not sure if it is correct language wise.
https://github.com/deep9/janus-gateway/tree/hotfix/token-secret-auth

@lminiero
Copy link
Member

The logic in the HTTP transport plugin was changed on purpose, since there was an issue documented in #2520 and fixed in #2524, which is indeed the commit you referenced to. If you think you have a fix for this, please prepare a pull request rather than linking to a branch, as that would force me to look for the changes manually.

@lminiero
Copy link
Member

Closing as your PR has been merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants