proxmox test #715

cdalar · 2025-10-07T15:58:15Z

No description provided.

Copilot

Pull Request Overview

This PR adds Proxmox Virtual Environment support to onctl, enabling users to manage VMs on Proxmox infrastructure using the same interface as other cloud providers.

Implements complete Proxmox provider with VM lifecycle management (create, list, destroy, SSH)
Adds configuration template and comprehensive documentation for Proxmox setup
Integrates Proxmox as a supported cloud provider in the main application

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
internal/providerpxmx/common.go	Proxmox client initialization with API authentication
internal/cloud/proxmox.go	Main provider implementation with CloudProviderInterface methods
internal/files/init/proxmox.yaml	Default configuration template for Proxmox VMs
docs/PROXMOX.md	User documentation with setup instructions and examples
cmd/root.go	Integration of Proxmox provider into main application
PROXMOX_IMPLEMENTATION.md	Technical implementation summary and documentation

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-07T15:59:20Z

internal/providerpxmx/common.go

+	tlsConfig := &tls.Config{
+		InsecureSkipVerify: true, // For self-signed certificates
+	}


The TLS configuration unconditionally disables certificate verification. Consider making this configurable via an environment variable like PROXMOX_INSECURE_TLS to allow secure connections in production environments.

Copilot · 2025-10-07T15:59:20Z

internal/cloud/proxmox.go

+		return "", err
+	}
+
+	SSHKeyMD5 := fmt.Sprintf("%x", md5.Sum(publicKey))


MD5 is cryptographically insecure. Consider using SHA-256 for key hashing instead of MD5.

Copilot · 2025-10-07T15:59:20Z

internal/cloud/proxmox.go

+	// Wait for VM to start
+	time.Sleep(5 * time.Second)


Hard-coded sleep duration should be configurable or use proper polling with timeout. Consider implementing a wait mechanism that checks VM status periodically with a maximum timeout.

Suggested change

// Wait for VM to start

time.Sleep(5 * time.Second)

// Wait for VM to start by polling status with timeout

const maxWait = 60 * time.Second

const pollInterval = 2 * time.Second

startTime := time.Now()

for {

status, err := p.Client.GetVmState(ctx, newVmRef)

if err != nil {

return Vm{}, fmt.Errorf("failed to get VM state: %v", err)

}

if status.Status == "running" {

break

}

if time.Since(startTime) > maxWait {

return Vm{}, fmt.Errorf("timeout waiting for VM to start")

}

time.Sleep(pollInterval)

}

Copilot · 2025-10-07T15:59:21Z

internal/cloud/proxmox.go

+	}
+
+	// Wait for VM to stop
+	time.Sleep(3 * time.Second)


Another hard-coded sleep duration. Consider using a configurable timeout or polling mechanism to check if the VM has actually stopped before proceeding with deletion.

internal/cloud/proxmox.go

+		return fmt.Errorf("invalid VM ID: %v", err)
+	}
+
+	vmRef := pxapi.NewVmRef(pxapi.GuestID(vmID))


To fix this issue, ensure that server.ID is a valid non-negative integer within the bounds of uint32 before converting it. Specifically, after parsing the string to an integer, you should explicitly check that the vmID value is between 0 and math.MaxUint32 (inclusive). If the bounds check fails, return an appropriate error instead of performing the conversion. This change should be implemented just after parsing the ID and before passing it to pxapi.GuestID. You will need to import the math package to access math.MaxUint32 if it is not already imported in the snippet.

internal/providerpxmx/common.go

