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

User creation and authentication #25

Merged
merged 29 commits into from
Apr 22, 2020
Merged

User creation and authentication #25

merged 29 commits into from
Apr 22, 2020

Conversation

icidasset
Copy link
Contributor

Adds the following functions:

  • createAccount
  • didJWT
  • didKey
  • isUsernameAvailable
  • updateRoot

publish.sh Outdated Show resolved Hide resolved
@icidasset icidasset marked this pull request as ready for review April 15, 2020 20:53
@icidasset icidasset requested a review from dholms April 16, 2020 21:13
src/common.ts Outdated Show resolved Hide resolved
src/user/identity.ts Outdated Show resolved Hide resolved
@dholms
Copy link
Contributor

dholms commented Apr 17, 2020

Nice this looks really good 👍

I think a few shortcomings in keystore-idb are putting more crypto code here than there really needs to be. I'll see about making those updates soon, shouldn't be too much work

We should still allow byte operations in the keystore-idb interface, but we actually are working with strings here so it's not necessary for this.

So yeah in summary:

  • move isRSAKeystore function
  • simplify signatures by using keystore interface methods

And this should be good to go 👍


For future reference, if you have to really dig into the internals like this:

  const signed = await operator.signBytes(
    utils.strToArrBuf(`${encodedHeader}.${encodedPayload}`, 8),
    ks.writeKey.privateKey,
    ks.cfg.hashAlg
  )

then it probably means either:

  • the abstraction is off and should be fixed in the library
  • the library isn't being used quite right and there's probably a better way to do it

these libraries are made by us & primarily for us, so if something seems clunky, ping me and we can change it at the library level 👌

function jwtAlgorithm(cryptoSystem: CryptoSystem): string | null {
switch (cryptoSystem) {
case CryptoSystem.ECC: return 'Ed25519';
case CryptoSystem.RSA: return 'RS256';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dholms I just did this instead of adding this isRSA and isECC methods. Is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that's great 👍
Except... WebCrypto doesn't support Ed25519, it uses NIST Curves (P-256, P-384, P-521). Sooo if we want to support those, we'll need to figure out magic bytes & what not for them on the server...

We may want to have CryptoSystem.ECC return null until we get that set up server-side. The ts-sdk always generates RSA keys unless the user overrides it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohhh ok, woops. I assumed ECC was the same as ED 😅 Yeah, we should discuss this later. In the meantime, will remove the ecc/ed stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah too many acronyms, it's all so confusing 😅

ED is a type of ECC. But multiformats are supposed to be completely self-describing so each ECC curve needs it's own ✨ magic bytes ✨

}

removeSyncHook(hook: SyncHook): Array<SyncHook> {
return this.syncHooks.filter(h => h !== hook)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added hooks here ☝️

Copy link
Contributor

@dholms dholms left a comment

Choose a reason for hiding this comment

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

Sweet this is lookin good! 🎉 🎉

@icidasset icidasset merged commit d7a500c into master Apr 22, 2020
@icidasset icidasset deleted the user branch April 22, 2020 14:12
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.

2 participants