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

crypto/ecdsa: add methods to convert keys to crypto/ecdh format #56088

Closed
pedroalbanese opened this issue Oct 6, 2022 · 16 comments
Closed

crypto/ecdsa: add methods to convert keys to crypto/ecdh format #56088

pedroalbanese opened this issue Oct 6, 2022 · 16 comments
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
Milestone

Comments

@pedroalbanese
Copy link

pedroalbanese commented Oct 6, 2022

Update, Nov 9 2022: Proposal is #56088 (comment)


Greetings!

I need Privacy-Enhanced Mail (PEM) support for Curve25519 private keys, more precisely X25519, like OpenSSL. Currently Go's standard libraries only support Ed25519 private keys, not Curve25519 for this task.

Thanks in Advance!

@gopherbot gopherbot added this to the Proposal milestone Oct 6, 2022
@pedroalbanese pedroalbanese changed the title proposal: affected/package: Curve25519 proposal: affected/package: Curve25519, x509 Oct 6, 2022
@ianlancetaylor ianlancetaylor changed the title proposal: affected/package: Curve25519, x509 proposal: x/crypto: add Curve25519 Oct 7, 2022
@ianlancetaylor ianlancetaylor added the Proposal-Crypto Proposal related to crypto packages or other security issues label Oct 7, 2022
@ianlancetaylor
Copy link
Contributor

CC @golang/security

@seankhliao
Copy link
Member

doesn't this already exist https://pkg.go.dev/golang.org/x/crypto/curve25519#X25519

@pedroalbanese
Copy link
Author

pedroalbanese commented Oct 7, 2022

Hi,

This already exists, but x509 library doesn't support it, so it's not possible to convert private keys to PEM format, more precisely the MarshalPKCS8PrivateKey and MarshalECPrivateKey functions (I don't know which one will be used in this case) doesn't recognize the OID of Curve25519 private keys, I remember being able to convert the public key, but private key is not supported.

package main

import (
	"crypto/x509"
	"encoding/pem"
	"log"
)

func main() {

	pKey := `-----BEGIN PRIVATE KEY-----
MCowBQYDK2VuAyEAfLLsWKkI/7EmTOkSf4fyHuRHDnKk6qNncWDzV8jlIUU=
-----END PRIVATE KEY-----`

	block, _ := pem.Decode([]byte(pKey))

	if block == nil || block.Type != "PRIVATE KEY" {
		log.Fatal("failed to decode PEM block containing private key")

	}

	key, err := x509.ParsePKCS8PrivateKey(block.Bytes)
	if err != nil {
		log.Println("Parse PKI Error:", err)
		return
	}

	log.Println(key)
}

2009/11/10 23:00:00 Parse PKI Error: asn1: structure error: tags don't match (2 vs {class:0 tag:16 length:5 isCompound:true}) {optional:false explicit:false application:false private:false defaultValue:<nil> tag:<nil> stringType:0 timeType:0 set:false omitEmpty:false} int @2

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

What are the API changes or implementation changes you are proposing in the x509 package?

/cc @golang/security

@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@pedroalbanese
Copy link
Author

pedroalbanese commented Oct 12, 2022

API changes or implementation changes I'm proposing in the x509 package:

(I don't understand if this question is for me)

x509.MarshalPKCS8PrivateKey() and x509.ParsePKCS8PrivateKey().

I'm afraid the OID of algorithm need to be included.

@FiloSottile
Copy link
Contributor

MarshalECPrivateKey is hardcoded to deal with crypto/ecdsa keys, and Curve25519 is not compatible with that package, or ever used with ECDSA.

MarshalPKCS8PrivateKey and maybe MarshalPKIXPublicKey are more flexible. What we need to support a new key type is a discrete OID to tell the key apart, and a Go type to marshal/parse from/into. RFC 8410, Section 3 gives us dedicated OIDs for Curve25519 keys, and we now have crypto/ecdh.PrivateKey and PublicKey for X25519 keys, so this should be easily doable.

Unfortunately, we can't do the same for crypto/ecdh keys based on NIST curves, because those use the same OIDs as the ECDSA keys on the same curves, regrettably. We could make MarshalPKCS8PrivateKey and MarshalPKIXPublicKey support crypto/ecdh keys, but then the Parse counterparts would produce crypto/ecdsa keys, instead of round-tripping cleanly. This was discussed in #52221 (comment). Maybe that's acceptable if crypto/ecdsa keys have methods to produce crypto/ecdh keys. Opinions?

@FiloSottile FiloSottile changed the title proposal: x/crypto: add Curve25519 proposal: crypto/x509: support marshaling X25519 (and other ECDH) keys Oct 19, 2022
@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

Maybe that's acceptable if crypto/ecdsa keys have methods to produce crypto/ecdh keys.

That seems like a reasonable path forward. Any objections to doing that?

@rsc
Copy link
Contributor

rsc commented Nov 2, 2022

OK, it sounds like maybe there are no objections. ecdh does not import ecdsa, so we should be able to add methods in ecdsa. I guess it would be

package ecdsa

func (k *PrivateKey) ECDH() *ecdh.PrivateKey
func (k *PublicKey) ECDH() *ecdh.PublicKey

Do I have that right?

@rsc rsc changed the title proposal: crypto/x509: support marshaling X25519 (and other ECDH) keys proposal: crypto/ecdsa: add methods to convert keys to crypto/ecdh format Nov 2, 2022
@rsc
Copy link
Contributor

rsc commented Nov 9, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@gopherbot
Copy link

Change https://go.dev/cl/450815 mentions this issue: crypto/x509: add support for PKCS8/PKIX X25519 key encodings

@FiloSottile
Copy link
Contributor

I ended up adding an error return, since crypto/ecdsa keys can be invalid and crypto/ecdh can't.

package ecdsa

func (k *PrivateKey) ECDH() (*ecdh.PrivateKey, error)
func (k *PublicKey) ECDH() (*ecdh.PublicKey, error)

gopherbot pushed a commit that referenced this issue Nov 16, 2022
This specifically doesn't add support for X25519 certificates.
Refactored parsePublicKey not to depend on the public PublicKeyAlgorithm
values, and ParseCertificate/ParseCertificateRequest to ignore keys that
don't have a PublicKeyAlgorithm even if parsePublicKey supports them.

Updates #56088

Change-Id: I2274deadfe9bb592e3547c0d4d48166de1006df0
Reviewed-on: https://go-review.googlesource.com/c/go/+/450815
Reviewed-by: Roland Shoemaker <roland@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Joedian Reid <joedian@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

Error returns makes sense to me.

@gopherbot
Copy link

Change https://go.dev/cl/450816 mentions this issue: crypto/ecdsa,crypto/x509: add encoding paths for NIST crypto/ecdh keys

@rsc
Copy link
Contributor

rsc commented Nov 16, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: crypto/ecdsa: add methods to convert keys to crypto/ecdh format crypto/ecdsa: add methods to convert keys to crypto/ecdh format Nov 16, 2022
@rsc rsc modified the milestones: Proposal, Backlog Nov 16, 2022
@pedroalbanese
Copy link
Author

Thank You very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-FinalCommentPeriod
Projects
None yet
Development

No branches or pull requests

7 participants