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

feat: add TLS for gRPC server endpoint #477

Merged
merged 11 commits into from
Jun 9, 2024

Conversation

DWJ-Squirtle
Copy link
Contributor

What type of PR is this?

Which issue(s) this PR fixes:

Fixes #

cmd/server.go Outdated
@@ -170,7 +181,15 @@ func (o *serverOption) preRunE(cmd *cobra.Command, args []string) (err error) {

grpcOpts = append(grpcOpts, oauth.NewAuthInterceptor(o.oauthGroup))
}

if o.tls=="tlsup"{
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to use bool instead of string here if you just want to have an option to determine if you enable the TLS.

cmd/server.go Outdated
dir,_:=os.Getwd()
cwd:=filepath.Dir(dir)
flags.StringVarP(&opt.tls,"tls-grpc","",os.Getenv("TLS_MODE"),"The tls mode, supported: tlsup. Keep it empty to disable tls")
flags.StringVarP(&opt.tlsCert, "cert-file", "", filepath.Join(cwd, "certs",""), "The path to the certificate file")
Copy link
Owner

Choose a reason for hiding this comment

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

IMO, the default value should be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope users can use relative paths to read the certificates, so I did it this way

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, the default value is incorrect because it's a directory instead of a file path. Assume user A uses the CLI like the below:

atest server --cert-file my-cert

It will read the file from the current directory.

certs/README.md Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Please move it to https://github.com/LinuxSuRen/api-testing/tree/dd90d1e90ef00db1af0657782004c934ea5e16b9/docs.

You can rename the filename to tls.md or certificate.md as well.

@LinuxSuRen LinuxSuRen changed the title add TLS to grpc feat: add TLS for gRPC server endpoint Jun 6, 2024
@LinuxSuRen LinuxSuRen added enhancement New feature or request ospp 开源之夏 https://summer-ospp.ac.cn/ labels Jun 6, 2024
Copy link
Owner

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

I cannot visit the web UI when setting the tls. See the errors below:

connection error: desc = \"transport: authentication handshake failed: tls: failed to verify certificate: x509: certificate is not valid for any names, but wanted to match localhost\"

This is my step to start server:

cd console/atest-ui
npm i && npm run build-only
cd -
openssl genrsa -out server.key 2048
openssl req -new -x509 -key server.key -out server.crt -days 36500 -subj "/C=US/ST=Denial/L=Springfield/O=Dis/CN=www.example.com"
go run . server --tls-grpc --cert-file server.crt --key-file server.key --console-path console/atest-ui/dist --local-storage 'bin/*.yaml'

@DWJ-Squirtle
Copy link
Contributor Author

I cannot visit the web UI when setting the tls. See the errors below:

connection error: desc = \"transport: authentication handshake failed: tls: failed to verify certificate: x509: certificate is not valid for any names, but wanted to match localhost\"

This is my step to start server:

cd console/atest-ui
npm i && npm run build-only
cd -
openssl genrsa -out server.key 2048
openssl req -new -x509 -key server.key -out server.crt -days 36500 -subj "/C=US/ST=Denial/L=Springfield/O=Dis/CN=www.example.com"
go run . server --tls-grpc --cert-file server.crt --key-file server.key --console-path console/atest-ui/dist --local-storage 'bin/*.yaml'

You need to use SAN certificates, as only SAN certificates are currently supported. I understand that CN certificates have been deprecated

@LinuxSuRen
Copy link
Owner

Have you tested it from the web page?

  • Generate TLS certificate files
  • Run server via go run . server --tls-grpc --cert-file server.crt --key-file server.key --console-path console/atest-ui/dist --local-storage 'bin/*.yaml'
  • Visit the web page. try to create test suite and run some tests

Copy link

sonarcloud bot commented Jun 8, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Owner

@LinuxSuRen LinuxSuRen left a comment

Choose a reason for hiding this comment

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

LGTM

I have tested it manually. It works well. And thanks for your effort.

openssl genrsa -out server.key 2048
# Generate self-signed certificate
openssl req -new -x509 -key server.key -out server.crt -days 36500 \
-subj "/C=US/ST=Denial/L=Springfield/O=Dis/CN=www.example.com"
Copy link
Owner

Choose a reason for hiding this comment

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

Usually the new line will start with a tab for a single command.

@LinuxSuRen LinuxSuRen merged commit 2e0779d into LinuxSuRen:master Jun 9, 2024
15 checks passed
LinuxSuRen pushed a commit that referenced this pull request Jun 17, 2024
* chore(deps): update louislam/uptime-kuma docker tag to v1.23.3

* Update app version [skip ci]

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: github-action update-app-version <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ospp 开源之夏 https://summer-ospp.ac.cn/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants