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

NOISSUE - Implementation of gRPC mTLS #1848

Merged
merged 11 commits into from
Aug 16, 2023
Merged

Conversation

arvindh123
Copy link
Contributor

@arvindh123 arvindh123 commented Jul 5, 2023

Pull request title should be MF-XXX - description or NOISSUE - description where XXX is ID of issue that this PR relate to.
Please review the CONTRIBUTING.md file for detailed contributing guidelines.

What does this do?

This PR aims to add MTLS (Mutual Transport Layer Security) support for the gRPC client in the Mainflux . It includes changes to both the gRPC server side and client side. The implementation of MTLS ensures secure communication between the client and server by requiring authentication and encryption for the transmitted data.

Which issue(s) does this PR fix/relate to?

Put here Resolves #XXX to auto-close the issue that your PR fixes (if such)
No

List any changes that modify/break current functionality

No

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

@arvindh123 arvindh123 changed the title NOISSUES: Implementation of gRPC mTLS NOISSUE: Implementation of gRPC mTLS Jul 5, 2023
@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #1848 (ef8bfc6) into master (9dbe87f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1848   +/-   ##
=======================================
  Coverage   67.16%   67.16%           
=======================================
  Files         118      118           
  Lines        9076     9076           
=======================================
  Hits         6096     6096           
  Misses       2351     2351           
  Partials      629      629           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@arvindh123 arvindh123 marked this pull request as ready for review July 10, 2023 15:31
@arvindh123 arvindh123 requested a review from a team as a code owner July 10, 2023 15:31
Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need documentation for this, probably with a PR to MF docs.

Makefile Outdated Show resolved Hide resolved
docker/addons/bootstrap/docker-compose.grpc-mtls.yaml Outdated Show resolved Hide resolved
docker/ssl/docker-compose.grpc-mtls.yaml Outdated Show resolved Hide resolved
internal/clients/grpc/connect.go Outdated Show resolved Hide resolved
internal/clients/grpc/connect.go Outdated Show resolved Hide resolved
@dborovcanin dborovcanin force-pushed the grpc_mtls branch 2 times, most recently from 1b32f08 to bb53a21 Compare July 12, 2023 09:29
Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am getting these warning on

make run args "-d"


GNU Make 4.3
Built for x86_64-pc-linux-gnu
Copyright (C) 1988-2020 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Reading makefiles...
Reading makefile 'Makefile'...
Updating makefiles....
 Considering target file 'Makefile'.
  Looking for an implicit rule for 'Makefile'.
  Trying pattern rule with stem 'Makefile'.
  Trying implicit prerequisite 'Makefile.o'.
  Trying pattern rule with stem 'Makefile'.
  Trying implicit prerequisite 'Makefile.c'.
  Trying pattern rule with stem 'Makefile'.
  Trying implicit prerequisite 'Makefile.cc'.
  Trying pattern rule with stem 'Makefile'.
  Trying implicit prerequisite 'Makefile.C'.
  Trying pattern rule with stem 'Makefile'.
  Trying implicit prerequisite 'Makefile.cpp'.

Makefile Outdated Show resolved Hide resolved
@arvindh123 arvindh123 force-pushed the grpc_mtls branch 2 times, most recently from 818b45a to e70bfc1 Compare July 12, 2023 15:19
docker/.env Outdated Show resolved Hide resolved
docker/.env Outdated Show resolved Hide resolved
internal/clients/grpc/connect.go Outdated Show resolved Hide resolved
internal/clients/grpc/connect.go Outdated Show resolved Hide resolved
internal/server/grpc/grpc.go Show resolved Hide resolved
internal/server/grpc/grpc.go Show resolved Hide resolved
internal/clients/grpc/connect.go Outdated Show resolved Hide resolved
internal/clients/grpc/connect.go Show resolved Hide resolved
Copy link
Member

