feat(auth): Support using env variables to supplement and/or override config values. - #11
Open
spbsoluble wants to merge 32 commits into
Open
feat(auth): Support using env variables to supplement and/or override config values.#11spbsoluble wants to merge 32 commits into
spbsoluble wants to merge 32 commits into
Conversation
…sKey Extends the existing AKEYLESS_API_URL override pattern in InitClient so AKEYLESS_AUTH_TYPE, AKEYLESS_ACCESS_ID, and AKEYLESS_ACCESS_KEY can also override their corresponding configured connection parameters at runtime, letting deployments control Akeyless connection details at the infrastructure level instead of only via manifest.json/Command portal. Closes #10
…de in README) [skip ci]
…r-credential-overrides # Conflicts: # README.md # docs/akeyless.md
GetPassword_BadCredentials_ThrowsInvalidClientConfigurationException asserted that bad server-config credentials fail auth, but AKEYLESS_ACCESS_ID/ AKEYLESS_ACCESS_KEY (required for every other integration test to run) now override those credentials via this PR's own env var override feature, making auth succeed instead of failing. Clear both env vars for the duration of this test only, and disable assembly-level parallelization so that doesn't race with AkeylessApiClientTests reading the same vars. Also documents the previously-missing Debug_K8sOrchestratorSecret_PrintsRawValue test in the integration tests README.
…ides' into feature/env-var-credential-overrides
The CI's auto-generate-docs workflow ran a stale/buggy doctool version against this branch (docsource/akeyless.md unchanged), which re-stripped Extension Mechanics from README.md and duplicated content into docs/akeyless.md — the exact bug already fixed on this branch via the "regenerate with fixed doctool" commit. Restoring that fixed content.
ResolveEnvOverride only checked IsNullOrEmpty, so a whitespace-only env var (e.g. AKEYLESS_ACCESS_ID=" ") was treated as an active override rather than falling back, contradicting its own doc comment and the codebase's existing whitespace-only-is-empty convention elsewhere. Switch to IsNullOrWhiteSpace, and add whitespace-only unit tests for all four env vars alongside the existing empty-string cases. Also log which specific env var is overriding (never the value) when an override is active, so an incident investigation can tell whether the effective connection parameter used at runtime actually matches Command's recorded configuration — previously nothing distinguished a configured value from an env-var-overridden one in production logs.
…ides' into feature/env-var-credential-overrides # Conflicts: # README.md
Same recurring issue: the CI auto-generate-docs workflow (Keyfactor Bootstrap Workflow, pinned keyfactor/actions@v5) ran a stale/buggy doctool version against this branch again (docsource/akeyless.md unchanged), stripping the Extension Mechanics section from README.md. Restoring the correct content. This has now happened on every push to this branch — see PR discussion for a permanent fix recommendation.
spbsoluble
changed the title
…g effective URL - InitClient now re-validates the resolved AuthType against the supported-auth-type allowlist before use. Previously an AKEYLESS_AUTH_TYPE override bypassed the validation BuildAkeylessConfiguration already performed on the configured value, so a typo'd or unsupported override silently skipped authentication (no throw, AuthToken left empty) and let execution continue into an unauthenticated secret fetch. Unrecognized overrides now fail fast with InvalidClientConfigurationException. - ResolveEnvOverride now trims the resolved value, since env vars sourced from file-mounted secrets/configmaps commonly carry an incidental trailing newline, which previously failed the exact-match auth-type switch and could send whitespace-polluted credentials to Akeyless. - The "Connecting to Akeyless at" debug log now reports the effective (post-override) URL used to construct the API client instead of the stale pre-override configured value, so log-based incident review can actually confirm the true destination host.
…th failure - ResolveEnvOverride now rejects a value that still contains an embedded line break after trimming (InvalidClientConfigurationException). Trim() only strips leading/trailing whitespace, so a value sourced from a multi-line mounted secret file could otherwise flow unchanged into the structured log messages that echo AccessId/AuthType, forging extra log lines. - InitClient's ApiException catch block now logs the AccessId that the failed authentication attempt used. It previously omitted AccessId (unlike the success and empty-token paths), so a failed auth attempt left no record of which identity was used -- a real gap once AKEYLESS_ACCESS_ID can make the runtime identity diverge from Command's recorded configuration.
…\n/\r ResolveEnvOverride's embedded-line-break guard only checked for '\n'/'\r', missing other control characters (ESC/ANSI escape sequences, NUL, form feed, vertical tab) and the Unicode line/paragraph separators (U+2028/U+2029) that survive Trim() and .NET's char.IsWhiteSpace() the same way an embedded '\n' would, and could equally forge or tamper with rendered log output on sinks that treat them as line breaks or escape codes. Switched the check from a '\n'/'\r' blocklist to rejecting on Unicode category (char.IsControl, plus LineSeparator/ParagraphSeparator), which covers all of the above in one check.
…ApiException - ResolveEnvOverride's character filter covered Control and Line/ParagraphSeparator categories but missed Format-category characters (Cf) such as U+202E RIGHT-TO-LEFT OVERRIDE and zero-width characters, which could make a logged AccessId/AuthType render differently than its actual content. Broadened the rejection to all "other" Unicode categories (Control, Format, Surrogate, PrivateUse, OtherNotAssigned) plus LineSeparator/ParagraphSeparator. - InitClient's ApiException catch no longer passes the exception object to Logger.LogError. The adjacent comment already said ex.Message is excluded because Akeyless's error response may echo back credentials, but passing `ex` as the exception parameter still exposes it, since most ILogger providers render an attached exception's Message/ToString() independent of the message template -- the comment's intent wasn't actually enforced by the code.
…and configured values Rounds 3-5 kept finding one more Unicode category (ANSI escapes, U+2028/U+2029, bidi-override/zero-width Format characters, now variation selectors) that survived the growing character blocklist -- an inherently unbounded whack-a-mole problem. Replaced ResolveEnvOverride's blocklist with a single printable-ASCII allowlist (EnsurePrintableAscii), which rejects everything outside 0x20-0x7E in one check instead of naming specific bad characters. This also closes a second, more consequential gap: the blocklist only ever guarded the env-var override path. A malicious/malformed AccessId or Url supplied through Command's normal server configuration (no override involved) reached the exact same log statements completely unvalidated. The allowlist is now applied to the final resolved value in InitClient regardless of which source it came from, so both paths get the same guarantee.
…AkeylessConfiguration logs them EnsurePrintableAscii was only ever called from InitClient, which runs on a later call path (GetAkeylessSecretAsync -> InitClient) than BuildAkeylessConfiguration's own "Using Akeyless URL ..." and "Access key auth configured with AccessId ..." debug logs. A malicious/malformed value supplied via Command's server configuration (no env var override needed) was echoed by those two log statements before InitClient's validation ever ran -- the allowlist protected the override path and the InitClient-side re-resolution of the configured value, but not the earlier BuildAkeylessConfiguration log statements that fire first. Added the same EnsurePrintableAscii calls to BuildAkeylessConfiguration, right after each value is read and before it's logged.
EnsurePrintableAscii was the one validation-throw site in the file with no corresponding Logger.LogError call -- every other validation failure (ValidateRequiredParameter, ValidateAuthTypeAccessKey, the unsupported-auth-type checks, model validation, the ApiException catch) logs before throwing. Made the method an instance method so it can log the parameter name (never the value) before throwing, giving an audit trail for rejected log-injection/ spoofing attempts instead of silent fail-closed with no record.
Collapses the repeated resolve-override-then-EnsurePrintableAscii pattern for Url/AuthType/AccessId/AccessKey into a single helper, so the sequencing only needs to be verified in one place instead of four.
The SupportedAuthMethods guard clause immediately above already proves authType is "access_key" by this point, making the switch a no-op wrapper.
BuildAkeylessConfiguration validated configured connection values with EnsurePrintableAscii but never trimmed them first, unlike env-var overrides (which ResolveEnvOverride already trims). A previously-working configured value with an incidental trailing newline/tab — a common artifact of hand-edited manifest.json or portal paste — silently worked before this PR's hardening (Uri/HttpClient normalize it) but now hard-fails on every GetPassword call. Trim configured values the same way overrides are, so the hardening doesn't regress previously-valid configuration. Also dedupes the EnvVarScope test helper (previously copy-pasted across both test projects) into a single linked file, and reuses AkeylessConstants.DefaultAkeylessApiUrl instead of a duplicated literal default in InitClient.
…override BuildAkeylessConfiguration's new .Trim() calls threw an unhandled NullReferenceException when the connectionConfiguration dictionary had an explicit null value (key present, value null) for AuthType or Url, bypassing the previously-handled InvalidClientConfigurationException path and its logging. Both now default/fall back the same way an absent key already did, before trimming. Also bumps ResolveEnvOverride's "Environment variable override active" log from Information to Warning: a host that already has AKEYLESS_AUTH_TYPE/AKEYLESS_ACCESS_ID/AKEYLESS_ACCESS_KEY set for an unrelated reason (e.g. a co-located Akeyless CLI using the same variable names) will now silently authenticate with a different identity than Command's recorded configuration after upgrading, with zero config change on Command's side. This is the PR's intended behavior extended from the pre-existing AKEYLESS_API_URL override, not a bug, but the upgrade-collision risk deserves louder visibility than Information level, plus a documented callout in docsource/akeyless.md.
…basePath BuildAkeylessConfiguration checked string.IsNullOrEmpty before substituting the default Url, so a whitespace-only configured value (e.g. a single stray space) skipped the fallback, then got trimmed into an empty string that passed EnsurePrintableAscii vacuously (LINQ .All() over an empty sequence). The empty base path then reached the Akeyless SDK client with no further validation. Switched to IsNullOrWhiteSpace, matching the existing env-var-override path which already handled this correctly. Also collapses four sets of near-duplicate [Fact] tests (Url/AccessId/ AccessKey/AuthType env-var unset/empty/whitespace-only, differing only in the literal) into four [Theory]/[InlineData] tests, consistent with the file's existing Theory usage.
BuildAkeylessConfiguration already guarantees configurationInfo.Url is never null/whitespace, so InitClient's second "?? DefaultAkeylessApiUrl" fallback was unreachable.
…r Theory Was a standalone Fact with a byte-for-byte identical body to the adjacent control-character Theory, differing only in the injected string.
Collapses the detailed per-fix bullet points into higher-level summary entries for the release notes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AKEYLESS_API_URLenvironment-variable override pattern inInitClient(akeyless-pam/AkeylessPam.cs) to also coverAuthType,AccessId, andAccessKey:AKEYLESS_AUTH_TYPEoverridesconfigurationInfo.AuthTypeAKEYLESS_ACCESS_IDoverridesconfigurationInfo.AccessIdAKEYLESS_ACCESS_KEYoverridesconfigurationInfo.AccessKeyResolveEnvOverridehelper used by all four env vars (includingAKEYLESS_API_URL) so precedence is consistent: env var (non-empty) > configured parameter > default (URL only).InitClientnow references the resolved (post-override)AccessIdin the debug/info/error messages that already logged it — no new fields are logged, andAccessKeyis still never logged.Empty-string handling (design decision)
The original
AKEYLESS_API_URLline used a plain??againstEnvironment.GetEnvironmentVariable(...), which only falls back when the env var is unset — an env var explicitly set to""would have "won" and blanked out a valid configured value. Rather than leave that edge case in place for the URL and add a different, inconsistent rule for the other three,ResolveEnvOverridetreats an env var set to an empty string the same as an unset one for all four variables (AKEYLESS_API_URLincluded). This is a minor behavior tweak to the existingAKEYLESS_API_URLhandling, but avoids a footgun where an empty/misconfigured environment variable could silently break a workingmanifest.json/Command portal configuration. No test previously depended on the old "empty string wins" behavior for the URL.InitClientis changed — request validation (ValidateServerConfigurationParams/ValidateAuthTypeAccessKey) is unchanged and out of scope, soAccessId/AccessKeymust still be present in the server configuration parameters for the config to build successfully; the env vars override the value used once configuration is valid, matching the literal scope of the issue.Docs
docsource/akeyless.md: expanded the## Configurationsection to document all four env vars in a precedence table (wasAKEYLESS_API_URL-only).README.mdanddocs/akeyless.mdviadoctooldotnet generate-docs.CHANGELOG.md: added av1.1.0entry describing the new overrides.Test plan
dotnet build akeyless-pam/akeyless-pam.csproj -c Release— succeeds, no warningsdotnet test tests/AkeylessPam.Unit.Tests/— 40/40 passing (12 new tests covering override/fallback/empty-string behavior for all four env vars)dotnet test --collect:"XPlat Code Coverage"— ~86.5% line coverage[assembly: CollectionBehavior(DisableTestParallelization = true)]since the new tests mutate process-wide env vars and would otherwise race with other test classes running in parallelCloses #10