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

x/crypto/ocsp: request and response extensions are not supported #20001

Open
wumb0 opened this issue Apr 16, 2017 · 5 comments

Comments

@wumb0
Copy link

commented Apr 16, 2017

Go Version: go version go1.7 darwin/amd64
GOARCH="amd64"
GOOS="darwin"

I was writing code dealing with OCSP and found that extensions inside the tbsRequest are not supported in the current x/crypto/ocsp code. This is required to implement the nonce extension common to OCSP requests (openssl ocsp uses it by default). RFC 6960 defines that the TBSRequest should support extensions in section 4.1.1.
The RFC additionally defines the ResponseData structure to have responseExtensions which is also missing from the go OCSP code. This is presented in section 4.2.1.

I have already written code to address this issue but it would break existing code that uses the library. If this issue is accepted I will change it up so it doesn't do that and submit a Gerrit review request for it.

That code can be found here
UPDATE: a gerrit review of the actual code is here
The changes of note are on lines 95, 126, 414 in the ParseRequest function, and 702 in the CreateResponse function.

My proposed fix would be to move the ParseRequest code to a new function ParseRequestWithExtensions, which would look similar to my posted implementation of ParseRequest. ParseRequest would call ParseRequestWithExtensions and throw out the extensions keeping the current functionality. It would also add response extensions to the response via the response template passed into the CreateResponse function (line 702).

Please comment with any requests for clarification or if I am out of line on wanting this to go into master :)

@gopherbot gopherbot added this to the Unreleased milestone Apr 16, 2017

@odeke-em odeke-em changed the title x/crypto/ocsp does not support request and response extensions x/crypto/ocsp: request and response extensions are not supported Apr 17, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

/cc @agl

@agl agl self-assigned this Apr 17, 2017

@agl

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2017

OCSP nonces are unused in practice in the WebPKI, as far as I know. They require online signing of OCSP responses so I don't see that changing.

Even if that were not the case, why not add elements to the Request structure rather than change the function signature?

@wumb0

This comment has been minimized.

Copy link
Author

commented Apr 17, 2017

I had only implemented it since openssl was complaining by default and it sticks to the RFC. Openssl stopped complaining when I implemented them even though I don't sign responses.
My proposed addition (different than the example code) would modify the request structure, but also add another function for those that wanted to get extensions back from a function call for further processing.
Modifying the Request structure doesn't make sense because these extensions are in the tbsRequest.
I see your point about nonces not being necessary, though. I don't know where this protocol will be going in the future so it may be helpful to have access to those extensions at some point.

@wumb0

This comment has been minimized.

Copy link
Author

commented Mar 21, 2018

An update to this issue, I have submitted a review for it: https://go-review.googlesource.com/c/crypto/+/101915
The code has been modified to not break existing implementations but to add a function that will return request extensions with the parsed request and also enable a developer to add response extensions to their response via template.

@andybons

This comment has been minimized.

Copy link
Member

commented Mar 21, 2018

@andybons andybons added the NeedsFix label Mar 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.