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

Use recommended crypto library to create ECC seed #480

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

Conversation

yawangwang
Copy link
Collaborator

Per discussions from #471 (comment), remove curve P224 as it is not supported by crypto/ecdh

@yawangwang
Copy link
Collaborator Author

/gcbrun

Copy link
Contributor

@alexmwu alexmwu left a comment

Choose a reason for hiding this comment

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

I am not an expert here, so I'm not sure when someone would want to pick a given curve. However, we should not remove support for the deprecated library given the GCP vTPM supports P224 unless there's a good reason besides the deprecated library.

sudo tpm2_getcap ecc-curves
TPM2_ECC_NIST_P256: 0x3
TPM2_ECC_NIST_P521: 0x5
TPM2_ECC_NIST_P384: 0x4
TPM2_ECC_NIST_P224: 0x2

@@ -131,25 +132,45 @@ func createECCSeed(ek tpm2.Public) (seed, encryptedSeed []byte, err error) {
if err != nil {
return nil, nil, err
}
priv, x, y, err := elliptic.GenerateKey(curve, rand.Reader)

ecdsaPriv, err := ecdsa.GenerateKey(curve, rand.Reader)
Copy link
Contributor

Choose a reason for hiding this comment

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

We're doing ecdh here so you should use the utilities in crypto/ecdh instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need EC points {x, y} to compute ECC seed in the following operations. Using crypto/ecdh utilities is an ideal way, however they don't provide a method to expose the EC points {x, y} in public key.

crypto/ecdsa provides a method to do that, also there are methods which convert each key type to their counterparts in the the ecdh package. See line 141 that converts an ecdsa private key to its corresponding ecdh private key.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. LGTM

@yawangwang
Copy link
Collaborator Author

I am not an expert here, so I'm not sure when someone would want to pick a given curve. However, we should not remove support for the deprecated library given the GCP vTPM supports P224 unless there's a good reason besides the deprecated library.

sudo tpm2_getcap ecc-curves
TPM2_ECC_NIST_P256: 0x3
TPM2_ECC_NIST_P521: 0x5
TPM2_ECC_NIST_P384: 0x4
TPM2_ECC_NIST_P224: 0x2

I agree to keep the deprecated library till P224 is supported by crypto/ecdh. There is a proposal golang/go#59783 that seeks for this support. So I'll leave this PR open for now.

@yawangwang yawangwang changed the title Remove curve P224 support for creating ECC seed Use recommended crypto library to create ECC seed Aug 23, 2024
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.

2 participants