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

Refactor all the RegisterSignedStatement workflows #51

Merged
merged 26 commits into from
Jan 7, 2025

Conversation

JAG-UK
Copy link
Contributor

@JAG-UK JAG-UK commented Dec 3, 2024

Addresses #10 , #12 , #13 , #14 , #20 and #22

Cleans up async workflow
Cleans up async error reporting
Cleans up locators
More defined/single responsibilities for endpoint calls and returns

@robinbryce
Copy link

We now seem to have about 3 different status values to deal with

status: "running" in the content in various apis

the http status

the response code in the problem details.

Is it possible to remove status: "running" | "success" in favour of just relying on the http status codes ?

/ title / -1: "Not Found",
/ detail / -2: "Receipt with entry ID <id> not known to this Transparency Service",
/ instance / -3: "urn:ietf:params:scitt:error:receipt:not-found",
/ response-code / -4: 404,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that this was maybe explained and I missed it, but I am wondering if there is more redundancy here than necessary?
Are the response code doing something more than the HTTP response codes? Are we making implementers have to deal with potentially inconsistent responses?
Why do we have title/detail/instance? I can see the point in short/long forms, but it's not clear what role the third value plays here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's certainly a whiff of redundancy. I kept the response-code in because (unfortunately, but perhaps to some advantage) CoAP codes are slightly different to HTTP codes.

I like instance because it offers a chance for richer machine-driven interpretation of errors, which feels useful given that our early implementers seem to be using things like no-code builders and GitHub actions for their environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

How important/useful is it to use CoAP here? It seems to me that HTTP codes are expressive enough to capture all of what we do we with SCRAPI, but perhaps I am missing something?
100% agree on enabling machine-driven interpretation, but I haven't spotted an obvious situation where HTTP codes won't do the job (yet?), and I do suspect there is a benefit to keeping SCRAPI as simple as possible.

Choose a reason for hiding this comment

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

I also liked instance.

when I did our implementation, having a response-code was useful in the aysnc operation status check. So that the request to check could have a 200/OK status, but the response content could be a problem details that indicated "still running" (aka 202/accepted).

