-
Notifications
You must be signed in to change notification settings - Fork 554
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
DID SDK #133
Conversation
@@ -0,0 +1,19 @@ | |||
{ |
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.
need to figure out how to move this all to repo root so we can stop duplicating it in every package
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.
Are you referring to cascading
https://eslint.org/docs/user-guide/configuring/configuration-files#cascading-and-hierarchy
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.
yes but i think it runs into some hiccups in monorepos cause it stops looking after a package.json
but i haven't really looked into this yet. not enough friction to really warrant it 😅
did-sdk/src/ion/ion.ts
Outdated
operation: 'update', | ||
content: params, | ||
previous: this.getPreviousOperation('update'), | ||
update: await generateKeyPair(options.keyType, options), |
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.
oh every update you set a new update keypair?
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.
Assuming their SDK is correct, yeah you rotate the signing keypair with every update.
kty: 'EC' | 'OKP' | ||
crvOrSize: 'P-256' | 'P-384' | 'P-521' | ||
} | ||
export type KeyType = |
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.
do we want to support NIST curves here? I always go back & forth on it. but webcrypto & a lot of device TPMs only support RSA + NIST curves.
I'm using NIST P-256 in AWAKE for instance
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 decided to see what the deal is with webcrypto curves and came across this interesting exchange. I know tptacek isn't the end-authority on all crypto but he tends to know what he's talking about. The other thing I found is unconfirmed statements that Microsoft has been the blocker on 25519.
We might not really have a choice not to use the NIST curves. We need the widest support possible (TPMs) and it's probably safer to be using WebCrypto than js/wasm versions of crypto.
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.
that's sorta my sense as well. TPM with NIST P-256 is definitely better than browser storage with ed25519. So we certainly shouldn't be dogmatic about not using NIST curves cause then we lose out on most available TPMs.
in the browser, the benefit of webcrypto is:
- you can store keys as "unexportable" in IDB. so an attack can do operations with them but not actually steal the key material
- operations are faster cause they're at the system level instead of javascript runtime which means you get some hardware acceleration on things like sha256
downside is obviously the downside of NIST over Ed25519:
- longer public keys
- not constant time
- possibility of a backdoor
did-sdk/README.md
Outdated
console.log(did.getService('SomeService')) // => ServiceEndpoint | ||
|
||
// create a did | ||
const didKey = await didSdk.key.create('ed25519', { |
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.
should we have defaults here
if I just const didKey = await didSdk.key.create()
It should use a good random and only make the dev specify if they have a reason to.
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 this comment is on an outdated version. I have the readme showing create('ed25519')
right now.
|
||
#### Defined in | ||
|
||
[src/did-documents.ts:37](https://github.com/bluesky-social/bluesky-prototype/blob/05593da/did-sdk/src/did-documents.ts#L37) |
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.
better to update the url to adx from bluesky-prototype
https://github.com/bluesky-social/adx/
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.
is github markdown happy with a relative uri ../../src/did-documents.ts#L37
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.
Ah hmm, I'll see if typedoc
supports that.
getService(type: string): ServiceEndpoint { | ||
if (!this.didDoc.service || this.didDoc.service.length === 0) | ||
throw new Error('Service not found') | ||
const service = this.didDoc.service.find((s) => s.type === type) |
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 do we want to do if there is more than one service match?
This just return the first one works but feel awkward.
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.
We could add an optional type filter on listServices()
, but generally dunno what else we would do than return the first match.
} | ||
|
||
getSuffix() { | ||
return this.getURI('short').split(':').pop() || '' |
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.
split here should probably have a limit did:ion:<did-suffix>:<extra-state>
dose getSuffix intend to return only any state that is after a :
in the suffix?
Adds
@adx/did-sdk
with support for did:key, did:web, and did:ion. Includes: