feat: implement endorsements lifecycle mgmt. API by shefali-kamal · Pull Request #453 · veraison/services

shefali-kamal · 2026-09-04T08:31:56Z

This PR implements the REST API endpoints and handlers for activation and deactivation of endorsement triples as specified by the spec (See
https://github.com/veraison/docs/blob/main/api/endorsement-provisioning/endorsement-provisioning.yaml).

update some copyrights
use application/concise-problem-details+cbor
add integration-tests

Addresses Issue: #402

setrofim · 2026-09-16T08:05:24Z

+	"github.com/veraison/swid"
+)
+
+type Query struct {


I think it's better if this doesn't clash unnecessarily with the CoSERV Query. Maybe define it as

type Query struct { coserv.Query Profile *eat.Profile `cbor:"265,keyasint,omitempty" json:"profile,omitempty"` }

and then have some additional validation to make sure ResultType is not set.

At the very least, the fields that the two queries have in common should have the same CBOR and JSON keys.

I don't see why they will clash; they're two different types of Query in separate namespaces.

It's not a good idea to embed coserv.Query inside lifecycle.Query. They have some fields in common, but that is where the similarity ends. Right now, it will be simple to validate that ResultType is not set, but it takes away the freedom from lifecycle.Query to evolve separately from coserv.Query. We do not want to keep adding more validation checks if coserv.Query changes in future.

CBOR keys are different, but I don't see how that poses any significant problem. And there are no JSON keys in coserv.Query.

They have some fields in common, but that is where the similarity ends.

I disagree. They're both queries used to select triples/corims from some underlying store. While they are used in different contexts, and so have some differing fields, the similarity is more than incidental -- conceptually, they do the same thing: select triples/corims.

but it takes away the freedom from lifecycle.Query to evolve separately from coserv.Query.

That's kind of the point. I want that freedom taken way, unless it's absolutely needed -- I'd rather not see two query formats diverge when they don't have to. This is why I suggested embedding -- it minimizes the risk of the two queries adding functionally similar fields under different keys in the future.

If you'd rather avoid the validation complications that come with embedding, that's fine, but the key values should still be aligned; and I'd leave a comment here referencing coserv.Query. That way, when someone wants to extend this in the future, they'd at least have something prompting them to have a look at the other structure to make sure this remains aligned in case the other query was already extended in a similar way.

CBOR keys are different, but I don't see how that poses any significant problem. And there are no JSON keys in coserv.Query.

The problem is the unnecessary confusion this will cause during debugging, when you see similar structures in CBOR dumps/diags with jumbled up fields and have to remember which key should correspond to which field in which context. This may not be a huge deal, but on the other hand, the cost to staying consistent is virtually nil; so why diverge?

conceptually, they do the same thing: select triples/corims.

This is true, and that is why EnvironmentSelector and RimSelector are entirely borrowed from coserv.

If you'd rather avoid the validation complications that come with embedding, that's fine, but the key values should still be aligned; and I'd leave a comment here referencing coserv.Query. That way, when someone wants to extend this in the future, they'd at least have something prompting them to have a look at the other structure to make sure this remains aligned in case the other query was already extended in a similar way.

This assumes that coserv.Query will always be ahead of lifecycle.Query. It is possible that some features are needed in the latter that do not make much sense for the former. If coserv.Query is embedded, I need to go there and make the change, which does not sound good. Especially when CoSERV query is an IETF standard and ELM query is somewhat less serious.

For example, having a JSON serialization is more useful for lifeycle.Query than for coserv.Query, because it allows vendors/provisioners to create queries from JSON templates. An issue I raised was related to this concern, where I requested for EnvironmentSelector to support JSON tags.

This is my primary concern, otherwise I'm on board with your idea.

This is true, and that is why EnvironmentSelector and RimSelector are entirely borrowed from coserv.

Exactly; so as well as re-using the field types, you should re-use the field keys (for the same reason that you're re-using the types).

If coserv.Query is embedded, I need to go there and make the change, which does not sound good.

Only if it's warranted for coserv.Query (e.g. if you want to add a new field which is not relevant to coserv you just add it to the embedding struct), and in that case, yes, that's exactly what you should do; this is the point -- we want to avoid accidental divergence. Note that even without embedding you'd still have to do that since you re-using coserv types for the fields anyway.

Especially when CoSERV query is an IETF standard and ELM query is somewhat less serious.

Which is why the burden is on the ELM query to stay current with coserv and not other way round.

For example, having a JSON serialization is more useful for lifeycle.Query than for coserv.Query, because it allows vendors/provisioners to create queries from JSON templates. veraison/corim#282 was related to this concern, where I requested for EnvironmentSelector to support JSON tags.

You're kinda proving my point here. You're re-using the coserv types (which is absolutely the Right Thing to do, rather than re-implementing them); so the burden to keep in sync already exists to an extent. All embedding would do is extend it to the outer struct fields.

Anyway, I am fine with not embedding as long as

the CBOR keys are aligned

There a comment linking this to coserv.Query

Embedding would just mean we won't have to rely on future maintainer reading the comment, which would be preferable.

coserv.Query is now embedded in lifecycle.Query. Updated JSON-related stuff and the above-mentioned issue accordingly.

shefali-kamal · 2026-09-16T15:09:02Z

@setrofim we have addressed the review comments. Please check

thomas-fossati · 2026-09-17T09:45:01Z

 	return allPEM.Bytes(), nil
 }
+
+func (o *GRPC) SetEndorsementsActive(


lacking a better place, I am leaving the comment here: I am confused by the absence of per-tenant scoping of the API. Am I missing something?

I'm a bit unfamiliar with what "per-tenant scoping" means. I noticed tenant IDs being used in some calls and log statements but unclear on how it actually affects the operations. Can you describe this or point me to a reference?

Our multi-tenant support is somewhat vestigial at the moment, but we do try to maintain the plumbing for it. In the other API this is done vial labels that are constructed from the combination of tenant id (currently hard-coded to a dummy value) and the attestation scheme name derived fromm the media type (e.g. https://github.com/veraison/services/blob/main/vts/trustedservices/trustedservices_grpc.go#L274).

However, in this case, the media type just identifies the query, and not a specific scheme; so that approach won't work. Fully supporting this will require updating corim-store to either allow pattern matching for the label or support tenant ID as a distinct field (in retrospect, bundling it into label may not have been the best idea).

For now, however, you can at least make sure that the tenant ID gets propagated from the front-end to the VTS, the way it is done for the other API.

For now, however, you can at least make sure that the tenant ID gets propagated from the front-end to the VTS, the way it is done for the other API.

👍 That sounds like a sensible first step.

Let's also add a tracking issue to veraison/corim-store for the

updat[e] corim-store to either allow pattern matching for the label or support tenant ID as a distinct field (in retrospect, bundling it into label may not have been the best idea).

part

done: veraison/corim-store#29

thomas-fossati · 2026-09-17T09:48:50Z

+	b, err := cbor.Marshal(prob)
+	if err != nil {
+		log.Error(logger, "failed to marshal problem details to CBOR", "error", err)
+		c.AbortWithStatus(status)


Suggested change

c.AbortWithStatus(status)

c.AbortWithStatus(status)

return

thomas-fossati · 2026-09-17T09:50:30Z

 	logFunc("problem encountered", "title", prob.Title, "detail", prob.Detail)
 }
+
+// ConciseProblem is a representation of the problem details structure defined


This is already implemented in coserv/api/handler.go::reportProblem()

You're right, and the reportProblem() function is also same as provisioning/api/common.go:ReportConciseProblem(). But reportProblem() cannot be used as it's unexported.
Should we put this stuff in a common place so that it can be referenced by all?

But we don't want provisioning to be importing from coserv; Though thinking about it, this doesn't belong in log either since this is nothing to do with logging and is related to REST APIs instead.

This is actually a common problem with our frontends -- we don't want them to depend on each other so they end up duplicating a lot of functionality. We used to have an issue to address this (#192) that was closed after being for ages because it seemed that we would never actually get around to it. Maybe it's worth re-openning?

Anyway, short-term, this should probably move into provisioning rather than be in 'log'.

EDIT (sorry, @atulfj, I didn't referesh before posting and didn't see your reply):

Should we put this stuff in a common place so that it can be referenced by all?

That would be the right thing to do, though that would be a bigger change affecting the other front-ends (and other API probably -- actually need evaluate how much can be make common). That is best done as a separate refactor, rather than as part of this pull. For now, just move this out of log into provisioning.

provisioning is a reasonable place, I'll move it there.

I vote for re-opening the issue and doing it incrementally instead of in one big sweep. We can start with moving just ReportProblem() to a common place and let other such stuff settle in over time.

We can start with moving just ReportProblem() to a common place and let other such stuff settle in over time.

I specifically want to avoid doing this piecemeal and instead do this as a single logically-coherent refactor. That way things are less likely to fall through the cracks.

I don't see a problem with taking a piecemeal approach, but I also don't want to block this PR on a refactoring issue.
As long as it's tracked and we promise not to forget about it this time 😄, I'm happy.

I've reopened the issue; so we're now tracking this.

I've moved both ConciseProblem struct and LogConciseProblem() to provisioning (to avoid an import cycle with log). Added a comment there as a reminder to restructure.

thomas-fossati · 2026-09-17T09:55:40Z

+		}
+
+		// read body
+		payload, err := io.ReadAll(c.Request.Body)


Shouldn't we use http.MaxBytesReader?

yep, updated

thomas-fossati · 2026-09-17T09:57:20Z

 		o.logger.Errorw("submit endorsement failed", "error", err)

-		if errors.Is(err, errors.New("no connection")) {
+		if strings.Contains(err.Error(), "no connection") {


this could test vtsclient.NoConnectionError directly

thanks, using it now

setrofim · 2026-09-17T11:23:55Z

+			if strings.Contains(err.Error(), "no connection") {
+				ReportConciseProblem(c,
+					http.StatusInternalServerError,
+					err.Error(),


Do not insert the actual error into the response; that might leak internal server state. Log the actual error here , and in the response, set something generic like "problem updating the store".

Submit() also has this issue. Should I change in all places?

Yes, thank you (please do changes unrelated to lifecycle management as a separate commit).

setrofim · 2026-09-17T11:26:48Z

+
+			o.logger.Errorw(action+" endorsement failed", "error", err)
+
+			if strings.Contains(err.Error(), "no connection") {


I don't think this is a safe thing to do; it's also possible that there could be database-related errors that aren't related to the input (e.g. if the db hasn't been migrated).

I think it's better to invert the logic and detect specific errors (such as ErrNoMatch) to be reported as BadRequest and to report everything else as InteranalServerError.

Catching ErrNoMatch is tricky because VTS only returns string-form error details in a proto/status.proto structure. The best we can do for matching is to compare the strings. Should I go ahead with it or create a better mechanism using grpc's status codes by importing the structure they provide: grpc/status/status.proto?

String matching should be fine for now (that's what you're currently doing anyway). IIRC ErrNoMatch is always a fixed string.

Done for now.

But it seems a broad issue - even in Submit(), we need to infer whether the error was because of the corim being malformed or because of an internal error. If we have a catch-all InternalServerError, the information is not enough (or correct) for the API user to know what the real problem was. So for now, I've avoided inverting the current logic.

thomas-fossati

🚢 it, thanks!

yogeshbdeshpande · 2026-09-23T13:46:54Z

@shefali-kamal I will review it by tomorrow.

This PR implements the REST API endpoints and handlers for activation and deactivation of endorsement triples as specified by the spec (See https://github.com/veraison/docs/blob/main/api/endorsement-provisioning/endorsement-provisioning.yaml). - update some copyrights - use application/concise-problem-details+cbor - add integration-tests Signed-off-by: Kumar, Atul <Atul.Kumar@fujitsu.com>

- catch specific 5xx errors and don't send entire error string in API response Signed-off-by: Atul Kumar <Atul.Kumar@fujitsu.com>

atulfj · 2026-09-25T07:22:19Z

Hi @yogeshbdeshpande , it took some time to rebase with the new changes (endorsement store plugin). You can now review this, thanks!

yogeshbdeshpande · 2026-09-25T09:08:27Z

Hi @atulfj , yes doing it now!!

yogeshbdeshpande

some comments, I will finish it by end of the day today!

yogeshbdeshpande · 2026-09-25T09:09:58Z


 var (
 	// When the store does not support the operation.
 	// Example 1: store supportes scheme A and C, but


Suggested change

// Example 1: store supports scheme A and C, but

yogeshbdeshpande · 2026-09-25T09:12:14Z


 	AddCorimBytes(data []byte, scheme string, activate bool) error
+
+	SetEndorsementsState(label string, request []byte, state bool) error


Just a fundamental question, is representing state as a bool, only allows Active/InActive, which may be ok for now, but when further state of DeActivated is added, how are we going to handle that?

For now, the activation state is a boolean that can be toggled. To handle (permanent) deactivation a.k.a revocation in future, there could be two ways:

change the activation state to an int/string that can represent all the states

add a new DB column just for revocation status (another boolean)

I would prefer option 1. with an int, but right now we're bound to the DB schema that corim-store provides - which is a boolean activation state.

Yes- option 1 is preferred with a pre-defined codes lile 1., 2, and 3 set in the API documentation, and be complied with!

is there an issue you can track for this future enhancement..?

Tracking now in both corim-store and services

veraison/corim-store#30
#475

yogeshbdeshpande · 2026-09-25T09:14:19Z

+message SetEndorsementsStateRequest {
+  string tenant_id = 1;
+  bytes data = 2;
+  bool set_active = 3;


is the name intuitive, should it not be called as activation_status

set_active denotes a "request" to change the activation state, so IMO an imperative name is better

set_active indicates an action that the state is set to Active, which is not always true.

The activation state property is named is_active in corim-store, so set_active (or perhaps set_isactive) can be seen as the boolean value that we're setting is_active to.
I've derived this naming largely from there itself: setActive() in corim-store

yogeshbdeshpande · 2026-09-25T09:26:22Z

+	"github.com/veraison/eat"
+	"github.com/veraison/swid"
+)
+


Please add a suitable comment on line 14, to explain what is the purpose of this query struct and how it encompasses CoServ Queries

yogeshbdeshpande · 2026-09-25T09:27:38Z

this file diff is not viewable, any issues ?

hmm, I can see it.. maybe refresh?

yogeshbdeshpande · 2026-09-25T09:30:54Z

+		}
+	default:
+		// should not get here -- query already validated
+		panic("invalid ELM query")


If the Endorsement State could not be set, why should one Panic, should we NOT inform the caller the cause of Failure so that corrective action can be taken..?

This case will not be reachable due to the validation check on L98, so it's ok to panic.
Earlier discussion regarding this: #453 (comment)

yogeshbdeshpande · 2026-09-25T09:36:29Z

+			return err
+		}
+
+		if _, err := query.Profile.Get(); err == nil {


Is there a mechanism to check existing state and throw a Warnig if no state change has happened, so that the caller can be Warned ..?

That could be helpful, but for an idempotent operation, it should be ok to not warn the caller if no state change happened. A successful response implies that the endorsements are in the state that the caller requested for, whether the state was changed or not.

The intention was to detect accidental series of invocations and flooding the requests!

setrofim reviewed Sep 4, 2026

View reviewed changes

atulfj mentioned this pull request Sep 4, 2026

feature: endorsements activation policy in the provisioning API #454

Open

shefali-kamal force-pushed the endorsement-lifecycle-mgmt-api branch from c75c352 to 0d3626e Compare September 8, 2026 14:49

setrofim reviewed Sep 14, 2026

View reviewed changes

Comment thread provisioning/api/handler.go Outdated

shefali-kamal requested a review from setrofim September 15, 2026 05:21

shefali-kamal force-pushed the endorsement-lifecycle-mgmt-api branch from 0d3626e to a2b25d3 Compare September 15, 2026 14:32

setrofim reviewed Sep 16, 2026

View reviewed changes

shefali-kamal requested a review from setrofim September 16, 2026 15:08

thomas-fossati reviewed Sep 17, 2026

View reviewed changes

setrofim reviewed Sep 17, 2026

View reviewed changes

setrofim mentioned this pull request Sep 17, 2026

HTTP/REST API code restructure #192

Open

atulfj mentioned this pull request Sep 18, 2026

enhancement: add JSON support to EnvironmentSelector veraison/corim#282

Open

shefali-kamal force-pushed the endorsement-lifecycle-mgmt-api branch 2 times, most recently from 3338303 to 4171659 Compare September 21, 2026 05:14

shefali-kamal requested review from setrofim and thomas-fossati September 22, 2026 09:20

setrofim approved these changes Sep 23, 2026

View reviewed changes

thomas-fossati approved these changes Sep 23, 2026

View reviewed changes

atulfj added 2 commits September 24, 2026 18:40

fix: do not send internal logs on server errors

311638b

- catch specific 5xx errors and don't send entire error string in API response Signed-off-by: Atul Kumar <Atul.Kumar@fujitsu.com>

shefali-kamal force-pushed the endorsement-lifecycle-mgmt-api branch from 4171659 to 311638b Compare September 24, 2026 15:52

yogeshbdeshpande reviewed Sep 25, 2026

View reviewed changes

atulfj mentioned this pull request Sep 25, 2026

feature: support permanent deactivation / revocation of endorsements veraison/corim-store#30

Open


		o.logger.Errorw(action+" endorsement failed", "error", err)

		if strings.Contains(err.Error(), "no connection") {


		AddCorimBytes(data []byte, scheme string, activate bool) error

		SetEndorsementsState(label string, request []byte, state bool) error

Conversation

shefali-kamal commented Sep 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

shefali-kamal commented Sep 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

setrofim Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

setrofim Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

setrofim Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

setrofim Sep 17, 2026 •

edited

Loading

setrofim Sep 17, 2026 •

edited

Loading

setrofim Sep 17, 2026 •

edited

Loading