Skip to content

Add explicit SSH public key configuration support#348

Closed
sayalibhavsar wants to merge 1 commit intomainfrom
gh-323
Closed

Add explicit SSH public key configuration support#348
sayalibhavsar wants to merge 1 commit intomainfrom
gh-323

Conversation

@sayalibhavsar
Copy link
Contributor

@sayalibhavsar sayalibhavsar commented Feb 3, 2026

Description

Add --ssh_public_key_file CLI option to bin/burden and a ssh_public_key_path Terraform variable across AWS, Azure, and GCP so users can specify public and private SSH keys independently
instead of relying on hardcoded or .pub-derived paths.

Before/After Comparison

Before: Public key hardcoded to ~/.ssh/id_rsa.pub (Azure) or derived by appending .pub to the private key path (GCP); no way to specify a separate public key.

After: New --ssh_public_key_file option and ssh_public_key_path variable let users set the public key path explicitly; defaults to ~/.ssh/id_rsa.pub for backward compatibility.

Documentation Check

No updates needed — optional flag with a backward-compatible default.

Clerical Stuff

This closes #227
Relates to JIRA: RPOPC-492

@qodo-code-review
Copy link

qodo-code-review bot commented Feb 3, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #323
🔴 During installation, check whether `/home/uploads` exists.
If /home/uploads is missing, print a non-blocking warning: Warning: /home/uploads
directory not found. Some wrappers may fail until it is created.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Silent yq failures: The new kit-directory validation swallows yq parsing/command errors (redirecting stderr
and defaulting to "none"), which can silently skip validation and provides no
actionable error context when yq is missing or templates are invalid.

Referred Code
# Extract upload_extra value using yq
upload_extra=$(yq -r '.upload_extra // "none"' "$template_file" 2>/dev/null || echo "none")

# Skip if upload_extra is 'none', empty, or null
if [[ "$upload_extra" == "none" ]] || [[ -z "$upload_extra" ]] || [[ "$upload_extra" == "null" ]]; then
    continue
fi

# Check if upload_extra is an array or a string
is_array=$(yq -r '.upload_extra | type' "$template_file" 2>/dev/null || echo "null")

if [[ "$is_array" == "array" ]]; then
    # It's an array, process each element
    while IFS= read -r path; do
        if [[ "$path" != "none" ]] && [[ -n "$path" ]]; then
            if [[ ! -e "$path" ]]; then
                missing_kits+=("$template_name:$path")
            fi
        fi
    done < <(yq -r '.upload_extra[]' "$template_file" 2>/dev/null)
else

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Missing default propagation: When --ssh_key_file is provided but --ssh_public_key_file is not, the PR omits writing
ssh_public_key into ansible_vars_main.yml, which may cause downstream Terraform templating
(config_info.ssh_public_key) to be unset and fail depending on unseen defaults.

Referred Code
if [[ $gl_ssh_key_file == "" ]]; then
	echo "  ssh_key: $HOME/.ssh/id_rsa" >> ansible_vars_main.yml
	echo "  ssh_public_key: $HOME/.ssh/id_rsa.pub" >> ansible_vars_main.yml
else
	echo "  ssh_key: ${gl_ssh_key_file}" >> ansible_vars_main.yml
	if [[ $gl_ssh_public_key_file != "" ]]; then
		echo "  ssh_public_key: ${gl_ssh_public_key_file}" >> ansible_vars_main.yml
	fi
fi

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Feb 3, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix missing public key path

In bin/burden, if a user provides a private key but not a public key,
automatically derive the public key path by appending .pub to the private key
path to prevent Terraform failures.

bin/burden [1911-1919]

 		if [[ $gl_ssh_key_file == "" ]]; then
 			echo "  ssh_key: $HOME/.ssh/id_rsa" >> ansible_vars_main.yml
 			echo "  ssh_public_key: $HOME/.ssh/id_rsa.pub" >> ansible_vars_main.yml
 		else
 			echo "  ssh_key: ${gl_ssh_key_file}" >> ansible_vars_main.yml
 			if [[ $gl_ssh_public_key_file != "" ]]; then
 				echo "  ssh_public_key: ${gl_ssh_public_key_file}" >> ansible_vars_main.yml
+			else
+				echo "  ssh_public_key: ${gl_ssh_key_file}.pub" >> ansible_vars_main.yml
 			fi
 		fi
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug where the public key path is not set if only a private key is provided, which would cause Terraform to fail.

High
General
Consolidate duplicate file verification functions

In bin/burden, consolidate the duplicate verify_ssh_key_file and
verify_ssh_public_key_file functions into a single, generic function that
accepts a file path and description.

bin/burden [2351-2363]

-verify_ssh_key_file()
+verify_file_exists()
 {
-	if [[ ! -f $1 ]]; then
-		cleanup_and_exit "Error: ssh key file $1 does not exist." 1
+	local file_path="$1"
+	local file_description="$2"
+	if [[ ! -f "$file_path" ]]; then
+		cleanup_and_exit "Error: $file_description file '$file_path' does not exist." 1
 	fi
 }
 
-verify_ssh_public_key_file()
-{
-	if [[ ! -f $1 ]]; then
-		cleanup_and_exit "Error: ssh public key file $1 does not exist." 1
-	fi
-}
-

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies code duplication and proposes a good refactoring to improve maintainability, which is a valid but moderate-impact improvement.

Low
  • Update

