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.

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>

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 from a2b25d3 to 3338303 Compare September 18, 2026 14:31

atulfj added 2 commits September 21, 2026 10:16

fix: do not send internal logs on server errors

4171659

- 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 3338303 to 4171659 Compare September 21, 2026 05:14


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

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

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