golang / go

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

proposal: crypto/x509: ability to add custom curve when parsing X509 certificate #32874

Open trung opened 5 years ago

trung commented 5 years ago

Per https://github.com/golang/go/issues/26776, using a third party library for custom curve is advised.

However, when parsing x509 certificate (x509.ParseCetificate()), it is not possible to supply custom curve.

My proposal is to offer a configuration that can be used to supply a function to return elliptic.Curve from asn1.ObjectIdentifier to complement the default namedCurveFromIOD()

trung commented 5 years ago

From the current flow: https://github.com/golang/go/blob/fbde753a58e286c405a04388816ed044486151bb/src/crypto/x509/x509.go#L519-L553

I think of injecting custom functions something like:

type OIDToCurveFunc func(oid asn1.ObjectIdentifier) elliptic.Curve
type CurveToOIDFunc func(curve elliptic.Curve) (asn1.ObjectIdentifier, bool)

var (
    CustomIOIDToCurveFunc OIDToCurveFunc
    CustomCurveToOIDFunc CurveToOIDFunc
)

func namedCurveFromOID(oid asn1.ObjectIdentifier) elliptic.Curve {
    switch {
    case oid.Equal(oidNamedCurveP224):
        return elliptic.P224()
    case oid.Equal(oidNamedCurveP256):
        return elliptic.P256()
    case oid.Equal(oidNamedCurveP384):
        return elliptic.P384()
    case oid.Equal(oidNamedCurveP521):
        return elliptic.P521()
    }
    if CustomIOIDToCurveFunc != nil {
        return CustomIOIDToCurveFunc(oid)
    }
    return nil
}

func oidFromNamedCurve(curve elliptic.Curve) (asn1.ObjectIdentifier, bool) {
    switch curve {
    case elliptic.P224():
        return oidNamedCurveP224, true
    case elliptic.P256():
        return oidNamedCurveP256, true
    case elliptic.P384():
        return oidNamedCurveP384, true
    case elliptic.P521():
        return oidNamedCurveP521, true
    }
    if CustomCurveToOIDFunc != nil {
        return CustomCurveToOIDFunc(curve)
    }
    return nil, false
}
rsc commented 5 years ago

Globals are probably not the right answer here. Perhaps there should be an x509.Parser that can be configured with extra hooks, a bit like there is an xml.Decoder to configure the XML parser.

Or perhaps unrecognized curves should be exposed in some general way and client code can just post-process.

/cc @FiloSottile

trung commented 5 years ago

Agreed with globals. There might be breaking changes once introducing x509.Parser as x509.ParseCertifcate() is used in number of places.

FiloSottile commented 4 years ago

crypto/x509 primarily serves the WebPKI, and there is no use of custom curves there. X.509 is a sprawling standard, so it's critical for the crypto/x509 package to stay focused on its use case.

I will keep this open to collect feedback about use cases, as things of course change over time, but we are not going to support this for now.

Joh4nnesHartl commented 2 years ago

