Skip to content

feat(credentials): store CLI secrets in the OS keyring (AST-164107) - #1545

Open
cx-prathmesh-borle wants to merge 3 commits into
mainfrom
feature/AST-164107-go-keyring-credential-storage
Open

feat(credentials): store CLI secrets in the OS keyring (AST-164107)#1545
cx-prathmesh-borle wants to merge 3 commits into
mainfrom
feature/AST-164107-go-keyring-credential-storage

Conversation

@cx-prathmesh-borle

Copy link
Copy Markdown

Summary

Today the CLI writes the API key and the client secret as plain text in the config file. Any person or program that can read the file can read the secrets. This PR stores these two secrets in the operating system keyring instead. The keyring is the secure credential storage that is built into Windows, macOS, and Linux.

This PR also fixes a broken command, cx utils env. The old command did not show correct values. The new code fixes this.

Related issue: AST-164107.
Branch has one commit: 08c78ef7 feat(credentials): store CLI secrets in the OS keyring (AST-164107).
The commit changes 43 files. It adds about 2,372 lines and removes about 226 lines.

The problem

The CLI stores two secret values:

  • the API key (cx_apikey)
  • the client secret (cx_client_secret)

Before this PR, the CLI stored both values as plain text in the file ~/.checkmarx/checkmarxcli.yaml. Plain text means anyone who opens the file can read the secret. This is a security risk.

Before this PR, the command cx utils env also had a bug. The command read the environment variables with the wrong variable names. As a result, it always showed empty values, even when the values were set. The command looked broken to a user.

The goal

  1. Store the API key and the client secret in the OS keyring, not in the plain text file.
  2. Give the user a way to turn this off, for machines that have no keyring. For example, build servers.
  3. Move old secrets from the config file into the keyring automatically, the first time the user runs any command.
  4. Stop secrets from appearing in debug logs.
  5. Fix the cx utils env command.

High-level design

The CLI now has one component that controls all reads and writes of a secret. This component is the resolver (internal/credentialstore.Resolver). Every part of the CLI that needs a secret must ask the resolver. No part of the CLI reads or writes a secret directly.

The resolver can read a secret from four places. The resolver checks these four places in a fixed order:

  1. A command line flag, for example --apikey. This value exists only for the current command run.
  2. An environment variable, for example CX_APIKEY.
  3. The OS keyring.
  4. The config file, as plain text. This is the old storage method. The CLI keeps it for backward compatibility.

The resolver stops at the first place that has a value. This order does not change between commands.

flowchart LR
    subgraph WRITE["When the user sets or clears a secret"]
        W1["cx configure, cx auth login, or cx auth logout"] --> W2["The resolver saves or deletes the value"]
        W2 --> W3{"Is the keyring turned on?"}
        W3 -->|"Yes"| W4["Save in the OS keyring"]
        W3 -->|"No"| W5["Save in the config file"]
    end

    subgraph READ["When any command needs a secret"]
        R1["A command asks the resolver for a value"] --> R2{"Was a flag or environment variable given?"}
        R2 -->|"Yes"| R6["Use that value"]
        R2 -->|"No"| R3{"Is the keyring turned on?"}
        R3 -->|"Yes"| R4["Read the OS keyring"]
        R3 -->|"No, or value not found there"| R5["Read the config file"]
        R4 --> R6
        R5 --> R6
    end
Loading

The user controls the keyring with one setting: the environment variable CX_KEYRING_MODE. This setting has three modes.

Mode What the CLI does when it reads a secret What the CLI does when it writes a secret
auto (default) Reads the keyring first. If the value is not there, reads the config file. Writes to the keyring.
required Reads the keyring only. Does not read the config file. Writes to the keyring only.
disabled Reads the config file only. Never touches the keyring. Writes to the config file only.

If the user sets CX_KEYRING_MODE to an unknown value, the CLI does not stop. It prints a warning and uses auto mode instead.

How old secrets move to the keyring

The first time the user runs any command after this update, the CLI checks the config file for old secrets. If the CLI finds a secret there, and the keyring mode allows it, the CLI copies the secret into the keyring. After the CLI confirms the copy is correct, it removes the secret from the config file. If the copy step fails, the CLI keeps the secret in the config file and tries again on the next run. The secret is never lost.

flowchart TD
    A["User runs any CLI command"] --> B["The CLI checks the config file for old secrets"]
    B --> C{"Is the keyring turned off?"}
    C -->|"Yes"| D["Do nothing. Keep using the config file."]
    C -->|"No"| E{"Does the config file have a secret?"}
    E -->|"No"| F["Nothing to move"]
    E -->|"Yes"| G["Copy the secret into the keyring"]
    G --> H{"Did the CLI confirm the copy?"}
    H -->|"Yes"| I["Remove the secret from the config file"]
    H -->|"No"| J["Keep the secret in the config file. Try again next time."]
Loading

Where secrets are stored in the keyring

The CLI stores each secret under one keyring service name, checkmarx-ast-cli. Inside that service, the CLI creates one keyring entry for the API key and one entry for the client secret.

The entry name does not contain the config file path in plain text. Instead, the CLI hashes the file path, and uses part of the hash as the entry name. This has two benefits.

  1. Two different config files never share the same keyring entry.
  2. A person who looks at the list of keyring entries cannot see the file path.

Low-level design

This section lists the main code changes. Function names are in code font. The text next to each name explains what the function does, in plain language.

New folder: internal/configfile

This folder is new. It is the only code that reads and writes the plain text config file.

  • Load reads the file. If the file is missing, it returns an empty result, not an error.
  • Save writes the file.
  • SetKey writes one value into the file.
  • ScrubKey removes one value from the file.

