Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Deprecate server.CryptoProvider for kas.keyring #1834

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

dmihalcik-virtru
Copy link
Member

@dmihalcik-virtru dmihalcik-virtru commented Jan 3, 2025

Proposed Changes

  • Replace access.CryptoProvider with recrypt.Provider, with simplified interface for implementation of 3rd party crypto layers
  • refactors code to use it
  • supports for old config formats, adds more tests around them
  • TK (later): Expose this to service constructor somehow? How can I pass a live object to a packed-in service?

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

@dmihalcik-virtru dmihalcik-virtru changed the title feat!(core): Replaces crypto provider with new interface feat(core)!: Replaces crypto provider with new interface Jan 3, 2025
jentfoo
jentfoo previously approved these changes Jan 6, 2025
@dmihalcik-virtru dmihalcik-virtru force-pushed the feature/cryptwo branch 3 times, most recently from 23f9d81 to 6f2bb64 Compare January 9, 2025 21:37
@dmihalcik-virtru dmihalcik-virtru changed the title feat(core)!: Replaces crypto provider with new interface feat(core)!: Deprecate server.CryptoProvider for kas.keyring Jan 10, 2025
@dmihalcik-virtru dmihalcik-virtru marked this pull request as ready for review January 10, 2025 17:06
@dmihalcik-virtru dmihalcik-virtru requested review from a team as code owners January 10, 2025 17:06
@dmihalcik-virtru dmihalcik-virtru changed the title feat(core)!: Deprecate server.CryptoProvider for kas.keyring feat(core): Deprecate server.CryptoProvider for kas.keyring Jan 10, 2025
- New service crypto package, recrypto, which provides a shared interface for necessary TDF operations, both for ZTDF (Wrap) and Nano (Derive)
- This includes a shift to yet another configuration for the crypto layer. Notably, the keys can now be completely configured in the `services.kas.keyring`, without needing the `server.cryptoprovider` field
logger.Debug("updating kas key configuration", slog.String("namespace", ns), slog.Any("legacyConfig", cfg.Server.CryptoConfig2024))
// Upgrade the the kas configuration, if there is a legacy `CryptoProvider` configuration
// present in the otdf server config.
if cfg.Server.CryptoConfig2024 != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cfg.Server.CryptoConfig2024 is this the legacy config renamed. should the new config be named cfg.Server.CryptoConfig2025. I have also seen append _V1
is this versioned in the yaml?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no new config under Server; after this it is moved entirely to the services.kas field.

The type names are not exposed in the YAML config file, and the names of the fields can/will be overridden with tags (e.g. mapstructure:"config2025") as needed. The config struct types and their fields need to be exported (capitalized) for use with the mapstructure` library, but we have as of yet not treated them as part of the API and subject to standard versioning considerations. However, we may want to do that before marking this whole library as 1.x.

I did a grep through our internal code and was unable to find references to the moved/removed/renamed types

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.

3 participants