Enhance corim display Command to Support Unsigned CoRIMs #14

ravjot07 · 2024-12-20T18:16:22Z

Problem:
The corim display command was limited to handling only signed CoRIM files. Attempting to display an unsigned CoRIM resulted error:

Error: error decoding signed CoRIM from my-unsigned-corim.cbor: failed CBOR decoding for COSE-Sign1 signed CoRIM: cbor: cannot unmarshal map into Go value of type cbor.RawTag

Solution:

Modified the display function to attempt decoding the input file as a signed CoRIM first. If this fails, it falls back to decoding as an unsigned CoRIM.
Implemented a loop to convert []corim.Tag to [][]byte before passing it to the displayTags function.

Closes Issue 49 form Corim Repository (veraison/corim#49)

Signed-off-by: Ravjot Singh <[email protected]>

deeglaze · 2024-12-20T22:51:35Z

cmd/corimDisplay.go

+	// try to decode as a signed CoRIM
+	var s corim.SignedCorim
+	if err = s.FromCOSE(corimCBOR); err == nil {
+		// successfully decoded as signed CoRIM


The code in this branch should be in a displaySignedCorim function to avoid the rightward drift

@deeglaze okiee should i create separate functions for signed and unsigned corims and call them in display function?

That’s my preference

okieee @deeglaze i will update this file. Any more suggestions?

Today I just started my holiday break. Splitting up into functions is a fine enough improvement for now.

thanks @deeglaze really appreciate you suggestion...

@deeglaze i have made some adjustments to the code according to your recommendations can you review them...

Signed-off-by: Ravjot Singh <[email protected]>

deeglaze · 2024-12-22T22:01:30Z

cmd/corimDisplay.go

 	}

-	if metaJSON, err = json.MarshalIndent(&s.Meta, "", "  "); err != nil {
-		return fmt.Errorf("error decoding CoRIM Meta from %s: %w", signedCorimFile, err)
+	fmt.Println("Corim:")


Please use consistent capitalization.

Suggested change

fmt.Println("Corim:")

fmt.Println("CoRIM:")

Resolved...

deeglaze · 2024-12-22T22:14:20Z

cmd/corimDisplay.go

+
+	if showTags {
+		fmt.Println("Tags:")
+		// convert []corim.Tag to [][]byte


I don’t think I’d put the conversion here. Maybe displayTags converts and calls displayTagBytes if you need the bytes form.

deeglaze · 2024-12-22T22:28:25Z

cmd/corimDisplay.go

-			} else {
-				fmt.Printf(">> unmatched CBOR tag: %x\n", cborTag)
-			}
+		// convert []corim.Tag to [][]byte


A translation here is duplicative.

i will add this code to display function itself

deeglaze · 2024-12-22T22:35:09Z

cmd/corimDisplay.go

 	}

 	return nil
 }

+func display(corimFile string, showTags bool) error {


Generally you’d want a rendering function to be versatile in input and output types, so I’d recommend reading from an io.Reader and writing to an io.Writer. You can then make convenience functions that prepare the options for you. In terms of options, showTags as a boolean input makes calling positions hard to interpret and makes adding options more difficult. I’d recommend making the writer and showTags options fields of a DisplayOptions struct to take as the only other argument from the reader.

@deeglaze I appreciate your recommendation to utilize io.Reader and io.Writer along with a DisplayOptions struct to enhance the versatility and flexibility of our rendering functions.

However, I’ve observed that the majority of the code in our repository currently follows a different approach. To maintain consistency throughout the project, I believe it would be more effective to adhere to our existing coding patterns.

Could you please share some references or examples in contexts similar to our project? This would help me better understand how we might integrate these improvements in a way that aligns with our current structure...

Here’s another cli in go that uses the pattern of (context, subject, options) https://github.com/google/gce-tcb-verifier/tree/main/gcetcbendorsement

writing to os.Stdout and writing to string buffer shouldn’t need fundamentally different implementations.

@deeglaze may be i could raise a new issue about changing to this pattern all the instances where there is io kinda situation

deeglaze · 2024-12-22T22:36:30Z

cmd/corimDisplay.go

+// displayTags processes and displays embedded tags within a CoRIM
+func displayTags(tags [][]byte) {
+	for i, e := range tags {
+		// ensure the tag has at least 4 bytes (3 for tag identifier and 1 for data)


The description of the constant’s significance should be above its definition as a named constant. Then change the literal to the named constant.

deeglaze · 2024-12-22T22:38:54Z

cmd/corimDisplay.go

+			continue
+		}
+
+		// Split tag identifier from data


This is probably better suited to a function that identifies a tag into an enum that you can then use in the switch. I think that function would belong in the corim library.

This is probably better suited to a function that identifies a tag into an enum that you can then use in the switch. I think that function would belong in the corim library.

I reviewed the corim library but couldn't find an existing function to identify tags and map them to enums. Let me know if you'd like me to implement the tag identification function or if there are other directions you'd prefer.

If you implement it here unexported, we can move it to the corim repo later.

yup i will surly do that but i was thinking that i maybe could raise a new PR about this as this issue was about display of unsigned CoRIMs

@deeglaze i will start working on this....

Signed-off-by: Ravjot Singh <[email protected]>

ravjot07 · 2024-12-23T01:06:05Z

@deeglaze can you guide about test why they are failing do i need to update display_test file ??

ravjot07 · 2024-12-23T12:21:26Z

@deeglaze

there is some issue with mock deps i am not able to run the test even just after cloning

Priyanshuthapliyal2005 · 2024-12-23T13:56:07Z

@deeglaze there is some issue with mock deps i am not able to run the test even just after cloning

yeah , i face the same issue in repo mocks file is missing in cmd and
I tried something similar 4–5 days ago but faced an issue while switching the display between signed and unsigned. I encountered the error:
PS D:\opensource\cocli> cocli corim display --file unsigned-corim.cbor
Error: error decoding signed CoRIM from unsigned-corim.cbor: failed CBOR decoding for COSE-Sign1 signed CoRIM: cbor: invalid COSE_Sign1_Tagged object
PS D:\opensource\cocli>

deeglaze · 2024-12-23T14:07:22Z

The makefile uses mockgen to create the mocks files. I’ll need to fuss with the CI to see why it’s now failing. I’m on holiday break right now, so no promises I can get to it soon.

Priyanshuthapliyal2005 · 2024-12-23T14:10:40Z

@ravjot07 you can try this to create mocks : make presubmit

Signed-off-by: Ravjot Singh <[email protected]>

ravjot07 · 2024-12-23T15:47:27Z

The makefile uses mockgen to create the mocks files. I’ll need to fuss with the CI to see why it’s now failing. I’m on holiday break right now, so no promises I can get to it soon.

@deeglaze i think there was some issue on my side as in mockgen installation and its been resolved now.

ravjot07 · 2024-12-23T15:50:31Z

@ravjot07 you can try this to create mocks : make presubmit

@Priyanshuthapliyal2005 yaa it worked this time actually there was installation errors, and then i installed mockgen package manually so it worked

deeglaze · 2024-12-23T15:50:34Z

cmd/corimDisplay_test.go

 	}
 	cmd.SetArgs(args)

 	fs = afero.NewMemMapFs()
-	err := afero.WriteFile(fs, "invalid.cbor", testSignedCorimInvalid, 0644)


What about the invalid.cbor testdata is unsuitable now? This orphans the test file and embedding variable.

@deeglaze yee, i kinda missed this will update this..

@deeglaze I have updated the test. I really appreciate your help and guidance, especially while you are on holiday. Thank you so much for taking the time to assist me—it means a lot!

ravjot07 · 2024-12-23T15:51:48Z

@deeglaze there is some issue with mock deps i am not able to run the test even just after cloning

yeah , i face the same issue in repo mocks file is missing in cmd and I tried something similar 4–5 days ago but faced an issue while switching the display between signed and unsigned. I encountered the error: PS D:\opensource\cocli> cocli corim display --file unsigned-corim.cbor Error: error decoding signed CoRIM from unsigned-corim.cbor: failed CBOR decoding for COSE-Sign1 signed CoRIM: cbor: invalid COSE_Sign1_Tagged object PS D:\opensource\cocli>

@Priyanshuthapliyal2005 ya that the whole point for this PR is to enhance the display command to support unsigned CoRIMs

Update: corim display Command to Support Unsigned CoRIMs

73c5fd1

Signed-off-by: Ravjot Singh <[email protected]>

deeglaze reviewed Dec 20, 2024

View reviewed changes

Refractor code into more modeluar function

a88974a

Signed-off-by: Ravjot Singh <[email protected]>

deeglaze requested changes Dec 22, 2024

View reviewed changes

minor changes

cb3ad12

Signed-off-by: Ravjot Singh <[email protected]>

Fixes: ci issue

c7b831b

Signed-off-by: Ravjot Singh <[email protected]>

deeglaze reviewed Dec 23, 2024

View reviewed changes

deeglaze merged commit 0d8fae8 into veraison:main Dec 24, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance corim display Command to Support Unsigned CoRIMs #14

Enhance corim display Command to Support Unsigned CoRIMs #14

ravjot07 commented Dec 20, 2024 •

edited

Loading

deeglaze Dec 20, 2024

ravjot07 Dec 20, 2024

deeglaze Dec 21, 2024

ravjot07 Dec 21, 2024

deeglaze Dec 21, 2024

ravjot07 Dec 21, 2024 •

edited

Loading

ravjot07 Dec 22, 2024

deeglaze Dec 22, 2024

ravjot07 Dec 22, 2024

deeglaze Dec 22, 2024 •

edited

Loading

deeglaze Dec 22, 2024

ravjot07 Dec 22, 2024

deeglaze Dec 22, 2024 •

edited

Loading

ravjot07 Dec 22, 2024

deeglaze Dec 23, 2024

ravjot07 Dec 23, 2024

deeglaze Dec 22, 2024

deeglaze Dec 22, 2024

ravjot07 Dec 22, 2024

deeglaze Dec 23, 2024

ravjot07 Dec 23, 2024

ravjot07 Dec 23, 2024

ravjot07 commented Dec 23, 2024

ravjot07 commented Dec 23, 2024

Priyanshuthapliyal2005 commented Dec 23, 2024 •

edited

Loading

deeglaze commented Dec 23, 2024

Priyanshuthapliyal2005 commented Dec 23, 2024

ravjot07 commented Dec 23, 2024

ravjot07 commented Dec 23, 2024

deeglaze Dec 23, 2024

ravjot07 Dec 23, 2024

ravjot07 Dec 23, 2024

ravjot07 commented Dec 23, 2024

Enhance corim display Command to Support Unsigned CoRIMs #14

Enhance corim display Command to Support Unsigned CoRIMs #14

Conversation

ravjot07 commented Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravjot07 Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deeglaze Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deeglaze Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravjot07 commented Dec 23, 2024

ravjot07 commented Dec 23, 2024

Priyanshuthapliyal2005 commented Dec 23, 2024 • edited Loading

deeglaze commented Dec 23, 2024

Priyanshuthapliyal2005 commented Dec 23, 2024

ravjot07 commented Dec 23, 2024

ravjot07 commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravjot07 commented Dec 23, 2024

ravjot07 commented Dec 20, 2024 •

edited

Loading

ravjot07 Dec 21, 2024 •

edited

Loading

deeglaze Dec 22, 2024 •

edited

Loading

deeglaze Dec 22, 2024 •

edited

Loading

Priyanshuthapliyal2005 commented Dec 23, 2024 •

edited

Loading