skip cert when local by tavdog · Pull Request #23 · tronbyt/firmware-esp32

tavdog · 2025-04-08T09:50:28Z

if the majority of users will using tronbyt locally then it makes sense to allow for skiping cert when on local network.

gemini-code-assist

Hello @tavdog, I'm Gemini Code Assist¹! 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!

Summary of Changes

This pull request introduces a mechanism to skip SSL certificate verification when the application is running on a local network. The primary motivation, as stated in the PR description, is to simplify the development experience for users who primarily use tronbyt locally. This is achieved by adding a new function is_local_address that checks if a given URL points to a local address. If it does, the crt_bundle_attach option is not set in the HTTP client configuration, effectively skipping certificate verification. Additionally, the sdkconfig.defaults file is modified to enable insecure TLS settings by default.

Highlights

Skip Certificate Verification: Introduces a mechanism to bypass SSL certificate verification for local network addresses.
Local Address Detection: Implements the is_local_address function to identify URLs pointing to local resources based on prefixes like '10.', '192.168.', '172.', '127.', and '.local'.
Default Insecure TLS Settings: Enables insecure TLS settings by default in sdkconfig.defaults for local development.

Changelog

sdkconfig.defaults
- Enabled CONFIG_ESP_TLS_INSECURE and CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY by default.
src/remote.c
- Added declaration of is_local_address function.
- Conditionally attaches the certificate bundle based on whether the URL is a local address.
- Implemented the is_local_address function to determine if a URL is local.

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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

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 issue 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 is currently in preview and 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 to provide feedback.

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.

For local dev's ease,
Cert checks we now appease,
A quick, simple test,
To put minds at rest,
No HTTPS, if you please.

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. ↩

gemini-code-assist

Code Review

This pull request aims to skip certificate verification when connecting to a local address. The implementation adds a is_local_address function to check the URL and modifies the remote_get function to conditionally skip certificate verification. While the core functionality seems sound, there are some areas for improvement regarding security, maintainability, and error handling.

Summary of Findings

Security Risk: Insecure Certificate Handling: Skipping certificate verification introduces a significant security risk. Malicious actors could intercept communication if the connection isn't truly local. Consider alternative approaches to identify local connections that don't compromise security.
Maintainability: Function Clarity and Documentation: The is_local_address function could benefit from more detailed comments explaining the logic behind the IP address range checks and the rationale for including specific prefixes. Adding a clear description of the function's purpose and parameters would enhance maintainability.
Error Handling: Missing Error Checks: The is_local_address function lacks error handling for potential issues like buffer overflows in strncpy and strtol. Adding checks for these conditions would improve robustness.
Efficiency: Redundant String Operations: The is_local_address function repeatedly calls strlen and strncmp. Consider optimizing these operations to improve efficiency.

Merge Readiness

This pull request introduces a significant security vulnerability by disabling certificate verification. I strongly recommend not merging this pull request until the security concerns are addressed. Even after addressing the security and maintainability issues, further review and approval from other reviewers are required before merging. I cannot approve this pull request under any circumstances.

gemini-code-assist · 2025-04-08T09:54:43Z

+CONFIG_ESP_TLS_INSECURE=y
+CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY=y


Setting CONFIG_ESP_TLS_INSECURE=y and CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY=y globally is a major security risk. This should not be done globally. Consider using environment variables or build flags to control this setting for specific builds or configurations. How will this be controlled in production?

of course it's gonna say that.

I tested it with just CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY=y and it still fails to fetch https over LAN.

gemini-code-assist · 2025-04-08T09:54:43Z

+  if (is_local_address(url)) {
+    ESP_LOGI(TAG, "local address, skipping cert validation");
+  } else {
+    config.crt_bundle_attach = esp_crt_bundle_attach;
+  }


While the conditional check for local addresses is a step in the right direction, simply skipping certificate verification is still insecure. Explore alternative, secure methods for identifying local connections. For example, you could check for a specific local IP address range or use a more robust method to verify the identity of the server. What are the implications of this approach in a production environment?

Suggested change

if (is_local_address(url)) {

ESP_LOGI(TAG, "local address, skipping cert validation");

} else {

config.crt_bundle_attach = esp_crt_bundle_attach;

}

if (is_local_address(url)) {

ESP_LOGI(TAG, "local address, using secure local connection method");

// Implement secure local connection method here

} else {

config.crt_bundle_attach = esp_crt_bundle_attach;

}

gemini-code-assist · 2025-04-08T09:54:43Z

