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

Cut #6.500, #6.502 in FromCOSE #133

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

deeglaze
Copy link
Collaborator

Top level tags are not interpretable as a COSE object. The protected header content-type is not meant to be application/rim+cbor according to the spec draft (Issue #132).

Support the spec's optional unsigned corim tag in COSE payload.

Copy link
Contributor

@thomas-fossati thomas-fossati left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Top level tags are not interpretable as a COSE object.
The protected header content-type is not meant to be
application/rim+cbor according to the spec draft (Issue veraison#132).

Support the spec's optional unsigned corim tag in COSE payload.

Signed-off-by: Dionna Glaze <[email protected]>
@deeglaze
Copy link
Collaborator Author

I don't have permissions to merge. Did we decide I'd become a maintainer in the Veraison call? Or did I just suggest it? @yogeshbdeshpande

@thomas-fossati
Copy link
Contributor

I don't have permissions to merge. Did we decide I'd become a maintainer in the Veraison call? Or did I just suggest it? @yogeshbdeshpande

I made you an admin to both veraison/corim and veraison/cocli.

@yogeshbdeshpande
Copy link
Contributor

I don't have permissions to merge. Did we decide I'd become a maintainer in the Veraison call? Or did I just suggest it? @yogeshbdeshpande

I made you an admin to both veraison/corim and veraison/cocli.

@deeglaze : to your question: Did we decide I'd become a maintainer in the Veraison call?

Yes certainly, as I said in the call, your contributions to Veraison project are highly Welcome & Appreciated!

@yogeshbdeshpande
Copy link
Contributor

@deeglaze: Thank you for your changes, look good to me.!

This change also demands a suggested change in the veraison/docs repository, as we would recommend the usage of Content-type to be in-line with the CoRIM draft. If you are ok, then please also change:
https://github.com/veraison/docs/blob/main/api/endorsement-provisioning/README.md

and also the associated yaml file in the same location. Note: I have added you to that repo so you should be able to submit!

Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

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

LGTM!

@thomas-fossati
Copy link
Contributor

I suggest before merging this that we wait for the associated CoRIM PR to be merged.

@thomas-fossati
Copy link
Contributor

@deeglaze: Thank you for your changes, look good to me.!

This change also demands a suggested change in the veraison/docs repository, as we would recommend the usage of Content-type to be in-line with the CoRIM draft. If you are ok, then please also change: https://github.com/veraison/docs/blob/main/api/endorsement-provisioning/README.md

This is not strictly connected. The Content-Type in the docs is already out of sync with Veraison's implementation and should've been fixed irrespective of this change.

and also the associated yaml file in the same location. Note: I have added you to that repo so you should be able to submit!

If Dionna wants to do it, great, but she should not feel obligated to do it :-)

@yogeshbdeshpande
Copy link
Contributor

@deeglaze: Thank you for your changes, look good to me.!
This change also demands a suggested change in the veraison/docs repository, as we would recommend the usage of Content-type to be in-line with the CoRIM draft. If you are ok, then please also change: https://github.com/veraison/docs/blob/main/api/endorsement-provisioning/README.md

This is not strictly connected. The Content-Type in the docs is already out of sync with Veraison's implementation and should've been fixed irrespective of this change.

and also the associated yaml file in the same location. Note: I have added you to that repo so you should be able to submit!

If Dionna wants to do it, great, but she should not feel obligated to do it :-)

@deeglaze: Thank you for your changes, look good to me.!
This change also demands a suggested change in the veraison/docs repository, as we would recommend the usage of Content-type to be in-line with the CoRIM draft. If you are ok, then please also change: https://github.com/veraison/docs/blob/main/api/endorsement-provisioning/README.md

This is not strictly connected. The Content-Type in the docs is already out of sync with Veraison's implementation and should've been fixed irrespective of this change.
@thomas-fossati I am referring to content-type as Content-Type: application/rim+cbor which we is part of documentation and also in CoRIM repo and also in apiclient repo

and also the associated yaml file in the same location. Note: I have added you to that repo so you should be able to submit!

If Dionna wants to do it, great, but she should not feel obligated to do it :-)

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.

3 participants