@sayalibhavsar sayalibhavsar force-pushed the gh-323 branch 2 times, most recently from 80b034f to fb414d6 Compare February 3, 2026 18:38
@sayalibhavsar
Copy link
Contributor Author

sayalibhavsar commented Feb 3, 2026

The removal of the auto-derivation (${gl_ssh_key_file}.pub) is intentional, not a flaw. Users should explicitly specify the public key path when needed for provisioning scenarios, rather than having it automatically derived.

Test Results: https://gist.github.com/sayalibhavsar/236e35ee79ec92b36ef02f71cc497a05

@sayalibhavsar sayalibhavsar self-assigned this Feb 17, 2026
@github-actions
Copy link

PullHero Review

Provider: generativelanguage.googleapis.com Model: gemini-2.0-flash

Okay, I've reviewed the changes. Here's my assessment:

Overview

This PR introduces the ability to specify a separate SSH public key file. Currently, the code assumes the public key is located at ~/.ssh/id_rsa.pub based on the private key specified, but this change allows for greater flexibility in key management by explicitly defining the public key path.

Analysis

Code Quality and Maintainability:

  • The code is generally well-structured and easy to follow.
  • The addition of a new global variable (gl_ssh_public_key_file) and corresponding functions (verify_ssh_public_key_file, set_general_value) is a clean and logical extension of the existing framework.
  • The modifications to the Terraform template files (tfvars.j2) are straightforward and maintain consistency.

Potential Bugs or Security Issues:

  • It is important to consider if the tool should verify that the public key actually corresponds to the private key. While this adds complexity, it could prevent misconfiguration where the wrong public key is used. The current code trusts that the user provides the correct key pair.
  • The code adds a new parameter, but doesn't enforce its use, meaning the old behavior persists. This may or may not be desired, but should be explicitly documented.

Adherence to Project Conventions:

  • The changes seem to follow the established coding style and conventions of the project.
  • The introduction of the new option (--ssh_public_key_file) is consistent with the existing CLI option structure.

Documentation Completeness:

  • The bin/burden usage output has been updated to include the new option, which is good.
  • The docs/command_line_reference.md file needs to be updated to reflect the new command line option and explain its usage.
  • The documentation should clearly state that if --ssh_public_key_file is not specified, the tool defaults to assuming the public key is located at ~/.ssh/id_rsa.pub (based on ssh_key_path).
  • The documentation should discuss the security implications of using separate public and private key files.

Specific Feedback:

  • Recommendation: Add a check to verify_ssh_public_key_file to ensure the file is readable and contains a valid public key format.
  • Suggestion: Consider adding a warning message if only --ssh_key_file is provided, reminding the user of the default public key path assumption.
  • Suggestion: Add a check in create_ansible_options() to verify if gl_ssh_public_key_file is set, if it's not, then fall back to the default assumption ssh_public_key: $HOME/.ssh/id_rsa.pub so that the ansible code can use it.
  • Improvement: Update docs/command_line_reference.md to fully document the new option with examples.

Vote: -1

The code itself is good, however, documentation needs to be updated to reflect the changes. Additionally, it would be better to have a check to make sure the user is providing a valid public key file.

Vote: -1

PullHero

@github-actions
Copy link

This relates to RPOPC-492

@qodo-code-review
Copy link

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: update_parent_issue

Failed stage: Set parent issues state [❌]

Failed test name: ""

Failure summary:

The action failed during a step that updates GitHub issue labels using the GitHub CLI (gh issue
edit).
- When attempting to remove labels from the parent issue 323 (see REMOVE_ISSUES at log line
205), GitHub returned: GraphQL: Resource not accessible by integration (removeLabelsFromLabelable)
(log line 212).
- This indicates the token used (GH_TOKEN) does not have sufficient permissions (or
the workflow is running in a restricted context, e.g., from a fork) to perform the GraphQL mutation
that removes labels.
- Because gh reported failed to update 1 issue (log line 213), the script
exited non-zero and the job failed with exit code 1 (log line 214).

Relevant error logs:
1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

197:  �[36;1mfor issue in $PARENT_ISSUES; do�[0m
198:  �[36;1m  echo "Updating $issue to $PR_STATUS" &&�[0m
199:  �[36;1m  gh -R redhat-performance/zathras issue edit $issue $REMOVE_ISSUES && �[0m
200:  �[36;1m  gh -R redhat-performance/zathras issue edit $issue --add-label=pr_$PR_STATUS�[0m
201:  �[36;1mdone�[0m
202:  shell: sh -e {0}
203:  env:
204:  ISSUE_STATE: pr_inprogress
205:  REMOVE_ISSUES: --remove-label pr_approved --remove-label pr_review --remove-label pr_inprogress
206:  GH_TOKEN: ***
207:  PR_NUMBER: 345
208:  PARENT_ISSUES: 323
209:  PR_STATUS: review
210:  ##[endgroup]
211:  Updating 323 to review
212:  failed to update https://github.com/redhat-performance/zathras/issues/323: GraphQL: Resource not accessible by integration (removeLabelsFromLabelable)
213:  failed to update 1 issue
214:  ##[error]Process completed with exit code 1.
215:  Post job cleanup.

@kdvalin
Copy link
Member

kdvalin commented Feb 17, 2026

Want output from a uperf run; verify this does not break old scenario files, provide output

@sayalibhavsar sayalibhavsar deleted the gh-323 branch February 24, 2026 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow user to supply both public and private SSH key to Zathras

2 participants