Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add global "security" section to swagger openapi schema #255

Conversation

holgerstolzenberg
Copy link

🎯 Goal

Hopefully enables ApiKeyAuth for all endpoints for proper usage with OpenAPI Generator.

πŸ™‹β€β™‚οΈ Situation

We are currently looking into SpiceDB as a go to option for authorization.
As of project constraints, we cannot rely upon gRPC and have to use the HTTP API.

🧐 The problem

Working with the API via "simple" clients (e.g. curl) works, but for proper integration a pre packaged client SDK would be really nice. As I could not find such a thing, I started on creating my own using OpenAPI generator tooling, leveraging https://github.com/authzed/authzed-go/blob/main/proto/apidocs.swagger.json spec file.

The client generation in general works, but using it always caused an authentication exception, which was very strange.
After debugging through the OpenAPI generated code (Java), I realised the ApiKeyAuth does not get applied, causing no Authorization header to be sent.

This is caused by the OpenAPI spec file not to be "precise" enough πŸ˜‰

πŸ™ The fix

I created a quick fix to the schema that fixes the issue by setting the security to be applied to ApiKeyAuth for all endpoints of the schema. The generated client (Java) is now confirmed to be working.

It would be cool to have that merged to the upstream, yet there might be some open points:

❓ Open points

  • Is the schema file auto generated (e.g. by CI) in will the change get lost with the next update to it? If so, the security config might need to be applied somewhere else.
  • Need all endpoints to be secured, or are some of them to be excluded? If exclusion does not work, do we then have to explicitly annotate the endpoints and remove the global security configuration section?

Hopefully enables ApiKeyAuth for all endpoints for proper usage with OpenAPI Generator.
@holgerstolzenberg holgerstolzenberg requested a review from a team as a code owner November 6, 2024 12:10
Copy link

github-actions bot commented Nov 6, 2024

CLA Assistant Lite bot All contributors have signed the CLA ✍️ βœ…

@holgerstolzenberg
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

authzedbot added a commit to authzed/cla that referenced this pull request Nov 6, 2024
@holgerstolzenberg holgerstolzenberg force-pushed the feature/openapi-global-security-definition branch from 652e7f1 to 688e3bd Compare November 6, 2024 14:57
@tstirrat15
Copy link
Contributor

@holgerstolzenberg thank you for putting this up! It's definitely welcome.

  1. This is autogenerated: https://github.com/authzed/authzed-go/blob/main/magefiles/buf.gen.yaml#L35-L43. It looks like if we want to annotate the API with this such that it gets generated, we'd need to add this to the proto: How can I set route level "Bearer" Security in Swagger?Β grpc-ecosystem/grpc-gateway#1089 and that would likely go in https://github.com/authzed/api
  2. All endpoints use the same security

@holgerstolzenberg
Copy link
Author

holgerstolzenberg commented Nov 6, 2024

Okay so I dug around and tried to understand how your buf tooling works, the links provided by @tstirrat15 already contain the necessary information.

At first, here is the documentation to the Swagger Authentication stuff, it provides all information that you need: Swagger Authentication v2

So for me it looks like if the change has to go to authzed/api into openapi.proto.
Taken the information here, just adding

security: {
  security_requirement: {
    key: "ApiKeyAuth"
  }
}

should do the trick and should be a low hanging fruit. I could try to provide a PR but maybe you are faster to try....

One hint:

Starting with OpenAPI v3, new authentication options are present, like e.g. bearer. For me it feels a bit mixed up that currently you have to use an API key based authentication resulting in an Authorization: Bearer XYZ HTTP header that feels more like bearer token authentication, but without token refreshes... but this is just a random thought, not related to the issue at hands.

So how do we continue @tstirrat15 ?

@holgerstolzenberg
Copy link
Author

Another cool thing would be to have the final public openapi spec to be published to a more prominent location.

@tstirrat15
Copy link
Contributor

@holgerstolzenberg any thoughts on a good place for that? We have a postman collection, perhaps? It's also available in the SpiceDB binary if you run serve with the http flag turned on: https://authzed.com/docs/spicedb/getting-started/client-libraries#http-clients

@holgerstolzenberg
Copy link
Author

holgerstolzenberg commented Nov 9, 2024

Good question - never thought about that. A short lookup if there is such a thing like a public OpenAPI spec registry just revealed https://apis.guru, but I never used it on my own.

So here is the thing. In the end the spec should be available from a public URL that is discoverable, _rememberable, versioned, etc. - it does not matter that much, where that then actually is. The Postman collection might be a good place, but I have no clue how publication works and if you can push your spec file there.

In the end, you could also publish the spec file to your own domain, e.g. https://authzed.com/api/v1.1.0/openapi.spec πŸ€·πŸ»β€β™‚οΈ

It is nice to get the spec from an running instance, but in terms of pipeline performance I will always rather rely on an public URL then on a docker image that I need to download and start with each pipeline run.

It is like with any other dependency, I just want to consume that thing from somewhere (like a Java dep from Maven Central).

Also, your authzed/api project should be the single source of truth, you already create the spec there and publish it to here. Normally, authzed/go should also just consume the versioned spec from a remote location instead of having it in the src tree here.

Technically, it is totally fine to got to https://raw.githubusercontent.com/authzed/authzed-go/refs/tags/${{ env.spicedb-version }}/proto/apidocs.swagger.json, but it is weird that we are discussing API publication in a PR here, whereas it IMHO would belong into a discussion at authzed/api.

Long story short - a semantically or ideologically valid public location would be really nice 🀩

✌️

vroldanbet added a commit to authzed/api that referenced this pull request Nov 11, 2024
SpiceDB does not use ApiKeyAuth authentication, but Bearer authentication, where
the type of bearer token is an API Key.

This was reported in authzed/authzed-go#255,
indicating that folks generating code out of the OpenAPI definition
will have errors because the generated error did not properly provide
the preshared key with the expected `Authorization: Bearer <psk>`
format.

See https://swagger.io/docs/specification/v3_0/authentication/api-keys/
See https://swagger.io/docs/specification/v3_0/authentication/bearer-authentication/
vroldanbet added a commit to authzed/api that referenced this pull request Nov 11, 2024
SpiceDB does not use ApiKeyAuth authentication, but Bearer authentication, where
the type of bearer token is an API Key.

However, the OpenAPI v2 Spec, which is the one supported by grpc-gateway,
does not support bearer authentication:
https://swagger.io/docs/specification/v2_0/authentication/authentication/

Still, the grpc-gateway maintainers indicated in
grpc-ecosystem/grpc-gateway#1089
that bearer is actually supported in grpc-gateway generator.

This was reported in authzed/authzed-go#255,
indicating that folks generating code out of the OpenAPI definition
will have errors because the generated error did not properly provide
the preshared key with the expected `Authorization: Bearer <psk>`
format.

I'm not 100% sure if this is a legit intermediate state
between v2 and v3 we can leverage, but the current generated
code is clearly broken anyway.

See https://swagger.io/docs/specification/v3_0/authentication/api-keys/
See https://swagger.io/docs/specification/v3_0/authentication/bearer-authentication/
@vroldanbet
Copy link
Contributor

@holgerstolzenberg I've elaborated in authzed/api#121 what the fix could look like. Unfortunately grpc-gateway does not generate a v3 OpenAPI spec, but v2, and v2 does not support bearer authentication, yet the authors seem to suggest it's supported, leaving it in a potentially non-compliant v2 state? 🀷🏻

I've opened #258 with the regenerated spec, would you mind giving it a go? It would be awesome also if you could share how to generate these: I think it would be interesting to provide these generate client libraries for other folks not relying on gRPC.

@holgerstolzenberg
Copy link
Author

@tstirrat15 - I have tested your changes and it is confirmed working, see following build:

https://github.com/ewerk/authzed-http-client/actions/runs/11780525757/job/32811323020

Yet your change only changed the name of the authentication method. For the user in the end it still looks like this (as "type": "apiKey"):

ApiClient getApiClient() {
    final var client = new ApiClient(restClient, objectMapper, ApiClient.createDefaultDateFormat());
    client.setBasePath("http://localhost:8443");
    client.setApiKey(apiKey("AlsoLetMeInAgain"));
    return client;
}

For me this is okay as long as it works, but is also a bit confusing. I had already opened up a PR at authzed/api with the same changes keeping the old auth name, why have you decided against this change?

The project https://github.com/ewerk/authzed-http-client is incubating. It is just a bunch of GitHub actions to build clients via OpenAPI generator. There you can find how the stuff is build. I would be willing to transition the project to authzed namespace if you want to take owner-/maintainership.

@vroldanbet
Copy link
Contributor

@holgerstolzenberg I think you may have confused me with @tstirrat15. I opened a PR because I missed you have opened another PR.

It's surprising to me that your proposal actually works unless there is some sort of plumbing in the generated grpc-gateway that transparently handles API key style of authentication.

With that said, I prefer your proposal because it's actually OpenAPI v2 compliant, whereas mine isn't. I'm just surprised it works, I can spend some cycles trying to understand how that works under the hood.

@holgerstolzenberg
Copy link
Author

@vroldanbet yes you are right, I confused you both. Why should my supposal not work, in the end it is just a name πŸ€·πŸ»β€β™‚οΈ.

BTW - as said it is confirmed working. I do not think it is necessary for you to do more round trips, especially not if we stay on v2. So I would suggest you drop your draft PR and approve mine at authzed/api, then I can try if everything client generation still works.

@vroldanbet
Copy link
Contributor

@vroldanbet yes you are right, I confused you both. Why should my supposal not work, in the end it is just a name πŸ€·πŸ»β€β™‚οΈ.

@holgerstolzenberg as much as I appreciate the contribution and that it just works, SpiceDB does not officially support ApiKeyAuth, and if this works, is because of happenstance. We can't blindly accept contributions without fully understanding what's happening under the hood. We can make an informed decision once we understand what's going on.

As you can see in grpc-gateway codebase, the gateway will forward any Authorization header as-is to SpiceDB.

// For backwards-compatibility, pass through 'authorization' header with no prefix.
if key == "Authorization" {
    pairs = append(pairs, "authorization", val)
}

So this only works if you prepend Bearer to your preshared key, effectively turning it into bearer authentication, disguised as ApiKeyAuth.

Now, armed with that information, we can make an informed decision. In light of SpiceDB using a code generator that complies with OpenAPI v2, and that bearer is not supported by it, I'd suggest moving forward with your proposal: even though it's effectively the same, one is OpenAPI v2 spec compliant, the other is not. If and when grpc-gateway adopts OpenAPI v3, then we can move to the more correct Bearer Authentication.

I'd also propose adding a description field to your PR, just like I did in mine, clarifying that the user must prepend the preshared key with Bearer.

Thanks!

@vroldanbet
Copy link
Contributor

closing in favor of authzed/api#120. Once that lands, we can regenerate the all the SDK.

@vroldanbet vroldanbet closed this Nov 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2024
@tstirrat15
Copy link
Contributor

@holgerstolzenberg the newly-generated schema is available here: https://raw.githubusercontent.com/authzed/authzed-go/refs/heads/main/proto/apidocs.swagger.json

Does that have the behavior you're looking for?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants