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

[theory]

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]

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]

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]

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]

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]

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.

[juergw]

I agree that we should try to avoid breaking people.

But here I really prefer to not revert this change, because it makes the keyset.Handle immutable, which is a property that I think is important to have.

I'll leave this issue open for now, to make this issue visible to other users that may run into this.