Skip to content

Conversation

@YuanTingHsieh
Copy link
Collaborator

Add a way for users to use self signed certificates if they prefer.

Description

Add a way for users to use self signed certificates if they prefer.

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • Breaking change (fix or new feature that would cause existing functionality to change).
  • New tests added to cover the changes.
  • Quick tests passed locally by running ./runtest.sh.
  • In-line docstrings updated.
  • Documentation updated.

Copilot AI review requested due to automatic review settings October 28, 2025 19:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for allowing self-signed SSL certificates in the device simulator by introducing an allow_self_signed configuration option. This enables the simulator to connect to endpoints using self-signed certificates for development and testing purposes.

  • Introduces allow_self_signed configuration parameter in ConfigParser
  • Modifies FegApi to accept and use the allow_self_signed flag for SSL verification
  • Updates device simulator to pass the configuration through to the API client

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
nvflare/edge/simulation/config.py Adds parsing and storage of allow_self_signed boolean configuration parameter
nvflare/edge/simulation/feg_api.py Updates FegApi constructor and HTTP request handling to support disabling SSL verification when self-signed certificates are allowed
nvflare/edge/simulation/run_device_simulator.py Passes allow_self_signed configuration from parser to FegApi instantiation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 95 to 97
response = requests.post(
url, params=params, json=body, headers=self.common_headers, verify=False if self.allow_self_signed else True
)
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

When verify=False is used with requests.post(), it triggers InsecureRequestWarning from urllib3. These warnings should be suppressed when allow_self_signed is True to avoid cluttering logs. Add import urllib3 at the top of the file and call urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) in the __init__ method when allow_self_signed is True.

Copilot uses AI. Check for mistakes.
check_positive_number("get_job_timeout", n)
self.get_job_timeout = n

n = config.get("allow_self_signed", False)
Copy link

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

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

The allow_self_signed configuration value is not validated to ensure it's a boolean type. If a user provides a non-boolean value (e.g., a string like "true" or an integer like 1), it could lead to unexpected behavior. Add type validation using check_object_type(\"allow_self_signed\", n, bool) after retrieving the value, similar to how other configuration values are validated in this method.

Suggested change
n = config.get("allow_self_signed", False)
n = config.get("allow_self_signed", False)
check_object_type("allow_self_signed", n, bool)

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

Added allow_self_signed configuration option to enable self-signed certificate support in edge simulator by disabling SSL verification in HTTP requests.

  • Added allow_self_signed boolean field to ConfigParser that reads from config file
  • Modified FegApi constructor to accept allow_self_signed parameter
  • Updated _do_post() method to conditionally set verify=False when self-signed certificates are allowed
  • Wired the configuration through from ConfigParser to FegApi via run_device_simulator.py

Issues Found:

  • Missing type validation for allow_self_signed config parameter (could cause runtime errors if non-boolean value provided)
  • urllib3 will emit InsecureRequestWarning warnings when SSL verification is disabled, which should be suppressed for cleaner output

Confidence Score: 3/5

  • This PR is safe to merge with minor fixes needed for production readiness
  • The implementation correctly adds self-signed certificate support, but has two issues: missing boolean validation for the config parameter (could cause runtime errors) and missing warning suppression for urllib3 (causes noisy output). Both are straightforward fixes that don't affect core functionality
  • Pay attention to nvflare/edge/simulation/feg_api.py and nvflare/edge/simulation/config.py for the validation and warning suppression fixes

Important Files Changed

File Analysis

Filename Score Overview
nvflare/edge/simulation/config.py 3/5 Added allow_self_signed config parameter, but missing type validation for boolean value
nvflare/edge/simulation/feg_api.py 2/5 Implemented SSL verification bypass using verify=False, but missing urllib3 warning suppression

Sequence Diagram

sequenceDiagram
    participant User
    participant ConfigParser
    participant Simulator
    participant FegApi
    participant Server

    User->>ConfigParser: load config with allow_self_signed
    ConfigParser->>ConfigParser: parse allow_self_signed flag
    User->>Simulator: run_simulator()
    Simulator->>FegApi: create with allow_self_signed flag
    FegApi->>FegApi: store allow_self_signed
    
    loop For each API request
        Simulator->>FegApi: get_job/get_task/report_result
        FegApi->>FegApi: _do_post()
        alt allow_self_signed is True
            FegApi->>Server: requests.post(verify=False)
        else allow_self_signed is False
            FegApi->>Server: requests.post(verify=True)
        end
        Server-->>FegApi: response
        FegApi-->>Simulator: parsed response
    end
Loading

3 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

YuanTingHsieh and others added 2 commits November 6, 2025 12:59
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

Adds allow_self_signed configuration option to enable self-signed SSL certificates in the edge simulator.

Key Changes:

  • Added allow_self_signed boolean field to ConfigParser with proper validation
  • Modified FegApi.__init__ to accept allow_self_signed parameter
  • Updated _do_post method to disable SSL verification when allow_self_signed=True
  • Integrated the new parameter through the simulator workflow in run_device_simulator.py

Issues Already Identified:

  • Previous comments correctly identified that urllib3.disable_warnings should be called once during initialization rather than on every request
  • Previous comments correctly identified missing boolean validation (now implemented)

Confidence Score: 4/5

  • This PR is safe to merge with minor performance concern already noted in previous comments
  • The implementation correctly threads the allow_self_signed flag through the system with proper validation. The previous comments identified the two main issues: inefficient warning suppression on every request and the need for boolean validation (which is now implemented). No new critical issues found.
  • nvflare/edge/simulation/feg_api.py needs attention for the urllib3 warning suppression efficiency issue

Important Files Changed

File Analysis

Filename Score Overview
nvflare/edge/simulation/config.py 5/5 Added allow_self_signed boolean config field with proper validation matching existing patterns
nvflare/edge/simulation/feg_api.py 3/5 Added SSL verification bypass for self-signed certs, but urllib3 import and warning suppression happen on every request (inefficient)

Sequence Diagram

sequenceDiagram
    participant Config as ConfigParser
    participant Sim as Simulator
    participant Device as SimulatedDevice
    participant Api as FegApi
    participant Server as Edge Server

    Config->>Config: Load allow_self_signed from JSON config
    Config->>Config: Validate allow_self_signed is boolean
    
    Sim->>Device: Create simulated devices
    
    Device->>Api: Initialize FegApi(endpoint, device_info, user_info, allow_self_signed)
    Api->>Api: Store allow_self_signed flag
    
    Device->>Api: Make API request (get_job/get_task/report_result)
    Api->>Api: _do_post()
    
    alt allow_self_signed == True
        Api->>Api: Import urllib3
        Api->>Api: urllib3.disable_warnings(InsecureRequestWarning)
        Api->>Server: requests.post(..., verify=False)
    else allow_self_signed == False
        Api->>Server: requests.post(..., verify=True)
    end
    
    Server-->>Api: Response
    Api-->>Device: Parsed response
Loading

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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.

2 participants