gRPC Support

[mohammed90]

This PR introduces gRPC support with Protobuf.

Closes #78

Continuous feedback is preferred instead of waiting for me to finish.

Tasks:

• refactor existing code so it can be reused more readily between HTTP and gRPC servers
• remove the current server into authn-server/http
• work on a parallel authn-server/gRPC package that can be preferred with a command flag
• if and when appropriate, bring in grpc-gateway to replace the authn-server/http package
• protect the gRPC API from abuse (existing REST API validates Origin header, gRPC does .... [insert something here])
• tests, tests, tests
• [N/A] Fix mTLS with with cmux & add tests for it (I feel this is going to be a giant test)
• Address comments here. It's pretty tricky to eliminate dependency on vendor directory.

Critical bug in current branch:

• mounting on a subpath not working. My hunch is wrapping the handler with http.StripPrefix should fix it, but I'm yet to tackle it.
• Work around this "bug".

Resources used and/or might need as reference to complete PR:

• So you want to use GoGo Protobuf
• Authentication in gRPC
• HTTP server from gRPC with Gateway
• gRPC-Gateway Error Handler
• Advanced gRPC Error Usage
• Workaround to marshal application/x-www-form-urlencoded w/ grpc-gateway
• https://github.com/grpc-ecosystem/go-grpc-prometheus

[coveralls]

Coverage decreased (-22.4%) to 55.017% when pulling 63c381c on Mohammed90:grpc-api into 2201d5b on keratin:master.

[cainlevy]

i have a bit of reading to do before i can provide more useful feedback! here's what came to mind on my first pass:

• we'll need to consider what this means for having separate public and private routing tables
• we should check how the gateway handles being mounted on a subpath (e.g. mydomain.com/authn)

[mohammed90]

For the first point, I have 2 ideas from the top of my head:

1. Instead of 1 service in the proto file for the entire application, e.g. PrivateAuthN and PublicAuthN. The gRPC server is then wrapped with an interceptor that checks strings#HasPrefix "/grpc.PrivateAuthN". If true, a form of authentication is applied (e.g. Basic Auth or TLS Mutual Auth); otherwise the request is passed immediately.

2. Maintain a map of the private routes. An interceptor checks the requested route. If the route is in the map, then the request must be authenticated.

The first approach has the advantage of being low-maintenance and not prone to missing out some routes. Again, those are from the top of my head solutions and may not be the best. More research could reveal cleaner approaches.

Looking more into the generated and example code, it seems that the gateway listens to HTTP on a different port, calls the corresponding gRPC method, and hands back the response to the HTTP request. Given that, it's possible to not use grpc-gateway and just listen on the different ports. But this will cause duplication of logic between the 2 listeners. Using the gateway will avoid the duplicate logic but will require serious refactor of the API package.

P.S.: I hate how the generated code dropped the test coverage by ~50% 😒

[cainlevy]

My main goal with creating separate routing tables was making it easy to firewall the admin endpoints away from public traffic, so maintaining separate listeners is important to me.

But I'm a little saddened by the thought of running four listeners:

1. public HTTP
2. public gRPC
3. admin HTTP
4. admin gRPC

Each of these would need to be a configurable port, and configuration complexity makes me 😞.

Those four listeners could become three if admin endpoints could standardize on gRPC. I haven't worked with backend clients built on protobuf before, but it should be reasonable as long as backend clients stick to the platform list.

Hmm, but when I look at the routes in the meta package it's hard to imagine them standardizing on gRPC. The GET /metrics endpoint must remain HTTP, for example, since Prometheus decided to drop protobuf in 2.x.

