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: add Raw field to ocsp.Response type #38340

Open
nathanejohnson opened this issue Apr 9, 2020 · 7 comments
Open

x/crypto: add Raw field to ocsp.Response type #38340

nathanejohnson opened this issue Apr 9, 2020 · 7 comments

Comments

@nathanejohnson
Copy link

@nathanejohnson nathanejohnson commented Apr 9, 2020

I would love to see a Marshal method to the ocsp.Response type. I have created a PR here to illustrate how this might look:

golang/crypto#115

I'd love some feedback!

@gopherbot gopherbot added this to the Unreleased milestone Apr 9, 2020
@andybons andybons changed the title x/crypto: Proposal: add Marshal method to ocsp.Response type proposal: x/crypto: add Marshal method to ocsp.Response type Apr 10, 2020
@gopherbot gopherbot added the Proposal label Apr 10, 2020
@andybons
Copy link
Member

@andybons andybons commented Apr 10, 2020

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Apr 14, 2020

Hello! What do you need Marshal for, as opposed to CreateResponse? If you were to change anything in the Response the Signature would become invalid, and so would the output of Marshal.

Request.Marshal makes more sense because requests are not signed and because CreateRequest is more of a high-level helper, while CreateResponse takes a Response as a template.

@nathanejohnson
Copy link
Author

@nathanejohnson nathanejohnson commented Apr 14, 2020

@FiloSottile In my case, Marshal is still useful because there is currently no other easy way to convert an ocsp.Reponse to its DER form. For my use case I'm not generating the responses, nor modifying them, but fetching them from upstream responders. And after a point, I will need to renew the response. So I can either store it in DER form and parse it every time I want to inspect the NextUpdate etc, store two representations of it (Response and []byte), or have a Marshal method. Your point about modifications invalidating the signature is of course correct, however the Marshal() method uses the TBSResponseData for most of it anyway. To that point, improving the comments above the method would probably be a good idea.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Apr 14, 2020

Would a Raw field work as well, like in crypto/x509.Certificate?

@nathanejohnson
Copy link
Author

@nathanejohnson nathanejohnson commented Apr 14, 2020

@FiloSottile Actually yes

@nathanejohnson
Copy link
Author

@nathanejohnson nathanejohnson commented Apr 14, 2020

@FiloSottile would you like me to try that approach with this PR, or should it be separate?

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Apr 14, 2020

Feel free to adapt the current PR.

This is small enough that I'll be bold and approve the proposal directly.

@FiloSottile FiloSottile changed the title proposal: x/crypto: add Marshal method to ocsp.Response type proposal: x/crypto: add Raw field to ocsp.Response type Apr 14, 2020
@gopherbot gopherbot added the Proposal label Apr 14, 2020
@FiloSottile FiloSottile changed the title proposal: x/crypto: add Raw field to ocsp.Response type x/crypto: add Raw field to ocsp.Response type Apr 14, 2020
@FiloSottile FiloSottile removed the Proposal label Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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