feat: add JSON API for FDO Owner Server with OpenAPI specification - #130
feat: add JSON API for FDO Owner Server with OpenAPI specification#130djach7 wants to merge 11 commits into
Conversation
dfb5a39 to
12406f4
Compare
mmartinv
left a comment
There was a problem hiding this comment.
While I think this is a good step forward to having an implementation we need more refinement and agree on the tools we are going to use and how to split the APIs, e.g.: the health api is going to be use by all the servers and we don't need 3 different implementations, the same will happen with the device CA cert management)
730470d to
ede9b40
Compare
kgiusti
left a comment
There was a problem hiding this comment.
Couple 'o questions: First, why doesn't the patch use the generated ServerInterface/Handler code? Seems like we're not leveraging a big chunk of the generated functionality. When I look at the server examples on the oapi-codegen website it appears that we only need to provide the ServiceInterface impl and the Http server setup code.
Second, what's the reason the patch uses the chi server as opposed to the go-native (or other server options)? Not implying it's wrong or anything - I'm not familiar with these other server impls - just interested in the motivation for choosing chi. Thanks
ede9b40 to
b25df8d
Compare
b25df8d to
1b3e00f
Compare
|
@kgiusti Thanks for reviewing! I just pushed an update that hopefully addresses all of your comments. In response to your direct questions:
|
f5a914b to
458f37d
Compare
69a7b42 to
d248805
Compare
58490ef to
da6bad2
Compare
fc7cef6 to
42b2c72
Compare
|
@kgiusti Just pushed up a change related to what we talked about on Monday. The ownerinfo endpoint should be completely gone, and there's some JSON unmarshaling happening now as well. The JSON stuff isn't fully where I'd like it to be, right now it just does some unmarshaling for validation and then remarshals it to send it to |
e5d6cae to
e08b7f4
Compare
mmartinv
left a comment
There was a problem hiding this comment.
Too many things to review and to modify, I just reviewed and commented a few.
I think we should lower the scope of this PR to a single API to simplify things and make faster iterations.
Why don't we just restrict this PR to the rvto2addr.yaml API which is the most simple API (the health API is already implemented in #156 in a better way)?
Once we have that we can start with voucher.yaml after merging resell.yaml into it.
What do you think?
| summary: Health check | ||
| operationId: getHealth | ||
| responses: | ||
| '200': |
There was a problem hiding this comment.
The the API returns 200 only? How do we know when it fails?
| properties: | ||
| status: | ||
| type: string | ||
| enum: ["healthy"] |
There was a problem hiding this comment.
This returns always "healthy" which doesn't seem to be right. I think it would help if we also add a "message" to help with the troubleshooting. Please see https://github.com/fido-device-onboard/go-fdo-server/pull/156/changes#diff-fd178fe5d04fd5b4e01530fa592af9681aafb6fdb64d09cba9c950e5f68e8088R30
| required: true | ||
| schema: | ||
| type: string | ||
| pattern: '^[a-fA-F0-9]{32}$' |
There was a problem hiding this comment.
I don't think we should allow uppercase letters in the GUID, lowercase and numbers are enough. The same for other occurrences
| VoucherInsertResponse: | ||
| type: object | ||
| required: | ||
| - processed | ||
| - inserted | ||
| properties: | ||
| processed: | ||
| type: integer | ||
| description: Number of vouchers processed | ||
| inserted: | ||
| type: integer | ||
| description: Number of vouchers successfully inserted | ||
|
|
||
| VoucherResponse: | ||
| type: object | ||
| required: | ||
| - voucher | ||
| - encoding | ||
| properties: | ||
| voucher: | ||
| type: string | ||
| description: Base64 encoded voucher data | ||
| encoding: | ||
| type: string | ||
| enum: ["pem"] | ||
| description: Voucher encoding format | ||
|
|
||
| RVTO2Addr: | ||
| type: object | ||
| required: | ||
| - rvto2addr | ||
| properties: | ||
| rvto2addr: | ||
| type: array | ||
| items: | ||
| type: array | ||
| items: | ||
| oneOf: | ||
| - type: integer | ||
| - type: string | ||
| description: Rendezvous TO2 address information No newline at end of file |
There was a problem hiding this comment.
These are specific types, the should go in the respective APIs, please see #156
| description: Voucher resell endpoints | ||
|
|
||
| paths: | ||
| /owner/resell/{guid}: |
There was a problem hiding this comment.
I think this should be part of the voucher API (/vouchers/{guid}/extend). I don't really like the "resell" word for this: the API is going to be used by the Owner and the also by the Manufacturer and the Manufacturer is not "reselling" but "selling" for the first time. As both the Manufacturer and Owner can shell their devices, the way I would think about this is, independently if I am an Owner or a Manufacturer: If I own the device I can sell it by extending the voucher.
| RVTO2Addr: | ||
| type: object | ||
| required: | ||
| - rvto2addr | ||
| properties: | ||
| rvto2addr: | ||
| type: array | ||
| items: | ||
| type: array | ||
| items: | ||
| oneOf: | ||
| - type: integer | ||
| - type: string |
There was a problem hiding this comment.
This definition is wrong and seems to correspond to the rvinfo not to the rvto2addr, please see https://github.com/fido-device-onboard/go-fdo-server/pull/154/changes#diff-2e7681f1cfb4420391185bff11a9619926890b4b81b8558b59ac964c251e5628R92-R113
| description: Rendezvous TO2 address management endpoints | ||
|
|
||
| paths: | ||
| /owner/redirect: |
There was a problem hiding this comment.
| /owner/redirect: | |
| /rvto2addr: |
| paths: | ||
| /owner/redirect: | ||
| get: | ||
| summary: Get owner redirect information |
There was a problem hiding this comment.
As I said in previous comments: If you are going to rename Owner Info,
I think Rendezvous TO2 Address or even Device on-boarding address/URL is more accurate than Owner Redirect Info.
I am wondering why are you defining the rvto2addr.yaml API from scratch in this way when there's something already done at https://github.com/fido-device-onboard/go-fdo-server/pull/154/changes#diff-2e7681f1cfb4420391185bff11a9619926890b4b81b8558b59ac964c251e5628R16
| return sqlDB.Close() | ||
| } | ||
|
|
||
| // FetchRvInfoJSON reads the rvinfo JSON from the database |
There was a problem hiding this comment.
I think you are mixing things here: RVInfo is not part of the owner API.
| @@ -0,0 +1,9 @@ | |||
| package: health | |||
| output: handlers/health/gen.go | |||
| generate: | |||
There was a problem hiding this comment.
You need to add std-http-server: true to the generate: in all the configurations (except the one for components)
This pull request adds a JSON API for the FDO Owner Server that returns structured JSON documents while maintaining full backward compatibility with existing PEM-based clients. Changes - OpenAPI 3.0.3 Specification - Complete API specification with type-safe Go code generation - JSON Response System - All endpoints return structured JSON with proper content negotiation - Error Handling Consolidation - Centralized error patterns across API handlers - Content-Type Enforcement - Strict PEM upload validation per OpenAPI specification - Go Generate Workflow - Proper tool dependency management and code generation pipeline - Production Optimization - Eliminated debugging artifacts and fixed code smells - Shared Component Structure - Organized OpenAPI specs for future manufacturer/rendezvous servers API Endpoints Enhanced - /health - Server health status with version information (JSON) - /api/v1/ownerinfo - Owner redirect configuration management (JSON) - /api/v1/owner/vouchers - Ownership voucher operations (JSON) New Files - api/openapi/owner-server.yaml - OpenAPI 3.0.3 specification - api/openapi/generated.go - Generated Go types and server interfaces - api/openapi/config.yaml - oapi-codegen configuration - api/openapi/generate.go - go:generate implementation - api/openapi/README.md - Code generation documentation - api/openapi/shared/ - Shared components for future server specs - api/handlers/responses.go - JSON response utilities and error handling - api/handlersTest/json_api_test.go - JSON API validation tests - api/handlersTest/helpers_test.go - Test helper functions Modified Files - api/handlers/vouchers.go - JSON responses and simplified PEM handling - api/handlers/health.go - JSON health endpoint implementation - api/handlers/ownerinfo.go - JSON owner redirect API with error consolidation - api/handlers/rvinfo.go - Consistent error handling patterns - api/handlersTest/vouchers_test.go - Content-type validation tests - api/routes.go - Updated imports for generated types - scripts/fdo-utils.sh - Optimized utility functions for JSON API - Makefile - go generate workflow and tool management - go.mod / go.sum - oapi-codegen dependencies - .gitignore - Generated file handling Signed-off-by: djach7 <djachimo@redhat.com>
…handlers Signed-off-by: djach7 <djachimo@redhat.com>
…erate Signed-off-by: djach7 <djachimo@redhat.com>
…s between server definitions Signed-off-by: djach7 <djachimo@redhat.com>
…uplication in owner spec Signed-off-by: djach7 <djachimo@redhat.com>
Signed-off-by: djach7 <djachimo@redhat.com>
Signed-off-by: djach7 <djachimo@redhat.com>
Signed-off-by: djach7 <djachimo@redhat.com>
…h Miguel's suggestions Signed-off-by: djach7 <djachimo@redhat.com>
Signed-off-by: djach7 <djachimo@redhat.com>
Signed-off-by: djach7 <djachimo@redhat.com>
e08b7f4 to
039cd93
Compare
| } | ||
|
|
||
| func (s *Server) handleGetVouchers(w http.ResponseWriter, r *http.Request) { | ||
| // TODO: Implement voucher listing logic |
Check notice
Code scanning / devskim
A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note
This pull request adds a JSON API for the FDO Owner Server that returns structured JSON documents while maintaining full backward compatibility with existing PEM-based clients.
Changes
API Endpoints Enhanced
New Files
Modified Files