(Separately: would you start a task list in the OP? I'd like to include https://github.com/grpc-ecosystem/go-grpc-prometheus.)

[cainlevy]

I'd like to think about ways of approaching gRPC gradually and incrementally. The high level strategy here is to run gRPC with a HTTP gateway, but I wonder if there's a way to instead make it an optional (unofficial) build until it's mature?

1. refactor existing code so it can be reused more readily between HTTP and gRPC servers
2. move the current server into authn-server/http
3. work on a parallel authn-server/gRPC package that can be preferred with a command flag
4. if and when appropriate, bring in grpc-gateway to replace the authn-server/http package

What do you think? I'd be happy to work on steps 1 and 2, myself, if you had any recommendations for how to accomplish better reuse.

[mohammed90]

Dang, that GET /metrics in particular is an annoying obstacle.

I can't imagine how to use grpc-gateway to keep the HTTP endpoints without having gRPC endpoint as well (i.e. gRPC being optional) because the HTTP server (from the grpc-gateway) internally calls the gRPC server (see here). Perhaps during the transition period, a flag will select whether to only enable the HTTP server (current implementation) or the gRPC server which includes the HTTP API.

The proposed approach looks good. I can't think of any improvements on it. As you've suggested, you'll do better on the refactor (steps 1 and 2) because you're more familiar with the codebase than I am. I'll take up the gRPC part. I'll start by splitting the AuthN service into 2 services, public service and administration service, so they can listen on different ports.

I've update the OP.

[cainlevy]

Just realized that the OAuth endpoints must also use standard HTTP.

I don't see any way to run a pure gRPC service without also offering HTTP on another port. The real question is how many endpoints are offered on that HTTP port: the minimum set (maybe OAuth & Prometheus) or the public set (is Prometheus public?).

[cainlevy]

I'd like to return to an earlier question: why not use the authn-js client to communicate directly with the AuthN HTTP handlers?

Using gRPC from the browser requires grpc-web, which is a different protocol that requires a backend proxy to communicate with gRPC services. This adds indirection and an extra point of failure for password security (e.g. accidental logging).

I've read https://improbable.io/games/blog/grpc-web-moving-past-restjson-towards-type-safe-web-apis and the benefits of grpc-web in their environment sound amazing. I'm a bit envious of the type-safe and automated integration between their backend and frontend.

Those benefits don't apply to AuthN, though, because AuthN is committed to maintaining a client for compatibility with non-gRPC frontend environments. The authn-js client exists and must continue to exist. It was hand-crafted, but is finished and already provides type safety with bundled TypeScript definitions.

So what benefits do you expect from AuthN supporting gRPC-web, compared to using the official authn-js client?

(Regardless of your answer to that question, I'm still interested in gRPC for "private" endpoints.)

[mohammed90]

gRPC-Web support is just a side-effect of having a gRPC API 😄AuthN currently is tedious to use for mobile clients (apps) because it expects to be called from a browser by expecting the Origin header. Mobile clients have to fake being a browser by setting the Origin header. If we keep going on the current path of this PR, current authn-js users can keep using the library thus calling the HTTP interface, gRPC-web users will be able to use the gRPC API, and mobile clients can use AuthN without having to fake being a browser.

Unless I misunderstood your question...

[cainlevy]

mobile clients can use AuthN without having to fake being a browser

Oooo, that's what I was missing! I didn't realize this was solving for non-browser clients. Yes, it makes sense now that Origin and Cookie headers would be problems. I've been expecting this moment but didn't recognize it when it arrived. 😅

I'll reach out on gitter to catch up more quickly on the topic.

[aybabtme]

With regard to using 4 listeners, you can use cmux to multiplex requests on a single listener to either HTTP or gRPC backends. See an example service that does it here: https://gist.github.com/soheilhy/bb272c000f1987f17063#file-server-go-L265-L284

CockroachDB used that for a while (not sure if they still do).

[mohammed90]

Thank you for the review, @aybabtme! Reviews are more welcome now than later exactly to not cause a hassle of breakage. Can I ask you to stick around this PR or do another round of review later? I'll address your current comments in my upcoming commits.

[mohammed90]

Perhaps I should fallback to runtime.HTTPStatusFromCode(code codes.Code) here?

[mohammed90]

Do you think it'd be better to split the proto files and the generated files into their own repo? This way, those using it as gRPC clients don't end up pulling the entire AuthN repo. I haven't explored this path with experimentation, just thinking.

[silasdavis]

Do you think it'd be better to split the proto files and the generated files into their own repo? This way, those using it as gRPC clients don't end up pulling the entire AuthN repo. I haven't explored this path with experimentation, just thinking.

I'd much rather not personally. The protobuf files change with the repo and it feels like this just adds busy work. Clients can pull individual files from github raw via permalink if they like. But cloning authn is hardly a big deal if you are depending on it anyway.

[mohammed90]

It seems possible to get rid of the envelopes by wrapping the outgoing message with the gateway's WithForwardResponseOption. That'd make for cleaner proto messages. I'll fiddle with it and see how it goes.

Post experiment: Nope.

Post post experiment: It works! (With forwarder, not forward option)

[dhillondeep]

What is the status of this PR?

[mohammed90]

What is the status of this PR?

Although I worked on it, both Lance and I have talked about how the size of the change makes it worrying, thus a bit hesitant to accept it. The size of it wasn't predicted as the approach was largely exploratory, i.e. we knew we'll be using gRPC-gateway (because it harmonizes gRPC API with REST API) but we can't foresee full architecture of the change because neither of us have used gRPC-gateway before.

[dhillondeep]

Although I worked on it, both Lance and I have talked about how the size of the change makes worrying, thus a bit hesitant to accept it. The size of it wasn't predicted as the approach was largely exploratory, i.e. we knew we'll be using gRPC-gateway (because it harmonizes gRPC API with REST API) but we can't foresee full architecture of the change because neither of us have used gRPC-gateway before.

I am working on grpc microservices and was thinking of using this project. I agree there will be substantial changes to the way this project is structured. Is it possible to open this conversation again because grpc support will help many people like me trying to use this project. GRPC has grown a lot in recent time and with proper discussion and planning, I think we could do this! I can help with this as well but I would like to ask what you all think

[cainlevy]

I agree with @mohammed90's description. There were a couple of unresolved design questions, but mostly it was a lot of work to create and review. I also expected it to be a maintenance challenge for new contributors.

A few paths forward might look like:

• a dedicated gRPC fork, using go imports to full advantage
• some reduced scope of work
• some new approach to gRPC & Rest hybrid

[dhillondeep]

You are right! In my opinion the safest option would be to have this work carried over in a fork and both projects can sync in terms of functionality in the meantime. If in future the fork gets mature enough that it can handle REST then a possible switch could be possible. This also allows grpc project to develop incrementally and not have to rely on breaking things.

I have forked the project to https://github.com/uptimize/authn-grpc and would like to start with this! @mohammed90 has done some incredible work here and would love to leverage that if I could

[cainlevy]

I'm inclined to close this PR and redirect attention to your fork. What are your opinions @mohammed90 ?

[mohammed90]

I'm inclined to close this PR and redirect attention to your fork. What are your opinions @mohammed90 ?

Sure, go ahead. I wasn't sure whether you wanted to keep it for discussion or not, and that's why I kept it open.