feat(core): introduce typed Secret with inline env/file/literal, BindServiceConfig; integrate in KAS #2799

strantalis · 2025-10-14T16:10:45Z

Proposed Changes

Add Secret type with redaction and safe JSON/log masking
Support inline secret directives: env:NAME, file:/path, literal:VALUE
Add BindServiceConfig helper with mapstructure decode hooks + validation
Implement file-based and env-based secret resolution (lazy/eager)
Integrate KAS: use Secret for root_key and BindServiceConfig
Improve env handling during reload to surface env-only keys
Add unit tests for Secret, BindServiceConfig, and inline decode forms
Update docs: “Secrets In Config” with examples and guidance
Remove temporary debug logs and experimental nested secret usage in KAS

DX

Plain strings are literals by default; use literal: to disambiguate values starting with env:/file:
Example:
- services.kas.root_key: "env:OPENTDF_SERVICES_KAS_ROOT_KEY"
- services.kas.preview.key_management: true

Checklist

I have added or updated unit tests
I have added or updated integration tests (if appropriate)
I have added or updated documentation

Testing Instructions

strantalis · 2025-10-14T16:36:24Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a Secret type with support for inline environment variables, file paths, and literal values, enhancing configuration security. It also adds a BindServiceConfig helper for simplified configuration binding and validation. The changes integrate these features into the KAS, improving environment handling and adding unit tests and documentation. The review focuses on correctness and potential issues in the new code.

service/kas/access/provider.go

service/kas/kas.go

service/pkg/config/config.go

service/pkg/config/secret.go

service/kas/kas.go

service/pkg/config/bind.go

service/pkg/config/secret.go

strantalis · 2025-10-14T18:39:21Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a robust Secret type for managing sensitive configuration values, along with a BindServiceConfig helper to streamline decoding and validation. The integration into KAS to handle the root_key is a great use case. The changes are well-structured and include thorough testing, which is excellent.

My review includes a few suggestions to enhance the implementation:

Improving the heuristic for redacting secrets in debug logs to prevent potential leaks.
Clarifying the documentation for structured secret configuration.
Minor improvements to error messages and code consistency for better maintainability.

service/pkg/config/config.go

docs/Configuring.md

service/pkg/config/decode.go

service/pkg/config/secret.go

…eral, BindServiceConfig; integrate in KAS - Add Secret type with redaction and safe JSON/log masking - Support inline secret directives: env:NAME, file:/path, literal:VALUE - Add BindServiceConfig helper with mapstructure decode hooks + validation - Implement file-based and env-based secret resolution (lazy/eager) - Integrate KAS: use Secret for root_key and BindServiceConfig - Improve env handling during reload to surface env-only keys - Add unit tests for Secret, BindServiceConfig, and inline decode forms - Update docs: “Secrets In Config” with examples and guidance - Remove temporary debug logs and experimental nested secret usage in KAS DX - Plain strings are literals by default; use literal: to disambiguate values starting with env:/file: - Example: - services.kas.root_key: "env:OPENTDF_SERVICES_KAS_ROOT_KEY" - services.kas.preview.key_management: true

dmihalcik-virtru

Can you avoid breaking compatibility with downstream users? Maybe print out a warning if the old root_key field is set, and use a new name (suggestion: envelope_key) from now on.

dmihalcik-virtru · 2025-10-16T18:43:54Z

service/kas/access/provider.go

 	RSACertID string `mapstructure:"rsacertid" json:"rsacertid"`

-	RootKey string `mapstructure:"root_key" json:"root_key"`
+	RootKey config.Secret `mapstructure:"root_key" json:"root_key"`


Even changes to 'experimental' features shouldn't break builds downstream. Deprecate instead of removing to avoid blocking the next release of downstream integrations.

Suggested change

RootKey config.Secret `mapstructure:"root_key" json:"root_key"`

// Deprecated: Use EnvelopeKey

RootKey string `mapstructure:"root_key" json:"root_key"`

EnvelopeKey config.Secret `mapstructure:"envelope_key" json:"envelope_key"`

There shouldn't be downstream impact. This should still support a string literal in yaml without any prefix.

ntrevino-virtru · 2025-10-20T16:30:08Z

service/pkg/config/config.go


+		// If an environment loader is present, configure orderedViper to read env directly.
+		for _, loader := range c.loaders {
+			if loader.Name() == LoaderNameEnvironmentValue {


The env loader should already do this. And it should be configured by the consumer of the library.

ntrevino-virtru · 2025-10-20T16:34:40Z

service/pkg/config/environment_value_loader.go

+	// Discover environment variables with the configured prefix and convert them to dotted keys
+	// Example: OPENTDF_SERVICES_KAS_ROOT_KEY -> services.kas.root_key
+	// Note: Viper treats keys case-insensitively; we normalize to lower-case dotted form here
+	prefix := l.envPrefix + "_"
+	for _, env := range os.Environ() {


This bit looks like what Viper should be doing already. If it is adding additional keys we should cache those keys and values so they can be returned by Get, otherwise we might hit this: https://github.com/opentdf/platform/blob/main/service/pkg/config/config.go#L219-L229

github-actions bot added comp:kas Key Access Server docs Documentation size/l labels Oct 14, 2025

strantalis changed the title ~~feat(service/config): introduce typed Secret with inline env/file/literal, BindServiceConfig; integrate in KAS~~ feat(core): introduce typed Secret with inline env/file/literal, BindServiceConfig; integrate in KAS Oct 14, 2025

gemini-code-assist bot reviewed Oct 14, 2025

View reviewed changes

service/pkg/config/config.go Outdated Show resolved Hide resolved

docs/Configuring.md Outdated Show resolved Hide resolved

service/pkg/config/decode.go Outdated Show resolved Hide resolved

service/pkg/config/secret.go Show resolved Hide resolved

strantalis marked this pull request as ready for review October 15, 2025 17:41

strantalis requested review from a team as code owners October 15, 2025 17:41

dmihalcik-virtru requested changes Oct 16, 2025

View reviewed changes

strantalis added 9 commits October 16, 2025 15:17

fix: implement review feedback

2248256

minor cleanups

5861113

minor fixes and doc updates

79d4320

format

5a4f902

simplify resolve switch statement

a2473ba

remove logging value

5d9b126

fix error return message

3fefb48

add more tests for nested arrays and maps

4275942

rebase on main

867b0d6

strantalis force-pushed the dspx-987/secrets branch from 5041371 to 867b0d6 Compare October 17, 2025 14:58

strantalis added 2 commits October 17, 2025 16:21

fix lint issues

a3df734

cleanup

126f006

ntrevino-virtru reviewed Oct 20, 2025

View reviewed changes

fix lint issues

d445f8e

feat(core): introduce typed Secret with inline env/file/literal, BindServiceConfig; integrate in KAS #2799

Are you sure you want to change the base?

feat(core): introduce typed Secret with inline env/file/literal, BindServiceConfig; integrate in KAS #2799

Uh oh!

Conversation

strantalis commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Checklist

Testing Instructions

Uh oh!

strantalis commented Oct 14, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

strantalis commented Oct 14, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmihalcik-virtru left a comment

Choose a reason for hiding this comment

Uh oh!

dmihalcik-virtru Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

strantalis Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

ntrevino-virtru Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

ntrevino-virtru Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

strantalis commented Oct 14, 2025 •

edited

Loading