Feature/longfellow zk/verification#289
Feature/longfellow zk/verification#289yimyitbarek wants to merge 13 commits intoSUNET:masv/infra/releasefrom
Conversation
| return reply, nil | ||
| } | ||
|
|
||
| func (c *Client) Verify(ctx context.Context, req *VerifyRequest) (*VerifyResponse, error) { |
There was a problem hiding this comment.
We should call this something less generic - eg VerifyZKP. Same goes for VerifyRequest -> VerifyZKPRequest etc
cmd/verifier/main.go
Outdated
| "vc/pkg/trace" | ||
|
|
||
| "flag" | ||
| "vc/internal/verifier/zk" |
There was a problem hiding this comment.
this package should be included in the PR
cmd/verifier/main.go
Outdated
| ctx = context.Background() | ||
| services = make(map[string]service) | ||
| serviceName string = "verifier" | ||
| certs = flag.String("cacerts", "/app/vc/internal/verifier/zk/certs.pem", "File containing issuer CA certs") |
There was a problem hiding this comment.
use config file, no flags please.
| return reply, nil | ||
| } | ||
|
|
||
| func (c *Client) Verify(ctx context.Context, req *VerifyRequest) (*VerifyResponse, error) { |
There was a problem hiding this comment.
Verifiy what? Must be more precise.
| "github.com/lestrrat-go/jwx/v3/jwe" | ||
|
|
||
| "encoding/hex" | ||
| "vc/internal/verifier/zk" |
dockerfiles/verifier.Dockerfile
Outdated
| RUN find /app -name "mdoc_zk.h" && find /app -name "*.a" | ||
|
|
||
| RUN --mount=type=cache,target=/root/.cache/go-build \ | ||
| CGO_ENABLED=1 \ |
There was a problem hiding this comment.
since zk require cgo, consider using a build tag: zk for this.
There was a problem hiding this comment.
in the same way as pkcs11 support is added.
|
please, test this. |
There was a problem hiding this comment.
Pull request overview
Implements Longfellow-ZK proof verification in the verifier service, adding a new HTTP endpoint that accepts proof material as JSON, initializes/verifies using Longfellow circuits, and returns verification status plus extracted claims.
Changes:
- Adds
/verification/verifyPOST endpoint in the verifier HTTP server. - Introduces
VerifyRequest/VerifyResponsetypes and anapiv1.Client.Verify()handler that decodes inputs and calls thezkverifier. - Adds Docker/Makefile wiring and verifier startup initialization for loading circuits and issuer CA certs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/verifier/httpserver/service.go | Adds verify route and modifies server timeouts. |
| internal/verifier/httpserver/endpoint_verification.go | Implements the HTTP handler for /verification/verify. |
| internal/verifier/httpserver/api.go | Extends the Apiv1 interface with Verify(). |
| internal/verifier/apiv1/handlers_verification.go | Adds request/response models and implements Client.Verify() calling zk. |
| dockerfiles/verifier.Dockerfile | New multi-stage build for CGO + ZK libs and runtime packaging. |
| cmd/verifier/main.go | Loads circuits and issuer CA at startup via flags. |
| Makefile | Adds zk fetch/copy target and a verifier Docker build target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Makefile
Outdated
| zk: | ||
| $(info Fetching Longfellow ZK) | ||
| mkdir -p build | ||
| if [ -d build/longfellow-zk ]; then \ | ||
| cd build/longfellow-zk && git pull; \ | ||
| else \ | ||
| git clone https://github.com/google/longfellow-zk.git build/longfellow-zk; \ | ||
| fi | ||
| rm -rf $(ZK_DST) | ||
| cp -r $(ZK_SRC) $(ZK_DST) | ||
| cp -r $(ZK_LIB_SRC) $(ZK_LIB_DST) | ||
| cp -r $(ZK_CERT_SRC) $(ZK_DST) | ||
|
|
There was a problem hiding this comment.
The zk target deletes and recreates internal/verifier/zk by cloning an external repository. Since the verifier code now imports vc/internal/verifier/zk, a plain go build ./... (or CI unit tests) will fail unless this target is run first. Consider integrating this fetch/generation step into the normal build pipeline (or using build tags / vendoring) so default builds/tests remain reproducible.
| server: &http.Server{ | ||
| ReadHeaderTimeout: 3 * time.Second, | ||
| ReadHeaderTimeout: 300 * time.Second, | ||
| ReadTimeout: 300 * time.Second, | ||
| WriteTimeout: 300 * time.Second, | ||
| IdleTimeout: 300 * time.Second, | ||
| }, |
There was a problem hiding this comment.
The verifier HTTP server timeouts were increased to 300s across the board. This is a significant regression compared to other services (which use a 3s ReadHeaderTimeout) and can make the server much more susceptible to slow-client / Slowloris-style resource exhaustion. Consider keeping ReadHeaderTimeout small (seconds), and only increasing ReadTimeout/WriteTimeout for the specific endpoint(s) that need it (or make these values configurable).
| // Longfellow-zk verifier | ||
| s.httpHelpers.Server.RegEndpoint(ctx, rgOIDCVerification, http.MethodPost, "verify", http.StatusOK, s.endpointVerify) | ||
|
|
There was a problem hiding this comment.
The new /verification/verify endpoint is registered without any rate limiting middleware. Proof verification is typically CPU-expensive, so leaving it unthrottled can enable trivial DoS. Consider adding a dedicated rate limiter for this endpoint (similar to token/authorize/register) or reusing an existing limiter with appropriate limits.
| func (c *Client) Verify(ctx context.Context, req *VerifyRequest) (*VerifyResponse, error) { | ||
| c.log.Debug("Processing ZK Proof", "transcript_len", len(req.Transcript)) | ||
| transcriptBytes, err := base64.StdEncoding.DecodeString(req.Transcript) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode transcript: %w", err) | ||
| } | ||
|
|
||
| cborBytes, err := base64.StdEncoding.DecodeString(req.ZKDeviceResponseCBOR) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decode device response: %w", err) | ||
| } | ||
|
|
||
| vreq, err := zk.ProcessDeviceResponse(cborBytes) | ||
| if err != nil { | ||
| c.log.Error(err, "CBOR processing failed") | ||
| return nil, fmt.Errorf("error processing cbor request: %w", err) | ||
| } | ||
|
|
||
| vreq.Transcript = transcriptBytes | ||
| ok, err := zk.VerifyProofRequest(vreq) | ||
|
|
There was a problem hiding this comment.
There are existing tests in this package (handlers_verification_test.go) but the new Verify() behavior is untested. Adding tests for base64 decode failures, CBOR processing errors, and the "invalid proof => no claims" behavior would help prevent regressions.
cmd/verifier/main.go
Outdated
| pem, err := os.ReadFile(*certs) | ||
| if err != nil { | ||
| panic("could not parse cacerts file") | ||
| os.Exit(1) | ||
| } | ||
| if err := zk.LoadIssuerRootCA(pem); err != nil { | ||
| panic("could not load issuer root CA") | ||
| os.Exit(1) | ||
| } |
There was a problem hiding this comment.
This error handling uses panic() and then calls os.Exit(1) (which is unreachable after panic). It also drops the underlying error, making diagnosis difficult. Prefer returning/logging the actual error and exiting cleanly (or just panic with the wrapped error, without os.Exit).
| func (s *Service) endpointVerify(ctx context.Context, c *gin.Context) (any, error) { | ||
| s.log.Debug("endpointVerificationDirectPost called") | ||
| c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, 2*1024*1024) |
There was a problem hiding this comment.
The debug log message inside endpointVerify says "endpointVerificationDirectPost called", which looks like a copy/paste error and makes tracing harder. Update it to accurately reflect the handler name (e.g., endpointVerify).
|
|
||
| "encoding/hex" | ||
| "vc/internal/verifier/zk" | ||
|
|
There was a problem hiding this comment.
This change introduces a dependency on the internal/verifier/zk package, but that package is not present in the repository (no directory/files found). As-is, the verifier will not compile. Either add the zk Go package/CGo bindings to the repo, or gate the integration behind a build tag and keep the default build working without running a separate fetch step.
| apiClaims := make([]ClaimElement, 0) | ||
|
|
||
| for _, items := range vreq.Claims { | ||
| for _, item := range items { | ||
| hexValue := hex.EncodeToString(item.ElementValue) | ||
|
|
||
| apiClaims = append(apiClaims, ClaimElement{ | ||
| ElementIdentifier: item.ElementIdentifier, | ||
| ElementValue: hexValue, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| //TODO: support more vc types | ||
| reply := &VerifyResponse{ | ||
| Status: ok, | ||
| Claims: map[string][]ClaimElement{ | ||
| "org.iso.18013.5.1": apiClaims, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Verify() returns parsed claims even when proof verification fails (Status=false / err!=nil). This can lead callers to accidentally consume unverified data (or enables the endpoint to be used as a "claims echo" service). Consider only returning claims when verification succeeds, and returning an empty claims map (or an error/4xx) on invalid proofs.
dockerfiles/verifier.Dockerfile
Outdated
| WORKDIR /app/vc/internal/verifier/zk | ||
| COPY ../internal/verifier/zk/lib ./lib | ||
| COPY ../internal/verifier/zk/lib/circuits/mdoc/circuits ./server/circuits/LONGFELLOW_V1 | ||
|
|
There was a problem hiding this comment.
These COPY instructions reference paths outside the Docker build context ("../internal/..."), which Docker will reject when building with context '.'. Update COPY sources to be relative to the build context root (e.g., "internal/verifier/zk/..."), or adjust the build context accordingly.
dockerfiles/verifier.Dockerfile
Outdated
| COPY --from=builder /app/vc/internal/verifier/zk/install/lib /usr/local/lib/ | ||
| COPY ../internal/verifier/zk/certs.pem /app/vc/internal/verifier/zk/certs.pem | ||
|
|
There was a problem hiding this comment.
This COPY also references a path outside the Docker build context ("../internal/verifier/zk/certs.pem"), which will fail for the same reason as the earlier COPY lines. Make the source path relative to the build context root.
|
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestVerifyZKP_Disabled(t *testing.T) { |
There was a problem hiding this comment.
Why is a test needed for functions not compiled in?
| if err != nil { | ||
| panic(err) | ||
| } | ||
| if cfg.Verifier == nil { |
| @@ -0,0 +1,54 @@ | |||
| //go:build zk | |||
|
|
|||
| package apiv1 | |||
There was a problem hiding this comment.
something is off here.. does internal/verifier/apiv1/handlers_verification_test.go the same thing as this file?
| "encoding/hex" | ||
| "fmt" | ||
|
|
||
| "proofs/server/v2/zk" // The CGO-heavy library |
| // Trust holds the trust evaluation configuration | ||
| Trust TrustConfig `yaml:"trust,omitempty"` | ||
| // ZKConfig is the longfellow-zk configuration | ||
| ZK ZKConfig `yaml:"zk" validate:"required"` |
There was a problem hiding this comment.
validate:"required" will hit even if the binary is not compiled with zk buid tag.
| } | ||
| } | ||
|
|
||
| func TestVerifyZKP(t *testing.T) { |
There was a problem hiding this comment.
needs to be in separate file with build restriction
| tracer: tracer, | ||
| server: &http.Server{ | ||
| ReadHeaderTimeout: 3 * time.Second, | ||
| ReadHeaderTimeout: 300 * time.Second, |
| container_name: "vc_dev_verifier" | ||
| hostname: "verifier.vc.docker" | ||
| image: docker.sunet.se/iam_vc/verifier:latest | ||
| image: verifier |
There was a problem hiding this comment.
use docker.sunet.se/iam_vc/verifier:local (local per latest commit)
make docker-build should build to that tag.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 25 changed files in this pull request and generated 18 comments.
Comments suppressed due to low confidence (1)
go.mod:221
- The module uses a local filesystem replace to /tmp/longfellow-zk/... which will not exist in CI or for other developers, making builds non-reproducible. Replace this with a real module dependency (versioned VCS module path), or vendor the dependency without relying on an absolute /tmp replace (e.g., use a relative path within the repo or publish a module).
replace proofs/server/v2 => /tmp/longfellow-zk/reference/verifier-service/server
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Longfellow-zk verifier | ||
| s.httpHelpers.Server.RegEndpoint(ctx, rgOIDCVerification, http.MethodPost, "verifyzkp", http.StatusOK, s.endpointVerifyZKP) |
There was a problem hiding this comment.
PR description says to call Verifier_URL/verification/verify, but the route registered here is POST /verification/verifyzkp. Either update the route to match the documented path or update the PR description/docs so integrators hit the correct endpoint.
| test-all-tags: ## Test with all build tags | ||
| $(info Testing with all build tags) | ||
| go test -tags "$(SAML_TAG),$(OIDCRP_TAG),$(VC20_TAG),$(PKCS11_TAG)" -v ./... | ||
| go test -tags "$(SAML_TAG),$(OIDCRP_TAG),$(VC20_TAG),$(PKCS11_TAG), $(ZK_TAG)" -v ./... |
There was a problem hiding this comment.
The -tags value here includes a space before $(ZK_TAG) inside the quoted, comma-separated list. That space becomes part of the tag name and can make go test fail with an invalid build tag. Remove the extra space so the tag list is well-formed.
| go test -tags "$(SAML_TAG),$(OIDCRP_TAG),$(VC20_TAG),$(PKCS11_TAG), $(ZK_TAG)" -v ./... | |
| go test -tags "$(SAML_TAG),$(OIDCRP_TAG),$(VC20_TAG),$(PKCS11_TAG),$(ZK_TAG)" -v ./... |
| # 1. Clone the external dependency | ||
| RUN git clone https://github.com/google/longfellow-zk.git /tmp/longfellow-zk |
There was a problem hiding this comment.
This build clones https://github.com/google/longfellow-zk.git without pinning a commit/tag, so Docker builds become non-reproducible and can break when upstream changes. Consider checking out a specific commit SHA or release tag (and documenting/updating it intentionally) to make builds deterministic.
| # 1. Clone the external dependency | |
| RUN git clone https://github.com/google/longfellow-zk.git /tmp/longfellow-zk | |
| # 1. Clone the external dependency at a specific commit or tag for reproducible builds | |
| ARG LONGFELLOW_ZK_COMMIT | |
| RUN git clone https://github.com/google/longfellow-zk.git /tmp/longfellow-zk && \ | |
| cd /tmp/longfellow-zk && \ | |
| git checkout "${LONGFELLOW_ZK_COMMIT}" |
| @@ -215,3 +216,5 @@ require ( | |||
| gopkg.in/yaml.v3 v3.0.1 // indirect | |||
| nhooyr.io/websocket v1.8.17 // indirect | |||
There was a problem hiding this comment.
go.mod still requires nhooyr.io/websocket, but the corresponding go.sum entries were removed in this PR. This will break go mod tidy/module verification in CI. Either restore the go.sum entries or run go mod tidy so go.mod/go.sum are consistent (and remove the require if it is no longer needed).
| nhooyr.io/websocket v1.8.17 // indirect |
| pem, err := os.ReadFile(certs) | ||
| if err != nil { | ||
| panic("could not parse cacerts file") | ||
| } | ||
| if err := zk.LoadIssuerRootCA(pem); err != nil { |
There was a problem hiding this comment.
The startup panics on CA cert loading drop the underlying error context (e.g., "could not parse cacerts file"). Include the actual err (and differentiate read vs parse failures) to make deployments diagnosable.
| Transcript string `json:"Transcript"` | ||
| ZKDeviceResponseCBOR string `json:"ZKDeviceResponseCBOR"` | ||
| } | ||
|
|
||
| type ClaimElement struct { | ||
| ElementIdentifier string `json:"ElementIdentifier"` | ||
| ElementValue string `json:"ElementValue"` | ||
| } | ||
|
|
||
| type VerifyZKPResponse struct { | ||
| Status bool `json:"Status"` | ||
| Claims map[string][]ClaimElement `json:"Claims"` |
There was a problem hiding this comment.
These response/claim JSON tags are also PascalCase (ElementIdentifier, ElementValue), which is inconsistent with other API responses in this repo and may break client expectations. Consider using lower-case / snake_case JSON names for consistency.
| Transcript string `json:"Transcript"` | |
| ZKDeviceResponseCBOR string `json:"ZKDeviceResponseCBOR"` | |
| } | |
| type ClaimElement struct { | |
| ElementIdentifier string `json:"ElementIdentifier"` | |
| ElementValue string `json:"ElementValue"` | |
| } | |
| type VerifyZKPResponse struct { | |
| Status bool `json:"Status"` | |
| Claims map[string][]ClaimElement `json:"Claims"` | |
| Transcript string `json:"transcript"` | |
| ZKDeviceResponseCBOR string `json:"zk_device_response_cbor"` | |
| } | |
| type ClaimElement struct { | |
| ElementIdentifier string `json:"element_identifier"` | |
| ElementValue string `json:"element_value"` | |
| } | |
| type VerifyZKPResponse struct { | |
| Status bool `json:"status"` | |
| Claims map[string][]ClaimElement `json:"claims"` |
| // Using your requested error string | ||
| return nil, fmt.Errorf("no item in credential cache matching id %s", req.Transcript) |
There was a problem hiding this comment.
The !zk implementation returns an error message about the credential cache ("no item in credential cache...") which is unrelated to ZK proof verification and will confuse API consumers. Return an error that clearly states ZK verification is disabled/not supported (ideally mappable to 501 Not Implemented).
| // Using your requested error string | |
| return nil, fmt.Errorf("no item in credential cache matching id %s", req.Transcript) | |
| // ZK verification is not available in !zk builds; return a clear "not supported" error | |
| return nil, fmt.Errorf("zk proof verification is disabled in this build (no zk support enabled)") |
| vreq.Transcript = transcriptBytes | ||
| ok, err := zk.VerifyProofRequest(vreq) | ||
|
|
There was a problem hiding this comment.
VerifyProofRequest returns (bool, error), but the error is not handled. If verification fails with an error, the handler currently proceeds to build a response anyway. Treat err != nil as a request failure (or map it to an explicit failure response).
| if err != nil { | ||
| c.log.Error(err, "invalid proof detected") | ||
| } | ||
|
|
||
| return reply, nil |
There was a problem hiding this comment.
When VerifyProofRequest returns an error, it is only logged and the handler still returns (reply, nil). This makes failed verification indistinguishable from success at the HTTP layer. Return a non-nil error (and/or an appropriate HTTP status) when verification fails.
| Transcript string `json:"Transcript"` | ||
| ZKDeviceResponseCBOR string `json:"ZKDeviceResponseCBOR"` | ||
| } | ||
|
|
||
| type ClaimElement struct { | ||
| ElementIdentifier string `json:"ElementIdentifier"` | ||
| ElementValue string `json:"ElementValue"` | ||
| } | ||
|
|
||
| type VerifyZKPResponse struct { | ||
| Status bool `json:"Status"` | ||
| Claims map[string][]ClaimElement `json:"Claims"` |
There was a problem hiding this comment.
The new ZK request struct uses PascalCase JSON field names (e.g., json:"Transcript"), which is inconsistent with the existing verifier API (typically lower-case / snake_case). Consider switching to lower-case JSON names (and add binding:"required" if appropriate) to match established API conventions.
| Transcript string `json:"Transcript"` | |
| ZKDeviceResponseCBOR string `json:"ZKDeviceResponseCBOR"` | |
| } | |
| type ClaimElement struct { | |
| ElementIdentifier string `json:"ElementIdentifier"` | |
| ElementValue string `json:"ElementValue"` | |
| } | |
| type VerifyZKPResponse struct { | |
| Status bool `json:"Status"` | |
| Claims map[string][]ClaimElement `json:"Claims"` | |
| Transcript string `json:"transcript"` | |
| ZKDeviceResponseCBOR string `json:"zk_device_response_cbor"` | |
| } | |
| type ClaimElement struct { | |
| ElementIdentifier string `json:"element_identifier"` | |
| ElementValue string `json:"element_value"` | |
| } | |
| type VerifyZKPResponse struct { | |
| Status bool `json:"status"` | |
| Claims map[string][]ClaimElement `json:"claims"` |


Summary
This PR implements longfellow-zk on the verifier side as per https://github.com/google/longfellow-zk/tree/main/reference/verifier-service/server.
Type of change
Related issues / tickets
Changes
Architecture decisions
Screenshots / recordings (if UI changes)
How to test
1, Deploy the verifier.
2, Get an example proof from https://github.com/google/longfellow-zk/tree/main/reference/verifier-service/server/examples
3, Make a call to Verifier_URL/verification/verify.
Checklist
Notes for reviewers