golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.97k stars 17.67k forks source link

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

Closed pedroalbanese closed 1 year ago

pedroalbanese commented 2 years ago

Update, Nov 9 2022: Proposal is https://github.com/golang/go/issues/56088#issuecomment-1300947275


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!

ianlancetaylor commented 2 years ago

CC @golang/security

seankhliao commented 2 years ago

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

pedroalbanese commented 2 years ago

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 commented 2 years ago

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

/cc @golang/security

rsc commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/golang/go/issues/52221#issuecomment-1119203506. Maybe that's acceptable if crypto/ecdsa keys have methods to produce crypto/ecdh keys. Opinions?

rsc commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

gopherbot commented 1 year ago

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

FiloSottile commented 1 year ago

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)
rsc commented 1 year ago

Error returns makes sense to me.

gopherbot commented 1 year ago

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

rsc commented 1 year ago

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

pedroalbanese commented 1 year ago

Thank You very much!