Basic EXTJWT support #948
Basic EXTJWT support #948
Conversation
|
Yeah any help on the mod side would be appreciated, I'm not quite sure what's going on there but I get the feeling that latest commit didn't help aha. |
DanielOaks
force-pushed
the
basic-extjwt-support
branch
from
e84695d to
102e260
Compare
|
note to self: updating go to the latest version is useful sometimes. cool in that case I think this does everything I want it to, happy for review~ |
| @@ -35,6 +35,7 @@ type Channel struct { | |||
| key string | |||
| members MemberSet | |||
| membersCache []*Client // allow iteration over channel members without holding the lock | |||
| memberJoinTimes map[*Client]time.Time | |||
There was a problem hiding this comment.
Ugh --- there must be some reason this is necessary, but I can't figure out what it is yet. I asked a question about this on the spec PR. It would nice if we didn't need this.
There was a problem hiding this comment.
I discussed this with prawn and it sounds like we can just send 1 as the join timestamp (indicating "the client is joined, but the join timestamp is unavailable"); we can probably wait a couple days and then remove this tracking.
There was a problem hiding this comment.
Once the spec is updated we can handle this however I guess, no worries
There was a problem hiding this comment.
The spec got updated to allow 1 here.
| } | ||
| } | ||
|
|
||
| // we default to a secret of `*`. if you want a real secret setup a service in the config~ |
There was a problem hiding this comment.
If you provide a service name (like jitsi-call or acme-image-hosting) then it'll use the secret defined in that service's config block. By default it just uses a secret of * because there's really no reason to have a default secret that's customised (anything custom will just use a service name anyways).
There was a problem hiding this comment.
Are we sure signing with a secret of * is correct? I found people saying that if there is no secret one should simply omit the signature: https://riptutorial.com/jwt/example/18558/unsigned-jwt
There was a problem hiding this comment.
Yes, we should not be mixing and matching with alg: none because of the very frequent issues that alg:none has caused in the past. I'm not sure whether using * in particular is correct, but if the spec recommends using none rather than hmac then it is a horrendous decision honestly.
There was a problem hiding this comment.
I'm not 100% clear on what's going on here but I have a really strong intuition that using a secret key of * is incorrect.
If I'm reading the spec correctly, we should be requiring the operator to specify a default key of some kind (which should be a genuine secret and not *), and if they don't we should disable JWT (the command and the ISUPPORT token).
Should we ask for clarification on the spec PR?
There was a problem hiding this comment.
The spec should clarify this, yeah. There should not be any way to have a custom default secret, any custom secret should be provided by using a service name. So we should probably remove this and only advertise JWT if there is at least one service configured.
There was a problem hiding this comment.
We got the clarification we wanted:
- The JWT header contains the signature algorithm, and the EXTJWT spec allows the use of any signature algorithm that can be verified. So I think what we have is fine. In the future, we could add, e.g.,
RSAPrivateKeyto theJWTServiceConfig, and then the EXTJWT handler could sign with RSA instead if that field is populated. - We should in fact have a default secret (and EXTJWT should be disabled if one is not specified).
There was a problem hiding this comment.
- So long as we never sign anything using the None algorithm, cool. Yeah could definitely add that key entry to the config in future no worries.
- But that... doesn't answer our question. Should the default be static (same on all nets, same everywhere, specified in the specification, etc) or should the default be decided by the network and it's totally OK if clients can't actually verify it? Prawn's response indicates that this is intended for use by clients that don't actually know anything about the ircd setup as well so would having a configurable default actually make sense there or would it e.g. screw up clients trying to just decode data (for example do any client libraries out there intentionally not decode stuff if the user doesn't have the correct verification key?)
There was a problem hiding this comment.
should the default be decided by the network and it's totally OK if clients can't actually verify it
I'm very confident that this is the correct answer. Clients do not parse/validate the JWT token themselves, they hand it off to a third party. That third party is assumed to hold the correct verification key. (The original use case for this is a closed ecosystem of an ircd and a Jitsi, controlled by the same operator.)
A "secret key" that is a constant value written into a specification just doesn't make sense.
| if err == nil { | ||
| maxTokenLength := 400 | ||
|
|
||
| for maxTokenLength < len(tokenString) { |
|
I rebased this and it is now #1136. |
Spec is laid out over here (the whole URL verification thing is probably getting removed so don't worry about that at all). We currently don't store the time when users join the channel so the
joinedclaim is just faked. For some reason my Go also removedgithub.aaakk.us.kg/goshuirc/e-nfafrom thego.modbut not sure if that's okay because it's marked as indirect? ¯\_(ツ)_/¯Either way this does the basics, let us know if you find anything wrong with this in the meantime.
Todos