Navigation Menu

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

Jump to bottom

crypto/tls: X509KeyPair cannot parse OpenSSL generated ECDSA keys #23591

Closed
chandra-ghub opened this issue Jan 28, 2018 · 5 comments
Closed

crypto/tls: X509KeyPair cannot parse OpenSSL generated ECDSA keys #23591

chandra-ghub opened this issue Jan 28, 2018 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.11

Comments

@chandra-ghub
Copy link

Please answer these questions before submitting your issue. Thanks!

I am trying to get go's standard tls package accept my openssl generated keys (Prime 256 curve ECDSA). Generated key and cert using OpenSSL -

# openssl ecparam -name prime256v1 -genkey -noout -out priv.pem -param_enc explicit
# openssl req -new -x509 -key priv.pem -out EC_server.pem -days 365

What version of Go are you using (go version)?

Tested with 1.9 and on play.golang.org too.
https://play.golang.org/p/V9GzXsn1zoG (copied the function X509KeyPair() to be able to debug)
https://play.golang.org/p/22G1XNbU8at (direct call to the library)

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

MACOS High Sierra

What did you do?

here you go -
https://play.golang.org/p/V9GzXsn1zoG with the library copied to debug the error
https://play.golang.org/p/22G1XNbU8at (without go's library code)

What did you expect to see?

A &tls.Certificate{} to have returned.

What did you see instead?

failed to parse private key error from private function parsePrivateKey() in tls.go library.
@odeke-em odeke-em changed the title tls.X509KeyPair() does not parse OpenSSL generated ECDSA keys crypto/tls: x509KeyPair cannot parse OpenSSL generated ECDSA keys Jan 28, 2018
@odeke-em
Copy link
Member

Thank for reporting this issue @chandra-ghub.

Seems like the certificate centric version of this bug is #21502, but am not sure.

I'll tag some crypto folks @agl @titanous @FiloSottile

@odeke-em odeke-em changed the title crypto/tls: x509KeyPair cannot parse OpenSSL generated ECDSA keys crypto/tls: X509KeyPair cannot parse OpenSSL generated ECDSA keys Jan 28, 2018
@bradfitz bradfitz added this to the Go1.11 milestone Jan 28, 2018
@bradfitz bradfitz added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 28, 2018
@agl
Copy link
Contributor

agl commented Jan 28, 2018

Drop this: -param_enc explicit

It causes OpenSSL to encode P-256 as if it were a random, arbitrary curve rather than specifying P-256 itself. The default (named_curve) is correct.

(For more details, see the OpenSSL documentation.)

@FiloSottile
Copy link
Contributor

As #21502 (comment) says, -param_enc explicit is not legal for TLS certificates anyway.

I looked into making the error message more useful like in #21502 (which is indeed the same issue but with certificates) but parsePrivateKey does trial decryption of 3 formats, so we would have to guess at which is an error and which is a mismatch. I don't love that.

What we can do is move certificate parsing, which has more useful error messages, above private key parsing in X509KeyPair. I'll CL that.

@gopherbot
Copy link

Change https://golang.org/cl/90435 mentions this issue: crypto/tls: parse certificate first in X509KeyPair to get better errors

@chandra-ghub
Copy link
Author

chandra-ghub commented Jan 29, 2018 •
edited

thanks @agl and @FiloSottile for your time. Can confirm that dropping -param_enc explicit from openssl's flags does the trick - https://play.golang.org/p/MS_FQ8cqqA8

Appreciate your acute diligence on the error messages, makes life so much easier for tls users going forward.

gopherbot pushed a commit that referenced this issue Mar 27, 2018
@FiloSottile
crypto/tls: parse certificate first in X509KeyPair to get better errors
bd18c09 
parsePrivateKey can't return useful error messages because it does trial
decoding of multiple formats.  Try ParseCertificate first in case it
offers a useful error message.

Fixes #23591

Change-Id: I380490a5850bee593a7d2f584a27b2a14153d768
Reviewed-on: https://go-review.googlesource.com/90435
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
FiloSottile added a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@FiloSottile
crypto/tls: parse certificate first in X509KeyPair to get better errors
3ec0b31 
parsePrivateKey can't return useful error messages because it does trial
decoding of multiple formats.  Try ParseCertificate first in case it
offers a useful error message.

Fixes golang#23591

Change-Id: I380490a5850bee593a7d2f584a27b2a14153d768
Reviewed-on: https://go-review.googlesource.com/90435
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
FiloSottile added a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@FiloSottile
crypto/tls: parse certificate first in X509KeyPair to get better errors
96364fa 
parsePrivateKey can't return useful error messages because it does trial
decoding of multiple formats.  Try ParseCertificate first in case it
offers a useful error message.

Fixes golang#23591

Change-Id: I380490a5850bee593a7d2f584a27b2a14153d768
Reviewed-on: https://go-review.googlesource.com/90435
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
@golang golang locked and limited conversation to collaborators Jan 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
6 participants
@bradfitz @agl @FiloSottile @odeke-em @gopherbot @chandra-ghub