Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[WIP] MSC3262 aPAKE authentication #3262
base: old_master
Are you sure you want to change the base?
[WIP] MSC3262 aPAKE authentication #3262
Changes from 14 commits
d5f3e93
f218050
fc4f4b1
86e4a6e
8b578fa
5696f28
9e91cf3
472fc9c
7a9f20c
7e33725
2c4702c
a7fecfa
71036d6
51144ab
3ba3262
8aabf4c
decfda0
df21803
eab94a1
6765a68
0da7675
f9d17d6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This raises an interesting question about how the Matrix user-interactive auth is supposed to work.
For a stage that requires multiple rounds of communication, should the
type
be the same for every request? ie, here it would bem.login.srp6a
for both init and verify, and the details would go in theauth
object.Or should each "round" of communication be its own UIA stage? In that case, it seems like the
flows
returned byGET /_matrix/client/r0/login
should include both stages, ie["m.login.srp6a.init", "m.login.srp6a.verify"]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flow only has one
GET
request at the very start, so returning justm.login.srp6a
seems like it's sufficient to me, supporting justinit
or justverify
makes little sense so I would argue that's implied.If we compare against the webrtc spec (which also has multiple stages) I think using
m.login.srp6a.init
andm.login.srp6a.verify
in POST to/login
makes sense. (webRTC in matrix usem.call.invite
,m.call.candidates
,m.call.answer
andm.call.hangup
). We defined anauth_id
in this MSC because we do need to keep track of the flows, you can'tverify
before having done theinit
.Then again: srp can map fairly well on the UIA, but that's a seperate MSC and I kept that out of scope here because there's no reason either this msc or the UIA msc needs to depend one another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh, I think I was getting confused between the UIA vs normal login. I thought at some point they were going to all use the UIA? Or maybe that was just a proposal that I saw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so too, I may have missed that the proposal got merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course this MSC actually needs to support UIA, because authentication doesn't only happen at
/login
so this is added now. It's more or less a duplicate of/login
except all SRP json stuff in put inside anauth
object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a malicious server starts reporting that it no longer supports SRP or that the client does not have SRP configured? It will take some really good UX design by clients to ensure that the client doesn't fall back to a regular login and leak the password to the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a big one, for quite a while we won't be able to assume the server has no access to the plaintext password. I would imagine that the long-term solution is to remove the
password
authentication from the matrix spec, and declare all apps still supporting the old password login as incompatible.I added some thoughts about this. Do you have any additions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is fine. I don't think there is way to enforce it yet. Maybe add a note that clients can consider indicating when SRP is/isn't used. Maybe that indication would even take a while to be enabled by default but it provides a nice path away from sending the password to the server.
Of course we shouldn't mandate client behaviour, however I think it is often helpful to explain what it could look like.