+bool is_local_address(const char* url) {
+  // Skip protocol prefix if present
+  const char* http_prefix = "http://";
+  const char* https_prefix = "https://";
+
+  if (strncmp(url, http_prefix, strlen(http_prefix)) == 0) {
+    url += strlen(http_prefix);
+  } else if (strncmp(url, https_prefix, strlen(https_prefix)) == 0) {
+    url += strlen(https_prefix);
+  }
+
+  // Make a copy of the host part (without the port)
+  char host[256];
+  strncpy(host, url, sizeof(host) - 1);
+  host[sizeof(host) - 1] = '\0';
+
+  // Remove port number if present
+  char* port = strchr(host, ':');
+  if (port != NULL) {
+    *port = '\0';  // Terminate string at the colon
+  }
+
+  // Check for .local TLD
+  size_t len = strlen(host);
+  if (len >= 6 && strcmp(host + len - 6, ".local") == 0) {
+    return true;
+  }
+
+  // Check if starts with common local IP prefixes
+  if (strncmp(host, "10.", 3) == 0) {
+    return true;
+  }
+
+  if (strncmp(host, "192.168.", 8) == 0) {
+    return true;
+  }
+
+  if (strncmp(host, "172.", 4) == 0) {
+    // Check for 172.16.0.0 to 172.31.255.255 range
+    char* end;
+    long second_octet = strtol(host + 4, &end, 10);
+    if (*end == '.' && second_octet >= 16 && second_octet <= 31) {
+      return true;
+    }
+  }
+
+  if (strncmp(host, "127.", 4) == 0) {
+    return true;
+  }
+
+  return false;
+}


The is_local_address function needs more robust error handling and clearer documentation. The strncpy function should check for potential buffer overflows. The strtol function should also be checked for errors. Add comments to explain the logic and the rationale behind the IP address ranges used. Consider using a more efficient approach to avoid redundant string operations.

bool is_local_address(const char* url) { // Skip protocol prefix if present const char* http_prefix = "http://"; const char* https_prefix = "https://"; if (strncmp(url, http_prefix, strlen(http_prefix)) == 0) { url += strlen(http_prefix); } else if (strncmp(url, https_prefix, strlen(https_prefix)) == 0) { url += strlen(https_prefix); } // Make a copy of the host part (without the port) char host[256]; if (strncpy(host, url, sizeof(host) - 1) == NULL) { //Error check ESP_LOGE(TAG, "strncpy failed"); return false; } host[sizeof(host) - 1] = '\0'; // Remove port number if present char* port = strchr(host, ':'); if (port != NULL) { *port = '\0'; // Terminate string at the colon } // Check for .local TLD size_t len = strlen(host); if (len >= 6 && strcmp(host + len - 6, ".local") == 0) { return true; } // Check if starts with common local IP prefixes char *end; long ip_num = strtol(host, &end, 10); if (*end != '.' || errno == ERANGE) { //Error check ESP_LOGE(TAG, "strtol failed"); return false; } // ... (rest of the IP address checks with error handling) return false; }

IngmarStein · 2025-04-08T11:35:19Z

+CONFIG_ESP_TLS_INSECURE=y
+CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY=y


IngmarStein · 2025-04-08T11:37:23Z

+  }
+
+  // Check if starts with common local IP prefixes
+  if (strncmp(host, "10.", 3) == 0) {


TLS certificates for local IP addresses? This doesn't make a lot of sense.
Certificates technically can be issued for IP addresses instead of FQDNs, but this is rare and it's only for public addresses.

yeah, any ip should just return true i guess.

Huh? You want is_local_address to return true for publicly routable IP addresses? I really don't think this is worth spending time on. Disabling TLS verification is worse than just not using it because it gives some false sense of security.

It's not disabled though. It only skips if you don't set the cert_bundle.

I understand that, but that flag is still only meant for testing and should not be set by default.
There's no good way to determine whether a given hostname is "local" or not. .local is not a TLD (https://en.wikipedia.org/wiki/.local) and only used for mDNS in a local network, but there can be infinitely other local domains (.lan is common, mine is .localdomain) or just plain hostnames resolved against a search domain delivered via DHCP.
Even if you resolve the hostname via DNS and look at whether the resulting IP (v4 and/or v6) are local, it's vulnerable to DNS spoofing.
Because there's no good way to do that, you can't decider when to supply the default CAs and when to skip verification.

so what's the solution for someone who wants to use https on a local address ?

Either use a valid certificate from a default CA (e.g. using the DNS-01 challenge), provide a custom CA certificate to the firmware, or use HTTP (without the S).

I personally use the first option, for example.

skip cert when local

ddea011

gemini-code-assist Bot reviewed Apr 8, 2025

View reviewed changes

gemini-code-assist Bot suggested changes Apr 8, 2025

View reviewed changes

tavdog mentioned this pull request Apr 8, 2025

shall we enable insecure mode for local https ? #22

Closed

IngmarStein requested changes Apr 8, 2025

View reviewed changes

tavdog added 2 commits April 8, 2025 12:12

just use strstr to detect .local and //192, etc.

68364ea

use strstr correctly

0cd709b

tavdog closed this Apr 8, 2025

tavdog deleted the skip_cert_when_local branch June 29, 2025 22:06

		CONFIG_ESP_TLS_INSECURE=y
		CONFIG_ESP_TLS_SKIP_SERVER_CERT_VERIFY=y

Conversation

tavdog commented Apr 8, 2025

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Changelog

Footnotes

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Summary of Findings

Merge Readiness

Uh oh!

gemini-code-assist Bot Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tavdog Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tavdog Apr 8, 2025 •

edited

Loading