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

Fix signed token auth not work on join to the videoroom #2810 #2812

Merged
merged 2 commits into from
Nov 23, 2021

Conversation

timsolov
Copy link
Contributor

@timsolov timsolov commented Nov 23, 2021

Fixes #2810

@januscla
Copy link

Thanks for your contribution, @timsolov! Please make sure you sign our CLA, as it's a required step before we can merge this.

@timsolov
Copy link
Contributor Author

Thanks for your contribution, @timsolov! Please make sure you sign our CLA, as it's a required step before we can merge this.

signed

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added a few notes inline.

auth.c Show resolved Hide resolved
auth.h Outdated
@@ -42,7 +42,7 @@ gboolean janus_auth_check_signature(const char *token, const char *realm);
* @param[in] token The token to validate
* @param[in] realm The token realm
* @param[in] desc The descriptor to search for
* @returns TRUE if the token is valid, not expired and contains the descriptor, FALSE otherwise */
* @returns When the token based authentication enabled: TRUE if the token is valid, not expired and contains the descriptor, FALSE otherwise. When the token based authentication disabled: always TRUE. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say "is enabled", rather than just "enabled", in both additions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -1162,6 +1162,7 @@ room-<unique room ID>: {

#include "plugin.h"

#include <string.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@@ -2997,10 +2998,13 @@ static int janus_videoroom_access_room(json_t *root, gboolean check_modify, gboo
/* signed tokens bypass pin validation */
json_t *token = json_object_get(root, "token");
if(token) {
char room_descriptor[26];
char room_descriptor[100];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this the actual cause of the issue? A string that was too small, and so was only containing a partial representation of the required token causing comparisons and checks to fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This affects on checking rooms with id in UUID format, the description have to be: room=3edb0cdd-7173-481d-82f0-5eb31937d20f but when it has only 26 characters it's looks like: room=3edb0cdd-7173-481d-82 and when we do strcmp we have not equal result.

if(gateway->auth_signature_contains(&janus_videoroom_plugin, json_string_value(token), room_descriptor))
return 0;
if(!gateway->auth_signature_contains(&janus_videoroom_plugin, json_string_value(token), room_descriptor)) {
g_snprintf(error_cause, error_cause_size, "Unauthorized (wrong token %s)", token);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing the if(error_cause) the other error management lines of code have, as error_cause may be NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@lminiero
Copy link
Member

lminiero commented Nov 23, 2021

Thanks for the quick fixes! This looks good to me, so I'll merge 👍
Edit: will let the CI make some tests too, as I see it hadn't been started automatically.

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

Successfully merging this pull request may close these issues.

Signed token authentication doesn't work well on join to the room.
3 participants