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

Ensure that statusListCredential can be dereferenced. #46

Merged
merged 2 commits into from
Jun 14, 2023

Conversation

msporny
Copy link
Member

@msporny msporny commented Jan 29, 2023

This PR addresses issue #39 by modifying the language in the specification to make it clear that if statusListCredential can't be dereferenced, that it should result in a validation error.


Preview | Diff

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

Just a few tiny tweaks, as noted above.

Copy link
Member

@brentzundel brentzundel left a comment

Choose a reason for hiding this comment

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

approved with Ted's fixes

index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
Copy link
Contributor

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

"not finding the status list should be a 404 and should be in the spec"
can this be added in this PR? can be another PR too I guess

@msporny
Copy link
Member Author

msporny commented Feb 4, 2023

@Sakurann wrote:

"not finding the status list should be a 404 and should be in the spec" can this be added in this PR? can be another PR too I guess

I don't think the spec currently speaks to HTTP, specifically (given that a different protocol scheme ... like a DID URL... might be used to dereference a URL). We do tell the implementer to throw a validation error if the dereference fails in step 3 here:

https://pr-preview.s3.amazonaws.com/w3c/vc-status-list-2021/pull/46.html#validate-algorithm

This PR doesn't try to establish specific error codes, like the VC Data Integrity spec does (see step 5):

https://w3c.github.io/vc-data-integrity/#generate-proof

We might want to do that in a future PR if the group feels like we need to be more specific about the types of errors that we throw. I'm currently undecided on whether its worth it to tell implementers how to expose specific types of errors.

Does that work for you, or would you like to see something more in the PR (or open an issue to track something that could be done in a future PR)?

Copy link
Contributor

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

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

When statuslistcredential is an HTTPS URL and it cannot be reached, HTTP 400 (Bad Request) status code must be returned.

@dlongley
Copy link
Contributor

dlongley commented Feb 5, 2023

@Sakurann,

When statuslistcredential is an HTTPS URL and it cannot be reached, HTTP 400 (Bad Request) status code must be returned.

Do we really want to define specific HTTP status codes for the case that the URL is HTTPS?

There are also multiple ways to look at this -- because the above statement would be a requirement placed on servers, not on clients. For servers, what does "it cannot be reached" mean? I'd think, specifically, that a "400 Bad Request" would only be returned because the client made some error in how it formulated its request.

I think if we're going to get into specific HTTP error code handling, we're going to have to be more robust. Also, it may be better to just define how clients should behave when receiving various 4xx / 5xx error codes vs. trying to be prescriptive in what we tell servers to do.

@iherman
Copy link
Member

iherman commented Feb 6, 2023

The issue was discussed in a meeting on 2023-01-31

  • no resolutions were taken
View the transcript

