v4.0.0 include identity by default #88
Conversation
* include identity in computation by default * nimbus test overrides routines to exclude identity * improved documentation in README.md fixes #87
| ## Notes | ||
| ## Identity and Nimbus implementation | ||
|
|
||
| This package's default configuration matches SRP6a: the identity is included in the verifier generation. This makes it impossible to detect if [two users share the same password](https://crypto.stackexchange.com/questions/8626/why-is-tls-srp-verifier-based-on-user-name/9430#9430) but also does not allow a client to change its "identity" without regenerating password. |
There was a problem hiding this comment.
Are there more vulnerabilities because of that or this is the only known one?
There was a problem hiding this comment.
I think it's the only known problem.
| } | ||
| public async computeClientEvidence( | ||
| _I: string, | ||
| _s: bigint, |
| public async computeIdentityHash(_: string, P: string): Promise<ArrayBuffer> { | ||
| return await this.hash(stringToArrayBuffer(P)); | ||
| public async computeIdentityHash(I: string, P: string): Promise<ArrayBuffer> { | ||
| return await this.hash(stringToArrayBuffer(I), stringToArrayBuffer(P)); |
There was a problem hiding this comment.
RFC adds ':' between identity and password, I think:
SHA1(I | ":" | P)
There was a problem hiding this comment.
ok let's follow that
| A: bigint, | ||
| B: bigint, | ||
| S: bigint, | ||
| ): Promise<bigint> { | ||
| return arrayBufferToBigInt( | ||
| await this.hash( | ||
| stringToArrayBuffer(I), |
There was a problem hiding this comment.
WHy is this needed? I think RFC only says hash A and B values for evidence. Notice that we can use any known to both client and server values here without impacting anything.
There was a problem hiding this comment.
I don't really know, so I will change it back.
|
Looks good to me. I personally like better version without identity in the verifier and I think recreate rfc is just one line change as last line of README points out :) What shall I read to get really scared about this? |
|
If we decide to implement this, considering it will break all implementations of the default library, we should probably provide a note about how to make it compatible with v3.0 in the README.md. Also I am concerned about how this would effect implementers of this library. Say I have a database of salts and verifiers. I see there is an update for this library related to security, so I decide to update and now none of my users can login. So now I have three non optimal options
Maybe we should check how other SRP implementations (cross language) deal with this. Do they include the identity? |
|
Hey, Josh, I don't see major issues with upgrade. I hope you never change version of dependency and rely on users to detect some issues :) To be backward compatible we just need to redefine verifier routine back to v3.0. This is already well documented in README I think. Does it make sense? |
Not sure what you mean. Users are simply free QA testers! ;) In seriousness, I think this is a major breaking change and if included should be more clear in the README. Maybe something towards the top: "Versions <4.0 did not include the identity for some operations...See below for how to make it compatible with versions <4.0." Also I think some of the variables in this section should probably be renamed: https://github.com/midonet/tssrp6a/tree/include_identity#usage |
|
hi, first of all thank you for all you do. But we shouldn't stop here forever. I think "can change the identity" is a bad design. We should assume that a well-designed system should not use things like the user's email as the identifier. Just imagine, currently my website uses user email as id, one day in the future I need to add phone number login option, I need to use another id to associate them. Likewise, if I need to provide users with a publicly accessible home page, like Twitter, I need to expose the user's private data in a URL, like this: mysite.com/user_email/post_id In fact, changing the identity requires no less work than changing the verifier. Going back to the question itself, I think we should release a major version, exporting two classes, one that includes identity in computation by default, and one that doesn't. We can release another major version at some point in the future, replace the old implementation with the new implementation. But I'm not sure if this is a good idea. React makes breaking changes in a similar way. Here is one of the examples: // version < 16
type FC<P = {}> = P & {
children: ReactNode
}
// version 16 - 18
type FC<P = {}> = P & {
children: ReactNode
}
type VFC<P = {}> = P & {
children?: never
}
// version >= 18
type FC<P = {}> = P
// VFC has been removed. |
Hi @wise1994 , thanks for the input! I think that the issue 'user-id =~ e-mail' is ortogonal to the issue 'cannot change username preserving password'. Does it make sense? |
fixes #87
@bufistov please share your thoughts :)