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

JWT Sign Type Fixing (#1771) #1776

Merged
merged 11 commits into from
Apr 8, 2024

Conversation

DevelopingEntitiesWithFuntations
Copy link
Contributor

Added and exposed the required env variable.

Added env variable to the jitsi-meet.cfg.lua and referenced it below
Added the possibility of an Env variable "JWT_SIGN_TYPE" in docker-compose.yml
prosody/rootfs/defaults/conf.d/jitsi-meet.cfg.lua Outdated Show resolved Hide resolved
prosody/rootfs/defaults/conf.d/jitsi-meet.cfg.lua Outdated Show resolved Hide resolved
Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

I'm not sure "null" as a string will work, we need to check that.

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

Ok, please do.

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

Is possible to run a CI/CD pipeline to test that?

@saghul
Copy link
Member

saghul commented Apr 4, 2024

Alas there is no CI for that :-/

Your change is not going to work though. It will set it to the string "null". And this is what the module code does: https://github.com/jitsi/jitsi-meet/blob/38be09fc5486979ad554d834702cabbaf5062cc0/resources/prosody-plugins/token/util.lib.lua#L100

So I think the way forward is to only set that config option is actually set, and provide no default.

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

@saghul what if we take nil instead of null? 🤔

@saghul
Copy link
Member

saghul commented Apr 4, 2024

You still have the quotes problem.

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

@saghul then remove the quotes?...

Alternatively, perhaps error out if no value is set otherwise continue?

@saghul
Copy link
Member

saghul commented Apr 4, 2024

Just add a condition and set it to whatever value the env var has, if it's set.

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

Let me get this straight - so I need to open up another PR on the Jitsi Meet GitHub to fix this and after that is merged you may merge this PR afterwards?

@saghul
Copy link
Member

saghul commented Apr 6, 2024

No, you can fix it right here. Just do (pseudocode): if .Env.FOO then the_setting = {{ .Env.FOO }}

So don't set it unconditionally, thus preserving the default value, if the user didn't set it.

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

Don't if conditions require unnecessary computation power and should be avoided for such cases?

I think I did what you wanted - {{ $JWT_SIGN_TYPE := .Env.JWT_SIGN_TYPE | default "RS256" -}}

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

Or you want smth like?:

{{- if .Env.JWT_SIGN_TYPE }}
{{ $JWT_SIGN_TYPE := .Env.JWT_SIGN_TYPE }}

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

or you want an or case with a default value?

@saghul
Copy link
Member

saghul commented Apr 6, 2024

No. DONT declare the auxiliary variable now, you can just if the declaration of the actual prosody variable.

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

I don't quite get you. Like this on Line 138 (my version)?:

{{ if .Env.JWT_SIGN_TYPE }}
    signature_algorithm = "{{ $JWT_SIGN_TYPE }}"
else
    signature_algorithm = "RS256"

@saghul
Copy link
Member

saghul commented Apr 6, 2024

Almost. Don't set signature_algorithm at all if the env car is not defined.

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

So just the first two lines from the example above then?:

{{ if .Env.JWT_SIGN_TYPE }}
    signature_algorithm = "{{ $JWT_SIGN_TYPE }}"

@saghul
Copy link
Member

saghul commented Apr 6, 2024

Yes!

@DevelopingEntitiesWithFuntations
Copy link
Contributor Author

Please review this and merge if it's correct ^^

@saghul saghul merged commit e939230 into jitsi:master Apr 8, 2024
1 check passed
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.

None yet

2 participants