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

proposal: x/crypto/ocsp: support request and response extensions #20001

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

proposal: x/crypto/ocsp: support request and response extensions #20001

wumb0 opened this issue Apr 16, 2017 · 8 comments
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@wumb0
Copy link

wumb0 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
Copy link
Member

/cc @agl

@agl agl self-assigned this Apr 17, 2017
@agl
Copy link
Contributor

agl 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
Copy link
Author

wumb0 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
Copy link
Author

wumb0 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
Copy link
Member

/cc @FiloSottile

@andybons andybons added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 21, 2018
@rsc rsc unassigned agl Jun 23, 2022
@gopherbot
Copy link

Change https://go.dev/cl/540735 mentions this issue: ocsp: support for request and response extensions

@maraino
Copy link

maraino commented Nov 8, 2023

I've submitted a PR that adds the support without creating new functions:
https://go-review.googlesource.com/c/crypto/+/540735

@FiloSottile FiloSottile changed the title x/crypto/ocsp: request and response extensions are not supported proposal: x/crypto/ocsp: support request and response extensions Nov 9, 2023
@FiloSottile
Copy link
Contributor

CL 540735 adds the following new fields to Request and Response.

// Request represents an OCSP request. See RFC 6960.
type Request struct {
	// ...
	Extensions     []pkix.Extension
}

// Response represents an OCSP response containing a single SingleResponse. See
// RFC 6960.
type Response struct {
	// ...

	// ResponseExtensions contains raw X.509 extensions from the
	// responseExtensions field of the OCSP response. When marshaling OCSP
	// responses, the ResponseExtensions field is ignored, see
	// ResponseExtraExtensions.
	ResponseExtensions []pkix.Extension

	// ResponseExtraExtensions contains extensions to be copied, raw, into any
	// marshaled OCSP response (in the responseExtensions field). Values
	// override any extensions that would otherwise be produced based on the
	// other fields. The ResponseExtraExtensions field is not populated when
	// parsing certificates, see ResponseExtensions.
	ResponseExtraExtensions []pkix.Extension
}

/cc @golang/proposal-review @golang/security

@FiloSottile FiloSottile modified the milestones: Unreleased, Proposal Nov 9, 2023
@FiloSottile FiloSottile added Proposal-Crypto Proposal related to crypto packages or other security issues and removed NeedsFix The path to resolution is known, but the work has not been done. labels Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
Status: Incoming
Development

No branches or pull requests

7 participants