@rodneyosodo rodneyosodo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dborovcanin to review, please

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the PR looks good. However, Makefile and the whole process of running services looks a little bit to complex now. I'd like you try to:

  1. Avoid repetition of compose files (especially since ssl/docker-compose.grpc-mtls.yml is just a repetition of 2 sections)
  2. Avoid two compose files for add-ons, as well
  3. Avoid special prefixes for addons
  4. Simplify Makefile (ssl/Makefile looks fine, only the root Makefile needs some love)

@drasko drasko changed the title NOISSUE: Implementation of gRPC mTLS NOISSUE - Implementation of gRPC mTLS Aug 8, 2023
author Arvindh <arvindh91@gmail.com> 1688570218 +0530
committer Arvindh <arvindh91@gmail.com> 1689754950 +0530

Rebase with master and squash commits

parent 1192325
author Arvindh <arvindh91@gmail.com> 1688570218 +0530
committer Arvindh <arvindh91@gmail.com> 1689174782 +0530

add: rootCA and clientCA in grpc server

Signed-off-by: Arvindh <arvindh91@gmail.com>

add: rootCA and client certificate in grpc client

Signed-off-by: Arvindh <arvindh91@gmail.com>

add: docker-compose for grpc-mtls and make target for mtls cert generation

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix: typo in makefile

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix: loadCertFile function in internal/clients/grpc/connect.go

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix: env.parser test

Signed-off-by: Arvindh <arvindh91@gmail.com>

remove: commented lines

Signed-off-by: Arvindh <arvindh91@gmail.com>

add: make commands

Signed-off-by: Arvindh <arvindh91@gmail.com>

update: make commands and grpc clients

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix: typo in makefile

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix: loadCertFile function in internal/clients/grpc/connect.go

Signed-off-by: Arvindh <arvindh91@gmail.com>

remove: commented lines

Signed-off-by: Arvindh <arvindh91@gmail.com>

update: make commands and grpc clients

Signed-off-by: Arvindh <arvindh91@gmail.com>

update: make commands and docker-compose

Signed-off-by: Arvindh <arvindh91@gmail.com>

add: end of line

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix: typos in makefile

Signed-off-by: Arvindh <arvindh91@gmail.com>

add: end of line

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix: typos in makefile

Signed-off-by: Arvindh <arvindh91@gmail.com>

revert: grafana port in .env

Signed-off-by: Arvindh <arvindh91@gmail.com>

change: loadCertFile function

Signed-off-by: Arvindh <arvindh91@gmail.com>

change: certficate logic

Signed-off-by: Arvindh <arvindh91@gmail.com>

change: env name and update in compose file

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix: makefile

Signed-off-by: Arvindh <arvindh91@gmail.com>

remove: tls env var

Signed-off-by: Arvindh <arvindh91@gmail.com>

change: ioutil to os for ReadFile

Signed-off-by: Arvindh <arvindh91@gmail.com>

change loadfile

Signed-off-by: Arvindh <arvindh91@gmail.com>

remove: test which is no needed

Signed-off-by: Arvindh <arvindh91@gmail.com>

fix: docker project name

Signed-off-by: Arvindh <arvindh91@gmail.com>

single docker-compose file

Signed-off-by: Arvindh <arvindh91@gmail.com>

single docker-compose file

Signed-off-by: Arvindh <arvindh91@gmail.com>

single docker-compose file

Signed-off-by: Arvindh <arvindh91@gmail.com>
Signed-off-by: Arvindh <arvindh91@gmail.com>
@arvindh123
Copy link
Contributor Author

arvindh123 commented Aug 14, 2023

@mainflux/contributors @mainflux/maintainers
To test this PR and to run gRPC with mTLS , following docs will be helpful

Running Mainflux Core Services

To run all the Mainflux core services, use the following command:

make run

This command will start all the mainflux core services.

If you want to run the Mainflux core services with gRPC mTLS enabled, use the following command:

GRPC_MTLS=true make run

If you want to run the Mainflux core services with gRPC TLS enabled, use the following command:

GRPC_TLS=true make run

This will start all the mainflux core services with secure communication using automatically generated certificates if they are not already available.

Running Specific Addons

If you only want to run specific addons, you can specify them in the command. For example, to run the timescale-reader and timescale-writer addons, use the following command:

make run_addons timescale-reader timescale-writer

This command will start the specified addons.

If you want to run specific addons with gRPC mTLS enabled, use the following command:

GRPC_MTLS=true make run_addons timescale-reader timescale-writer

If you want to run specific addons with gRPC TLS enabled, use the following command:

GRPC_TLS=true make run_addons timescale-reader timescale-writer

This will start the specified addons with secure communication using automatically generated certificates if they are not already available.

Running Addons Services

To run all the addons services, you can use the following command:

make run_addons

This command will start all the addons services.

If you want to run the addons services with gRPC mTLS enabled, you can use the following command:

GRPC_MTLS=true make run_addons

If you want to run the addons services with gRPC TLS enabled, you can use the following command:

GRPC_TLS=true make run_addons

This will start all the addons services with secure communication using automatically generated certificates if they are not already available.

target: /users-grpc-client${MF_USERS_GRPC_CLIENT_KEY:+.key}
- type: bind
source: ${MF_ADDONS_CERTS_PATH_PREFIX}${MF_USERS_GRPC_SERVER_CA_CERTS:-./ssl/certs/dummy/server_ca}
target: /users-grpc-server-ca${MF_USERS_GRPC_SERVER_CA_CERTS:+.crt}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add an empty line.

target: /things-grpc-client${MF_THINGS_AUTH_GRPC_CLIENT_KEY:+.key}
- type: bind
source: ${MF_ADDONS_CERTS_PATH_PREFIX}${MF_THINGS_AUTH_GRPC_SERVER_CA_CERTS:-./ssl/certs/dummy/server_ca}
target: /things-grpc-server-ca${MF_THINGS_AUTH_GRPC_SERVER_CA_CERTS:+.crt}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same.

target: /things-grpc-client${MF_THINGS_AUTH_GRPC_CLIENT_KEY:+.key}
- type: bind
source: ${MF_ADDONS_CERTS_PATH_PREFIX}${MF_THINGS_AUTH_GRPC_SERVER_CA_CERTS:-./ssl/certs/dummy/server_ca}
target: /things-grpc-server-ca${MF_THINGS_AUTH_GRPC_SERVER_CA_CERTS:+.crt}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. Chack all the other files for this, as well.

docker/docker-compose.yml Show resolved Hide resolved
Signed-off-by: Arvindh <arvindh91@gmail.com>
Signed-off-by: Arvindh <arvindh91@gmail.com>
Signed-off-by: Arvindh <arvindh91@gmail.com>
Signed-off-by: Arvindh <arvindh91@gmail.com>
Signed-off-by: Arvindh <arvindh91@gmail.com>
Signed-off-by: Arvindh <arvindh91@gmail.com>
Signed-off-by: Arvindh <arvindh91@gmail.com>
type security int

const (
WITHOUT_TLS security = iota
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to Go idiomatic way of denoting consts.

Makefile Outdated
BRANCH ?= $(shell git rev-parse --abbrev-ref HEAD 2>/dev/null || git describe --tags --abbrev=0 2>/dev/null )
empty:=
space:= $(empty) $(empty)
DOCKER_PROJECT ?= $(subst $(space),,$(USER_REPO)_$(BRANCH))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try doing it in one line, but making it readable?

Signed-off-by: Arvindh <arvindh91@gmail.com>
Signed-off-by: Arvindh <arvindh91@gmail.com>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dborovcanin dborovcanin merged commit fde4350 into absmach:master Aug 16, 2023
4 checks passed
@dborovcanin dborovcanin deleted the grpc_mtls branch August 16, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants