Feature/longfellow zk/verification - #327
Conversation
|
| identifier: "https://issuer.sunet.se" | ||
| wallet_url: "" | ||
| issuer_url: "http://apigw.vc.docker:8080" | ||
| signing_key_path: "/pki/signing_ec_private.pem" |
There was a problem hiding this comment.
use key_config, it has capability to use pkcs11 as well as crypto material on disk.
|
|
||
| issuer: | ||
| issuer_url: "http://apigw.vc.docker:8080" | ||
| identifier: "https://issuer.sunet.se" |
There was a problem hiding this comment.
identifier is not used anymore.
| # This must match the DocType in your curl | ||
| doc_type: "org.iso.18013.5.1.mDL" | ||
| certificate_chain_path: "/pki/signing_ec_chain.pem" | ||
| # This is the namespace your Rust code expects |
| static_host: "http://vc_dev_portal:8080/statics" | ||
| mdoc: | ||
| valid_duration: 3600 | ||
| signing_key_path: "/pki/signing_ec_private.pem" |
| - "eduid" | ||
| zk: | ||
| ca_certs_path: "/app/vc/internal/verifier/zk/certs.pem" | ||
| circuits_path: "/app/vc/internal/verifier/zk/circuits/" |
There was a problem hiding this comment.
is circuits_path the same as a path to a lib?
| reply := &VerifyZKPResponse{ | ||
| Status: ok, | ||
| Claims: map[string][]ClaimElement{ | ||
| "org.iso.18013.5.1": apiClaims, |
There was a problem hiding this comment.
in configuration, the key for "org.iso.18013.5.1" is named namespace, but here it's a vc type. What is it?
I notice that the mDL manual refers to org.iso.18013.5.1 as a namespace, but this application must deal with many different kinds of credentials.
There was a problem hiding this comment.
is zkp a thing for other credential types other then mDoc?
| notify: notify, | ||
| tracer: tracer, | ||
| server: &http.Server{}, // Timeouts and other defaults are set by httphelpers.Server.Default | ||
| server: &http.Server{ |
There was a problem hiding this comment.
have you tested with the default and found it won't worked?
| s.httpHelpers.Server.RegEndpoint(ctx, rgOIDCVerification, http.MethodPost, "confirm/:session_id", http.StatusOK, s.endpointConfirmCredentialDisplay) | ||
|
|
||
| // Longfellow-zk verifier | ||
| s.httpHelpers.Server.RegEndpoint(ctx, rgOIDCVerification, http.MethodPost, "verifyzkp", http.StatusOK, s.endpointVerifyZKP) |
There was a problem hiding this comment.
should this endpoint be in oidc space?
|
|
||
| type ZKConfig struct { | ||
| // Note the envconfig tags - this is how the loader finds them! | ||
| CACertsPath string `yaml:"ca_certs_path" envconfig:"VERIFIER_ZK_CA_CERTS" validate:"required"` |
There was a problem hiding this comment.
No envconfig please, stick with configuration file.
|
|
||
| func (s *Service) endpointVerifyZKP(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.
maybe this project should have a middleware default to prevent too big body?
There was a problem hiding this comment.
Pull request overview
This PR adds Longfellow ZK proof verification support to the verifier service (behind a zk build tag) and introduces an HTTP endpoint in the issuer service to issue ISO 18013-5 mdoc credentials.
Changes:
- Add a
POST /verification/verifyzkpverifier endpoint and ZK proof handling (CBOR parsing + CGO bridge to Longfellow verifier). - Add a
POST /mdocissuer endpoint to issue mdoc credentials. - Add build/packaging plumbing for ZK (Dockerfile, Makefile tags, CI steps, config fields).
Reviewed changes
Copilot reviewed 23 out of 29 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| vendor/proofs/server/v2/zk/vical.go | Fetch and load VICAL issuer roots into a cert pool. |
| vendor/proofs/server/v2/zk/roots.go | Global issuer root cert pool for ZK verification. |
| vendor/proofs/server/v2/zk/proofs.go | CGO bridge to Longfellow ZK verifier library and proof verification entrypoint. |
| vendor/proofs/server/v2/zk/circuits.go | Load circuits from disk and validate via circuit-id. |
| vendor/proofs/server/v2/zk/cbor.go | Parse ZKDeviceResponse CBOR formats and validate issuer cert chain. |
| vendor/modules.txt | Vendor metadata for the added proofs/server/v2 module. |
| sonar-project.properties | Adjust Sonar exclusions/ignore criteria (Dockerfiles). |
| pkg/model/config.go | Add verifier zk config block (paths for CA/circuits/libs). |
| internal/verifier/httpserver/service.go | Register ZK verification endpoint and adjust server timeouts. |
| internal/verifier/httpserver/endpoint_verification.go | Implement HTTP handler for verifyzkp. |
| internal/verifier/httpserver/api.go | Extend verifier API interface with VerifyZKP. |
| internal/verifier/apiv1/handlers_verification_zk_enabled.go | Implement ZK verification handler (enabled build). |
| internal/verifier/apiv1/handlers_verification_zk_disabled.go | Implement ZK verification handler (disabled build). |
| internal/verifier/apiv1/handlers_verification.go | Add request/response types for ZK verification. |
| internal/verifier/apiv1/handler_verification_zk_enabled_test.go | Add zk-tagged unit test for VerifyZKP (enabled). |
| internal/verifier/apiv1/handler_verification_zk_disabled_test.go | Add non-zk unit test for VerifyZKP (disabled). |
| internal/issuer/httpserver/service.go | Register POST /mdoc endpoint; switch apiv1 field to concrete client. |
| internal/issuer/httpserver/endpoints.go | Implement HTTP handler for mdoc issuance. |
| internal/issuer/apiv1/handlers_test.go | Add a basic mdoc issuance test. |
| go.mod | Add proofs/server/v2 requirement and local filesystem replace. |
| dockerfiles/worker | Switch builds to -mod=vendor and change dependency step. |
| dockerfiles/verifier.Dockerfile | Add a dedicated verifier image build that compiles Longfellow ZK libs + Go binary. |
| docker-compose.yaml | Adjust verifier image name for compose. |
| config.yaml | Add example issuer mdoc + verifier zk config; modify issuer settings. |
| cmd/verifier/zk_enabled_setup.go | Add verifier startup initialization for circuits + CA roots (zk build). |
| cmd/verifier/zk_disabled_setup.go | No-op ZK setup when zk build tag is off. |
| cmd/verifier/main.go | Call setupZK during verifier startup. |
| Makefile | Add ZK build tag plumbing, docker build target, and test target. |
| .github/workflows/test.yaml | Clone longfellow-zk into /tmp and run go mod tidy/vendor in CI. |
Comments suppressed due to low confidence (1)
go.mod:219
replace proofs/server/v2 => /tmp/...hard-codes a local filesystem path, making builds non-reproducible and breaking for anyone (and any CI runner) without that exact directory layout. This should be replaced with a repository-relative path committed to this repo (e.g., athird_party/checkout) or an actual VCS module path + version; avoid/tmpin go.mod.
proofs/server/v2 v2.0.0
)
require (
dario.cat/mergo v1.0.2 // indirect
github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
github.com/KyleBanks/depth v1.2.1 // indirect
github.com/Microsoft/go-winio v0.6.2 // indirect
github.com/PaesslerAG/gval v1.2.4 // indirect
github.com/beorn7/perks v1.0.1 // indirect
github.com/bytedance/gopkg v0.1.4 // indirect
github.com/bytedance/sonic v1.15.0 // indirect
github.com/bytedance/sonic/loader v0.5.0 // indirect
github.com/cayleygraph/quad v1.3.0 // indirect
github.com/cenkalti/backoff/v4 v4.3.0 // indirect
github.com/cenkalti/backoff/v5 v5.0.3 // indirect
github.com/cespare/xxhash/v2 v2.3.0 // indirect
github.com/cloudwego/base64x v0.1.6 // indirect
github.com/containerd/errdefs v1.0.0 // indirect
github.com/containerd/errdefs/pkg v0.3.0 // indirect
github.com/containerd/log v0.1.0 // indirect
github.com/containerd/platforms v0.2.1 // indirect
github.com/cpuguy83/dockercfg v0.3.2 // indirect
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/distribution/reference v0.6.0 // indirect
github.com/docker/docker v28.5.1+incompatible // indirect
github.com/docker/go-connections v0.6.0 // indirect
github.com/docker/go-units v0.5.0 // indirect
github.com/eapache/go-resiliency v1.7.0 // indirect
github.com/eapache/queue v1.1.0 // indirect
github.com/ebitengine/purego v0.8.4 // indirect
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/gabriel-vasile/mimetype v1.4.13 // indirect
github.com/gin-contrib/sse v1.1.0 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/go-ole/go-ole v1.2.6 // indirect
github.com/go-openapi/jsonpointer v0.22.5 // indirect
github.com/go-openapi/jsonreference v0.21.5 // indirect
github.com/go-openapi/spec v0.22.4 // indirect
github.com/go-openapi/swag/conv v0.25.5 // indirect
github.com/go-openapi/swag/jsonname v0.25.5 // indirect
github.com/go-openapi/swag/jsonutils v0.25.5 // indirect
github.com/go-openapi/swag/loading v0.25.5 // indirect
github.com/go-openapi/swag/stringutils v0.25.5 // indirect
github.com/go-openapi/swag/typeutils v0.25.5 // indirect
github.com/go-openapi/swag/yamlutils v0.25.5 // indirect
github.com/go-playground/locales v0.14.1 // indirect
github.com/go-playground/universal-translator v0.18.1 // indirect
github.com/goccy/go-json v0.10.6 // indirect
github.com/goccy/go-yaml v1.19.2 // indirect
github.com/golang-jwt/jwt/v4 v4.5.2 // indirect
github.com/google/go-cmp v0.7.0 // indirect
github.com/gorilla/context v1.1.2 // indirect
github.com/gorilla/securecookie v1.1.2 // indirect
github.com/grpc-ecosystem/grpc-gateway/v2 v2.28.0 // indirect
github.com/hashicorp/go-uuid v1.0.3 // indirect
github.com/jcmturner/aescts/v2 v2.0.0 // indirect
github.com/jcmturner/dnsutils/v2 v2.0.0 // indirect
github.com/jcmturner/gofork v1.7.6 // indirect
github.com/jcmturner/gokrb5/v8 v8.4.4 // indirect
github.com/jcmturner/rpc/v2 v2.0.3 // indirect
github.com/jinzhu/inflection v1.0.0 // indirect
github.com/jinzhu/now v1.1.5 // indirect
github.com/jonboulle/clockwork v0.5.0 // indirect
github.com/json-iterator/go v1.1.12 // indirect
github.com/kaptinlin/go-i18n v0.2.12 // indirect
github.com/kaptinlin/jsonpointer v0.4.17 // indirect
github.com/kaptinlin/messageformat-go v0.4.18 // indirect
github.com/klauspost/compress v1.18.4 // indirect
github.com/klauspost/cpuid/v2 v2.3.0 // indirect
github.com/leodido/go-urn v1.4.0 // indirect
github.com/lestrrat-go/blackmagic v1.0.4 // indirect
github.com/lestrrat-go/dsig v1.0.0 // indirect
github.com/lestrrat-go/dsig-secp256k1 v1.0.0 // indirect
github.com/lestrrat-go/httpcc v1.0.1 // indirect
github.com/lestrrat-go/httprc/v3 v3.0.4 // indirect
github.com/lestrrat-go/option/v2 v2.0.0 // indirect
github.com/lufia/plan9stats v0.0.0-20211012122336-39d0f177ccd0 // indirect
github.com/magiconair/properties v1.8.10 // indirect
github.com/mattermost/xml-roundtrip-validator v0.1.0 // indirect
github.com/mattn/go-isatty v0.0.20 // indirect
github.com/moby/docker-image-spec v1.3.1 // indirect
github.com/moby/go-archive v0.1.0 // indirect
github.com/moby/patternmatcher v0.6.0 // indirect
github.com/moby/sys/sequential v0.6.0 // indirect
github.com/moby/sys/user v0.4.0 // indirect
github.com/moby/sys/userns v0.1.0 // indirect
github.com/moby/term v0.5.0 // indirect
github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
github.com/modern-go/reflect2 v1.0.2 // indirect
github.com/morikuni/aec v1.0.0 // indirect
github.com/mr-tron/base58 v1.2.0 // indirect
github.com/multiformats/go-base32 v0.1.0 // indirect
github.com/multiformats/go-base36 v0.2.0 // indirect
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
github.com/opencontainers/go-digest v1.0.0 // indirect
github.com/opencontainers/image-spec v1.1.1 // indirect
github.com/pelletier/go-toml/v2 v2.2.4 // indirect
github.com/pierrec/lz4/v4 v4.1.26 // indirect
github.com/pkg/errors v0.9.1 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/power-devops/perfstat v0.0.0-20210106213030-5aafc221ea8c // indirect
github.com/pquerna/cachecontrol v0.2.0 // indirect
github.com/prometheus/client_golang v1.23.2 // indirect
github.com/prometheus/client_model v0.6.2 // indirect
github.com/prometheus/common v0.66.1 // indirect
github.com/prometheus/procfs v0.16.1 // indirect
github.com/quic-go/qpack v0.6.0 // indirect
github.com/quic-go/quic-go v0.59.0 // indirect
github.com/rcrowley/go-metrics v0.0.0-20250401214520-65e299d6c5c9 // indirect
github.com/richardlehane/mscfb v1.0.6 // indirect
github.com/richardlehane/msoleps v1.0.6 // indirect
github.com/segmentio/asm v1.2.1 // indirect
github.com/shirou/gopsutil/v4 v4.25.6 // indirect
github.com/shopspring/decimal v1.4.0 // indirect
github.com/sirosfoundation/g119612 v0.0.0-20260108094825-5b3123230280 // indirect
github.com/sirupsen/logrus v1.9.3 // indirect
github.com/tiendc/go-deepcopy v1.7.2 // indirect
github.com/tklauser/go-sysconf v0.3.12 // indirect
github.com/tklauser/numcpus v0.6.1 // indirect
github.com/twitchyliquid64/golang-asm v0.15.1 // indirect
github.com/ugorji/go/codec v1.3.1 // indirect
github.com/valyala/fastjson v1.6.10 // indirect
github.com/x448/float16 v0.8.4 // indirect
github.com/xdg-go/pbkdf2 v1.0.0 // indirect
github.com/xdg-go/scram v1.2.0 // indirect
github.com/xdg-go/stringprep v1.0.4 // indirect
github.com/xuri/efp v0.0.1 // indirect
github.com/xuri/nfp v0.0.2-0.20250530014748-2ddeb826f9a9 // indirect
github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78 // indirect
github.com/yusufpapurcu/wmi v1.2.4 // indirect
go.opentelemetry.io/auto/sdk v1.2.1 // indirect
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.49.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.42.0 // indirect
go.opentelemetry.io/otel/metric v1.42.0 // indirect
go.opentelemetry.io/proto/otlp v1.10.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.yaml.in/yaml/v2 v2.4.2 // indirect
go.yaml.in/yaml/v3 v3.0.4 // indirect
golang.org/x/arch v0.25.0 // indirect
golang.org/x/mod v0.34.0 // indirect
golang.org/x/net v0.52.0 // indirect
golang.org/x/sync v0.20.0 // indirect
golang.org/x/sys v0.42.0 // indirect
golang.org/x/text v0.35.0 // indirect
golang.org/x/tools v0.43.0 // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20260316180232-0b37fe3546d5 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20260316180232-0b37fe3546d5 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
)
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.
| { | ||
| name: "process valid base64 payload", | ||
| req: &VerifyZKPRequest{ | ||
| Transcript: transcript, | ||
| ZKDeviceResponseCBOR: deviceCBOR, | ||
| }, | ||
| expectError: false, | ||
| }, |
There was a problem hiding this comment.
This test expects the "valid base64 payload" case to succeed, but VerifyZKP depends on runtime state that isn't initialized in the test (trusted issuer roots loaded into IssuerRoots, circuits loaded into the circuit map, and the CGO verifier library available at link/runtime). As written, this will be flaky or fail whenever the zk tag is enabled. Consider mocking zk.VerifyProofRequest/issuer validation, or make the test assert only the decoding/validation behavior and skip proof verification unless an explicit integration-test flag/env is set.
|
|
||
| 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), $(ZK_TAG)" -v ./... |
There was a problem hiding this comment.
There is an extra space before $(ZK_TAG) in the -tags list. Go's -tags flag expects a clean comma-separated list; the space can lead to an invalid/ignored tag depending on parsing. Remove the space to ensure zk is actually enabled when running this target.
| 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 ./... |
| - name: Sync dependencies | ||
| run: | | ||
| go mod tidy | ||
| go mod vendor |
There was a problem hiding this comment.
Running go mod tidy + go mod vendor inside CI mutates go.mod/go.sum/vendor during the workflow, which makes the build non-deterministic and can mask the fact that the repo isn't kept tidy/vendor'd in git. It also works around the /tmp-based replace rather than fixing it. Prefer committing the correct module dependency (or removing the local replace) and have CI verify go mod tidy/go mod vendor are already clean (or run tests with -mod=vendor without rewriting files).
| - name: Sync dependencies | |
| run: | | |
| go mod tidy | |
| go mod vendor | |
| - name: Verify dependencies are tidy and vendored | |
| run: | | |
| go mod tidy | |
| go mod vendor | |
| git diff --exit-code -- go.mod go.sum vendor/ |
| // 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 the verifier endpoint is 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 operators know the correct endpoint.
| func TestMakeMDoc_Only(t *testing.T) { | ||
| ctx := context.Background() | ||
| log := logger.NewSimple("test") | ||
| client := mockNewClient(ctx, t, "ecdsa", log) | ||
|
|
||
| realIssuer := &mdoc.Issuer{} | ||
|
|
||
| client.mdocIssuer = realIssuer | ||
|
|
||
| deviceKeyHex := "a501020326200121582065eda5bd2d497ef0d35502f5846014e4a66a17ef65476a029587428f6426466322582042f4c664323c932a393086603a1f81d894e77227ed9097e38317769539257609" | ||
| deviceKeyBytes, _ := hex.DecodeString(deviceKeyHex) | ||
|
|
||
| req := &CreateMDocRequest{ | ||
| Scope: "mdl", | ||
| DocType: "org.iso.18013.5.1.mDL", | ||
| DocumentData: []byte(`{"given_name": "John"}`), | ||
| DevicePublicKey: deviceKeyBytes, | ||
| DeviceKeyFormat: "cose", | ||
| } | ||
|
|
||
| got, err := client.MakeMDoc(ctx, req) | ||
| if err != nil { | ||
| t.Logf("Note: Real issuer failed (likely missing keys): %v", err) | ||
| } else { | ||
| require.NoError(t, err) | ||
| assert.NotNil(t, got) | ||
| } |
There was a problem hiding this comment.
This test logs and continues on error, so it can pass even if MakeMDoc is broken (and it likely will fail in many environments if signing keys/config aren't present). To make it meaningful, either fully mock mdoc.Issuer and assert deterministic behavior, or turn this into an explicit integration test that is skipped unless required test fixtures/env are present.
| if cfg.Verifier == nil || cfg.Verifier.ZK.CircuitsPath == "" || cfg.Verifier.ZK.CACertsPath == "" { | ||
| return fmt.Errorf("ZK build requires circuits_path and cacerts_path in config") | ||
| } | ||
| zk.LoadCircuits(cfg.Verifier.ZK.CircuitsPath) |
There was a problem hiding this comment.
setupZK ignores the error return from zk.LoadCircuits(...), so verifier startup will succeed even if circuits fail to load (and later verification requests will fail unexpectedly). Also cfg.Verifier.ZK.LibPath is required in config but never used here. Please handle/return the LoadCircuits error and either remove LibPath from config or use it (e.g., to configure library search path) so the config contract matches behavior.
| if cfg.Verifier == nil || cfg.Verifier.ZK.CircuitsPath == "" || cfg.Verifier.ZK.CACertsPath == "" { | |
| return fmt.Errorf("ZK build requires circuits_path and cacerts_path in config") | |
| } | |
| zk.LoadCircuits(cfg.Verifier.ZK.CircuitsPath) | |
| if cfg.Verifier == nil || cfg.Verifier.ZK.CircuitsPath == "" || cfg.Verifier.ZK.CACertsPath == "" || cfg.Verifier.ZK.LibPath == "" { | |
| return fmt.Errorf("ZK build requires circuits_path, cacerts_path, and lib_path in config") | |
| } | |
| if err := os.Setenv("LD_LIBRARY_PATH", cfg.Verifier.ZK.LibPath); err != nil { | |
| return fmt.Errorf("could not set ZK library path: %w", err) | |
| } | |
| if err := zk.LoadCircuits(cfg.Verifier.ZK.CircuitsPath); err != nil { | |
| return fmt.Errorf("could not load ZK circuits: %w", err) | |
| } |
| server: &http.Server{ | ||
| ReadHeaderTimeout: 5 * time.Second, | ||
| ReadTimeout: 30 * time.Second, | ||
| WriteTimeout: 60 * time.Second, | ||
| IdleTimeout: 120 * time.Second, | ||
| }, |
There was a problem hiding this comment.
The timeouts set on http.Server here will be overwritten by httphelpers.Server.Default(...) (which unconditionally sets Read/Write/Idle/ReadHeader timeouts). As-is, this block is misleading and effectively dead code; either rely on Default (and remove this) or change Default/the call site so the intended verifier-specific timeouts actually take effect.
| cfg *model.Cfg | ||
| log *logger.Log | ||
| server *http.Server | ||
| apiv1 Apiv1 | ||
| apiv1 *apiv1.Client | ||
| gin *gin.Engine | ||
| tracer *trace.Tracer | ||
| httpHelpers *httphelpers.Client |
There was a problem hiding this comment.
Switching Service.apiv1 from an interface to a concrete *apiv1.Client reduces testability and bypasses the existing internal/issuer/httpserver/api.go abstraction. Instead, extend the Apiv1 interface to include MakeMDoc and keep Service.apiv1 typed as that interface so handlers can be unit-tested with mocks.
| 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 JSON tags are capitalized (e.g., "Transcript") and the request has no validation tags. This is inconsistent with the rest of the API (which uses lowercase/snake_case tags + validate:"required") and will surprise clients/binders. Suggest switching to consistent lowercase JSON field names and adding validation tags (and/or helpers.Check) to ensure required fields are present.
| 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" validate:"required"` | |
| ZKDeviceResponseCBOR string `json:"zk_device_response_cbor" validate:"required"` | |
| } | |
| 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"` |
| } | ||
|
|
||
| func (s *Service) endpointVerifyZKP(ctx context.Context, c *gin.Context) (any, error) { | ||
| s.log.Debug("endpointVerificationDirectPost called") |
There was a problem hiding this comment.
The debug log message in this endpoint says endpointVerificationDirectPost called, which looks like a copy/paste and makes logs confusing when debugging ZK requests. Update it to the correct endpoint name (e.g., endpointVerifyZKP called).
| s.log.Debug("endpointVerificationDirectPost called") | |
| s.log.Debug("endpointVerifyZKP called") |
# Conflicts: # .github/workflows/test.yaml # config.yaml # dockerfiles/worker # internal/verifier/apiv1/handlers_verification.go # internal/verifier/httpserver/service.go # pkg/model/config.go # sonar-project.properties
- pkg/model/config.go: Verifier.ZK was a non-pointer struct with
validate:"required" subfields, so config validation failed for every
verifier deployment that doesn't configure zk (including default,
non-zk-tagged builds). Made it *ZKConfig with omitempty.
- handlers_verification_zk_enabled.go: VerifyZKP swallowed the error
from zk.VerifyProofRequest and always returned (reply, nil), so proof
verification failures were silently reported as HTTP 200. Now returns
the error.
- handlers_verification_zk_disabled.go: misleading "no item in
credential cache" error when ZK is simply build-tag-disabled; also
logged raw user-supplied Transcript as "id". Now returns a clear
"ZK support is disabled" error and logs only the transcript length.
- zk_enabled_setup.go: zk.LoadCircuits error was discarded, so a
verifier would start successfully even if circuits failed to load.
- Fixed bare "vc/..." import paths (missing github.com/SUNET/ prefix)
in cmd/verifier/zk_{enabled,disabled}_setup.go,
internal/issuer/httpserver/endpoints.go, and
internal/issuer/apiv1/handlers_test.go — these predate this merge
and broke the default (non-zk) build.
- dockerfiles/worker: HEAD's merge side had a RUN instruction with no
command (Docker build syntax error); took upstream's rewritten
multi-stage build instead.
- internal/issuer/httpserver/service.go + api.go: switching
Service.apiv1 to a concrete *apiv1.Client bypassed the existing
Apiv1 interface abstraction. Extended the interface with MakeMDoc
instead.
- handlers_verification.go: VerifyZKPRequest/Response used capitalized
JSON tags with no validation, inconsistent with the rest of the API
(no external client depends on this new endpoint yet). Switched to
lowercase snake_case tags + validate:"required".
- endpoint_verification.go: copy-pasted debug log message
("endpointVerificationDirectPost called" inside endpointVerifyZKP).
- Makefile: stray space in test-all-tags' -tags list could cause the
zk tag to be dropped depending on go's flag parsing.
- config.yaml: audit_log used "enabled" but the config model's yaml
tag is "enable" — silently ignored, sample would never enable audit
logging as written.
Left for the PR author: default verifier build now requires the
"zk"+"dynamic" build config (Makefile), which pulls in CGO + the
Longfellow native library for all verifier builds unless explicitly
overridden — worth confirming this is the intended default. Also left
the mDoc-issuer test that always reports PASS regardless of outcome,
and the zk-tagged verification test that assumes uninitialized runtime
state, since fixing those properly needs a decision on mocking
strategy rather than a mechanical fix.
envconfig tags on ZKConfig fields were dead weight: the only envconfig.Process call in the codebase (pkg/configuration/config.go) processes a single VC_CONFIG_YAML var to locate the config file, not per-field overrides for the parsed Cfg struct. Per masv3971's review comment on config.go:514, this config should stay YAML-file-only.
|
Rebased onto latest Fixed:
Build, Left for @yimyitbarek to weigh in on (design/architecture calls, not mechanical bugs):
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Run the final image as a non-root "vcservice" user instead of defaulting to root (docker:S6471, was failing the PR's new-code security rating gate). User/group creation is idempotent since the docker.sunet.se/iam_vc/verifier base image may already define it. - Pin golang:latest -> golang:1.26.5, matching go.mod and the convention already used in dockerfiles/worker and dockerfiles/gobuild. - apt install --no-install-recommends and alphabetized package list.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 30 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
internal/verifier/httpserver/service.go:208
- The new ZK endpoint is registered as POST
/verification/verifyzkp, but the PR description/test instructions say to call/verification/verify. Also, the endpoint is currently registered unconditionally even in!zkbuilds (where the handler will always return an error), which makes the API surface confusing.
Consider (a) aligning the route name with the documented path and (b) only registering it when ZK is configured/enabled.
// Longfellow-zk verifier
s.httpHelpers.Server.RegEndpoint(ctx, rgOIDCVerification, http.MethodPost, "verifyzkp", http.StatusOK, s.endpointVerifyZKP)
internal/verifier/apiv1/handler_verification_zk_enabled_test.go:28
- This
zk-tagged unit test expects the “valid base64 payload” case to succeed, butCreateTestClientWithMockdoesn’t load circuits or issuer roots. As a result,VerifyZKPwill deterministically fail atzk.VerifyProofRequestwith “invalid circuit id …” (or earlier), making this test fail whenever-tags zkis used.
Either initialize the required ZK state within the test (load circuits + CA) or change the test to only assert the decoding/validation behavior that is actually deterministic in this environment.
{
name: "process valid base64 payload",
req: &VerifyZKPRequest{
Transcript: transcript,
ZKDeviceResponseCBOR: deviceCBOR,
},
expectError: false,
},
Makefile:443
docker-build-verifierrunsgo mod tidyandgo mod vendoron the host before building. With the current/tmp/...replace, this target will fail unless the external repo has already been cloned locally, and it can also unexpectedly rewrite go.mod/go.sum/vendor.
Since the Dockerfile already builds with -mod=vendor, consider removing these host-side steps (or moving dependency syncing inside the Docker build where the dependency is fetched).
docker-build-verifier: _check-reserved-tag ## Build Docker image for verifier with ZK support
$(info Building Docker image 'verifier' with ZK support)
go mod tidy
go mod vendor
docker build --build-arg SERVICE_NAME=verifier \
--build-arg GO_BUILD_TAGS=$(ZK_TAG) \
--tag verifier \
--file dockerfiles/verifier.Dockerfile .
cmd/verifier/zk_enabled_setup.go:16
- setupZK’s config error message refers to
cacerts_path, but the documented/configured key isca_certs_path. Also, ZKConfig currently requireslib_path, but setupZK doesn’t validate it, so a misconfiguredlib_pathwon’t be caught here.
Update the validation and message to match the actual field names and required config.
if cfg.Verifier == nil || cfg.Verifier.ZK == nil || cfg.Verifier.ZK.CircuitsPath == "" || cfg.Verifier.ZK.CACertsPath == "" {
return fmt.Errorf("ZK build requires circuits_path and cacerts_path in config")
}
internal/verifier/apiv1/handler_verification_zk_disabled_test.go:9
- This test file isn’t gofmt-formatted (indentation/import order). Please run gofmt so it matches the rest of the Go codebase formatting and avoids noisy diffs in future edits.
import (
"testing"
"github.com/stretchr/testify/assert"
)
| gopkg.in/yaml.v3 v3.0.1 // indirect | ||
| ) | ||
|
|
||
| replace proofs/server/v2 => /tmp/longfellow-zk/reference/verifier-service/server |
# Conflicts: # config.yaml
|
Rebased onto latest `main` after #505 merged. Two conflicts in `config.yaml` (both sides added independent scopes/fields — kept both), and a build failure surfaced by #505's `CreateMDocRequest` redesign: `internal/issuer/apiv1/handlers_test.go`'s `TestMakeMDoc_Only` still set the now-removed `DocType` field and didn't supply the now-required `MDDL` schema bytes. Fixed by supplying a minimal valid MDDL schema instead of the doctype string. Build, vet, and |
# Conflicts: # docs/CONFIGURATION.md # pkg/model/config.go
|
Rebased onto latest |
|
|
One more item for the "left for author" list from the earlier summary: Copilot flagged `go.mod`'s `replace proofs/server/v2 => /tmp/longfellow-zk/reference/verifier-service/server` — a hardcoded absolute `/tmp` path, only valid after CI's "Clone Missing Dependency" step runs. This is the same underlying concern as the already-flagged `go mod tidy && go mod vendor`-in-CI item: the `zk`-tagged build isn't self-contained, it depends on a specific external clone happening at a specific absolute path first. Since `vendor/proofs/server/v2/zk/` already has a committed copy, this replace directive may only matter for refreshing that vendored copy, not for normal `-mod=vendor` builds — but I don't want to guess at changing the vendoring/CI strategy without your input, since it's tied to the same open "should zk be opt-in vs default, how should longfellow-zk be sourced" question. |
Summary
This PR integrates longfellow-zk into the verifier, following the reference implementation. It also enables the issuer service to issue mdoc verifiable credentials (VCs).
Type of change
Related issues / tickets
Changes
The issuer service accepts mdoc VC requests via the mdoc endpoint
The issuer issues mdoc VCs
The verifier service loads the ZK circuits during initialization.
It accepts the proof as a JSON file from the holder.
It verifies the proof and returns the result.
Architecture decisions
Screenshots / recordings (if UI changes)
How to test
Issuer :
1, Deploy the issuer service.
2, Send a request to Issuer_URL/mdoc including the scope, document data, and other parameters in the payload.
Verifier:
1, Deploy the verifier service.
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