All four functions lock the file before they change it. The lock stops two CLI commands from writing to the file at the same time.

New folder: internal/credentialstore

This folder is new. It holds the resolver, the keyring connection, and the migration code.

  • Resolver.Resolve reads a secret. It follows the four-step order described above in "High-level design".
  • Resolver.Store writes a secret. It follows the keyring mode setting.
  • Resolver.Clear deletes a secret. It also follows the keyring mode setting.
  • Migrate runs the one-time copy of old secrets, described above.
  • PolicyFromEnv reads the CX_KEYRING_MODE environment variable and turns it into one of the three modes.

Changed files that now use the resolver

These files used to read the secret value directly from the configuration library, Viper. Now they call Resolver.Resolve instead.

  • internal/wrappers/client.go — sends the API key and the client secret to the Checkmarx server.
  • internal/commands/auth.go — checks if the user is logged in.
  • internal/commands/chat-sast.go — checks if an API key is available for the AI chat feature.
  • internal/commands/agenthooks/mcp/bridge.go — the bridge for the MCP agent integration.
  • internal/wrappers/configuration/configuration.go — the cx configure command.
  • internal/commands/util/env.go — the cx utils env command.

These files used to write the secret value with viper.Set and a file-write helper function. Now they call Resolver.Store or Resolver.Clear instead.

  • cx configure (both the interactive form and cx configure set)
  • cx auth login
  • cx auth logout

Removed code

The CLI no longer connects the API key and the client secret to command-line flags through Viper. internal/params/binds.go and internal/commands/root.go had these two connections. This PR removes them, because the resolver now owns both values.

The CLI also removes params.AstToken. This constant stored the short-lived access token inside Viper. The CLI now marks this token as sensitive with logger.RegisterSensitiveValue instead. This stops the token from leaking into a config file write, by accident, in the future.

Bug fixes in plain language

Fix 1: secrets could appear in debug logs

Before this fix, a user could run a command with --debug, and the API key or the client secret could show up in the log output. This happened because the log-cleaning code looked for the secret value inside Viper, but Viper no longer held the value after other changes in this PR.

The fix: the resolver now tells the logger about every secret value it returns, through logger.RegisterSensitiveValue. The logger replaces the value with *** before it prints anything.

Fix 2: cx configure show displayed a blank credential

Before this fix, the cx configure show command tried to read the secret directly from Viper. Because Viper no longer holds the secret, the command showed a blank value instead of a masked value.

The fix: the command now asks the resolver for the value, then masks it, before it shows the result to the user.

Fix 3: cx utils env showed wrong information

Before this fix, cx utils env had three problems:

  1. It listed environment variables in a random order each time.
  2. It looked up the wrong variable name, so most values showed as blank, even when the user had set them.
  3. It printed secrets as plain text.

The fix: the command now uses a fixed list and a fixed order. It looks up the correct variable name. It shows secrets with only the last four characters visible, for example ******1234.

Note for other tools that read CLI output: cx utils env output is different now. Tools that parse this output must expect the new format.

Fix 4: a failed configure set on a secret did not report an error

Before this fix, if the CLI could not write a secret, for example because the keyring was locked, the cx configure set command still returned a success result.

The fix: the command now returns an error, and stops, if the write to the resolver fails.

What changes for the user

  • The API key and the client secret move out of the plain text config file and into the OS keyring, automatically, the first time the user runs a command after the update.
  • cx auth login --help now says the login token is stored in the OS keyring, not in the config file.
  • cx utils env output looks different. See Fix 3 above.
  • A new environment variable, CX_KEYRING_MODE, controls the storage location. The default behavior needs no change from the user.
  • On a machine with no OS keyring, for example many build servers, the user should set CX_KEYRING_MODE=disabled. The CLI then uses the plain text config file only, the same as before this PR.

Testing

  • go build ./... and go vet run clean on all changed packages.
  • golangci-lint reports zero issues in the two new folders.
  • All unit tests in internal/credentialstore pass.
  • Targeted unit tests in internal/commands, internal/commands/util, and internal/wrappers pass.
  • Manual test steps covered: cx utils env, cx configure set, cx configure show, the interactive cx configure, secret migration from an old config file, the read order (flag, then environment variable, then keyring, then config file), all three keyring modes, cx auth login, cx auth logout, and a check that no secret appears in debug output or log files.
  • Three test setup functions now force CX_KEYRING_MODE=disabled before unit tests run. This stops unit tests from touching a real keyring on a developer machine.

Known follow-up work, not part of this PR

  • The new internal/configfile folder has no dedicated test file yet.
  • configfile.Save rewrites the whole file in place. If the CLI stops in the middle of this write, the file can become corrupt. A safer method is to write to a temporary file first, then rename it.
  • One file, auth_logout.go, has an import order that does not match the project's code format rule.
  • The function DefaultStore is no longer called from anywhere. It can be removed.
  • A few extra files in the repository root, for example debug.log and some design notes, should move to the docs folder or be deleted before merge.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)

Signed-off-by: Prathmesh Borle <65400885+cx-prathmesh-borle@users.noreply.github.com>
… coverage

Ensure explicitly empty credential flags take precedence over environment variables.
Track explicit flag state via map presence instead of non-empty values.
Fix auth validation flag-combination tests and 7 golangci-lint findings.
Add credentialstore/configfile tests to restore coverage above 85%.

Signed-off-by: Prathmesh Borle <65400885+cx-prathmesh-borle@users.noreply.github.com>
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