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

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

Open
trung opened this issue Jul 1, 2019 · 15 comments
Open
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues Proposal-Hold
Milestone

Comments

@trung
Copy link
Contributor

trung commented Jul 1, 2019

Per #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 trung changed the title cyrpto/x509: ability to add custom curve when parsing X509 certificate crypto/x509: ability to add custom curve when parsing X509 certificate Jul 1, 2019
@trung trung changed the title crypto/x509: ability to add custom curve when parsing X509 certificate proposal: crypto/x509: ability to add custom curve when parsing X509 certificate Jul 1, 2019
@gopherbot gopherbot added this to the Proposal milestone Jul 1, 2019
@trung
Copy link
Contributor Author

trung commented Jul 2, 2019

From the current flow:

go/src/crypto/x509/x509.go

Lines 519 to 553 in fbde753

var (
oidNamedCurveP224 = asn1.ObjectIdentifier{1, 3, 132, 0, 33}
oidNamedCurveP256 = asn1.ObjectIdentifier{1, 2, 840, 10045, 3, 1, 7}
oidNamedCurveP384 = asn1.ObjectIdentifier{1, 3, 132, 0, 34}
oidNamedCurveP521 = asn1.ObjectIdentifier{1, 3, 132, 0, 35}
)
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()
}
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
}
return nil, false
}

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
Copy link
Contributor

rsc commented Jul 2, 2019

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

@rsc rsc added the Proposal-Crypto Proposal related to crypto packages or other security issues label Jul 2, 2019
@trung
Copy link
Contributor Author

trung commented Jul 3, 2019

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

@FiloSottile
Copy link
Contributor

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
Copy link

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
Copy link

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
Copy link

ZackaryWelch commented Apr 13, 2023

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
Copy link

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
Copy link

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
Copy link

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
Copy link

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

@kwe
Copy link

kwe commented Mar 26, 2024

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

@Joh4nnesHartl
Copy link

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
Copy link

Lazybin commented Apr 19, 2024

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

@jhollmannk
Copy link

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.

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

No branches or pull requests