Skip to content

[ROSAENG-61275 ] fix :remove SSM credential logging at Info level - #981

Merged
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
samanthajayasinghe:agent/8-remove-ssm-token-logging
Aug 11, 2026
Merged

[ROSAENG-61275 ] fix :remove SSM credential logging at Info level#981
openshift-merge-bot[bot] merged 1 commit into
openshift:mainfrom
samanthajayasinghe:agent/8-remove-ssm-token-logging

Conversation

@samanthajayasinghe

@samanthajayasinghe samanthajayasinghe commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Remove logger.Infof calls for TokenValue and StreamUrl in runSSMsession (cmd/ocm-backplane/cloud/ssm.go). These sensitive AWS SSM session credentials were logged at Info level on every invocation of "ocm-backplane cloud ssm-session", exposing the websocket auth token to terminal scrollback and log collectors. The token alone is sufficient to hijack an active SSM session to a customer EC2 instance (CWE-532, CWE-312).

SessionId is retained at Debug level since it is not a secret but useful for troubleshooting. The session JSON is already passed privately to session-manager-plugin via argv, so these log lines served no functional purpose.

Added three tests verifying that TokenValue, StreamUrl, and SessionId do not appear in log output at Info level.

Note: golangci-lint could not run in the sandbox (network restriction on install). go vet passed. One pre-existing test failure in common_test.go (network-dependent test blocked by sandbox) is unrelated to this change.

What type of PR is this?

  • fix (Bug Fix)
  • feat (New Feature)
  • docs (Documentation)
  • test (Test Coverage)
  • chore (Clean Up / Maintenance Tasks)
  • other (Anything that doesn't fit the above)

What this PR does / Why we need it?

Which Jira/Github issue(s) does this PR fix?

Special notes for your reviewer

Unit Test Coverage

Guidelines

  • If it's a new sub-command or new function to an existing sub-command, please cover at least 50% of the code
  • If it's a bug fix for an existing sub-command, please cover 70% of the code

Test coverage checks

  • Added unit tests
  • Created jira card to add unit test
  • This PR may not need unit tests

Pre-checks (if applicable)

  • Ran unit tests locally
  • Validated the changes in a cluster
  • Included documentation changes with PR
  • Backward compatible

/label tide/merge-method-squash

Summary by CodeRabbit

  • Bug Fixes
    • Improved security by preventing sensitive SSM session credentials and connection details from appearing in application logs.
    • Retained only the non-sensitive session identifier at debug level for troubleshooting.

@openshift-ci openshift-ci Bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 11, 2026
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f499ccfd-e4c9-43e9-a886-7387976b0d50

📥 Commits

Reviewing files that changed from the base of the PR and between 595c7d8 and a663bda.

📒 Files selected for processing (1)
  • cmd/ocm-backplane/cloud/ssm_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • cmd/ocm-backplane/cloud/ssm_test.go

Walkthrough

SSM session logging now excludes StreamUrl and TokenValue. SessionId is logged only at debug level. Tests capture Logrus output and verify that session values are absent at Info level.

Changes

SSM logging security

Layer / File(s) Summary
Redact and verify SSM session logs
cmd/ocm-backplane/cloud/ssm.go, cmd/ocm-backplane/cloud/ssm_test.go
The SSM session path logs only SessionId at debug level. Tests mock session execution and verify that TokenValue, StreamUrl, and SessionId are absent from Info-level logs.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: wanghaoran1988, xiaoyu74

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the removal of SSM credential logging at Info level, which is the main change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2026

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/ocm-backplane/cloud/ssm_test.go`:
- Around line 148-174: Update the BeforeEach/AfterEach setup to capture the
original Logrus output writer from log.StandardLogger().Out before calling
log.SetOutput(&logBuf), then restore that captured writer in AfterEach instead
of unconditionally setting os.Stderr; keep the existing log level restoration
unchanged.
🪄 Autofix

❌ Autofix failed (check again to retry)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: cb5e44ea-f8a1-4659-8243-cf7df502033c

📥 Commits

Reviewing files that changed from the base of the PR and between 8d18130 and 595c7d8.

📒 Files selected for processing (2)
  • cmd/ocm-backplane/cloud/ssm.go
  • cmd/ocm-backplane/cloud/ssm_test.go

Comment thread cmd/ocm-backplane/cloud/ssm_test.go
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown

An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference

@codecov-commenter

codecov-commenter commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.53%. Comparing base (8d18130) to head (a663bda).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #981      +/-   ##
==========================================
- Coverage   54.54%   54.53%   -0.02%     
==========================================
  Files          82       82              
  Lines        6308     6306       -2     
==========================================
- Hits         3441     3439       -2     
  Misses       2417     2417              
  Partials      450      450              
Files with missing lines Coverage Δ
cmd/ocm-backplane/cloud/ssm.go 55.47% <100.00%> (-0.61%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Remove logger.Infof calls for TokenValue and StreamUrl in
runSSMsession (cmd/ocm-backplane/cloud/ssm.go). These sensitive
AWS SSM session credentials were logged at Info level on every
invocation of "ocm-backplane cloud ssm-session", exposing the
websocket auth token to terminal scrollback and log collectors.
The token alone is sufficient to hijack an active SSM session
to a customer EC2 instance (CWE-532, CWE-312).

SessionId is retained at Debug level since it is not a secret
but useful for troubleshooting. The session JSON is already
passed privately to session-manager-plugin via argv, so these
log lines served no functional purpose.

Added three tests verifying that TokenValue, StreamUrl, and
SessionId do not appear in log output at Info level.

Note: golangci-lint could not run in the sandbox (network
restriction on install). go vet passed. One pre-existing
test failure in common_test.go (network-dependent test
blocked by sandbox) is unrelated to this change.

Closes #8
@samanthajayasinghe
samanthajayasinghe force-pushed the agent/8-remove-ssm-token-logging branch from 595c7d8 to a663bda Compare August 11, 2026 22:46
@openshift-ci

openshift-ci Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@samanthajayasinghe: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@geowa4 geowa4 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.

/lgtm
/approve

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2026
@openshift-ci

openshift-ci Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geowa4, samanthajayasinghe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot
openshift-merge-bot Bot merged commit a65b76e into openshift:main Aug 11, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants