Skip to content

feat: support separate session keyrings and Passage identity files - #436

Open
issei-m wants to merge 5 commits into
ByteNess:mainfrom
issei-m:codex/separate-session-keyring
Open

feat: support separate session keyrings and Passage identity files#436
issei-m wants to merge 5 commits into
ByteNess:mainfrom
issei-m:codex/separate-session-keyring

Conversation

@issei-m

@issei-m issei-m commented Aug 31, 2026

Copy link
Copy Markdown

Closes #433.

Summary

This adds optional keyring configuration for cached sessions through --session-* options and AWS_VAULT_SESSION_* environment variables.

When no session-specific setting is provided, aws-vault continues to use the same keyring instance as before. Unspecified session settings inherit the primary keyring configuration.

Long-lived credentials and SSO OIDC tokens remain in the primary keyring, while cached temporary credentials use the session keyring.

Independently of session keyring configuration, --passage-identities-file and AWS_VAULT_PASSAGE_IDENTITIES_FILE can now select the identity file used by the primary Passage backend. Passage identity selection is provided by ByteNess/keyring#109, released in keyring v1.13.0.

Testing

  • go test ./...
  • go vet ./...
  • Manually tested on macOS with IAM user profiles with and without MFA, and with AWS IAM Identity Center (SSO)

The separate Passage stores and identities were tested with the following setup:

mkdir -p /tmp/aws-vault/credentials
mkdir -p /tmp/aws-vault/sessions

age-keygen -o /tmp/aws-vault/key.txt
age-keygen -o /tmp/aws-vault/key2.txt

aws-vault \
  --backend=passage \
  --pass-dir=/tmp/aws-vault/credentials \
  --passage-identities-file=/tmp/aws-vault/key.txt \
  add test

aws-vault \
  --backend=passage \
  --pass-dir=/tmp/aws-vault/credentials \
  --passage-identities-file=/tmp/aws-vault/key.txt \
  --session-backend=passage \
  --session-pass-dir=/tmp/aws-vault/sessions \
  --session-passage-identities-file=/tmp/aws-vault/key2.txt \
  exec test -- env | grep AWS

AI disclosure

OpenAI Codex assisted with the implementation, tests, documentation, and review. I reviewed and edited the complete diff and manually tested the behaviour described above.

Checklist

  • One logical change; unrelated fixes split into separate PRs
  • New behaviour has tests; bug fixes include a regression test where practical
  • Docs and/or README.md updated if the change is user-visible, breaking changes are called out explicitly
  • No real credentials, access keys, session tokens, MFA serials, or full account IDs anywhere in the diff, tests, or description

@issei-m
issei-m requested a review from mbevc1 as a code owner August 31, 2026 01:21
@github-actions github-actions Bot added documentation Improvements or additions to documentation dependencies Pull requests that update a dependency file feat labels Aug 31, 2026
@mbevc1

mbevc1 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Thanks for the PR and it's a solid start with right inheritance model and split. There are also still some things to sort out before merging:

  1. Sessions already in the primary keyring become unreachable after opting in.

Once a user sets any --session-* flag, every session code path switches to the session keyring: ClearCommand (cli/clear.go:40), ListCommand (cli/list.go:99), AddCommand (cli/add.go:90), and RotateCommand (cli/rotate.go:139). Their pre-existing session entries stay in the primary keyring, where CredentialKeyring.Keys() filters them out as session keys (vault/credentialkeyring.go:21), so list won't show them, clear won't remove them, and RemoveOldSessions never runs against that store again. The cached credentials themselves expire, so this isn't an exposure spiral, but the keyring items are stranded permanently with no CLI way to reach them.

Potential cheapest fix: in ClearCommand, when the two keyrings aren't the same instance, also sweep sessions from the primary. Failing that, a line in managing-sessions.md saying "run aws-vault clear before enabling a session keyring" would close it. But I'd prefer to have a more holistic fix in place.

  1. A nil TempCredentialsCreator.SessionKeyring panics at retrieve time.

SessionKeyring is a new exported field on an exported struct in a v7 module. An external consumer constructing TempCredentialsCreator{Keyring: ckr} the old way still compiles, and then dies. Confirmed:

PANIC: runtime error: invalid memory address or nil pointer dereference

on (&vault.SessionKeyring{Keyring: nil}).Get(...).NewTempCredentialsProvider's signature change is a compile-time break they'd notice, but the struct literal path is silent. Three lines in GetProviderForProfile fix it and restore source compatibility:

if t.SessionKeyring == nil {
	t.SessionKeyring = t.Keyring.Keyring
}

Also some other minor findings:

  • cli/global.go:145 - if backend != "" is unreachable from the CLI, since --backend carries Default(backendsAvailable[0]). It also hides an ordering coupling: Keyring() mutates a.KeyringConfig.AllowedBackends in place (global.go:121), and Keyrings() calls it first, so a hypothetical empty backend would leak the primary's resolved backend into the session config. Dropping the guard (or clearing AllowedBackends on the copy) makes it order-independent.
  • 1Password and Proton Pass have no session overrides. Seven store-location knobs got --session-* twins; --op-vault-id, --op-item-title-prefix, --op-item-tag, --proton-pass-share-id, and --proton-pass-item-title-prefix did not. Those users can only separate by switching backend entirely. (wincred/kwallet/keyctl have no flags at all today, so that's symmetric with the status quo — not a new gap.) Worth one sentence in the docs about which backends support same-backend separation.
  • --biometrics can't be split. It's applied to a.KeyringConfig in PreAction and therefore inherited by the session config. Passage users get what #433 asked for via separate identity files, but a macOS-keychain user can't do "TouchID for credentials, no prompt for sessions" — arguably the same motivating shape. A follow-up --session-biometrics, not this PR.
  • RemoveCommand's sessionKeyring param is dead on the non---sessions-only path. Harmless, slightly confusing signature.
  • --passage-identities-file is a second user-facing flag riding along on the primary config. It's needed to make the feature usable and it's in the docs table, but it's independently useful and worth a release-note mention rather than being folded into "session keyring support".

@issei-m issei-m changed the title feat: support a separate session keyring feat: support separate session keyrings and Passage identity files Sep 1, 2026
@issei-m

issei-m commented Sep 2, 2026

Copy link
Copy Markdown
Author

@mbevc1 Thanks for the review. Here’s a quick update.

Addressed

Sessions already in the primary keyring become unreachable after opting in.

clear now sweeps the primary keyring first, then the separate session keyring. Added a regression test covering both.

1Password and Proton Pass have no session overrides.

Added session overrides for the five store-location options mentioned, with tests and docs.

--passage-identities-file is worth a release-note mention.

Updated the PR title and summary so it appears separately in the generated release notes.

Not changed

A nil TempCredentialsCreator.SessionKeyring panics at retrieve time.

You’re right that this breaks backward compatibility from a semantic-versioning perspective. It doesn’t affect aws-vault CLI users, but it could break projects using this module as a library. Would a major-version bump be more appropriate, or should I preserve v7 compatibility with the nil fallback?

if backend != "" is unreachable from the CLI.

Kept it because it preserves backend auto-detection for directly constructed values and matches the primary keyring code.

--biometrics can’t be split.

I left this out because I don’t see a likely use case for configuring biometrics differently between the primary and session keyrings. It can be revisited if a concrete use case comes up.

RemoveCommand’s sessionKeyring param is dead on the non-SessionsOnly path.

Kept it as-is since it is used by the SessionsOnly branch and causes no behavioral issue.

@mbevc1

mbevc1 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Thanks @issei-m , would you mind resolving conflicts as well?

@mbevc1

mbevc1 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Oh, also to answer your question;

The PR already changes the signatures of NewTempCredentialsProvider and NewSSORoleCredentialsProvider so the nil guard therefore isn't the semver lever change. It's more of a robustness fix - it turns a silent runtime panic on the struct-literal path into a non-event.

Since it should be a three-line fallback, let's not go with the major bump.

Allow cached sessions to use an independently configured keyring while preserving the existing shared-keyring default. Keep long-lived credentials and OIDC tokens in the primary keyring.

Refs ByteNess#433

AI-assisted with OpenAI Codex.
@issei-m
issei-m force-pushed the codex/separate-session-keyring branch from 5359d3c to 191711e Compare September 12, 2026 10:32
@github-actions github-actions Bot removed the dependencies Pull requests that update a dependency file label Sep 12, 2026
@issei-m

issei-m commented Sep 13, 2026

Copy link
Copy Markdown
Author

@mbevc1 Added the nil fallback in GetProviderForProfile as suggested, and rebased onto the latest main to resolve the conflicts. I also added debug messages to distinguish primary and session keyring initialization, including when they share the same instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation feat

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow session cache to use a separately configured keyring backend

2 participants