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

Transit byok import endpoints #15414

Merged
merged 16 commits into from
May 16, 2022
Merged

Transit byok import endpoints #15414

merged 16 commits into from
May 16, 2022

Conversation

schultz-is
Copy link
Contributor

Summary

This adds two new endpoints to the Transit backend that enable BYOK (bring your own key):

  • keys/:name/import, which imports an external key into a new Transit key for use within Vault.
  • keys/:name/import_version, which imports an external key into a new version of an existing Transit key for use within Vault.

The intended use cases for Transit BYOK are seamless migration to Vault from legacy HSM/KMS solutions and import of externally-generated shared keys into an organization that already uses Transit. Since the security of imported keys cannot be guaranteed, and because the import process is complicated, this feature is not recommended for use without a full understanding of security implications.

@schultz-is schultz-is added this to the 1.11.0-rc1 milestone May 13, 2022
@schultz-is schultz-is requested a review from a team May 13, 2022 15:34
@schultz-is schultz-is force-pushed the transit-byok-import-endpoints branch from a82449d to 6633769 Compare May 13, 2022 18:07
Copy link
Collaborator

@sgmiller sgmiller left a comment

Choose a reason for hiding this comment

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

Pretty good overall, just a few comments. Appreciate the thorough test coverage.

builtin/logical/transit/path_import.go Show resolved Hide resolved
builtin/logical/transit/path_import.go Outdated Show resolved Hide resolved
builtin/logical/transit/path_import.go Show resolved Hide resolved
builtin/logical/transit/path_import.go Outdated Show resolved Hide resolved
builtin/logical/transit/path_import.go Show resolved Hide resolved
builtin/logical/transit/path_import.go Show resolved Hide resolved
sdk/helper/keysutil/policy.go Show resolved Hide resolved
rculpepper and others added 15 commits May 13, 2022 14:16
…nvert Transit wrapping key endpoint to use shared wrapping key retrieval method. Disallow import of convergent keys to Transit via BYOK process.
…specify OAEP random oracle hash function used to wrap ephemeral AES key.
… panic in Transit import. Proactively zero out ephemeral AES key used in Transit imports.
…ral key is of the size specified byt the RFC.
…n to avoid errors on BYOK keys with allow_rotation=false.
…d Transit import unit tests. Added unit tests for Transit import_version endpoint.
… but reject with an error when the field is provided.
@schultz-is schultz-is force-pushed the transit-byok-import-endpoints branch from 6633769 to d8d310a Compare May 13, 2022 19:16
@schultz-is schultz-is requested a review from sgmiller May 16, 2022 15:13
@schultz-is schultz-is merged commit 063ca95 into main May 16, 2022
@schultz-is schultz-is deleted the transit-byok-import-endpoints branch May 16, 2022 16:50
Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

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

Sorry late to the review party, a few comments but overall 👍

AllowImportedKeyRotation: allowRotation,
}

switch keyType {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since we test within parseHashFn in a case-insensitive fashion, should we do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might as well!

lm.cache.Store(req.Name, p)
}

lock.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this have been written as a deferred statement after the lock.Lock() on line 466? If we error out we aren't releasing the lock correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, good catch. If this errored, it'd deadlock that policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants