Feat(profile/psa): extract and consolidate PSA profile package and removes PSA/CCA-specific types from COMID #253

abhiraj-ku · 2026-01-29T13:25:05Z

This PR consolidates PSA logic into the profiles package and removes PSA/CCA-specific COMID types to simplify and generalize the implementation. It reduces profile-specific coupling, and updates test fixtures accordingly, improving maintainability and extensibility for future profiles.

…pe TaggedBytes Signed-off-by: Abhishek kumar <[email protected]>

Signed-off-by: Abhishek kumar <[email protected]>

…ckage Signed-off-by: Abhishek kumar <[email protected]>

thomas-fossati

Looks great. Thanks for all your careful work!

I was expecting this PR to contain the PSA-08 implementation and was a little disoriented at first. I now understand that this is a significant restructuring of the existing logic (pre-08), and I'm looking forward to seeing the next steps!

Could you please fix the organisation of the commits and the messages? (In particular, I believe there are no new features here, so the feat label should not be used in the headings.

abhiraj-ku · 2026-02-02T17:43:31Z

Hey Thomas, thanks for the review! You’re right, this PR is focused on restructuring existing logic, not new features. I’ll clean up the commit messages and squash/rewrite the commit history accordingly.

yogeshbdeshpande · 2026-02-03T12:27:03Z

profiles/psa/implid.go

@@ -0,0 +1,122 @@
+// Copyright 2021-2024 Contributors to the Veraison project.


The Headers in all files should be reflected upto 2026, now!

ok i'll fix this and amend with psa commit

yogeshbdeshpande · 2026-02-03T12:35:35Z

Started reviewing, will finish by end of day today!

abhiraj-ku · 2026-02-03T12:37:13Z

Started reviewing, will finish by end of day today!

Ok,sure

yogeshbdeshpande · 2026-02-03T13:20:40Z

profiles/psa/psa.go

+	}
+
+	// Register PSA-specific ClassID factory (psa.impl-id)
+	// CBOR tag 560 is already registered for TaggedBytes in comid/cbor.go


@abhiraj-ku I do not see the tag 560 been applied as you are NOT instantiating ImplID as Tagged Bytes in the Methoid NewImplIDClassID).

yogeshbdeshpande

The new Interface functions for Instatiating ClassID using ImplID needs to be re-written to claim that TaggedBytes are used instead:

yogeshbdeshpande · 2026-02-03T13:35:28Z

profiles/psa/psa_test.go

+	for i := range implID {
+		implID[i] = byte(i)
+	}
+	class := comid.NewClassBytes(implID)


This approach should be used when instantiating ClassID using Implementation ID

yogeshbdeshpande

see further comments...

setrofim

A note on commit messages:

do not capitalize the classification tag, so feat not Feat (note: you generally should capilize the initial word of the title, but classification tags are an exception, in the same way as you wouldn't capilize a variable name (if it's not capitalized in code) even if its the first word in a sentence.)
Keep the commit message titles (i.e. the inital line of the message) short, and use the commit body to fully describe the change, rather than trying to put everything into the commit title. (note: the common advice is the 50/72 rule, i.e. the title should be at most 50 characters, and the commit body should be formatted to 72-column lines.

setrofim · 2026-02-05T09:34:48Z

comid/classid.go

 	}
 }

+type IClassIDValue interface {


Why was this moved below SetUUID and GetUUID mothod definitions? Please revert to the original location.

setrofim · 2026-02-05T09:39:16Z

comid/classid.go

+// RegisterClassIDFactory registers a new IClassIDValue factory without registering
+// a CBOR tag. Use this when the underlying CBOR tag is already registered (e.g., for
+// profile-specific types that reuse existing CBOR tags).
+func RegisterClassIDFactory(factory IClassIDFactory) error {


This doesn't seem right. A CBOR tag identifies the type. You cannot have the same tag map to different types. And a profile cannot "re-use" a tag for a different type. This will cause problems.

Agreed—one tag must map to one type. The factory was a leftover from exploring profile conveniences; I’ve removed it and will push the fix with the other requested changes

setrofim · 2026-02-05T09:41:18Z

comid/cryptokeys_test.go

+		Add(MustNewPKIXBase64Key(TestECPubKey))
+
+	result := keys.String()
+	assert.Contains(t, result, "[")


I'm not sure what these asserts are testing? Since all the inputs are known, you should be comparing to the exact expected output, not just that output features square brackets.

setrofim · 2026-02-05T09:43:31Z

comid/ueid_test.go

 	}
 }
