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

encoding/asn1: Empty integer checks fail on certain OpenSSL CSRs #15373

Closed
rolandshoemaker opened this issue Apr 19, 2016 · 6 comments
Closed

encoding/asn1: Empty integer checks fail on certain OpenSSL CSRs #15373

rolandshoemaker opened this issue Apr 19, 2016 · 6 comments
Assignees
Milestone

Comments

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Apr 19, 2016

  1. What version of Go are you using (go version)?
    go1.6
  2. What operating system and processor architecture are you using (go env)?
    Ubuntu 15.10 x86_64
  3. What did you do?
    Build a CSR using pre-1.0.2 OpenSSL language bindings and attempt to parse it using x509.ParseCertificateRequest (Gist including short generator).
  4. What did you expect to see?
    No error
  5. What did you see instead?
    asn1: structure error: empty integer is returned.

(extra) 6. Cause
The pre-1.0.2 OpenSSL API (at least using the Python bindings) requires that the CSR version field be explicitly set otherwise it will not be populated (but the field will be included) causing the empty integer error.

The Let's Encrypt team ran into this when attempting to migrate from 1.5 -> 1.6 and while we've been able to update our client code to solve the issue there are still a relatively large number being produced by users with old or different clients (~1.4% of CSRs we currently receive each week have this issue).

Removing the empty integer check, along the lines of the negative serial check, wouldn't be a great idea, since this is extremely useful elsewhere. One solution might be to add a workaround ASN1 tag, brokenInt or something, which knows how to parse an int in this situation (i.e. if empty just use 0).

cc @bradfitz @agl

@agl agl self-assigned this Apr 19, 2016
@agl
Copy link
Contributor

@agl agl commented Apr 19, 2016

Can you provide the CSR?

@rolandshoemaker
Copy link
Member Author

@rolandshoemaker rolandshoemaker commented Apr 19, 2016

-----BEGIN CERTIFICATE REQUEST-----
MIICWjCCAUICADAWMRQwEgYDVQQDEwtleGFtcGxlLmNvbTCCASIwDQYJKoZIhvcN
AQEBBQADggEPADCCAQoCggEBAMpwCSKfLhKC3SnvLNpVayAEyAHVixkusgProAPZ
RBH0VAog/r4JOfoJez7ABiZ2ZIXXA2gg65/05HkGNl9ww+sa0EY8eCty/8WcHxqz
afUnyXOJZuLMPJjaJ2oiBv/3BM7PZgpFzyNZ0/0ZuRKdFGtEY+vX9GXZUV0A3sxZ
MOpce0lhHAiBk/vNARJyM2+O+cZ7WjzZ7R1T9myAyxtsFhWy3QYvIwiKVVF3lDp3
KXlPZ/7wBhVIBcVSk0bzhseotyUnKg+aL5qZIeB1ci7IT5qA/6C1/bsCSJSbQ5gn
QwIQ0iaUV/SgUBpKNqYbmnSdZmDxvvW8FzhuL6JSDLfBR2kCAwEAAaAAMA0GCSqG
SIb3DQEBCwUAA4IBAQBxxkchTXfjv07aSWU9brHnRziNYOLvsSNiOWmWLNlZg9LK
dBy6j1xwM8IQRCfTOVSkbuxVV+kU5p+Cg9UF/UGoerl3j8SiupurTovK9+L/PdX0
wTKbK9xkh7OUq88jp32Rw0eAT87gODJRD+M1NXlTvm+j896e60hUmL+DIe3iPbFl
8auUS+KROAWjci+LJZYVdomm9Iw47E+zr4Hg27EdZhvCZvSyPMK8ioys9mNg5Tth
HB6ExepKP1YW3HpQa1EdUVYWGEvyVL4upQZOxuEA1WJqHv6iVDzsQqkl5kkahK87
NKTPS59k1TFetjw2GLnQ09+g/L7kT8dpq3Bk5WoB
-----END CERTIFICATE REQUEST-----
@rolandshoemaker rolandshoemaker changed the title encoding/asn1: Empty integers are encoding/asn1: Empty integer checks fail on certain OpenSSL CSRs Apr 19, 2016
@rolandshoemaker
Copy link
Member Author

@rolandshoemaker rolandshoemaker commented Apr 19, 2016

It seems like I forgot to actually finish writing the title of this issue :|

@bradfitz bradfitz added this to the Go1.7Maybe milestone Apr 19, 2016
@rsc
Copy link
Contributor

@rsc rsc commented May 17, 2016

It's probably too late for Go 1.7 at this point. I'd like to see it fixed but since Go 1.6 also had the problem it won't make things significantly worse to leave it in Go 1.7.

I am assuming Let's Encrypt has already forked their own copy of ParseCertificateRequest (and asn1) in order to make forward progress. All the fields in x509.CertificateRequest are public so it looks like it should be possible to create them with custom code.

@rsc rsc modified the milestones: Go1.8, Go1.7Maybe May 17, 2016
@agl
Copy link
Contributor

@agl agl commented May 17, 2016

I think the bar for adding workarounds to the base library should be very high. It's unfortunate that we didn't catch this in earlier Go versions and so these Let's Encrypt clients have managed to survive. But I don't like the idea of cementing in the expectation that an empty value for the version is ok here.

@rolandshoemaker
Copy link
Member Author

@rolandshoemaker rolandshoemaker commented May 17, 2016

Thinking more about this I agree with @agl that adding a exception to this is, in the long term, the wrong solution.

Let's Encrypt is able to work around the problem relatively easily, and is happy enough to wait until the broken client population is small enough to just switch to throwing an error on the empty value as is the current behavior.

@agl agl closed this May 17, 2016
@golang golang locked and limited conversation to collaborators May 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.