Skip to content

feat(pam): add E2E tests for SSH#145

Open
saifsmailbox98 wants to merge 11 commits intomainfrom
saif/pam-132-add-end-to-end-tests-for-ssh-pam
Open

feat(pam): add E2E tests for SSH#145
saifsmailbox98 wants to merge 11 commits intomainfrom
saif/pam-132-add-end-to-end-tests-for-ssh-pam

Conversation

@saifsmailbox98
Copy link
Contributor

Description 📣

Adds E2E tests for PAM SSH resource

Type ✨

  • Bug fix
  • New feature
  • Improvement
  • Breaking change
  • Documentation

Tests 🛠️

# Here's some code block to paste some code snippets

…to-end-tests-for-ssh-pam

# Conflicts:
#	.github/workflows/run-cli-e2e-tests.yml
#	e2e/README.md
#	e2e/openapi-cfg.yaml
#	e2e/packages/client/client.gen.go
#	e2e/pam/postgres_test.go
…stom-built Dockerfile; add support for multiple SSH auth methods (password, public-key, certificate) in PAM E2E tests.
…test logic by iterating directly over auth methods.
@linear
Copy link

linear bot commented Mar 9, 2026

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 9, 2026

Greptile Summary

This PR adds E2E tests for the PAM SSH resource, covering password, public-key, and certificate-based authentication methods. Each sub-test spins up a containerized Alpine SSH server via testcontainers, creates a PAM resource and account through the API, and verifies an interactive SSH session routed through the Infisical gateway. Shared test infrastructure (SetupPAMInfra, LoginUser, getOutboundIP) is extracted into a new pam_helpers.go file, and the Command helper gains DisableTempHomeDir and Stdin fields to support these tests. The implementation is well-structured and mirrors the real user flow (including the curl | bash certificate setup).

Key findings:

  • The provisioning bearer token is interpolated directly into a bash -c shell command string in configureCertAuth, exposing it in process argument lists and Docker exec logs — an environment variable should be used instead.
  • getOutboundIP returns the first non-loopback IPv4 address without preferring physical NICs over Docker bridge addresses; in some CI topologies this could return a Docker bridge IP (which still works in practice but is non-deterministic).
  • The new ### Running PAM tests README section is inserted in a position that makes the immediately following "Alternatively" sentence misleading — the two PAM sections should be consolidated or the new section renamed for clarity.

Confidence Score: 4/5

  • Safe to merge — all issues are style and robustness nits in test-only code with no production impact.
  • The PR adds well-structured E2E tests with no changes to production code. The identified concerns (token in command string, non-deterministic IP selection, documentation placement) are minor and limited to the test infrastructure.
  • e2e/pam/ssh_test.go (token in command string), e2e/pam/pam_helpers.go (IP selection ordering)

Important Files Changed

Filename Overview
e2e/pam/ssh_test.go New E2E test file covering three SSH auth methods (password, public-key, certificate). Logic is sound; the main concern is the provisioning bearer token being interpolated into a shell command string passed to bash -c, which exposes it in process arguments and Docker exec logs.
e2e/pam/pam_helpers.go New shared PAM test infrastructure helpers; SetupPAMInfra and LoginUser are well-structured. getOutboundIP returns the first non-loopback IPv4 without preference ordering, which could non-deterministically return a Docker bridge address in some CI environments.
e2e/pam/testdata/ssh-server/entrypoint.sh Entrypoint script uses set -e, correctly generates host keys, creates test user with hardcoded test credentials, conditionally installs authorized keys, and uses exec so sshd becomes PID 1 (required for the SIGHUP reload in configureCertAuth).
e2e/util/helpers.go Added DisableTempHomeDir flag and Stdin support to the Command struct, enabling the SSH test to share a single HOME directory across login and PAM commands and to pipe interactive input. Changes are clean and backward-compatible.
e2e/README.md New "Running PAM tests" section duplicates context already present in the surrounding PAM section; placement causes the existing "Alternatively" sentence to read as referring to the new section, creating mild confusion.

Comments Outside Diff (2)

  1. e2e/pam/pam_helpers.go, line 27-37 (link)

    Non-deterministic interface selection may return Docker bridge IP

    net.InterfaceAddrs() does not guarantee a stable ordering across environments. In hosts where the Docker bridge interface (docker0, 172.17.0.x) has a lower interface index than eth0, this function returns the Docker bridge address. That address IS reachable from containers via the bridge, so tests will not break in practice, but in certain exotic CI setups (e.g. rootless Docker, custom bridge networks) it may not be the intended host address.

    Consider excluding link-local and private Docker-range addresses, or sorting results to prefer the most specific non-Docker address:

    for _, addr := range addrs {
        ipNet, ok := addr.(*net.IPNet)
        if !ok || ipNet.IP.IsLoopback() || ipNet.IP.To4() == nil {
            continue
        }
        // Skip Docker default bridge range (172.17.0.0/16) to prefer physical NICs
        if ipNet.IP[0] == 172 && ipNet.IP[1] == 17 {
            continue
        }
        return ipNet.IP.String()
    }

    This is a minor robustness concern rather than a blocking bug.

  2. e2e/README.md, line 9-39 (link)

    Duplicated/confusing section heading in documentation

    The new ### Running PAM tests section is inserted directly after an existing PAM section that already explains how to run PAM tests (go test … /pam). This creates two adjacent sections covering the same topic, and the sentence immediately after the new block ("Alternatively, you can export environment variables manually:") was written to follow the earlier content — it now appears to be an alternative to the new section, which is misleading.

    Consider either:

    • Merging the new content into the existing PAM section, or
    • Renaming the new section to something more specific (e.g., ### Running SSH and Postgres PAM sub-tests) to clearly signal it expands on the base instructions.

    Rule Used: When naming sections in documentation, use clear a... (source)

    Learnt From
    Infisical/infisical#3652

Last reviewed commit: c20b0d7

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.

1 participant