FAQ
On 2012/10/29 13:46:37, agl1 wrote:

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go
File src/pkg/crypto/x509/sec1.go (right):

https://codereview.appspot.com/6767045/diff/8001/src/pkg/crypto/x509/sec1.go#newcode32
src/pkg/crypto/x509/sec1.go:32: func ParseSEC1PrivateKey(der []byte) (key
*ecdsa.PrivateKey, err error) {
On 2012/10/25 15:20:58, jsing wrote:
On 2012/10/25 14:19:03, agl1 wrote:
Where is this format from? It's odd to have two ASN.1 values
concatenated,
rather than in a SEQUENCE and SEC1 C.2 doesn't seem to specify
this format.
Agreed. It appears that 'openssl ecparam' writes out an
ECDomainParameters
immediately followed by the ECPrivateKey structure. This is similar
to what it
does in the PEM case, where there are two PEM blocks in the same
file (the
first
an 'EC PARAMS' block, the second an 'EC PRIVATE KEY' block). In the
case of a
named curve, the ASN.1 can simply be an OID. I'm not entirely sure
how we
should
handle this...
I don't think that we should handle it. I think it's basically a
problem with
OpenSSL. It makes sense in the PEM case, but it's nonsense for DER.
Apologies for the delay in getting back to this.

Generally speaking, I agree with your sentiment. OpenSSL seems to take
the approach that EC keys can be generated with non-named parameters,
which means that the curve parameters need to be included separately.
This is obvious in the PEM case where there are two PEM blocks, however
their DER output also includes two ASN.1 values (this occurs even in the
named curve case). If we choose not support this then we will not be
able to load DER EC keys as currently generated by OpenSSL.
If we eliminate this method, we can remove the current
ParseECPrivateKey wrapper
too, right?
Yes.

https://codereview.appspot.com/6767045/

Search Discussions

  • Agl at Nov 13, 2012 at 3:43 pm
    https://codereview.appspot.com/6767045/diff/11001/src/pkg/crypto/x509/sec1.go
    File src/pkg/crypto/x509/sec1.go (right):

    https://codereview.appspot.com/6767045/diff/11001/src/pkg/crypto/x509/sec1.go#newcode41
    src/pkg/crypto/x509/sec1.go:41: func ParseECPrivateKey(der []byte) (key
    *ecdsa.PrivateKey, err error) {
    I thought this was going away?

    https://codereview.appspot.com/6767045/
  • Joel Sing at Nov 13, 2012 at 3:57 pm

    On 14 November 2012 02:43, wrote:
    https://codereview.appspot.com/6767045/diff/11001/src/pkg/crypto/x509/sec1.go
    File src/pkg/crypto/x509/sec1.go (right):

    https://codereview.appspot.com/6767045/diff/11001/src/pkg/crypto/x509/sec1.go#newcode41
    src/pkg/crypto/x509/sec1.go:41: func ParseECPrivateKey(der []byte) (key
    *ecdsa.PrivateKey, err error) {
    I thought this was going away?
    If the decision is to not to support DER generated via OpenSSL ecparam
    then we can drop ParseSEC1PrivateKey. The ParseECPrivateKey() function
    is still needed for PEM EC PRIVATE KEY blocks, with parseECPrivateKey
    being necessary to handle PKCS8 encapsulated EC keys.
  • Adam Langley at Nov 13, 2012 at 4:35 pm

    On Tue, Nov 13, 2012 at 10:57 AM, Joel Sing wrote:
    If the decision is to not to support DER generated via OpenSSL ecparam
    then we can drop ParseSEC1PrivateKey. The ParseECPrivateKey() function
    is still needed for PEM EC PRIVATE KEY blocks, with parseECPrivateKey
    being necessary to handle PKCS8 encapsulated EC keys.
    I'm pretty certain that the DER output of ecparam is a bug. It makes
    some sense in PEM format because they are two separate PEM blocks, but
    ramming two DER streams together is completely non-standard.


    Cheers

    AGL
  • Jsing at Nov 14, 2012 at 10:39 am

    On 2012/11/13 16:35:12, agl1 wrote:
    On Tue, Nov 13, 2012 at 10:57 AM, Joel Sing
    wrote:
    If the decision is to not to support DER generated via OpenSSL
    ecparam
    then we can drop ParseSEC1PrivateKey. The ParseECPrivateKey()
    function
    is still needed for PEM EC PRIVATE KEY blocks, with
    parseECPrivateKey
    being necessary to handle PKCS8 encapsulated EC keys.
    I'm pretty certain that the DER output of ecparam is a bug. It makes
    some sense in PEM format because they are two separate PEM blocks, but
    ramming two DER streams together is completely non-standard.
    Agreed. I've removed ParseSEC1PrivateKey() and the associated test.

    PTAL.

    https://codereview.appspot.com/6767045/
  • Agl at Nov 14, 2012 at 4:27 pm
    LGTM.

    (Let me know if you need me to land it in the tree.)

    https://codereview.appspot.com/6767045/
  • Jsing at Nov 14, 2012 at 4:41 pm
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=15a03e58cd8d ***

    crypto/x509: add support for SEC1/EC private keys.

    Add support for parsing SEC1 EC private keys and PKCS8 encapsulated
    EC private key structures.

    R=agl
    CC=golang-dev
    http://codereview.appspot.com/6767045


    http://codereview.appspot.com/6767045/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 13, '12 at 2:03p
activeNov 14, '12 at 4:41p
posts7
users2
websitegolang.org

2 users in discussion

Jsing: 4 posts Agl: 3 posts

People

Translate

site design / logo © 2022 Grokbase