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: breaking api change in cryptobyte String.ReadASN1Integer since v0.4.0 leads to panic #63987

Open
shomron opened this issue Nov 7, 2023 · 1 comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@shomron
Copy link

shomron commented Nov 7, 2023

What version of Go are you using (go version)?

$ go version
go version go1.21.3 darwin/arm64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
...
GOARCH='arm64'
GOOS='darwin'
...

What did you do?

golang.org/x/crypto 0.4.0 introduced a breaking API change in the AddASN1Int64() method.
Previously, reflection was used to determine the underlying type of the provided out argument.
As of golang/crypto@2c47667 - it instead uses type assertions.

This is a breaking change - while previously one could pass named types whose underlying types are integers,
that same code as of x/crypto >=0.4.0 will panic.

Reproduction:

https://go.dev/play/p/1__wSXq30mR

...

package main

import (
	"testing"

	"golang.org/x/crypto/cryptobyte"
)

type MyInt int64

func Test_ReadASN1Int64(t *testing.T) {
	const expectedVal = 1234

	b := cryptobyte.NewBuilder(nil)
	b.AddASN1Int64(expectedVal)
	buf, err := b.Bytes()
	if err != nil {
		t.Fatalf("failed serializing to buffer: %v", err)
	}

	var out MyInt // underlying type is int64

	t.Logf("deserializing type: %T", out)
	input := cryptobyte.String(buf)
	if !input.ReadASN1Integer(&out) {
		t.Errorf("deserializing integer failed")
	}
}

What did you expect to see?

I expected code similar to the reproduction above to continue functioning with newer versions of x/crypto.
If this was not feasible, I would have at a minimum expected a release note describing the breaking change,
or to see it reflected in semantic versioning of the library.

What did you see instead?

Working code began to panic upon upgrading x/crypto >= 0.4.0.

@gopherbot gopherbot added this to the Unreleased milestone Nov 7, 2023
@mauri870 mauri870 added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 7, 2023
@bcmills
Copy link
Contributor

bcmills commented Nov 7, 2023

(attn @FiloSottile @rolandshoemaker)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants