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.

Sign up for GitHub

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

Jump to bottom

kms: add support for MinKMS and remove some unused/broken code #19368

Merged
merged 1 commit into from
May 7, 2024
Merged

kms: add support for MinKMS and remove some unused/broken code #19368

merged 1 commit into from
May 7, 2024

Conversation

aead
Copy link
Member

@aead aead commented Mar 28, 2024

Description

This commit adds support for MinKMS. Now, there are three KMS implementations in internal/kms: Builtin, MinIO KES and MinIO KMS.

Adding another KMS integration required some cleanup. In particular:

  • Various KMS APIs that haven't been and are not used have been removed. A lot of the code was broken anyway.
  • Metrics are now monitored by the kms.KMS itself. For basic metrics this is simpler than collecting metrics for external servers. In particular, each KES server returns its own metrics and no cluster-level view.
  • The builtin KMS now uses the same en/decryption implemented by MinKMS and KES. It still supports decryption of the previous ciphertext format. It's backwards compatible.
  • Data encryption keys now include a master key version since MinKMS supports multiple versions (~4 billion in total and 10000 concurrent) per key name.

Motivation and Context

KMS

How to test this PR?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Optimization (provides speedup with no functional changes)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Fixes a regression (If yes, please add commit-id or PR # here)
  • Unit tests added/updated
  • Internal documentation updated
  • Create a documentation update request here

shtripat
shtripat approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@shtripat shtripat left a comment

Choose a reason for hiding this comment

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

lgtm

@harshavardhana
Copy link
Member

KMS enabled tests seem to crash

harshavardhana
harshavardhana reviewed Mar 28, 2024
View reviewed changes
Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

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

are the rest from the mineos branch?

cmd/common-main.go Outdated Show resolved Hide resolved
@bh4t bh4t assigned aead Apr 9, 2024
@harshavardhana
Copy link
Member

Please rebase this PR with latest master @aead @allanrogerr

@aead aead force-pushed the minkms-upstream branch 2 times, most recently from 11fe7b9 to 80879fb Compare May 1, 2024 20:01
@harshavardhana
Copy link
Member

@aead once this is merged. Can we simply force merge this to downstream mineos?

Since there are conflicts in cherry picking want to make sure that is perfectly fine.

@aead
Copy link
Member Author

aead commented May 1, 2024 •
edited

@aead once this is merged. Can we simply force merge this to downstream mineos?
Since there are conflicts in cherry picking want to make sure that is perfectly fine.

I think so, yes @harshavardhana

@harshavardhana
Copy link
Member

@aead PTAL regular tests seem to be failing - NOTE these tests run with MINIO_KMS_SECRET_KEY

@aead aead force-pushed the minkms-upstream branch 2 times, most recently from 62f1bf6 to c3f10a2 Compare May 7, 2024 18:58
@aead
kms: add support for MinKMS and remove some unused/broken code
be4a96c 
This commit adds support for MinKMS. Now, there are three KMS
implementations in `internal/kms`: Builtin, MinIO KES and MinIO KMS.

Adding another KMS integration required some cleanup. In particular:
 - Various KMS APIs that haven't been and are not used have been
   removed. A lot of the code was broken anyway.
 - Metrics are now monitored by the `kms.KMS` itself. For basic
   metrics this is simpler than collecting metrics for external
   servers. In particular, each KES server returns its own metrics
   and no cluster-level view.
 - The builtin KMS now uses the same en/decryption implemented by
   MinKMS and KES. It still supports decryption of the previous
   ciphertext format. It's backwards compatible.
 - Data encryption keys now include a master key version since MinKMS
   supports multiple versions (~4 billion in total and 10000 concurrent)
   per key name.

Signed-off-by: Andreas Auernhammer <github@aead.dev>
@harshavardhana harshavardhana merged commit 8b660e1 into master May 7, 2024
20 checks passed
@harshavardhana harshavardhana deleted the minkms-upstream branch May 7, 2024 23:55
@harshavardhana
Copy link
Member

harshavardhana commented May 7, 2024 •
edited

@allanrogerr or @shtripat can you guys take up task of adding kes service0 for testing test-decom in MinIO

  • running kes via the shell script or docker pick your choice
  • running minio multiple pools configured with kes, first pool is
    decommed, test sse-s3 and sse-kms both.
  • use s3-check-md5 to verify

Currently we have tests for the MINIO_KMS_SECRET_KEY adding kes would help. Thank you let me know.

@aead
Copy link
Member Author

aead commented May 8, 2024

@allanrogerr @shtripat Consider kes server --dev or see https://pkg.go.dev/github.com/minio/kes#Server
You can start a KES server like a Go HTTP server. Both are there to simplify writting tests and make them more reliable.

@shtripat
Copy link
Contributor

shtripat commented May 8, 2024

Test PR: #19695

@djwfyi djwfyi mentioned this pull request May 10, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harshavardhana harshavardhana harshavardhana approved these changes

@shtripat shtripat shtripat approved these changes

@allanrogerr allanrogerr allanrogerr approved these changes

Assignees

@aead aead

Labels
bugfix new-feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aead @harshavardhana @shtripat @allanrogerr @bh4t