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

Implement KMS AWS master key loading #552

Merged
merged 6 commits into from Jul 13, 2022

Conversation

Zhaars
Copy link
Contributor

@Zhaars Zhaars commented Jul 5, 2022

Implementation of KMS AWS MasterKey Loading.

Checklist

Draft implementation of KMS MasterKey Loading
Fixed cli flags descriptions
@Zhaars Zhaars requested a review from Lagovas July 5, 2022 21:30
keystore/kms/aws/encryptor.go Outdated Show resolved Hide resolved
}

var encryptor kms.Encryptor
switch keyID.Prefix() {
Copy link
Collaborator

@Lagovas Lagovas Jul 5, 2022

Choose a reason for hiding this comment

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

hm, it push me to think about registries. what about to use here registries approach as we did it for CryptoHandlers? Turn implementation on with build tag and register it at start? for example:

type EncryptorFactoryMethod interface {
  New(credentialPath string) (kms.Encryptor, error)
}
// or type EncryptorFactoryMethod func(credentialPath string) (kms.Encryptor, error)
var lock = sync.Lock{}
var encryptors map[string]EncryptorFactoryMethod
RegisterEncryptor(encryptor EncryptorFactoryMethod){
  lock.Lock()
  encryptors[encryptor.ID()] = encryptor
  lock.Unlock()
}

Then we can use :

func NewLoader(credentialPath, keyIdentifierURI string) (*Loader, error) {
	keyID, err := kms.NewKeyIdentifierFromURI(keyIdentifierURI)
	if err != nil {
		return nil, err
	}
        encryptorFactory, ok := encryptors[keyID]
        if !ok {
            log.Errorln("Unknown key ID")
        }
        encryptor, err := encryptorFactory.New(credentialPath) // or encryptorFactory(credentialPath)
}

Then our AWS implementation should have:

func init(){
  kms.RegisterEncryptor(someFactoryFunction)
}

With such approach we can extend with enterprise KMS support.

Additionally, we can make default loader that do nothing with master key (current approach where env variable store result master key. this implementation will return master key as is in Encrypt/Decrypt methods) and registered by default and +1 with AWS. Then current loader will be consistent with new approach

And +1 pros for that it be able to cut AWS support and all dependencies via build tags even in CE version. If client doesn't need AWS KMS support, it can compile it with build tag that turns off it. Result binary size and amount of dependencies will be less.

Added integrations tests to cover KMS functionality
@Zhaars Zhaars marked this pull request as ready for review July 8, 2022 07:15
@Zhaars Zhaars requested a review from Lagovas July 8, 2022 07:15
keystore/kms/aws/encryptor.go Outdated Show resolved Hide resolved
keystore/kms/base.go Show resolved Hide resolved
tests/test.py Outdated Show resolved Hide resolved
@Zhaars Zhaars requested a review from Lagovas July 8, 2022 16:16
Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

lgtm

Refactor AWS KMS MasterKey Loader to decrypt with alias
@Zhaars Zhaars requested a review from Lagovas July 12, 2022 08:38
keystore/keyloader/kms/kms_loader.go Outdated Show resolved Hide resolved
keystore/kms/aws/encryptor.go Show resolved Hide resolved
Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

lgtm

@Zhaars Zhaars merged commit 1072de8 into master Jul 13, 2022
@Lagovas Lagovas deleted the zhars/implement_kms_aws_master_key_loading branch July 13, 2022 15:47
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.

None yet

2 participants