1.2. Ensure that statusListCredential can be dereferenced. (pr vc-status-list-2021#46)

See github pull request vc-status-list-2021#46.

Manu Sporny: next item, is regarding dereferencing.
… issue #39 is about errors when you can't dereference..
… we don't use these values, so it should not matter if they fail to dereference..
… but it should fail during validation....
… new PR says throw an error during validation.
… if the ids can't be dereferenced.

Brent Zundel: https://github.com/w3c/vc-status-list-2021/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-asc.

@iherman
Copy link
Member

iherman commented Feb 6, 2023

The issue was discussed in a meeting on 2023-02-01

  • no resolutions were taken
View the transcript

3.7. Ensure that statusListCredential can be dereferenced. (pr vc-status-list-2021#46)

See github pull request vc-status-list-2021#46.

Manu Sporny: PR 46. We have language about the credential, what should you do if it can't be dereferenced. This PR adds some guidance..
… That's it regarding PRs.

Kristina Yasuda: To follow up on presentationSchema PR, did you say we need a Special Topic call?.

Manu Sporny: The group needs to discuss this in some manner. Could be done now, or on a Special Topic call?.
… Potentially we could resolve it now in 15 min if the WG understands.

Kristina Yasuda: I think Orie has opinions, might not be quick. Let's do it on the next WG call..
… Let's do issues..

Copy link

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

Much like OAuth deployments are more interoperable because the use of specific HTTP error codes are required where they make sense, I believe we should do the same.

HTTP 404 (Not Found) is one that would make sense in some circumstances.

@iherman
Copy link
Member

iherman commented Feb 9, 2023

The issue was discussed in a meeting on 2023-02-08

  • no resolutions were taken
View the transcript

3.4. Ensure that statusListCredential can be dereferenced. (pr vc-status-list-2021#46)

See github pull request vc-status-list-2021#46.

Manu Sporny: about if status list can be dereferenced.
… some objections.
… status list can live on any url, so not sure if we should get specific about http.

@msporny
Copy link
Member Author

msporny commented Mar 6, 2023

At present, merging this PR is blocked due to a disagreement on whether or not the specification algorithms should speak towards status list credentials that are dereferenced via HTTPS. We will probably need a WG or special topic call to get more input on this PR. /cc @Sakurann @brentzundel

@msporny msporny added discuss This issue is a discussion with no clear change requested DO NOT MERGE labels Mar 17, 2023
@mkhraisha
Copy link
Contributor

+1 to the idea that being unable to dereference the credential SHOULD result in a validation error.

I believe the spec itself does not speak to how we should return validation errors, and whether we need to specify what caused the validation to fail, and I believe that is something we can discuss, but should not block this PR.

@msporny msporny requested a review from TallTed April 25, 2023 15:11
@OR13
Copy link
Contributor

OR13 commented Apr 27, 2023

This PR should be merged.

@OR13
Copy link
Contributor

OR13 commented Apr 27, 2023

@Sakurann @selfissued please reconsider review, this is a blocker for conformance tests, and I cannot resolve them until this is fixed.

Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

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

LGTM

@OR13
Copy link
Contributor

OR13 commented Apr 27, 2023

@brentzundel if it is possible to discuss this PR on an upcoming call, that would be awesome.

@OR13
Copy link
Contributor

OR13 commented May 3, 2023

@msporny can you file an issue for 400 vs 404 so we can merge this?

Copy link

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

I'll approve this once the specific error returned defined.

If a proof fails, return a validation error.
Dereference the <code>statusListCredential</code> URL, and ensure that all
proofs verify successfully. If the dereference fails, or if any of the proof
verifications fail, return a validation error.

Choose a reason for hiding this comment

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

Please be more specific about the error returned. HTTP 404 (Not Found) is one that I would suggest.

Copy link
Contributor

@dlongley dlongley May 3, 2023

Choose a reason for hiding this comment

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

Saying "return HTTP 404" (instead of just "return a validation error") doesn't make sense here because these processing rules are for consumers. This statement is telling the client / processor that is trying to dereference the URL what to do. There's no expectation that this processing software must be running on a server itself.

We could say that one possible reason for a URL dereference failure is for the consumer to receive an HTTP 404 error from the server that is the authority for that URL. So, we could say that here, but what then about other dereference failures / different error codes from the server? I'd think at that point, that if the URL is an HTTPS URL, we would want to say that any 4xx/5xx error should be considered a dereferencing failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to handle this in another issue and get this PR merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

Issue #64 has been raised to track what HTTP responses are expected. Can you please re-review, @selfissued?

@iherman
Copy link
Member

iherman commented May 11, 2023

The issue was discussed in a meeting on 2023-05-10

  • no resolutions were taken
View the transcript

1.9. Ensure that statusListCredential can be dereferenced. (pr vc-status-list-2021#46)

See github pull request vc-status-list-2021#46.

Manu Sporny: handle 46.
… This PR46 need to make sure status list should be dereferenced. This is a data model spec, not related to the protocol used. Can open an issue to consider adding language around HTTP and maybe merge thereafter.

Orie Steele: This PR is an improvement, and should be merged, over the objections... Manu is correct, dereferencing might happen in a document loader that makes not network requests.

Manu Sporny: Without Mike or Kristina on the call we can't make progress.

Orie Steele: +1 dlongley, the objections don't make sense, the document loader seems to not be understood.

Manu Sporny: yes, also what dlongley said.

dlongely: the actual recommendations from Mike don't make sense here. We're saying when a client fails to deference something it generates a validation error. It doesn't make sense to have a client return an HTTP error, that's a server's job.

Orie Steele: complete agreement. A lot of confusion around the fundamentals. This is the same data model as in the core data spec.

Dave Longley: +1 to Orie.

Orie Steele: confusion over how dereferencing works is close to things that cause formal objections. If it's not clear how dereferencing works then there is a security problem.

Michael Prorock: agreement with the above comments. Need to get this merged.


@msporny msporny requested review from selfissued and Sakurann May 15, 2023 08:53
@msporny
Copy link
Member Author

msporny commented May 15, 2023

@OR13 wrote:

@msporny can you file an issue for 400 vs 404 so we can merge this?

Issue #64 has been raised to track the HTTP response code concerns. Can you please re-review @Sakurann and @selfissued?

@mprorock
Copy link
Contributor

This PR should be merged.

+1

If a proof fails, return a validation error.
Dereference the <code>statusListCredential</code> URL, and ensure that all
proofs verify successfully. If the dereference fails, or if any of the proof
verifications fail, return a validation error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
verifications fail, return a validation error.
verifications fail, return a validation error.
<p class="issue" data-number="64">note: the WG is tracking a desire to add some protocol guidance, especially for HTTP status code alignment</p>

@iherman
Copy link
Member

iherman commented May 25, 2023

The issue was discussed in a meeting on 2023-05-24

  • no resolutions were taken
View the transcript

1.5. Ensure that statusListCredential can be dereferenced. (pr vc-status-list-2021#46)

See github pull request vc-status-list-2021#46.

Orie Steele: I'd like to circle back to PR #46 on status list. The changes requested are assuming a particular transport protocol and it's possible that additional PRs could come in to improve the quality of the spec and I don't think we should hold up this PR.
… There's nothing in there, this is an improvement and others need to be made and I'd like to merge this so we can make incremental improvements.

Manu Sporny: In addition to what Orie said -- I raised issue #64 and that issue's title is "Add a section on expected server HTTP status codes", that's now being tracked. I believe that's what both Kristina and Mike Jones were asking for that.
… So I think there's a way to do it -- but doing it in this PR would just make the PR really big and potentially further delay it getting merged in.
… The only change request that was blocking this -- I requested a re-review from you, Kristina and Mike Jones.

Michael Prorock: Yeah, if we merge we can build on top of it.
… Can I add a note that references that issue?

Manu Sporny: Yes, totally fine.

Kristina Yasuda: Ok, I will re-review and hopefully MikeJ will read the notes.
… Thank you, Orie.
… Anything else?

Copy link

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

I'll approve this, expecting to address returning specific HTTP status codes when resolving issue #64 .

@iherman
Copy link
Member

iherman commented Jun 14, 2023

The issue was discussed in a meeting on 2023-06-14

  • no resolutions were taken
View the transcript

1.8. Ensure that statusListCredential can be dereferenced. (pr vc-status-list-2021#46)

See github pull request vc-status-list-2021#46.

joe: first 1 is 46.
… this was waiting for responses.

Kristina Yasuda: mike please review the PR.

@msporny
Copy link
Member Author

msporny commented Jun 14, 2023

Normative, multiple reviews, changes requested (some made, others tracked in issue #64), no objections, merging.

@msporny msporny merged commit 8f8f29f into main Jun 14, 2023
@msporny msporny deleted the msporny-deref-slc-url branch June 14, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss This issue is a discussion with no clear change requested DO NOT MERGE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants