Skip to content

Conversation

@rahulkhinchi-wq
Copy link

This pull request resolves the Semgrep security finding grpc-server-insecure-connection by preventing the creation of an insecure gRPC server instance.

Issue

Semgrep reported that the code was creating a gRPC server without TLS credentials when CertFile or KeyFile were missing:
grpc.NewServer()

Without credentials, the server runs without encryption, which exposes the application to risks such as unencrypted traffic, message tampering, and possible man-in-the-middle attacks.

Rule details:

  • Rule ID: grpc-server-insecure-connection
  • File: pkg/util/grpcconnection/config.go
  • Line: 74

Fix

The function now returns an error if either CertFile or KeyFile is not provided, instead of silently creating an insecure server.

Code change

-   return grpc.NewServer(), nil
+   return nil, fmt.Errorf("TLS credentials are required: both CertFile and KeyFile must be specified for secure gRPC server")
This enforces secure-by-default behavior and blocks insecure gRPC server instantiation.

Impact

Removes insecure default behavior.
Ensures that a secure TLS configuration is always provided.
Aligns gRPC server initialization with recommended security practices.

root and others added 2 commits November 14, 2025 12:19
…secure-connection-74-vTQ3JDXCbk

fix: semgrep-grpc-server-insecure-connection
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign chaunceyjiang for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Nov 25, 2025
@karmada-bot
Copy link
Collaborator

Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@karmada-bot
Copy link
Collaborator

Welcome @rahulkhinchi-wq! It looks like this is your first PR to karmada-io/karmada 🎉

@gemini-code-assist
Copy link

Summary of Changes

Hello @rahulkhinchi-wq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical security vulnerability identified by Semgrep, specifically grpc-server-insecure-connection. The core change modifies the gRPC server initialization logic to enforce the presence of TLS credentials. Previously, the system would silently create an unencrypted gRPC server if certificate and key files were absent, posing risks like unencrypted traffic and man-in-the-middle attacks. The updated implementation now returns an error in such cases, ensuring that all gRPC server instances are secure by default and align with recommended security practices.

Highlights

  • Security Fix for Insecure gRPC Connection: This pull request resolves a Semgrep security finding by preventing the creation of an insecure gRPC server instance when TLS credentials are not provided.
  • Enforced TLS Credentials: The NewServer function in ServerConfig now explicitly returns an error if either CertFile or KeyFile is missing, ensuring that gRPC servers are always initialized with secure TLS configurations.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@karmada-bot karmada-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 25, 2025
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly addresses a security vulnerability by preventing the creation of an insecure gRPC server. The change ensures that TLS credentials (both certificate and key file) are provided by returning an error if they are missing. My feedback includes a minor style suggestion for the new error message to align with Go conventions and maintain consistency with the existing codebase.

func (s *ServerConfig) NewServer() (*grpc.Server, error) {
if s.CertFile == "" || s.KeyFile == "" {
return grpc.NewServer(), nil
return nil, fmt.Errorf("TLS credentials are required: both CertFile and KeyFile must be specified for secure gRPC server")

Choose a reason for hiding this comment

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

medium

According to Go's error string conventions, and for consistency with the rest of the codebase, error messages should not be capitalized. Please consider starting the error message with a lowercase letter.

Suggested change
return nil, fmt.Errorf("TLS credentials are required: both CertFile and KeyFile must be specified for secure gRPC server")
return nil, fmt.Errorf("tls credentials are required: both CertFile and KeyFile must be specified for secure gRPC server")

@zhzhuang-zju
Copy link
Contributor

Semgrep reported that the code was creating a gRPC server without TLS credentials when CertFile or KeyFile were missing:
grpc.NewServer()

@rahulkhinchi-wq Thanks for reporting this. Could you share the full Semgrep report by email? (my email address: [email protected], Using email is to avoid the accidental exposure of security issues.)
Did it flag any other issues?

Also, I'm a bit skeptical whether this is actually a bug. First, TLS configuration is optional by design. Second, the Karmada-maintained installation tools configure certificates and enable TLS by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants