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

Generalize to support both JSON and CBOR Claims Sets #18

Closed
wants to merge 3 commits into from
Closed

Generalize to support both JSON and CBOR Claims Sets #18

wants to merge 3 commits into from

Conversation

laurencelundblade
Copy link
Contributor

Had to rename the document since it was named only for CBOR/CWT. Chose "UCS" but it could be "UTCS" or other too.

Didn't really have to change semantics of anything

Added and corrected CDDL. Note that even the CDDL for just UCCS was inadequate because it didn't define the tagged and untagged.

Added CDDL to glue into EAT

Claims Sets (UCCS) and discusses conditions for its proper use.
CBOR Web Token (CWT, RFC 8392) and JSON Web Token (JWT, RFC7519) Claims Sets sometimes do not need the
protection afforded by wrapping them into COSE or JOSE, as is required for a true
CWT or JWT. This specification defines a CBOR and JSON for such unprotected
Copy link
Collaborator

Choose a reason for hiding this comment

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

"a CBOR and JSON" seems too vague.

I guess there's more than one thing that is defined here:

  • a CBOR tag for UCCS
  • the CDDL for the claims-set
  • the glue CDDL to plug UCS into EAT

Copy link
Member

Choose a reason for hiding this comment

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

If the WG really is interested in extending the scope of this document, the building blocks Thomas illustrated must be reflected in a corresponding document structure.

Some "catch-all" changes now include assertions that might not be backed by implementations. For example:

The sub-Attester produces a UCS with the required Claims Set and sends the UCS through the Secure Channel to the lead Attester.

While in theory that might be possible, as an individual I have no knowledge of any activity that does generate JSON Evidence or plans to do this. As this is becoming a normative specification, does anybody can point me to such activities?

Copy link

@nedmsmith nedmsmith Oct 2, 2023

Choose a reason for hiding this comment

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

Normally we expect RFCs to have implementations behind them. Are there implementations for UCCS and UJCS? (I'm not expecting this PR to resolve the question, but asking since if there aren't implementations then that could be a factor if the PR is trying to focus on capabilities that are new to the original effort).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ctoken/xclaim implements UCCS. It has a very basic implementation of UJCS encoding, but no implementation of UJCS decoding.

Copy link

@quic-jodonogh quic-jodonogh left a comment

Choose a reason for hiding this comment

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

I can only find one small nit in the technical content, but I don't really like the way that the document now reads in that CWT/JWT, COSE/JOSE and similar appear now all over the document, and as a result, it reads awkwardly.

I think part of the problem is that there is an important semantic difference between UCCS and UJCS, and that is the fact that UCCS are always tagged - this is an important semantic difference because the intent of the attester is clearly signalled in a way that JSON doesn't allow, and this doesn't come out in the amended text.

I won't make this a blanket "reject" if the other editors are OK, but it just doesn't "feel" quite right to me.

@@ -395,16 +405,17 @@ factors such as:

# CDDL

{{-cwt}} does not define CDDL for CWT Claims Sets.
{{-cwt}} nor {{-jwt}} define CDDL for CWT Claims Sets.

Choose a reason for hiding this comment

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

I think you would need to write something about JWT here.

@laurencelundblade
Copy link
Contributor Author

I can only find one small nit in the technical content, but I don't really like the way that the document now reads in that CWT/JWT, COSE/JOSE and similar appear now all over the document, and as a result, it reads awkwardly.

I think part of the problem is that there is an important semantic difference between UCCS and UJCS, and that is the fact that UCCS are always tagged - this is an important semantic difference because the intent of the attester is clearly signalled in a way that JSON doesn't allow, and this doesn't come out in the amended text.

You are right that UCCS is always a CBOR tag as specified in the document, but I think that is wrong. It is one of the things I think must be fixed regardless of whether we do UJCS or not. (Use of or not of the CBOR tag is a characteristic of the surrounding protocol and is pretty much always optional).

But more importantly, can you clarify what intent is being signaled in regards to attestation? Is it more than just the identification of the data/message type?

I won't make this a blanket "reject" if the other editors are OK, but it just doesn't "feel" quite right to me.

I can see how the text is not ideal but from my point of view, it's tolerable, gets the job done and avoids me (and others) having to do the work for a whole separate document. Maybe you would do the same if you were me? :-)

@laurencelundblade
Copy link
Contributor Author

This addresses issues #20, #21, #22


~~~ cddl
UCCS = #6.601(Claims-Set)
UCCS-Token = UCCS-Tagged-Token / UCCS-Untagged-Token
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
UCCS-Token = UCCS-Tagged-Token / UCCS-Untagged-Token


~~~ cddl
UCCS = #6.601(Claims-Set)
UCCS-Token = UCCS-Tagged-Token / UCCS-Untagged-Token
UCCS-Tagged-Token = #6.601(Claims-Set)
Copy link
Collaborator

@cabo cabo Nov 4, 2023

Choose a reason for hiding this comment

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

Suggested change
UCCS-Tagged-Token = #6.601(Claims-Set)
UCCS = #6.601(Claims-Set)

UCCS = #6.601(Claims-Set)
UCCS-Token = UCCS-Tagged-Token / UCCS-Untagged-Token
UCCS-Tagged-Token = #6.601(Claims-Set)
UCCS-Untagged-Token = Claims-Set
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
UCCS-Untagged-Token = Claims-Set

UCCS-Token = UCCS-Tagged-Token / UCCS-Untagged-Token
UCCS-Tagged-Token = #6.601(Claims-Set)
UCCS-Untagged-Token = Claims-Set
UJCS-Token = Claims-Set
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
UJCS-Token = Claims-Set
UJCS = Claims-Set

@cabo cabo marked this pull request as draft November 5, 2023 06:54
@laurencelundblade
Copy link
Contributor Author

FYI, I'm planning to rework this in a very different way after discussion with Thomas, Henk and Cabo at the hackathon.

@cabo
Copy link
Collaborator

cabo commented Nov 7, 2023

Thank you all for commenting on the way; this will be reworked.

@cabo cabo closed this Nov 7, 2023
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.

6 participants