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

v2.3.0: manager.Add no longer adds to its keyset #27

Open
theory opened this issue Feb 15, 2025 · 5 comments
Open

v2.3.0: manager.Add no longer adds to its keyset #27

theory opened this issue Feb 15, 2025 · 5 comments
Assignees

Comments

@theory
Copy link

theory commented Feb 15, 2025

Describe the bug:

In v2.2.0 and earlier, after a call to manager.Add, the size of ks.KeysetInfo().GetKeyInfo() would increase. In v2.3.0, it does not (and Len() also doesn't return the proper number).

What was the expected behavior?

I would expect a key added to a keyset via manager.Add to be reflected in the number of keys in the set.

How can we reproduce the bug?

Try this code:

package main

import (
	"log"

	"github.com/tink-crypto/tink-go/v2/aead"
	"github.com/tink-crypto/tink-go/v2/keyset"
)

func main() {
	ks, err := keyset.NewHandle(aead.AES256GCMKeyTemplate())
	if err != nil {
		log.Fatal(err)
	}

	// size := ks.Len()
	size := len(ks.KeysetInfo().GetKeyInfo())
	if size != 1 {
		log.Fatalf("Expected 1 key but found %v", size)
	}

	mgr := keyset.NewManagerFromHandle(ks)
	mgr.Add(aead.AES128GCMSIVKeyTemplate())

	// size = ks.Len()
	size = len(ks.KeysetInfo().GetKeyInfo())
	if size != 2 {
		log.Fatalf("Expected 2 keys but found %v", size)
	}
	log.Println("OK")
}

With v2.3.0 the output is:

2025/02/15 13:51:14 Expected 2 keys but found 1
exit status 1

But with v2.2.0, it's:

2025/02/15 13:55:42 OK

Can you tell us more about your development environment?

go version go1.23.2 darwin/arm64

@theory theory changed the title manager.Add fails to add a key in v2.3.0 v2.3.0: manager.Add no longer adds to its keyset Feb 15, 2025
@theory
Copy link
Author

theory commented Feb 15, 2025

Updated the description, because the handle returned by manager.Handle does reflect the proper count. This works with both v2.2.0 and v2.3.0:

package main

import (
	"log"

	"github.com/tink-crypto/tink-go/v2/aead"
	"github.com/tink-crypto/tink-go/v2/keyset"
)

func try() {
	ks, err := keyset.NewHandle(aead.AES256GCMKeyTemplate())
	if err != nil {
		log.Fatal(err)
	}

	// size := ks.Len()
	size := len(ks.KeysetInfo().GetKeyInfo())
	if size != 1 {
		log.Fatalf("Expected 1 key but found %v", size)
	}

	mgr := keyset.NewManagerFromHandle(ks)
	mgr.Add(aead.AES128GCMSIVKeyTemplate())

	ks, _ = mgr.Handle()
	// size := ks.Len()
	size = len(ks.KeysetInfo().GetKeyInfo())
	if size != 2 {
		log.Fatalf("Expected 2 keys but found %v", size)
	}
	log.Println("OK")
}

The only change is the addition of this line:

	ks, _ = mgr.Handle()

@arxeiss
Copy link

arxeiss commented Feb 17, 2025

We are facing same issue. The problem is in keyset.NewManagerFromHandle because in v2.2.0 it created shallow copy, but in v2.3.0 it creates deep copy of keys. So you are adding keys to another list.

Thanks for you suggestion, however wouldn't expect such a breaking change in minor version update.

@juergw
Copy link
Contributor

juergw commented Feb 21, 2025

Thanks for reporting this. And sorry that our new version broke your code.

We are aware of this. We noticed that the manager doesn't do a deep copy of the handle it returns, and considered this a bug. In Tink, the Handle object is considered immutable. That's why all the mutation functions are in a different object called "Manager".

I think it should not be too difficult to update your code so that it works with the new implementation:, by adding some calls to mgr.Handle() as you did in the example code.

Does that work for you?

@theory
Copy link
Author

theory commented Feb 21, 2025

I can't speak for the others, but yes it worked once I figured it out. Seems fussier than I'd like but I understand the sentiment. Would be useful if the release notes mentioned the need to adapt to the change, and how.

@arxeiss
Copy link

arxeiss commented Feb 21, 2025

We are aware of this. We noticed that the manager doesn't do a deep copy of the handle it returns, and considered this a bug.

I'm not sure if this is real quote, but it circulates over internet

"If it's a bug that people rely on, it's not a bug, it's a feature." - Linus Torvalds

However, I have found, that the code which was affected was covered by tests (that's how I found the issue). But was actually dead code. So I deleted that all and was happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants