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: cryptobyte: encode int64 with context-specific tags #24973

Closed
mirtchovski opened this issue Apr 20, 2018 · 5 comments
Closed

x/crypto: cryptobyte: encode int64 with context-specific tags #24973

mirtchovski opened this issue Apr 20, 2018 · 5 comments

Comments

@mirtchovski
Copy link
Contributor

@mirtchovski mirtchovski commented Apr 20, 2018

We frequently have to deal with asn.1 formatted with context-specific
tags even for things like integers, for example:

http://lapo.it/asn1js/#A0108004010203048102050682040708090A

To do that efficiently with cryptobyte I've added two additional
functions to marshal such values to and from int64:

func (b *Builder) AddInt64(tag asn1.Tag, v int64)
func (s *String) ReadASN1Int64(out *int64, tag asn1.Tag) bool

I assume versions for uint64 will also be required.

Does it make sense to add those to the cryptobyte API or is there a workaround?

@gopherbot gopherbot added this to the Unreleased milestone Apr 20, 2018
@agl
Copy link
Contributor

@agl agl commented Apr 20, 2018

These are implicitly tagged values, right? (Which is why the existing functions aren't so great?)

If so, then adding some more helper functions seems perfectly reasonable. (Most of the stuff I come across is explicitly tagged, thus I haven't hit this much.)

But I'd name the Builder method something like AddASN1Int64WithTag (all the other ASN.1 methods are AddASN1… and we already have AddASN1Int64).

Likewise with String: ReadASN1Int64WithTag (or ReadASN1IntegerWithTag if you want to mirror the existing function that handles all the different types).

@mirtchovski
Copy link
Contributor Author

@mirtchovski mirtchovski commented Apr 20, 2018

Yes, they are implicitly tagged. I'll add the changes. Do you want me to do the same for uint64?

@agl
Copy link
Contributor

@agl agl commented Apr 20, 2018

If you expect to need uint64 too, then sure.

@mirtchovski
Copy link
Contributor Author

@mirtchovski mirtchovski commented Apr 20, 2018

I will leave uints until the need arises to keep things simple.

Thanks for the quick response. I've created https://go-review.googlesource.com/c/crypto/+/108456

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 20, 2018

Change https://golang.org/cl/108456 mentions this issue: x/crypto: cryptobyte: manage integers with implicit tags

@golang golang locked and limited conversation to collaborators Apr 20, 2019
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
4 participants
You can’t perform that action at this time.