[ENH] go/coordinator: grpcserver supports mTLS

[zhangjinpeng87]

Description of changes

Summarize the changes made by this PR.

• New functionality
  • Adding mTLS support for coordinator grpcserver, because we need to make sure the connection & data transport between different component is secure when these components deployed in public cloud env.

Test plan

How are these changes tested?

• Tests pass locally with pytest for python, yarn test for js, make test for golang

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?

[github-actions]

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

• Can you think of any use case in which the code does not behave as intended? Have they been tested?
• Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
• If appropriate, are there adequate property based tests?
• If appropriate, are there adequate unit tests?
• Should any logging, debugging, tracing information be added or removed?
• Are error messages user-friendly?
• Have all documentation changes needed been made?
• Have all non-obvious changes been commented?

System Compatibility

• Are there any potential impacts on other parts of the system or backward compatibility?
• Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

• Is this code of a unexpectedly high quality (Readbility, Modularity, Intuitiveness)

[HammadB]

Thanks @zhangjinpeng1987 - will defer to @beggers and @Ishiihara here. But this makes sense to me conceptually.

What are your thoughts on doing mutual TLS - or do you think server side is good enough?

[beggers]

Nice! @zhangjinpeng1987 thank you for working on this! I see two relevant threat vectors for the coordinator:

• A malicious actor gains access to our k8s network but not our pods. They want to read requests being sent to the coordinator.
• A malicious actor gains access to our k8s network but not our pods. They want to write requests to the coordinator, presumably to extract user data (GetDatabase et al).

TLS solves case 1 but not case 2 since TLS only ensures that the server is who they say they are. In the coordinator's case we also need to ensure that only verified clients can send requests to it which I believe requires mTLS + a private CA which only issues certs for our jobs.

@zhangjinpeng1987 I hate to ask but would you mind modifying this to work with mTLS? I think it should be a pretty small change from what we have already. Example: https://github.com/grpc/grpc-go/blob/591c48187c4b/examples/features/encryption/mTLS/server/main.go#L48-L82

[zhangjinpeng87]

Nice! @zhangjinpeng1987 thank you for working on this! I see two relevant threat vectors for the coordinator:

• A malicious actor gains access to our k8s network but not our pods. They want to read requests being sent to the coordinator.
• A malicious actor gains access to our k8s network but not our pods. They want to write requests to the coordinator, presumably to extract user data (GetDatabase et al).

TLS solves case 1 but not case 2 since TLS only ensures that the server is who they say they are. In the coordinator's case we also need to ensure that only verified clients can send requests to it which I believe requires mTLS + a private CA which only issues certs for our jobs.

@zhangjinpeng1987 I hate to ask but would you mind modifying this to work with mTLS? I think it should be a pretty small change from what we have already. Example: https://github.com/grpc/grpc-go/blob/591c48187c4b/examples/features/encryption/mTLS/server/main.go#L48-L82

@beggers thanks for your reminding, mTLS is definitely better in this case. Done.

[beggers]

Nice, thank you! Very happy to have this in the codebase.

[zhangjinpeng87]

@beggers seems the CI failure has nothing to do with this change, any way to fix it or rerun it?

[HammadB]

I reran, I think the error is somehow due to these changes, they are from the go dockerfile build.

[beggers]

It looks like the failing line is just creating shell autocomplete for the CLI tool which we use to run the coordinator inside of its Docker image -- should be safe to delete.

The line in question:

RUN chroma completion bash > ~/.bashrc

[HammadB]

I think we should understand why this change is breaking that?

…

On Thu, Nov 16, 2023 at 9:06 AM Ben Eggers ***@***.***> wrote: It looks like the failing line is just creating shell autocomplete for the CLI tool which we use to run the coordinator inside of its Docker image -- should be safe to delete. The line in question: RUN chroma completion bash > ~/.bashrc — Reply to this email directly, view it on GitHub <#1362 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKW32MW2SVBW5ETH2URLPDYEZBXXAVCNFSM6AAAAAA7DYX6POVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJUHA3DMNJWGE> . You are receiving this because you commented.Message ID: ***@***.***>

[beggers]

@zhangjinpeng1987 thank you! CI is happy now so merging this :^)