+	}
+
+	tlsConfig := &tls.Config{
+		InsecureSkipVerify: true, // For self-signed certificates


The correct way to fix this issue is to securely verify server certificates rather than disabling verification. If self-signed certificates are used, the recommended approach is to add the CA certificate to the trusted pool, so InsecureSkipVerify can remain false. You can do this with x509.NewCertPool and loading the CA from a PEM file or environment variable, then assign this pool to the RootCAs of the tls.Config.
To fix, change line 22 to avoid setting InsecureSkipVerify: true. Instead, set InsecureSkipVerify: false (or omit it for the default). In addition, offer to load CA certificates from a PEM file path given by an environment variable like PROXMOX_CA_CERT, if available. This can be done inside GetClient().
Add imports for crypto/x509 and io/ioutil if not already present, and update the TLS config to use a custom Root CA pool if a PEM CA file is configured.

The fix involves:

Changing InsecureSkipVerify to false.

(Optionally and preferably) Adding logic: If an environment variable (e.g., PROXMOX_CA_CERT) is set, read the CA cert(s) from that file, parse into a cert pool, and assign to tls.Config.RootCAs.

Add error handling for loading and parsing CA certs.

Add necessary imports at the top: crypto/x509 and io/ioutil.

This preserves secure TLS validation while still allowing self-signed certificates, when they are properly trusted.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

@codex fix this CI failure
@codex address that feedback

chatgpt-codex-connector · 2025-10-07T16:01:59Z

internal/cloud/proxmox.go

+	// Clone the VM
+	log.Println("[DEBUG] Cloning template", template, "to new VM", server.Name)
+	_, err = p.Client.CloneQemuVm(ctx, sourceVmr, cloneParams)
+	if err != nil {
+		return Vm{}, fmt.Errorf("failed to clone VM: %v", err)
+	}
+


Wait for Proxmox clone task before configuring VM

After invoking CloneQemuVm, the code immediately calls SetVmConfig and StartVm on the new VM with no wait. Proxmox cloning is asynchronous and keeps the target VM locked until the task finishes, so these subsequent calls typically fail with errors like “VM is locked” or “VM ID does not exist”. This causes most onctl create calls to error out even though the clone eventually completes. The task returned by CloneQemuVm should be waited on (WaitForCompletion/polling) before configuring or starting the VM.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2025-10-07T16:01:59Z

internal/providerpxmx/common.go

+	ctx := context.Background()
+	err = client.Login(ctx, tokenID, secret, "")
+	if err != nil {


Using password login for API token credentials blocks client initialization

GetClient reads PROXMOX_TOKEN_ID and PROXMOX_SECRET (documented as token-based auth) and then calls client.Login(ctx, tokenID, secret, ""). The Proxmox API expects username/password on Login; tokens must instead be set with SetAPIToken or similar and do not use the login endpoint. As written, authentication will fail with 401 when a token is supplied, so the provider exits before any command runs. Switch to the token-based authentication method or accept username/password explicitly.

Useful? React with 👍 / 👎.

socket-security · 2025-10-07T18:11:57Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	golang/github.com/Telmate/proxmox-api-go@v0.0.0-20251006192222-4174b8ef68b4

View full report

Copilot

Pull Request Overview

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

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-07T19:44:28Z

internal/providerpxmx/common.go

+	tlsConfig := &tls.Config{
+		InsecureSkipVerify: true, // For self-signed certificates


Skipping TLS verification poses a security risk by allowing man-in-the-middle attacks. Consider making this configurable via environment variable or adding a warning message about the security implications.

Suggested change

tlsConfig := &tls.Config{

InsecureSkipVerify: true, // For self-signed certificates

insecureSkipVerify := false

if os.Getenv("PROXMOX_INSECURE_SKIP_VERIFY") == "true" {

insecureSkipVerify = true

log.Println("WARNING: Skipping TLS verification. This poses a security risk and allows man-in-the-middle attacks.")

}

tlsConfig := &tls.Config{

InsecureSkipVerify: insecureSkipVerify,

Copilot · 2025-10-07T19:44:28Z

internal/cloud/proxmox.go

+	}
+
+	// Wait for VM to start
+	time.Sleep(5 * time.Second)


Hard-coded sleep duration makes the code less maintainable. Consider using a configurable timeout or implementing a proper wait mechanism that polls VM status until it's ready.

Copilot · 2025-10-07T19:44:28Z

internal/cloud/proxmox.go

+		return "", err
+	}
+
+	SSHKeyMD5 := fmt.Sprintf("%x", md5.Sum(publicKey))


MD5 is cryptographically broken and should not be used for security purposes. Consider using SHA-256 or another secure hash function for key fingerprinting.

codecov · 2025-10-12T16:16:33Z

Codecov Report

❌ Patch coverage is 0% with 266 lines in your changes missing coverage. Please review.
✅ Project coverage is 7.66%. Comparing base (1ae42b6) to head (295170b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/cloud/proxmox.go	0.00%	242 Missing ⚠️
internal/providerpxmx/common.go	0.00%	20 Missing ⚠️
cmd/root.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##            main    #715      +/-   ##
========================================
- Coverage   8.33%   7.66%   -0.68%     
========================================
  Files         31      33       +2     
  Lines       3036    3302     +266     
========================================
  Hits         253     253              
- Misses      2762    3028     +266     
  Partials      21      21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

proxmox test

1c99c9a

cdalar requested a review from Copilot October 7, 2025 15:58

Copilot AI reviewed Oct 7, 2025

View reviewed changes

github-advanced-security bot found potential problems Oct 7, 2025

View reviewed changes

chatgpt-codex-connector bot reviewed Oct 7, 2025

View reviewed changes

package fix

b82e29d

fix setAPI

24a5157

cdalar requested a review from Copilot October 7, 2025 19:43

Copilot AI reviewed Oct 7, 2025

View reviewed changes

cdalar added 2 commits October 8, 2025 09:53

fix tests

add12d3

chore

295170b

@@ -9,6 +9,7 @@
             	"os"
             	"strconv"
             	"time"
+            	"math"
             	pxapi "github.com/Telmate/proxmox-api-go/proxmox"
             	"github.com/cdalar/onctl/internal/tools"
@@ -143,8 +144,11 @@
             	if err != nil {
             		return fmt.Errorf("invalid VM ID: %v", err)
             	}
+            	if vmID < 0 || vmID > int(math.MaxUint32) {
+            		return fmt.Errorf("VM ID out of uint32 bounds: %d", vmID)
+            	}
-            	vmRef := pxapi.NewVmRef(pxapi.GuestID(vmID))
+            	vmRef := pxapi.NewVmRef(pxapi.GuestID(uint32(vmID)))
             	vmRef.SetNode(viper.GetString("proxmox.node"))
             	ctx := context.Background()

@@ -2,6 +2,8 @@
             import (
             	"crypto/tls"
+            	"crypto/x509"
+            	"io/ioutil"
             	"log"
             	"os"
@@ -18,8 +20,24 @@
             		os.Exit(1)
             	}
+            	// Load CA certificate if PROXMOX_CA_CERT is set, to allow verification for self-signed certs.
+            	var rootCAs *x509.CertPool
+            	caCertPath := os.Getenv("PROXMOX_CA_CERT")
+            	if caCertPath != "" {
+            		caCert, err := ioutil.ReadFile(caCertPath)
+            		if err != nil {
+            			log.Fatalf("Failed to read CA certificate from %s: %v", caCertPath, err)
+            		}
+            		rootCAs = x509.NewCertPool()
+            		if !rootCAs.AppendCertsFromPEM(caCert) {
+            			log.Fatalf("Failed to parse CA certificate from %s", caCertPath)
+            		}
+            	}
             	tlsConfig := &tls.Config{
-            		InsecureSkipVerify: true, // For self-signed certificates
+            		// Do not skip verification!
+            		InsecureSkipVerify: false,
+            		RootCAs:            rootCAs,
             	}
             	client, err := pxapi.NewClient(apiURL, nil, "", tlsConfig, "", 300)

-	// Wait for VM to start
-	time.Sleep(5 * time.Second)
+	// Wait for VM to start by polling status with timeout
+	const maxWait = 60 * time.Second
+	const pollInterval = 2 * time.Second
+	startTime := time.Now()
+	for {
+		status, err := p.Client.GetVmState(ctx, newVmRef)
+		if err != nil {
+			return Vm{}, fmt.Errorf("failed to get VM state: %v", err)
+		}
+		if status.Status == "running" {
+			break
+		}
+		if time.Since(startTime) > maxWait {
+			return Vm{}, fmt.Errorf("timeout waiting for VM to start")
+		}
+		time.Sleep(pollInterval)
+	}

		tlsConfig := &tls.Config{
		InsecureSkipVerify: true, // For self-signed certificates

-	tlsConfig := &tls.Config{
-		InsecureSkipVerify: true, // For self-signed certificates
+	insecureSkipVerify := false
+	if os.Getenv("PROXMOX_INSECURE_SKIP_VERIFY") == "true" {
+		insecureSkipVerify = true
+		log.Println("WARNING: Skipping TLS verification. This poses a security risk and allows man-in-the-middle attacks.")
+	}
+	tlsConfig := &tls.Config{
+		InsecureSkipVerify: insecureSkipVerify,

proxmox test #715

Are you sure you want to change the base?

proxmox test #715

Uh oh!

Conversation

cdalar commented Oct 7, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Check failure

Uh oh!

Copilot Autofix

Check failure

Copilot Autofix

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

socket-security bot commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 12, 2025

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

socket-security bot commented Oct 7, 2025 •

edited

Loading