code review 7501043: x509: expose uniformResourceIdentifier [6] in SubjectAl...

x509: expose uniformResourceIdentifier [6] in SubjectAltName extension.

[jonasp, 2013-03-05 23:42:14]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[shanemhansen, 2013-03-06 06:02:09]

Can you add a test to this? Also, for those of us less familiar with x509
v3 extensions,
a pointer to where URIs are defined for subject alt names would be
appreciated.

On Tue, Mar 5, 2013 at 4:42 PM, <jonas.p@gmail.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> x509: expose uniformResourceIdentifier [6] in SubjectAltName extension.
>
> Please review this at
https://codereview.appspot.**com/7501043/<https://codereview.appspot.com/7501...
>
> Affected files:
>   M src/pkg/crypto/x509/x509.go
>
>
> Index: src/pkg/crypto/x509/x509.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/x509/x509.go
> +++ b/src/pkg/crypto/x509/x509.go
> @@ -466,6 +466,7 @@
>         // Subject Alternate Name values
>         DNSNames       []string
>         EmailAddresses []string
> +       URIs           []string
>         IPAddresses    []net.IP
>
>         // Name constraints
> @@ -844,6 +845,9 @@
>                                         case 2:
>                                                 out.DNSNames =
> append(out.DNSNames, string(v.Bytes))
>                                                 parsedName = true
> +                                       case 6:
> +                                               out.URIs =
> append(out.URIs, string(v.Bytes))
> +                                               parsedName = true
>                                         case 7:
>                                                 switch len(v.Bytes) {
>                                                 case net.IPv4len,
> net.IPv6len:
> @@ -1095,7 +1099,7 @@
>                 n++
>         }
>
> -       if len(template.DNSNames) > 0 || len(template.EmailAddresses) > 0
> || len(template.IPAddresses) > 0 {
> +       if len(template.DNSNames) > 0 || len(template.EmailAddresses) > 0
> || len(template.URIs) > 0 || len(template.IPAddresses) > 0 {
>                 ret[n].Id = oidExtensionSubjectAltName
>                 var rawValues []asn1.RawValue
>                 for _, name := range template.DNSNames {
> @@ -1104,6 +1108,9 @@
>                 for _, email := range template.EmailAddresses {
>                         rawValues = append(rawValues, asn1.RawValue{Tag:
> 1, Class: 2, Bytes: []byte(email)})
>                 }
> +               for _, uri := range template.URIs {
> +                       rawValues = append(rawValues, asn1.RawValue{Tag:
> 6, Class: 2, Bytes: []byte(uri)})
> +               }
>                 for _, rawIP := range template.IPAddresses {
>                         // If possible, we always want to encode IPv4
> addresses in 4 bytes.
>                         ip := rawIP.To4()
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[remyoudompheng, 2013-03-06 07:04:58]

On 2013/3/6  <jonas.p@gmail.com> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> x509: expose uniformResourceIdentifier [6] in SubjectAltName extension.
>
> Please review this at https://codereview.appspot.com/7501043/

Is this fixing an issue marked for Go 1.1 (and which one) ? This an
API change, if no issue exists yet for this you should create one.

Rémy.

[jonasp, 2013-03-06 07:49:31]

> Can you add a test to this? Also, for those of us less familiar with x509
v3 extensions,
> a pointer to where URIs are defined for subject alt names would be
appreciated.

I extended x509_test.go to test for entries in URIs. The possibility of
uris is defined in RFC 5280, 4.2.1.6. There already is a comment in the
code (x509.go:807+) referencing this RFC and uris are listed as well.
Therefore I didn't add any further comments.

> Is this fixing an issue marked for Go 1.1 (and which one) ? This an
> API change, if no issue exists yet for this you should create one.

No, at the moment there is no issue for this. As this is more of a feature
than an issue, is the golang policy to open an issue for these things as
well? If that is the case I'll be happy to do so. First time submission,
sorry :)

As a sidenote: The API was changed recently anyway by

changeset:   15750:e1a94ec9f285
user:        Adam Langley <agl@golang.org>
date:        Fri Feb 15 10:40:17 2013 -0500
summary:     crypto/x509: support IP SANs.

which introduced IPAddresses handling of SubjectAltNames.

[bradfitz, 2013-03-06 08:06:21]

Btw, per the Go 1.1 freeze (
https://groups.google.com/forum/?fromgroups=#!topic/golang-dev/IY5JQ1nt2Zo)
this might be just too late, even though the code looks fine.  If agl is
cool with it and it gets a test, though, it could possibly go in.

On Tue, Mar 5, 2013 at 11:49 PM, Jonas Pollok <jonas.p@gmail.com> wrote:

> > Can you add a test to this? Also, for those of us less familiar with
> x509 v3 extensions,
> > a pointer to where URIs are defined for subject alt names would be
> appreciated.
>
> I extended x509_test.go to test for entries in URIs. The possibility of
> uris is defined in RFC 5280, 4.2.1.6. There already is a comment in the
> code (x509.go:807+) referencing this RFC and uris are listed as well.
> Therefore I didn't add any further comments.
>
> > Is this fixing an issue marked for Go 1.1 (and which one) ? This an
> > API change, if no issue exists yet for this you should create one.
>
> No, at the moment there is no issue for this. As this is more of a feature
> than an issue, is the golang policy to open an issue for these things as
> well? If that is the case I'll be happy to do so. First time submission,
> sorry :)
>
> As a sidenote: The API was changed recently anyway by
>
> changeset:   15750:e1a94ec9f285
> user:        Adam Langley <agl@golang.org>
> date:        Fri Feb 15 10:40:17 2013 -0500
> summary:     crypto/x509: support IP SANs.
>
> which introduced IPAddresses handling of SubjectAltNames.
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>

[rsc, 2013-03-06 20:17:00]

Please postpone this CL until after Go 1.1 is out unless this is needed to
fix a Go 1.1 bug. Even though it's small, we're having trouble keeping up
with the small extra pieces that keep coming in. We had to draw a line.

Thanks.

[agl2, 2013-03-06 20:27:05]

On Wed, Mar 6, 2013 at 3:16 PM, Russ Cox <rsc@golang.org> wrote:
> Please postpone this CL until after Go 1.1 is out unless this is needed to
> fix a Go 1.1 bug. Even though it's small, we're having trouble keeping up
> with the small extra pieces that keep coming in. We had to draw a line.

This needs a strong motivation also if its to go in at any point. What
uses URIs in X.509 and it is common enough to support?


Cheers

AGL

[jonasp, 2013-03-07 11:24:23]

> Please postpone this CL until after Go 1.1 is out unless this is needed
to fix a Go 1.1 bug.

Sure, no problem. What would be the standard procedure to do that?

> This needs a strong motivation also if its to go in at any point. What
> uses URIs in X.509 and it is common enough to support?

The reason for me to implement this was to allow WebID authentication over
TLS which is a mechanism relying on linked data and a self signed
certificate containing an URI in the subjectAltName extension, see
http://www.w3.org/2005/Incubator/webid/spec/ . I understand that GO can not
provide code for every authentication/validation mechanism in the world.
This is actually why I wanted to expose data present in the certificate to
code running outside of x509.go.

[bradfitz, 2013-03-07 16:40:37]

On Thu, Mar 7, 2013 at 3:24 AM, Jonas Pollok <jonas.p@gmail.com> wrote:

> > Please postpone this CL until after Go 1.1 is out unless this is needed
> to fix a Go 1.1 bug.
>
> Sure, no problem. What would be the standard procedure to do that?
>

Wait for Go 1.1 to come out, then send us a "ping" email.  (we don't have
multiple branches at this point... too much overhead. go1.1 will get its
own maintenance branch later)  You should also file a bug to track it,
referencing the patch.

[bradfitz, 2013-06-17 18:21:14]

R=agl

What is the status of this? Jonas, do you still need this?

Go 1.1 is out now, so this can be discussed now.

[jonasp, 2013-06-22 10:00:51]

Yes I would still like the certificate URI to be exposed in some way.
I filed a bug under https://code.google.com/p/go/issues/detail?id=5752

[rsc, 2013-12-18 20:18:52]

R=close

That bug has been marked fixed.