+
+func Test_UEID_Empty(t *testing.T) {


Suggested change

func Test_UEID_Empty(t *testing.T) {

func TestUEID_Empty(t *testing.T) {

setrofim · 2026-02-05T09:49:27Z

corim/unsignedcorim_test.go

+	// NOTE: Test skipped because CBOR test files contain PSA-specific types (tag 601)
+	// that are now registered only in profiles/psa, not in the base comid package.
+	// TODO: Regenerate CBOR test files using generic types (UUID, bytes, etc.)
+	t.Skip("Test uses PSA-specific CBOR tags in test data; regenerate with generic types")


Instead of skipping, you should update the offending inputs to not include the now-obsolete types (since which just need a valid CoRIM for this, you can replace them with any valid alteranative, e.g. a UUID instead of PSA ImplID).

setrofim · 2026-02-05T09:52:51Z

corim/unsignedcorim_test.go

+
+	var decoded UnsignedCorim
+	err = decoded.FromJSON(jsonData)
+	assert.NoError(t, err)


In addition to making sure that there is no error, you should also check to make sure that decoded is what you expect (i.e. you can do this by comparing some if its fields to the equivalents inside valid)

profiles/psa/psa.go

setrofim · 2026-02-05T10:05:54Z

profiles/psa/implid.go

+	"github.com/veraison/corim/comid"
+)
+
+const ImplIDType = "psa.impl-id"


You should be getting rid of this type. The implementation ID should now be represented by TaggedBytes, not a custom type. The addtional validation should be done as on top of the type via the validation methods (which you are already doing).

This also means yo do not need the (broken) factory registration function above.

setrofim · 2026-02-05T11:00:45Z

profiles/psa/psa.go

+
+// TriplesExtensions carries PSA-specific fields and constraints for Triples
+type TriplesExtensions struct {
+	PsaSwRelTriples *PsaSwRelTriples `cbor:"5,keyasint,omitempty" json:"psa-swrel-triples,omitempty"`


Why is this field necessary? You should not be defining any extension fields -- you just want the extension for its validation methods. (Also, code point 5 clashes with domain-membership-triple-record define by the CoRIM draft).

yogeshbdeshpande · 2026-02-09T09:59:42Z

comid/classid.go

+// The exact encoding is <CLASS_ID_TYPE> dependent. For the base implementation
+// types it is


Suggested change

// The exact encoding is <CLASS_ID_TYPE> dependent. For the base implementation

// types it is

// The exact encoding is <CLASS_ID_TYPE> dependent.

yogeshbdeshpande · 2026-02-09T10:01:38Z

comid/classid_test.go

-	tv := MustHexDecode(t, "d90258582061636d652d696d706c656d656e746174696f6e2d69642d303030303030303031")
-
-	expected := b64TestImplID()
+	tv := MustHexDecode(t, "d90230582061636d652d696d706c656d656e746174696f6e2d69642d303030303030303031")


When the Implementation ID goes in the profile section of the corim repo, then the ImplID based ClassID test should also go their.

yogeshbdeshpande · 2026-02-09T10:03:48Z

comid/example_psa_refval_test.go

@@ -2,150 +2,3 @@
 // SPDX-License-Identifier: Apache-2.0

 package comid


I think, you forgot to delete the file, but just left a file with package comid.

Please check and delete this file completely.

yogeshbdeshpande · 2026-02-09T10:05:47Z

comid/measurement.go

-}
-
 // IMkeyFactory defines the signature for the factory functions that may be
 // registred using RegisterMkeyType to provide a new implementation of the


Suggested change

// registred using RegisterMkeyType to provide a new implementation of the

// registered using RegisterMkeyType to provide a new implementation of the

yogeshbdeshpande

some more comments- will complete the review by end of the day today!!!

yogeshbdeshpande · 2026-02-09T10:32:27Z

corim/example_profile_test.go

 // ----- end of profile definition -----
 // The following code demonstrates how the profile might be used.

+// NOTE: Example_profile_unmarshal has been temporarily disabled because the test


Either create a github issue to track this pending work, or make sure you correct this before the submission of this Pull Request. Either way it is fine...

i think i need to replace PSA-specific types in the yaml test data with generic types(uuid for ex) that don't require separate registration, then regenerate the CBOR test files , right?

Yes essentially the old changes need to be removed and the current newly introduced types need to replace the same and then re-generate the CBOR from yaml files

yogeshbdeshpande · 2026-02-09T10:33:43Z

corim/profiles_test.go

 }

 func TestProfileManifest_Marshaling_UnMarshaling(t *testing.T) {
+	// NOTE: This test is skipped because it uses test data containing PSA-specific


Same comment, either create a new issue to complete the work or ensure this is complete and re-submit for the review!!

yogeshbdeshpande

Completed review, some comments have been left for you to review.

Rest all Looks Good to me.

Once you complete these minor comments, I will Approve!!!!

yogeshbdeshpande

see my response!!!

yogeshbdeshpande · 2026-02-09T12:48:20Z

corim/example_profile_test.go

 // ----- end of profile definition -----
 // The following code demonstrates how the profile might be used.

+// NOTE: Example_profile_unmarshal has been temporarily disabled because the test


i think i need to replace PSA-specific types in the yaml test data with generic types(uuid for ex) that don't require separate registration, then regenerate the CBOR test files , right?

Yes essentially the old changes need to be removed and the current newly introduced types need to replace the same and then re-generate the CBOR from yaml files

feat(profiles/psa): extract and consolidate PSA logic into profile package Signed-off-by: Abhishek kumar <[email protected]>

abhiraj-ku · 2026-02-09T12:58:29Z

@yogeshbdeshpande please check, i have pushed the changes

yogeshbdeshpande

LGTM! I have reviewed the changes and thanks for incorporating comments!

setrofim

Also minor nit: the "update class-ID tag from 600 to 560 in test fixtures" should be fixed-up into the prior commit. Test updates should be part of the same commit that necessitates them. (the title also lacks the conventional commits category tag)

setrofim · 2026-02-10T10:09:04Z

comid/classid_test.go

@@ -69,16 +54,15 @@ func TestClassID_UnmarshalCBOR_UUID_OK(t *testing.T) {
 }

 func TestClassID_UnmarshalCBOR_ImplID_OK(t *testing.T) {


This test should probably re named

Suggested change

func TestClassID_UnmarshalCBOR_ImplID_OK(t *testing.T) {

func TestClassID_UnmarshalCBOR_bytes_OK(t *testing.T) {

setrofim · 2026-02-10T10:09:55Z

comid/cryptokeys_test.go

@@ -1,9 +1,10 @@
-// Copyright 2023 Contributors to the Veraison project.
+// Copyright 2026 Contributors to the Veraison project.


Suggested change

// Copyright 2026 Contributors to the Veraison project.

// Copyright 2023-2026 Contributors to the Veraison project.

setrofim · 2026-02-10T10:14:46Z

comid/measurement.go


+// RegisterMkeyFactory registers a new IMKeyValue factory without registering
+// a CBOR tag. Use this when the underlying CBOR tag is already registered.
+func RegisterMkeyFactory(factory IMkeyFactory) error {


As stated in the earlier review, this should not exist. Registration of an new extension type must always be accompanied by a tag, otherwise CBOR decoding wont work.

(this should also not be necessary for this pull request, as you should not be registering any new key types).

setrofim · 2026-02-10T10:16:12Z

comid/ueid_test.go

@@ -1,4 +1,4 @@
-// Copyright 2024 Contributors to the Veraison project.
+// Copyright 2026 Contributors to the Veraison project.


Suggested change

// Copyright 2026 Contributors to the Veraison project.

// Copyright 2024-2026 Contributors to the Veraison project.

setrofim · 2026-02-10T10:16:25Z

comid/uuid_test.go

@@ -1,4 +1,4 @@
-// Copyright 2024 Contributors to the Veraison project.
+// Copyright 2026 Contributors to the Veraison project.


Suggested change

// Copyright 2026 Contributors to the Veraison project.

// Copyright 2024-2026 Contributors to the Veraison project.

setrofim · 2026-02-10T10:26:03Z

profiles/psa/implid.go

+		if nb := len(t); nb != ImplIDSize {
+			return nil, fmt.Errorf("bad psa.impl-id: got %d bytes, want %d", nb, ImplIDSize)
+		}
+		implIDBytes = make([]byte, ImplIDSize)


You don't need to be doing this inside every case -- you can just initialize the implIDBytes once at the beginning of the function, since it's always initialized to a 32-byte buffer, regardless of val

setrofim · 2026-02-10T10:42:36Z

profiles/psa/psa.go

+
+	// Register PSA-specific Mkey type (psa.refval-id) with CBOR tag 601
+	// This registers both the CBOR tag and the factory
+	if err := comid.RegisterMkeyType(PSARefValIDTag, NewMkeyPSARefvalID); err != nil {


No. You should not be registering a new type here. I'm not sure where you're getting this from?

Section 3.3 of the spec states that the value of the mkey MUST be "psa.software-component", i.e. it's a string (which is one of the standard CoRIM types for mkey) with a specific value. This is something you should be validating as part of your ValidTriples below, but you don't need to be registering a type for it.

setrofim · 2026-02-10T10:44:07Z

profiles/psa/psa.go

+func (o TriplesExtensions) ValidTriples(triples *comid.Triples) error {
+	// Validate Reference Values (Section 3.3)
+	if triples.ReferenceValues != nil {
+		refVals := (*extensions.Collection[comid.ValueTriple, *comid.ValueTriple])(triples.ReferenceValues)


This shouldn't be necessary. The for loop can just be for i, refVal := range triples.ReferenceValues.Values {

setrofim · 2026-02-10T10:44:59Z

profiles/psa/psa.go

+	}
+
+	// Access the Measurements collection
+	measurements := (*extensions.Collection[comid.Measurement, *comid.Measurement])(&refVal.Measurements)


again, this is not necessary, you can just directly use refVal.Measurements.Values

setrofim · 2026-02-10T10:46:42Z

profiles/psa/psarefvalid.go

I think you should be able to just completely get rid of this.

abhiraj-ku force-pushed the impl-id-sign-id-ver branch 2 times, most recently from e8cb9a4 to 2554414 Compare January 29, 2026 14:52

abhiraj-ku added 3 commits January 29, 2026 20:25

comid: ensure implementaition ID and signer ID field values are of ty…

0615aa5

…pe TaggedBytes Signed-off-by: Abhishek kumar <[email protected]>

update class-ID tag from 600 to 560 in test fixtures

815040b

Signed-off-by: Abhishek kumar <[email protected]>

feat(profiles/psa): extract and consolidate PSA logic into profile pa…

5bdb172

…ckage Signed-off-by: Abhishek kumar <[email protected]>

abhiraj-ku force-pushed the impl-id-sign-id-ver branch from 2554414 to 45a470e Compare January 29, 2026 14:56

thomas-fossati approved these changes Feb 2, 2026

View reviewed changes

yogeshbdeshpande reviewed Feb 3, 2026

View reviewed changes

yogeshbdeshpande requested changes Feb 3, 2026

View reviewed changes

yogeshbdeshpande reviewed Feb 3, 2026

View reviewed changes

setrofim requested changes Feb 5, 2026

View reviewed changes

abhiraj-ku force-pushed the impl-id-sign-id-ver branch from 45a470e to e668332 Compare February 7, 2026 12:07

yogeshbdeshpande reviewed Feb 9, 2026

View reviewed changes

refactor(comid): remove PSA and CCA specific types and cleanup

99eab24

feat(profiles/psa): extract and consolidate PSA logic into profile package Signed-off-by: Abhishek kumar <[email protected]>

abhiraj-ku force-pushed the impl-id-sign-id-ver branch from e668332 to 99eab24 Compare February 9, 2026 12:56

yogeshbdeshpande approved these changes Feb 9, 2026

View reviewed changes

setrofim requested changes Feb 10, 2026

View reviewed changes

		@@ -0,0 +1,122 @@
		// Copyright 2021-2024 Contributors to the Veraison project.

	func Test_UEID_Empty(t *testing.T) {
	func TestUEID_Empty(t *testing.T) {

		// The exact encoding is <CLASS_ID_TYPE> dependent. For the base implementation
		// types it is

		@@ -2,150 +2,3 @@
		// SPDX-License-Identifier: Apache-2.0

		package comid

	// registred using RegisterMkeyType to provide a new implementation of the
	// registered using RegisterMkeyType to provide a new implementation of the

		@@ -69,16 +54,15 @@ func TestClassID_UnmarshalCBOR_UUID_OK(t *testing.T) {
		}

		func TestClassID_UnmarshalCBOR_ImplID_OK(t *testing.T) {

	func TestClassID_UnmarshalCBOR_ImplID_OK(t *testing.T) {
	func TestClassID_UnmarshalCBOR_bytes_OK(t *testing.T) {

		@@ -1,9 +1,10 @@
		// Copyright 2023 Contributors to the Veraison project.
		// Copyright 2026 Contributors to the Veraison project.

	// Copyright 2026 Contributors to the Veraison project.
	// Copyright 2023-2026 Contributors to the Veraison project.

		@@ -1,4 +1,4 @@
		// Copyright 2024 Contributors to the Veraison project.
		// Copyright 2026 Contributors to the Veraison project.

	// Copyright 2026 Contributors to the Veraison project.
	// Copyright 2024-2026 Contributors to the Veraison project.

Feat(profile/psa): extract and consolidate PSA profile package and removes PSA/CCA-specific types from COMID #253

Are you sure you want to change the base?

Feat(profile/psa): extract and consolidate PSA profile package and removes PSA/CCA-specific types from COMID #253

Conversation

abhiraj-ku commented Jan 29, 2026

Uh oh!

thomas-fossati left a comment

Choose a reason for hiding this comment

Uh oh!

abhiraj-ku commented Feb 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande commented Feb 3, 2026

Uh oh!

abhiraj-ku commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

Uh oh!

setrofim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhiraj-ku commented Feb 9, 2026

Uh oh!

yogeshbdeshpande left a comment

abhiraj-ku commented Feb 3, 2026 •

edited

Loading