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

Create a new, more flexible CryptoProvider API and Injection point #1254

Open
dmihalcik-virtru opened this issue Aug 5, 2024 · 6 comments
Open

Comments

@dmihalcik-virtru
Copy link
Member

dmihalcik-virtru commented Aug 5, 2024

The current cryptoProvider API looks like this:

OLD

type CryptoProvider interface {
	// Gets some KID associated with a given algorithm.
	// Returns empty string if none are found.
	FindKID(alg string) string
	RSAPublicKey(keyID string) (string, error)
	RSAPublicKeyAsJSON(keyID string) (string, error)
	RSADecrypt(hash crypto.Hash, keyID string, keyLabel string, ciphertext []byte) ([]byte, error)

	ECPublicKey(keyID string) (string, error)
	ECCertificate(keyID string) (string, error)
	GenerateNanoTDFSymmetricKey(kasKID string, ephemeralPublicKeyBytes []byte, curve elliptic.Curve) ([]byte, error)
	GenerateEphemeralKasKeys() (any, []byte, error)
	GenerateNanoTDFSessionKey(privateKeyHandle any, ephemeralPublicKey []byte) ([]byte, error)
	Close()
}

We should use a new, simplified crypto provider API that provides clear functional boundaries and extension points.

NEW

type KeyIdentifier string;
type Algorithm string;
type KeyFormat string;
 
// CryptoProviders implement KAS key unwrap functionality.
// These include:
//   - Presenting stable public keys for wrapping client encryption keys.
//   - Backward compatibility
//   - Key agreement for nanoTDF and other EC based solutions
// This may be Closeable
type CryptoProvider interface {
	// Return current preferred key identifier(s) for wrapping with the given algorithm. 
	CurrentKID(alg Algorithm) []KeyIdentifier, err

	// Return one or more 'legacy' key identifiers that can be used when no KID is presented 
	// [optional]
	CurrentKID(a Algorithm) []string, err

	LegacyKIDs(a Algorithm) []string, err

	// Returns a public key or cert for the given key ID and algorithm in the given format.
	PublicKey(a Algorithm, k KeyIdentifier, f KeyFormat) (string, error)

	// Perform an unwrap using the given alg/keyid pair on the given wrapped key bytes
	Unwrap(a Algorithm, k KeyIdentifier, ciphertext []byte) ([]byte, error)

	// Derive a shared key. Note: alg includes curve if present?
	Derive(a Algorithm, k KeyIdentifier, ephemeralPublicKeyBytes []byte) ([]byte, error)
}

// Optional type to implement closeable crypto providers
// Probably not needed? But maybe we should do this anyway?
type Closeable interface {
	Close()
}

Do we need key generation? Arguably this should be under the hood and part of initialization/setup.

See also: #1049

@pflynn-virtru
Copy link
Member

pflynn-virtru commented Aug 8, 2024

I have had some issues with changing the implementation of this OLD interface. This NEW interface would have been nice to have. I approve

related like implementation: https://github.com/opentdf/platform/pull/1213/files#diff-42a6b8bf0a5ad498dd5d9f49458e53bf11ac3a6aaf3fa9c03e1e9b3c918bbe4b

@pflynn-virtru
Copy link
Member

Also, I would like this to be an exposed, non-internal interface

@mkleene
Copy link
Contributor

mkleene commented Oct 31, 2024

I like it! Do you think that we could get by without a symmetric key derivation and maybe bury that in Unwrap?

Also, is it possible to not have clients specifying the Algorithm and have that implied by the key?

I don't know the system well enough to know if this is the way that this should work.

@dmihalcik-virtru
Copy link
Member Author

Yeah, it may make sense to hide derive since that would allow us the freedom to implement a 'fully hsm' solution where the derived key is only exported in an encapsulation, if that makes sense from a security/cost/feature perspective

@jrschumacher
Copy link
Member

@dmihalcik-virtru is this work complete?

@dmihalcik-virtru
Copy link
Member Author

I've moved tracking of this to jira; I'll probably need to update the proposed API to enable ECEIS encryption for ztdf, as well

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

No branches or pull requests

4 participants