I stumble more & more on brainpool curves (https://datatracker.ietf.org/doc/html/rfc5639). It makes it impossible to parse certificates with this standard. In germany, a lot of big tech-companies are using these curves. It would be really great to make an Parser configurable / make 2 new functiona which take an callback for deserialization & serialization. I don't see that big of an issue in providing this functionallity, it should not break any existant implementations.

Joh4nnesHartl commented 1 year ago

Is there any update on this, especially when 1.20 will deprecate custom curve implementations? Still no way to provide a workaround?

cc @FiloSottile

ZackaryWelch commented 1 year ago

Strange to exclude over half of supported openssl curves. Running into this myself and I am having to inject the four Brainpool OIDs and their curves from another library. It'd be fairly trivial to support these, either the regular or both regular and twisted curves.

eriklupander commented 5 months ago

Any news on this issue?

I would like to support certificates signed with ECDSA keys using brainpool curves in the application I'm working on. I'd prefer to do that without relying on OpenSSL, or by parsing the cert bytes myself in order to get hold of the artifacts needed to use a 3rd party module with brainpool support.

Would really appreciate support for custom curves.

jhollmannk commented 4 months ago

We have the same problem here with the brainpool curves. Yes, it's a german thing but unfortunately we have no choice here as we just have to support this.

Does anyone have an example how to even do this? I'm at a loss. Do I have to replace the x509 package (or the methods I need to read a certificate) and do the same with the tls package when it comes to a TLS handshake?

@ZackaryWelch mentioned that he has to inject the brainpoool curves. I would like to know how and where to inject them.

kostas-zealid commented 4 months ago

We are interested in this as well. ICAO 9303 (which defines eMRTDs including NFC data structure and signatures inside all worldwide travel documents incl. passwords and ID cards) mandates custom curves with explicit parameters as part of Document Signing Certificate inside eMRTD NFC chips.

I wonder what is the best way forward for us given that we'd like to consider the option of validating these crypto signatures using native Go crypto.

MrWong99 commented 4 months ago

+1 for custom curves or atleast brainpool... Otherwise I will be just copy/pasting the x509 package and adding my required OIDs...

kwe commented 3 months ago

Just wondering if anyone has found a solution to the issue with eMRTDs and using the native Go crypto?

Joh4nnesHartl commented 3 months ago

Just wondering if anyone has found a solution to the issue with eMRTDs and using the native Go crypto?

Fork the existing go-crypto and add your OIDs, the curves used also need to implement the elliptic.Curve interface.

The only functions I had to adjust were namedCurveFromOID and oidFromNamedCurve inside x509/x509.go

for future versions you could also prepare a patch file for these changes.

Lazybin commented 2 months ago

Other languages can parse these curves, but Go can't. It's really uncomfortable to use it now.

jhollmannk commented 2 months ago

I would like to point out that if go wanted to also support brainpool elliptic curves in a certificate (you can generate one with openssl), the tls module would also need to be updated. Modifying x509 only suffices if you use the curves to manually generate a hash or check a signature or something like that.

septs commented 2 months ago

I really need brainpool elliptic curves support now.

ZackaryWelch commented 2 weeks ago

This is how I had done it in the past, editing x509.go to include the curves

  # Inject brainpool OIDs to x509.go
WORKDIR /usr/local/go/src/crypto/x509
RUN sed -i -e 's/{1, 3, 132, 0, 35}/{1, 3, 132, 0, 35}\noidNamedCurveP224Brainpool = asn1.ObjectIdentifier{1,3,36,3,3,2,8,1,1,5}\n\toidNamedCurveP256Brainpool = asn1.ObjectIdentifier{1,3,36,3,3,2,8,1,1,7}\n\toidNamedCurveP384Brainpool = asn1.ObjectIdentifier{1,3,36,3,3,2,8,1,1,11}\n\toidNamedCurveP512Brainpool = asn1.ObjectIdentifier{1,3,36,3,3,2,8,1,1,13}\n/g' x509.go && \

  sed -i -e 's/return elliptic.P521()/return elliptic.P521()\ncase oid.Equal(oidNamedCurveP224Brainpool):\n\treturn brainpool.P224r1()\n\tcase oid.Equal(oidNamedCurveP256Brainpool):\n\treturn brainpool.P256r1()\n\tcase oid.Equal(oidNamedCurveP384Brainpool):\n\treturn brainpool.P384r1()\n\tcase oid.Equal(oidNamedCurveP512Brainpool):\n\treturn brainpool.P512r1()\n/g' x509.go && \

  sed -i -e 's|\"crypto/sha512\"|"crypto/sha512"\n  brainpool '\"github.com/go-compile/rome/brainpool/bcurves\"' \n|g' x509.go && \
  go get github.com/go-compile/rome && go mod tidy

Then build your source against this version of Go, also adding the brainpool/bcurves library to your module.

Joh4nnesHartl commented 2 weeks ago

with the new rewrite of the x509 package in go1.23 I think this opens more possibilities.