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

Configure the JWT Claim Key for holding MediaMTX Permissions #3560

Closed

Conversation

izanbard
Copy link

@izanbard izanbard commented Jul 18, 2024

Why is this change needed?

I offer this change as when using Azure B2C the policy for naming custom JWT claims is to prefix with extension_<<app_id>>_. With MediaMTX having a fixed JWT of mediamtx_permissions it meant that I could not use Azure B2C as my IdP.

This will be the case for many people who need to secure video streams using JWTs.

What has been done?

The main changes are in manager.go:268-283 and conf.go:568-573.

  • manager.go: I have intercepted the automagic un-marshalling of the JWT in to customClaims object and done it manually instead. This means that the key used to search the claim for the mediamtx_permissions can be set at runtime rather than at compile time.
  • conf.go: in addition to setting the default for the new config key (authJWTClaimKey) to be mediamtx_permissions for backward compatibility, I have also added some validation to ensure the claim key is not blank and that it only has valid JWT chars in it [a-zA-Z0-9-_].

I have updated unit tests and made simple changes to the README.md and default mediamtx.yml files.

izanbard added 2 commits July 18, 2024 14:27
This change adds functionality to be able to choose the JWT clim key that is used to store the mediamtx_permissions.  This is useful if you do not have unlimited access to your IdP and there is a policy that your JWT extension claims should be of a specific format - I am looking at you, Azure B2C.
@izanbard izanbard marked this pull request as ready for review July 18, 2024 13:52
@izanbard
Copy link
Author

izanbard commented Aug 2, 2024

@aler9 The test above that has failed is nowhere near the code I have written; passes on my system and is similar to a fail you had yesterday that went away on retry. Please rerun the check.

@izanbard
Copy link
Author

izanbard commented Aug 2, 2024

@aler9 thank you

Copy link
Member

@aler9 aler9 left a comment

Choose a reason for hiding this comment

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

Hello, this feature is useful but you need to perform a couple of changes in order to get the patch merged:

  • the permission parsing logic is vulnerable to multiple crashes. In my opinion you should rewrite it entirely by using something like
type customClaims map[string]json.RawMessage
var cc customClaims
_, err = jwt.ParseWithClaims(v["jwt"][0], &cc, keyfunc)
...

rawClaim, ok := cc[m.JWTClaimKey]
if !ok {
   return fmt.Errorf(...)
}

var claim []conf.AuthInternalUserPermission
err := json.Unmarshal(rawClaim, &claim)
if err != nil {
   return err
}

// finally you can use the claim
  • you need to add the new key to apidocs/openapi.yaml too

internal/auth/manager.go Outdated Show resolved Hide resolved
internal/auth/manager.go Outdated Show resolved Hide resolved
extending and correcting the openapi schema
…diamtx into rekey-jwt-claim

# Conflicts:
#	apidocs/openapi.yaml
@izanbard izanbard requested a review from aler9 August 6, 2024 11:50
@izanbard
Copy link
Author

izanbard commented Aug 6, 2024

@aler9 I have made some changes to the code to protect against nils etc.

I have stuck with jwt.Parse as this will put all the claims into token.Claims as a jwt.MapClaims which I can then index at run time for the config set claim key. Using jwt.ParseWithClaims assumes that the passed in customClaims object is an implementation of the jwt.Claims interface. The suggestion to pass in a map[string]json.RawMessage, thus, is not appropriate. However I can, and have, marshalled the claim in json and then subsequently un-marshalled it out to in to the []conf.AuthInternalUserPermission

@aler9
Copy link
Member

aler9 commented Aug 26, 2024

i found a way to implement this feature that is slightly more efficient, since it avoids calling json.Marshal and json.Unmarshal in series. This has been moved here: #3692

Copy link
Contributor

This issue is mentioned in release v1.9.0 🚀
Check out the entire changelog by clicking here

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.

2 participants