Skip to content

Conversation

leefine02
Copy link
Contributor

No description provided.

Keyfactor and others added 30 commits August 6, 2025 13:43
@spbsoluble spbsoluble requested a review from Copilot September 25, 2025 17:01
Copy link

@Copilot 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 pull request introduces a new "Use Shell Commands" feature to the Remote File Orchestrator Extension that allows users to disable Linux shell commands when managing certificate stores on Linux servers. This is useful for environments where shell access is restricted and SFTP commands can be substituted for certain shell operations.

  • Adds a new UseShellCommands configuration option with default value "Y" that can be overridden per certificate store
  • Modifies the SSHHandler to conditionally use SFTP operations instead of shell commands when UseShellCommands is set to "N"
  • Refactors the ApplicationSettings initialization from instance-based to static constructor pattern

Reviewed Changes

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

Show a summary per file
File Description
integration-manifest.json Adds UseShellCommands custom field definitions for all 6 certificate store types
docsource/content.md Adds comprehensive documentation for the new Use Shell Commands setting and updates command usage table
RemoteFile/RemoteHandlers/SSHHandler.cs Implements conditional logic to use SFTP instead of shell commands based on UseShellCommands setting
RemoteFile/RemoteFileJobTypeBase.cs Adds UseShellCommands property handling and fixes variable naming
RemoteFile/RemoteCertificateStore.cs Updates Initialize method to accept useShellCommands parameter
RemoteFile/ReenrollmentBase.cs Removes redundant ApplicationSettings.Initialize call and passes UseShellCommands to Initialize
RemoteFile/ManagementBase.cs Removes redundant ApplicationSettings.Initialize call and passes UseShellCommands to Initialize
RemoteFile/InventoryBase.cs Removes redundant ApplicationSettings.Initialize call and passes UseShellCommands to Initialize
RemoteFile/Discovery.cs Removes redundant ApplicationSettings.Initialize call and hardcodes UseShellCommands to true for discovery
RemoteFile/ApplicationSettings.cs Converts to static constructor pattern and adds UseShellCommands property
RemoteFile.UnitTests/ApplicationSettingsTests.cs Removes manual ApplicationSettings.Initialize calls from unit tests
README.md Updates documentation with new UseShellCommands field and improves command usage descriptions
CHANGELOG.md Documents the new feature and bug fix
.github/workflows/keyfactor-starter-workflow.yml Updates workflow to newer version with additional configuration options
.github/workflows/keyfactor-merge-store-types.yml Removes entire workflow file

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

when attempting an SFTP/SCP connection. A modification was added to RemoteFile to check for this condition. Running RemoteFile
with Use Shell Commands = N will cause this validation check to NOT occur.
5. Both RFORA and RFKDB use proprietary CLI commands in order to manage their respective certificate stores. These commands
will still be executed when Use Shell Commands is set to Y.
Copy link
Preview

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

The documentation states that CLI commands will still be executed when 'Use Shell Commands is set to Y', but this appears to be incorrect. These commands should still execute when UseShellCommands is 'N' since they are required for store management, regardless of the shell command setting.

Suggested change
will still be executed when Use Shell Commands is set to Y.
will be executed regardless of the Use Shell Commands setting, as they are required for store management.

Copilot uses AI. Check for mistakes.

}
catch (Exception ex)
{
_logger.LogError(RemoteFileException.FlattenExceptionMessages(ex, "Error checking existence of file {path} using SFTP"));
Copy link
Preview

Copilot AI Sep 25, 2025

Choose a reason for hiding this comment

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

The error message contains a placeholder '{path}' that will not be interpolated. It should be $"Error checking existence of file {path} using SFTP" to properly format the path variable into the error message.

Suggested change
_logger.LogError(RemoteFileException.FlattenExceptionMessages(ex, "Error checking existence of file {path} using SFTP"));
_logger.LogError(RemoteFileException.FlattenExceptionMessages(ex, $"Error checking existence of file {path} using SFTP"));

Copilot uses AI. Check for mistakes.

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