Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(779)

Issue 12056043: code review 12056043: crypto/x509: expose arbitary X.509 extensions. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 8 months ago by agl1
Modified:
10 years, 7 months ago
Reviewers:
r
CC:
golang-dev, r, jpsugar
Visibility:
Public.

Description

crypto/x509: expose arbitary X.509 extensions. This change allows people who want to parse or set odd X.509 extensions to do so without having to add support for them all to the package. I tried to make it so that only a single member: Extensions would be needed. However, that would mean detecting when the caller had altered the contents of it so that parsing and marshaling a certificate wouldn't ignore all changes to the other members. This ended up being messy, thus the current design where there are two members: one for reading and another for writing. As crypto/x509 adds support for more extensions in the future, the raw extensions will still be in Extensions for older code that expects it there. Also, future extensions will be overridden by any raw extensions added to ExtraExtensions by code that was written before support was added.

Patch Set 1 #

Patch Set 2 : diff -r fb4878194fc7 https://code.google.com/p/go/ #

Patch Set 3 : diff -r 2e89feb1297b https://code.google.com/p/go/ #

Patch Set 4 : diff -r 2e89feb1297b https://code.google.com/p/go/ #

Total comments: 4

Patch Set 5 : diff -r 1ee3ab13e3b6 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -11 lines) Patch
M src/pkg/crypto/x509/x509.go View 1 2 3 4 13 chunks +43 lines, -11 lines 0 comments Download
M src/pkg/crypto/x509/x509_test.go View 1 2 4 chunks +27 lines, -0 lines 0 comments Download

Messages

Total messages: 7
agl1
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
10 years, 7 months ago (2013-08-28 22:03:08 UTC) #1
jpsugar
Why not put unrecognized extensions into ExtraExtensions so that marshal is a proper inverse of ...
10 years, 7 months ago (2013-08-28 22:12:03 UTC) #2
agl1
On Wed, Aug 28, 2013 at 6:12 PM, <jpsugar@google.com> wrote: > Why not put unrecognized ...
10 years, 7 months ago (2013-08-28 22:19:21 UTC) #3
jpsugar
On Wed, Aug 28, 2013 at 3:18 PM, Adam Langley <agl@golang.org> wrote: > ExtraExtensions overrides ...
10 years, 7 months ago (2013-08-28 22:26:45 UTC) #4
r
LGTM this is an API change, though, so please make sure to send a CL ...
10 years, 7 months ago (2013-08-28 22:27:15 UTC) #5
agl1
*** Submitted as https://code.google.com/p/go/source/detail?r=564ce4b5cc75 *** crypto/x509: expose arbitary X.509 extensions. This change allows people who ...
10 years, 7 months ago (2013-08-30 14:15:12 UTC) #6
agl1
10 years, 7 months ago (2013-08-30 14:18:07 UTC) #7
On Wed, Aug 28, 2013 at 6:26 PM, JP Sugarbroad <jpsugar@google.com> wrote:
> I was thinking it might put any extension not represented elsewhere
> into ExtraExtensions. That way there's no overlap by default. That
> might perhaps be more complex than needs to happen here -- how often
> do people do parse/modify/marshal?

I thought about this but didn't do it in the end.

I don't think parsing and reserialising certificates is something that
crypto/x509 is suited for. APIs like OpenSSL support it, but it means
always carrying the full complexity of X.509 in order that nothing
gets lost in translation and crypto/x509 very much tries to throw out
some obscure cases.

It would also lead to problems in the future: if support for an
extension is added to crypto/x509 then the duplication of that
extension goes from being byte-exact to probably not so. So I fear
that it would cause issues in the future.


Cheers

AGL
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b