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
fix: json ld fixes and aca-py interop fixes #1865
base: main
Are you sure you want to change the base?
fix: json ld fixes and aca-py interop fixes #1865
Changes from 9 commits
eee749f
34f1f3e
2c9e53b
29f7d73
9eeb5c8
187cf4e
b641f1a
5a15fb4
ee6403c
cd1befd
53d496b
888909d
61ab47b
41d4180
4493bb9
79bd5ce
aedc9b3
d9940a2
2a10e10
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.
I don't think this change is correct.
extractResolvableDidDocument
is called when the did document is notdid:peer:1
. If we resolve the did, instead of using an diddocument attachment, we REQUIRE the rotate attachment to be present. Could you explain a bit more why this change is needed?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 test case I ran into was agents or mediators using did:peer:2 without the intention of doing a did rotate or
alsoKnownAs
. It also is a part of the didExchange RFC that the value is optional for any reason so for interoperability sake I think this should remain.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.
If the did is not rotated from the invitation we can indeed ignore it. But if the did is rotated, not doing this check means the connection can be hijacked. I could send a response even though you created the invitation, because we don't require a signature from the invitation did. If the did is the same, then we don't have that issue and i'm ok with not requiring did rotate. But the current code doesn't take this into consideration
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 might be confused but the warn only happens if the attachment is not present, the else block is for the if statement on line 526 where it checks if the
didRotateAttachment
is defined. The signature is always checked if the rotate attachment is present. The else block with the warn was a suggestion made by @genaris when this PR was initially made.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 check that is now in place always allows didRotate to not be present, meaning we could just remove the check. So we need to be more specific in checking when didRotate needs to be present: when you rotate your did from the invitation. Even though the Aries RFC mentions it's optional, it's super important for security to do this check. did_rotate was added specifically for this reason, and it's only optional to not introduce a breaking change in the RFC. Before did rotate was introduced, it was basically not possible to rotate a did from the invitation if you were not using diddoc~attach and sign it.
If the did is rotated from the invitation, either a signed
diddoc~attach
must be present OR a signeddid_rotate~attach
must be presentThere 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 does this change in regex mean?
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 regex is specifically changed for
did:peer:2
. With the updates to the protocol ofdid:peer:2
the services on the encoded version are not always a list and can be multiple service appended instead.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 you have an example did:peer so I can add a test covering this?
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.
Why is this needed? The document loader should be able to load did documents, so I'm not sure if we want to bypass it here
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 document loader would resolve the did but not return the full did Document with the verification information applied. This might have changed since my testing because I added this while testing with
0.5.1
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'm sorry, could you elaborate a bit further? What do you mean with "not return the full did Document with the verification information applied"? What does it return?