Allow us to point all the grpc servers at files to pick up credentials

[Jonpez2]

I'm currently trying to plug together SPIFFE (https://github.com/spiffe) and buildbarn in a deployment on GKE, to give me well-managed mTLS. My current approach is to use the spiffe-helper (https://github.com/spiffe/spiffe-helper) to keep pem files up to date on the in one container, with spiffe-helper just invoking 'sleep infinity', and then having the buildbarn container share a filesystem so that it can read the resultant certs. Note that SPIFFE rotates the leaf certificates very frequently - on the order of once per hour. This is pretty painful however, as buildbarn uses jsonnet to pipe creds through to the process, which means that a hot reload of the rotated certs is impossible without a full restart.

I'm thinking that the best option here would be for buildbarn to allow specification of a set of files for certs, and then just use grpc's hot reloading code as demonstrated here - https://github.com/grpc/grpc-go/blob/master/security/advancedtls/examples/credential_reloading_from_files/server/main.go

Does that seem right? Or am I missing something obvious?

Thank you!

[Jonpez2]

Any thoughts on this please? Would you prefer me to propose a code change?

[EdSchouten]

Hey! Sorry for letting you wait.

I'm fine with supporting use cases like these. I think the cleanest way to support this is to factor out the keypair string fields we have in our config files to a separate Protobuf message. We can then use a oneof in there to choose between providing the key material inline, vs. using other frameworks.

Instead of using stuff like spiffe-helper, I'm also fine with integrating https://github.com/spiffe/go-spiffe/tree/main/v2. That way you don't need to run all sorts of sidecars just to get TLS working.

[Jonpez2]

My turn to apologize to you!

So actually an even better approach (for me anyway!) would be to upgrade to grpc 1.38.0 and then allow use of xds, like this: https://cloud.google.com/traffic-director/docs/security-proxyless-setup. That still requires a bit of changes inside buildbarn - specifically, all the servers will have to do this (https://github.com/grpc/grpc-go/blob/master/examples/features/xds/server/main.go#L82) - but it means that all the certificate specification and discovery and so forth becomes someone else's problem. Wdyt?

[EdSchouten]

Oh, that would be pretty sweet. Feel free to submit a PR to add a gRPC configuration for enabling xDS!

[Jonpez2]

On it

…

On Fri, Jul 16, 2021 at 1:29 PM Ed Schouten ***@***.***> wrote: Oh, that would be pretty sweet. Feel free to submit a PR to add a gRPC configuration for enabling xDS! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#120 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABN425PBELZED6KGFYSTMP3TYAQ3XANCNFSM45RSVGDQ> .

[Jonpez2]

Nothing is ever simple...
grpc/grpc-go#4601

[EdSchouten]

Hey @Jonpez2!

Would a change like this be of any use to you?

https://github.com/buildbarn/bb-storage/compare/eschouten/20210719-service-registrar

This would allow you to write something like this in pkg/grpc/server.go:

var s interface {
    grpc.ServiceRegistrar
    GetServiceInfo() map[string]grpc.ServiceInfo
    Serve(net.Listener) error
}
if useXDS {
    s = xds.NewGRPCServer(...)
} else {
    s = grpc.NewServer(...)
}

// The rest of the code that registers services and calls .Serve() can go here.

Just let me know and I'll merge this.

[Jonpez2]

Yes that looks perfect!

…

On Mon, 19 Jul 2021 at 09:25, Ed Schouten ***@***.***> wrote: Hey @Jonpez2 <https://github.com/Jonpez2>! Would a change like this be of any use to you? https://github.com/buildbarn/bb-storage/compare/eschouten/20210719-service-registrar This would allow you to write something like this in pkg/grpc/server.go: var s interface { grpc.ServiceRegistrar Serve(net.Listener) error }if useXDS { s = xds.NewGRPCServer(...) } else { s = grpc.NewServer(...) } // The rest of the code that registers services and calls .Serve() can go here. Just let me know and I'll merge this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#120 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABN425LMJEJJZQWKCTRQZYTTYPOP5ANCNFSM45RSVGDQ> .

[EdSchouten]

Great! Merged!

It looks like the construct is sufficient for you to achieve what you want, as long as you take the following into account:

• You need to upgrade gRPC to a version that includes grpc/grpc-go@145f12a, as reflection.Register() won't work otherwise.
• grpc_prometheus.Register() needs to be adjusted similarly. Looks like we can just cherry-pick Add GRPCServer interface to accept different grpc.Server implementations grpc-ecosystem/go-grpc-prometheus#102 for that.

[Jonpez2]

So the change you suggested in your codebase seems to trivially work, which is very nice. However grpc-prometheus appears to be rehousing itself in a different repo, and hasn't released since march. Do we need to port across to the new upstream?

…

On Mon, Jul 19, 2021 at 9:38 AM Ed Schouten ***@***.***> wrote: Great! Merged! It looks like the construct is sufficient for you to achieve what you want, as long as you take the following into account: - You need to upgrade gRPC to a version that includes grpc/grpc-go@ 145f12a <grpc/grpc-go@145f12a>, as reflection.Register() won't work otherwise. - grpc_prometheus.Register() needs to be adjusted similarly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#120 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABN425JJMRE22VLISWS552TTYPP75ANCNFSM45RSVGDQ> .

[EdSchouten]

It looks like v2 hasn't been released yet, so it wouldn't make a lot of sense to invest in that right now. I'd say, just put that PR that I linked above into the already existing patches/com_github_grpc_ecosystem_go_grpc_prometheus/ directory.

[Jonpez2]

ok!

[EdSchouten]

Considering that this issue hasn't received any updates for >1y, I'm going to close it. It should be easier nowadays to get xDS support added, especially with the preparations discussed above. Happy to receive contributions going forward!