But the specific detail of it being CoAP was very not helpful. Commonly, some cloud service we interact with (database or block storage) would have an error case with a natural http status, that would need translating to CoAP. Not everything had a good translation (429 for example doesn't have a translation at all) so the choices were pretty arbitrary.

Keeping response-code as an optional part of the problem details, but specifying that it be the https status codes would be my preference based on the experience of doing our implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

when I did our implementation, having a response-code was useful in the aysnc operation status check. So that the request to check could have a 200/OK status, but the response content could be a problem details that indicated "still running" (aka 202/accepted).

I am probably missing something obvious here, but the 202/200 distinction is enough for our implementation, I do not know what we would with an additional field.
Adding lots of redundant fields is at best a cost, and at worst a source of brittleness because it increases the chance that some won't be populated and/or read.

Choose a reason for hiding this comment

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

Ah, I think this has changed ? It was logical to me that "Resolve Receipt" should return 202 for "still running", but at the time I last looked I swear there were "strong opinions" that it was an abuse to use the status code like this (the request to check the status of the receipt has striclty speaking succeeded so should be 200). If that has been resolved, I don't think I would mis response-code and it would have made our implementation simpler (by a lot) to not have to deal with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tagging @henkbirkholz, as I believe CoAP was his request ⬆️

Copy link
Collaborator

Choose a reason for hiding this comment

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

If there's no quick resolution, I suggest deferring this to a separate issue to unblock merging this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that this is a separate issue and should not block this PR.

Copy link
Member

Choose a reason for hiding this comment

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

yes, coap in different PR please

@SteveLasker SteveLasker added this to the Draft 03 milestone Dec 31, 2024
@SteveLasker
Copy link
Collaborator

Note: I pushed a few IDNITS and MD formatting changes (ed29840, 22d4668, 2f97c58) to bring focus to the API changes.

Copy link
Collaborator

@SteveLasker SteveLasker left a comment

Choose a reason for hiding this comment

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

A few additional comments to review

draft-ietf-scitt-scrapi.md Outdated Show resolved Hide resolved
draft-ietf-scitt-scrapi.md Outdated Show resolved Hide resolved
draft-ietf-scitt-scrapi.md Outdated Show resolved Hide resolved
draft-ietf-scitt-scrapi.md Outdated Show resolved Hide resolved
draft-ietf-scitt-scrapi.md Show resolved Hide resolved
Copy link
Contributor

@aj-stein aj-stein left a comment

Choose a reason for hiding this comment

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

Some initial feedback, more to follow

Comment on lines +227 to +228
If the `payload` is detached, the Transparency Service depends on the client's authentication context in the Registration Policy.
If the `payload` is attached, the Transparency Service depends on both the client's authentication context (if present) and the verification of the Signed Statement in the Registration Policy.
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "depends on ... authentication context" mean in this context?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that implies "the response of/the behaviour of the TS", and it is trying to say that if the payload is detached (and therefore unavailable to the service), the TS does not (cannot!) use it as an input in its decision/state change, instead it solely considers the client authentication context.

Thinking about it, and going back to the earlier discussion about unprotected headers, I think I have changed my mind about detached payloads from "weird, but why not" to "oh no", because the lack of verification means that even the protected headers are unauthenticated in this case.

Which opens two cans of worms:

  1. All the same issues as unprotected headers, but worse, because now dependent on whether the payload is detached or not.
  2. Registration policy needs to be carefully written to take into account whether the payload was detached or not, and must consider, if at all, the protected headers quite differently in that case.

Choose a reason for hiding this comment

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

I understood authentication context to mean the authorization obtained by the submiter of the statement (which is often not going to be the issuer).

These lines just acknowlege that the minimal cryptographic verification of a statement with a detached payload is not possible on registration. While this is an issue for the scitt arch registration section's position on minimal registration checks, I am totally fine with this being so. It is very clear to me what it means for registration time checks.

A concrete example of registering a statement that has a detached payload and interesting verification would be registration of a signed statement that was a COSE Receipt. Why would I want to do this ? To record the receipt I relied on from some other decision and which had to be stripped from its logical home in the unprotected headers of a Transparent Statement.

In this case:

  • The payload is detached
  • The proof material that is used to re-produce the payload is contained in the un protected headers and so can not be registered (unless we do the nested statement thing)

However, verification is rigorously defined via profiles. Any proof that recreates the payload bytes is valid and should result in a positive verification result.

That the payload is detached is a security enforcing property. The payload, however it is obtained is always "just bytes". The transparent statement unprotected headers case was particularly hard because the headers were not opaque, and could not reliably be handled as "just bytes".

Copy link
Contributor

Choose a reason for hiding this comment

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

@robinbryce as you point out, this is not the only way to make a receipt transparent (and does not work if you are trying to make transparent a set of receipts). Making it (or them) part of the payload avoids this special case, and preserves the rest of the context.

The proof material that is used to re-produce the payload is contained in the un protected headers and so can not be registered (unless we do the nested statement thing)

That does not prevent verification of the signed statement by the TS though? We explicitly allow x5chain in the unprotected header, which may be necessary to verify Signed Statements with a payload. The unprotected evidence is not included in the ledger/counter-signed, but the protected header is authenticated.

The current wording says that a TS accepting a statement with a detached payload does not authenticate it, and so does not authenticate the protected header. Am I getting this wrong?

Choose a reason for hiding this comment

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

So first please note I just refer to receipts here because they are a handy concrete example for a real use of detached payloads. I am not looking for "special treatment" for registration of statements that happen to be receipts and I'm not looking to have the tail wag the dog.

That does not prevent verification of the signed statement by the TS though?

I think it does. Having the proof material is the only way to produce the detatched payload. To verify the cose signature, the detached payload must first be "re attached".

The current wording says that a TS accepting a statement with a detached payload does not authenticate it, and so does not authenticate the protected header. Am I getting this wrong?

I think that is correct, and in the case of a receipt is also fine. A receipt is always in reference to some other signed statement. It can only verify inclusion of something in a log it can't be "wrong" in that sense, and its issuer is the log itself. A receipts presence in another log says nothing about the validity or verification status of that receipt. In the specific case of receipts (which have well defined verification semantics) the whole notion of registration time checks seems redundant, and - for some proof types - actively un helpful.

The problem here is with the arch: specifically the hard fought and long discussed minimal registration checks

https://ietf-wg-scitt.github.io/draft-ietf-scitt-architecture/draft-ietf-scitt-architecture.html#name-registration-of-signed-stat

Particularly "TS Signed Statement Verification and Validation: The Transparency Service MUST perform signature verification per"

And I think that means what we say in 227 & 228 can't actually be allowed by the current arc ?

I think the TL;DR of the prevailing opinion is that statements are meaningless to relying parties and auditors and so on, unless some kind of minimal registration checks are always performed.

I have always disagreed with that opinion. Those that want strong enforcement are free to do so with what ever registration policies they define, including one that matches the current minimal requirements. The challenge with that was that then the arch depends for its trust model on something that is explicitly out of scope. I understood those objections. I just didn't aree with them :-). Where we landed was the very watered down minimal registration requirements we have today. I'm not sure that anyone was actually "happy" with that outcome. I can believe that some TS implementations will have very rigorous registration requirements indeed. Ours is at the opposite end of the spectrum.

In the case of registering a statement with a detached payload, a receipt is, in my opinion, an example of a signed statement that does not benefit from registration time checks at all - beyond the authorization of the submitter to call RegisterSignedStatement

Copy link
Collaborator

Choose a reason for hiding this comment

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

I struggled with these lines as well, and created #55, as I'm struggling to see great examples where detached signed statements really add much value, as you any verification requires fetching the payload. However, using https://datatracker.ietf.org/doc/draft-ietf-cose-hash-envelope/ provides a means to keep the payload small, while still providing verification.
I'd suggest we remove these two lines, and use #55 to replace the examples and recommendations to use cose hash envelope.

Suggested change
If the `payload` is detached, the Transparency Service depends on the client's authentication context in the Registration Policy.
If the `payload` is attached, the Transparency Service depends on both the client's authentication context (if present) and the verification of the Signed Statement in the Registration Policy.

Choose a reason for hiding this comment

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

Yep, cose hash envelope is going to be the right answer for all cases where the desire to detach is driven by payload size. I agree with this change. Additionally, I would like to avoid "banning" registering statements with detached payloads.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Additionally, I would like to avoid "banning" registering statements with detached payloads.

Fair, since COSE supports detached, I don't think we need to explicitly ban/block.
Do we want to delete these two lines, and move forward with #55 as the default for copy/paste simplicity of implementors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to delete these two lines, and move forward with #55 as the default for copy/paste simplicity of implementors?

That sounds good to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can either delete these lines here, or resolve in #55 if we want to finish up this PR.
Deferring to folks in the next Editors Meeting as I'll be on vacation.

Comment on lines +266 to +273
In cases where the registration request is accepted but the Transparency Service is not able to mint Receipts in a reasonable time, it returns a locator for the registration operation and a status code indicating the status of the operation, as in this non-normative example:

~~~ cbor-diag
{
/ locator / "OperationID": "67f89d5f0042e3ad42...35a1f190",
/ status / "Status": "running",
}
~~~
Copy link
Contributor

Choose a reason for hiding this comment

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

This feedback is a minor point of contention, but given a previous use above of locator is a locator and this is ID for the operation that could be appended to maybe kinda form a locator, perhaps call it what it is to minimize confusion?

Suggested change
In cases where the registration request is accepted but the Transparency Service is not able to mint Receipts in a reasonable time, it returns a locator for the registration operation and a status code indicating the status of the operation, as in this non-normative example:
~~~ cbor-diag
{
/ locator / "OperationID": "67f89d5f0042e3ad42...35a1f190",
/ status / "Status": "running",
}
~~~
In cases where the registration request is accepted but the Transparency Service is not able to mint Receipts in a reasonable time, it returns an identifier for the registration operation and a status code indicating the status of the operation, as in this non-normative example:
~~~ cbor-diag
{
/ identifier / "OperationID": "67f89d5f0042e3ad42...35a1f190",
/ status / "Status": "running",
}
~~~

Copy link
Collaborator

@SteveLasker SteveLasker Jan 2, 2025

Choose a reason for hiding this comment

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

locator is more generic, which can include an ID, while an ID isn't necessarily a locator.
It's the fun topic of Separating Identity from Location and Decoupling Location from Identity - Is this in the scope of purl?

Suggesting we keep locator

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggesting we keep locator

+1

Copy link
Member

Choose a reason for hiding this comment

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

so an URL is more generic than an URI?

ducks

Comment on lines +275 to +278
`Status` must be one of the following:

- "running" - the operation is still in progress
- "succeeded" - the operation succeeded and the Receipt is ready
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are moving to CBOR, will these be encoded strings with the words running or succeeded or some integer lookup? The latter seems more idiomatic and conducive to CBOR, why add strings in the mix if we are moving away from JSON?

draft-ietf-scitt-scrapi.md Outdated Show resolved Hide resolved
draft-ietf-scitt-scrapi.md Outdated Show resolved Hide resolved
Co-authored-by: A.J. Stein (Unofficial) <[email protected]>
Co-authored-by: A.J. Stein (Unofficial) <[email protected]>
@SteveLasker
Copy link
Collaborator

Thank you, we're getting closer...

@SteveLasker
Copy link
Collaborator

SteveLasker commented Jan 3, 2025

almost there...
Here's the delta from changes @JAG-UK proposed with the recent changes: 166bbbe...9c8fd24

Copy link
Collaborator

@SteveLasker SteveLasker left a comment

Choose a reason for hiding this comment

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

LGTM for main...9c8fd24

The remaining open issues could be resolved with subsequent PRs

@JAG-UK JAG-UK merged commit f3a590c into main Jan 7, 2025
2 checks 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
7 participants