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

Change how the authorized keys are published and used #57

Merged
merged 3 commits into from
Jun 7, 2024

Conversation

swcurran
Copy link
Collaborator

@swcurran swcurran commented Jun 3, 2024

I've updated the spec. to include the changes we discussed last week with regards to authorized keys, SCID generation, and pre-rotation. While I generally followed what we discussed, I updated the SCID generation in a way I think makes the most sense, varying from what we discussed. Please review and let me know what you think.

updateKeys is a list of did:key DIDs in the parameters, as we discussed.

The existing SCID generation was based on just the DIDDoc with placeholders. I changed that to make it the Log Entry (minus the Data Integrity proof, of course), but including the parameters, the versionId, versionTime and "value": "<DIDDoc>" with placeholders (literal string {SCID}) wherever it is needed. It seemed easier to understand to use the entire entry vs. just a subset.

@andrewwhitehead - I also didn't do the SAID approach of using a ########... placeholder because I thought it would be a pain for the DIDDoc writer. It is much easier to just use the {SCID} placeholder, and to explain it, and really doesn't make any difference.

Pre-rotation becomes MUCH easier than what we had before. No more "detecting" key changes in the DIDDoc, and the hashing is just of the did:key DIDs, not the DIDDoc key reference entry data structure.

I really struggled over the description of what are the active authorized keys for the initial entry and subsequent entries. I'll probably try ChatGPT to see if I can make it better. Suggestions gratefully accepted...

brianorwhatever
brianorwhatever previously approved these changes Jun 5, 2024
spec/specification.md Outdated Show resolved Hide resolved
spec/specification.md Outdated Show resolved Hide resolved
spec/specification.md Outdated Show resolved Hide resolved
spec/specification.md Show resolved Hide resolved
spec/specification.md Outdated Show resolved Hide resolved
spec/abstract.md Outdated Show resolved Hide resolved
spec/abstract.md Outdated Show resolved Hide resolved
spec/specification.md Outdated Show resolved Hide resolved
@swcurran
Copy link
Collaborator Author

swcurran commented Jun 5, 2024

@brianorwhatever -- The comments you made were mostly tweaks to the wording. Does that mean you are more or less happy with the approach? Especially around the SCID generation?

@brianorwhatever
Copy link
Contributor

I only briefly went through the scid generation and was pretty tired. I'm hopefully going to look at implementing it today so let's hold off on merging until after that

@swcurran
Copy link
Collaborator Author

swcurran commented Jun 5, 2024

Great. Definitely!

@brianorwhatever
Copy link
Contributor

@swcurran I believe I implemented what is here in decentralized-identity/trustdidweb-ts#9 and it seemed to make sense. Take a look there and see if you agree I matched what's written here. Seemed like the SCID generation alg made sense. Interested in hearing @andrewwhitehead feedback

@swcurran
Copy link
Collaborator Author

swcurran commented Jun 6, 2024

Middle of the night thinking. I think the nextKeys array should not collect used/unused ones as I proposed. Rather, each instance becomes the current one, and the previously current set of hashes are dropped. I think a bad implementation could allow any nextKey to be reused, which would be a vulnerability.

The more general rule for (I think) any parameter is that the most recent value is the current one.

Reasonable?

@brianorwhatever
Copy link
Contributor

I had just reached the same thought. Makes implementation easier as well 👌

@swcurran
Copy link
Collaborator Author

swcurran commented Jun 6, 2024

Good stuff. I'll update that. Makes the spec easier as well.

Signed-off-by: Stephen Curran <[email protected]>
@brianorwhatever
Copy link
Contributor

I think to differentiate we should use

updateKeys and nextKeyHashes

Signed-off-by: Stephen Curran <[email protected]>
@swcurran
Copy link
Collaborator Author

swcurran commented Jun 6, 2024

I think to differentiate we should use

updateKeys and nextKeyHashes

Done.

@brianorwhatever brianorwhatever merged commit d67e273 into decentralized-identity:main Jun 7, 2024
1 check passed
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