xsf / xeps

Hosts the markup for all XMPP Protocol Extensions.
https://xmpp.org/extensions/
Other
125 stars 121 forks source link

XEP-0388: Rework whole spec, namespace bump #1214

Closed tmolitor-stud-tu closed 1 year ago

tmolitor-stud-tu commented 1 year ago

This is a collaboration between @mwild1 and me. Most parts have already been discussed with the original author @dwd, too.

In short, this adds:

horazont commented 1 year ago

@dwd Ack please?

tmolitor-stud-tu commented 1 year ago

For reference, here is the rendered version: https://dyn.eightysoft.de/final/xep-0388.html

Flowdalic commented 1 year ago

For reference, here is the rendered version: https://dyn.eightysoft.de/final/xep-0388.html

It would probably help to have a diff from the current version. Especially for an upcoming list disccusion. I usually create HTML diffs for XEPs using htmldiff.

mwild1 commented 1 year ago

I generally agree with @dwd's points. The main thing I'll defend is having <user-agent> in here.

Honestly, I spent the past 5 years somehow thinking SASL2 already had a client identifier in it. I guess I made that up somewhere along the way, but I do think it makes sense. It's not the end of the world if it's in an separate XEP, but it's a solution to so many problems, and necessary for some SASL mechanisms (e.g. HT-*) because they don't provide any other key via which the server can determine which credentials it should be comparing against (maybe that's something to fix in those mechanisms, but there are other use cases too, including just debugging and diagnostic purposes).

As for XEP-0198 integration, the intent is not to define it in this spec, merely to say it's possible. It's covered by PR #1215 updating XEP-0198 itself. I prefer that PR over XEP-0397, as it reuses more of the existing protocol and fulfils more use cases without involving any extra stuff (e.g. token auth mechanisms).

Anyway, this should all be on the mailing list. If someone sparks a thread, I'll be happy to ^C^V everything there. If nobody does, I'll start one soon.

tmolitor-stud-tu commented 1 year ago

There's a lot of changes to digest here, beyond even those we discussed (and I reserve my right to change my mind even when we did discuss!).

Sure! I did not intend to imply that our discussionwas something like a pre-approval. I just wanted to note that you are informed that I was working on this XEP. Of course you can change your mind any time.

I don't think this is sensible to publish yet, and certainly not without extensible discussion on the list.

I think the way forward if to address some of your issues we don't need to discuss about and leave the rest for an open discussion on the list. Does this seem sensible to you?

Quick warning: I'm really short at time currently. Several days may pass until I'm able to answer.

tmolitor-stud-tu commented 1 year ago

I marked everything addressed in my last commits as "solved" to redirect the focus to unresolved things. Feel free to "unresolve" things if you see the need for additional changes and/or start a discussion about this on-list.

tmolitor-stud-tu commented 1 year ago

First rendered version of the split-out SCRAM upgrade tasks ProtoXEP: https://dyn.eightysoft.de/xeps/xep-scram-upgrade.html

tmolitor-stud-tu commented 1 year ago

lgtm

current rendered version: https://dyn.eightysoft.de/final/xep-0388.html current diff: https://dyn.eightysoft.de/final/xep-0388-diff.html

@mwild1 lgtm, too?

tmolitor-stud-tu commented 1 year ago

It's more that if you're using a multi-trip mechanism like SCRAM, then 0-RTT is fine for sending the SCRAM initial response, and it's no more or less secure. An attacker can at best repeat the client nonce, but that's hardly an issue. The problem is more likely to be that channel binding is trickier.

We just did not want to go down this rabbit hole and rather leave the 0-RTT stuff to be weakened and defined in more detail by other/future XEPs: It might work for SCRAM, but there might be other/future SASL mechanisms that would be more vulnerable if 0-RTT was used. Going the "default deny" route rather than the "default allow" seems like a sensible thing to do here.

tmolitor-stud-tu commented 1 year ago

You're presumably implying that if TLS is negotiated, the risk due to the first three points is mitigated. I'd state that explicitly.

I still don't understand what "3 points" you are referring to.

dwd commented 1 year ago

It's more that if you're using a multi-trip mechanism like SCRAM, then 0-RTT is fine for sending the SCRAM initial response, and it's no more or less secure. An attacker can at best repeat the client nonce, but that's hardly an issue. The problem is more likely to be that channel binding is trickier.

We just did not want to go down this rabbit hole and rather leave the 0-RTT stuff to be weakened and defined in more detail by other/future XEPs: It might work for SCRAM, but there might be other/future SASL mechanisms that would be more vulnerable if 0-RTT was used. Going the "default deny" route rather than the "default allow" seems like a sensible thing to do here.

You're not doing "default deny", you're doing "deny".

dwd commented 1 year ago

You're presumably implying that if TLS is negotiated, the risk due to the first three points is mitigated. I'd state that explicitly.

I still don't understand what "3 points" you are referring to.

The first three points made in this section, about additional exposure and so on.

tmolitor-stud-tu commented 1 year ago

It's more that if you're using a multi-trip mechanism like SCRAM, then 0-RTT is fine for sending the SCRAM initial response, and it's no more or less secure. An attacker can at best repeat the client nonce, but that's hardly an issue. The problem is more likely to be that channel binding is trickier.

We just did not want to go down this rabbit hole and rather leave the 0-RTT stuff to be weakened and defined in more detail by other/future XEPs: It might work for SCRAM, but there might be other/future SASL mechanisms that would be more vulnerable if 0-RTT was used. Going the "default deny" route rather than the "default allow" seems like a sensible thing to do here.

You're not doing "default deny", you're doing "deny".

But that "deny" could be overwritten by a later xep, no?

tmolitor-stud-tu commented 1 year ago

You're presumably implying that if TLS is negotiated, the risk due to the first three points is mitigated. I'd state that explicitly.

I still don't understand what "3 points" you are referring to.

The first three points made in this section, about additional exposure and so on.

Ah, I understand. Fixed here: https://github.com/xsf/xeps/pull/1214/commits/18fb29343be4c8ada47bb23450d2c08ce2e46afe

Neustradamus commented 1 year ago

To follow

tmolitor-stud-tu commented 1 year ago

@Kev this isn't "needs author" anymore, Dave already confirmed this on standards@

@dwd can you confirm this here again?

tmolitor-stud-tu commented 1 year ago

@dwd I'm sorry, I can't find your mail at standards@ confirming this can be merged. Am i hallucinating?

tmolitor-stud-tu commented 1 year ago

@Kev This is not NeedsAuthor anymore, but ReadyToMerge!

See here: https://mail.jabber.org/pipermail/standards/2023-January/039133.html

guusdk commented 1 year ago

@tmolitor-stud-tu I believe you can make the XEP validation happy if you reverse the order of lines 15 and 16. <sig> should go before <approver>, not the other way around.

Update: I was allowed to and have pushed a commit with this change to your branch.

tmolitor-stud-tu commented 1 year ago

Update: I was allowed to and have pushed a commit with this change to your branch.

Perfect, thanks!

guusdk commented 1 year ago

XEP state is currently 'deferred'. Changes are not purely editorial. As per https://github.com/xsf/xeps/blob/master/docs/TRIAGING.md this requires a move to 'experimental' state.

guusdk commented 1 year ago

In an attempt to move this forward (and after consultation in editors@) I've added a commit that changes the status from the XEP from Deferred to Experimental.

Kev commented 1 year ago

In an attempt to move this forward (and after consultation in editors@) I've added a commit that changes the status from the XEP from Deferred to Experimental.

Consultation, but then sadly not doing what I asked. I'll try to unravel this when I have a chance.

guusdk commented 1 year ago

not doing what I asked

I blame poor instructions :)

Should be fixed now.