Skip to content

Gangams/harden geneva configmap - #1771

Open
Ganga Mahesh Siddem (ganga1980) wants to merge 6 commits into
ci_prodfrom
gangams/harden-geneva-configmap
Open

Gangams/harden geneva configmap#1771
Ganga Mahesh Siddem (ganga1980) wants to merge 6 commits into
ci_prodfrom
gangams/harden-geneva-configmap

Conversation

@ganga1980

@ganga1980 Ganga Mahesh Siddem (ganga1980) commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

This pull request introduces significant improvements to the Geneva config parsing script, focusing on robust validation and secure handling of untrusted config map input. The changes ensure that all values derived from tenant-writable config maps are strictly validated and safely quoted before being used in shell environments, mitigating the risk of command injection. Additionally, the code is refactored for clarity and maintainability, with reusable validation and formatting functions.

Security and Validation Enhancements:

  • Added strict regex-based validation functions for all Geneva config values (environment, region, account, namespace, etc.), ensuring only safe, expected formats are accepted from untrusted config map input.
  • Updated the main config validation logic in isValidGenevaConfig to use the new validation functions, covering all required and optional fields, and preventing unsafe values from being accepted.

Safe Shell Quoting:

  • Introduced the toShellSingleQuoted function to safely single-quote all config map-derived values before writing them to environment files, ensuring that shell commands cannot be injected via config values. Updated all relevant export statements to use this quoting. [1] [2]

Namespace Handling Improvements:

  • Refactored the logic for collecting and joining infra and tenant namespaces into a reusable joinValidNamespaces function, which validates each namespace and constructs a safe, comma-separated list. This replaces previous manual string concatenation. [1] [2] [3]

Config Version Handling:

  • Added the resolveConfigVersion function to validate and default the Geneva config version, ensuring typos or invalid values do not break the integration. Updated assignment of config versions to use this function. [1] [2]

These changes greatly improve the security, reliability, and maintainability of the Geneva config parser by systematically validating and safely handling all untrusted input.harden geneva configmap values.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown

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 hardens how Geneva Logs integration settings are read from the tenant-writable container-azm-ms-agentconfig configmap and turned into a shell-sourced environment file, reducing the risk of shell injection when the agent runs as root.

Changes:

  • Update kubernetes/linux/main.sh to append geneva_config_env_var to ~/.bashrc without word-splitting/glob expansion.
  • Add allowlist validation + robust single-quoting when generating geneva_config_env_var in tomlparser-geneva-config.rb, including safer namespace list handling and config-version fallback logic.
  • Add Minitest regression coverage that attempts multiple injection payloads and verifies they cannot execute when the generated file is sourced.

Reviewed changes

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

File Description
kubernetes/linux/main.sh Switches to a safe line-preserving read/append loop for geneva_config_env_var before sourcing it.
build/common/installer/scripts/tomlparser-geneva-config.rb Validates configmap-derived values with anchored allowlists and emits single-quoted shell assignments; tightens namespace list handling and config version fallback.
build/common/installer/scripts/tomlparser-geneva-config_test.rb Adds regression tests to ensure configmap payloads cannot trigger command execution when the env file is sourced.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread build/common/installer/scripts/tomlparser-geneva-config_test.rb Outdated
Comment thread kubernetes/linux/main.sh Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

build/common/installer/scripts/tomlparser-geneva-config_test.rb:108

  • source_env_file ignores the exit status from bash -c, so if sourcing the generated env file fails (syntax error, set -e abort, etc.) the security tests can still pass because they only check for the sentinel file. Capturing and asserting a successful status makes these regression tests reliable and ensures the generated env file remains sourceable.
  def source_env_file(result)
    script = "set -e\n. \"#{result[:env_file_path]}\"\n" +
             ["MONITORING_GCS_ENVIRONMENT", "MONITORING_GCS_NAMESPACE", "MONITORING_GCS_ACCOUNT",
              "MONITORING_GCS_REGION", "MONITORING_CONFIG_VERSION", "MONITORING_GCS_AUTH_ID",
              "MONITORING_GCS_AUTH_ID_TYPE", "GENEVA_LOGS_INFRA_NAMESPACES",
              "GENEVA_LOGS_TENANT_NAMESPACES"].map { |name| "printf '%s=%s\\n' #{name} \"$#{name}\"" }.join("\n")
    stdout, = Open3.capture3({}, "bash", "-c", script, chdir: result[:workdir])
    stdout.lines.map { |line| line.chomp.split("=", 2) }.to_h
  end

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