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 Removed APIs Break my App #26

Open
theory opened this issue Jan 15, 2025 · 17 comments
Open

v2.3.0 Removed APIs Break my App #26

theory opened this issue Jan 15, 2025 · 17 comments
Assignees

Comments

@theory
Copy link

theory commented Jan 15, 2025

From the v2.3.0 release notes:

Removed primitiveset.PrimitiveSet, keyset.Primitives and
keyset.PrimitivesWithKeyManager from the public API. This API was never
really intended to be used outside Tink. We are making changes to Tink
internals and we prefer to break users at compile time. If this affects you,
please file an issue.

I have an app that relies on primitiveset.PrimitiveSet and keyset.PrimitivesWithKeyManager. I suspect many implementing their own keys might have a challenge, but because my app creates a key that simply encapsulates an AEAD key and a MAC key they're fairly essential.

keyset.PrimitivesWithKeyManager Usage

As described in #14 and #16, my app generates custom key info on output. It depends on keyset.PrimitivesWithKeyManager to get at the key info it needs, similar to this (error handling omitted for clarity):

ps, err := handle.PrimitivesWithKeyManager(&infoManager{})
keyInfo := make([]*mypb.KeyInfo, len(ps.EntriesInKeysetOrder))

for i, e := range ps.EntriesInKeysetOrder {
	params, ok := (e.Primitive).(*mypb.MyParams)

	keyInfo[i] = &mypb.KeyInfo{
		TypeUri:     e.TypeURL,
		KeyId:       e.KeyID,
		AeadScheme:  params.GetAeadScheme(),
		MacScheme:   params.GetMacScheme(),
	}
}

primitiveset.PrimitiveSet Usage

My app's custom keys are a fairly straightforward bundling of an AEAD and MAC key as a single notional "key". It provides an interface that includes the functionality of both AEAD and MAC keys. To do so, the implementation simply wraps the underlying primitiveset.PrimitiveSet to get at the key material required for its functionality, more or less like so (error handling omitted for clarity):

type wrappedMyKey struct {
	ps *primitiveset.PrimitiveSet
}

func New(handle *keyset.Handle) (MyKey, error) {
	ps, err := handle.Primitives()
	// elided code that uses ps.Primary.Primitive to verify that all keys in the handle are MyKeys
	return &wrappedMyKey{ps: ps}, nil
}

func (wh *wrappedMyKey) Encrypt(plaintext, associatedData []byte) ([]byte, error) {
	primary := wh.ps.Primary
	p, ok := (primary.Primitive).(MyKey)

	ct, err := p.Encrypt(plaintext, associatedData)
	output := make([]byte, 0, len(primary.Prefix)+len(ct))
	output = append(output, primary.Prefix...)
	output = append(output, ct...)

	return output, nil
}

func (wh *wrappedMyKey) ComputeMAC(data []byte) ([]byte, error) {
	primary := wh.ps.Primary
	primitive, ok := (primary.Primitive).(MyKey)
	mac, err := primitive.ComputeMAC(data)
	return mac, nil
}

There's are also Decrypt method, naturally. Hopefully this makes it fairly clear what my use case is.

@morambro
Copy link
Contributor

morambro commented Jan 15, 2025

Hi @theory,

keyset.PrimitivesWithKeyManager Usage

For tink.AEAD primitives, it is possible to get individual keyset.Entrys for existing AEAD primitives as follows (omitting error handling):

ks := // some keyset
for i := 0; i < ks.Len(); i++ {
  e, err := ks.Entry(i);
  status := e.KeyStatus()
  keyID := e.KeyID()
  switch actualKey := k.(type) {
    case *aesgcm.Key:
      params := actualKey.Parameters().(*aesgcm.Parameters)
      // Can have access to the paramters, such as key size, IV size and TAG size.
    case *aesgcmsiv.Key: ...
  }
}

We are planning to add similar types for other primitives as well. Would that help?

primitiveset.PrimitiveSet Usage

Can these be two different (existing) keys or do they have to be one key? An option could be registering a key manager that creates MyKey using registry.RegisterKeyManager(...) and use it as an AEAD or MAC when aead.New(kh) or mac.New(kh) are needed. Would that work?

@morambro morambro self-assigned this Jan 15, 2025
@theory
Copy link
Author

theory commented Jan 15, 2025

Hi @morambro

We are planning to add similar types for other primitives as well. Would that help?

Yes, I believe so, as long as it iterates over all keys, not just active keys. I assume it's a pretty straightforward interface to implement, yes?

Can these be two different (existing) keys or do they have to be one key?

Not really; they're tightly linked in my app. It's fine that they're internally two keys; like I said, MyKey is really just a wrapper around the other two keys. Makes thing nice and tidy for users.

The photo is pretty simple:

message MyKey {
  uint32 version = 1;
  // Tracks the AEAD and Mac schemes used by this key.
  HarkParams params = 2;
  // Stores the AEAD key material and configuration.
  google.crypto.tink.KeyData aead_key_data = 3;
  // Stores the MAC key material and configuration.
  google.crypto.tink.KeyData mac_key_data = 4;
}

An option could be registering a key manager that creates MyKey using registry.RegisterKeyManager(...) and use it as an AEAD or MAC when aead.New(kh) or mac.New(kh) are needed. Would that work?

That's essentially how it's implemented now, though it uses templates, so relies on registry.GetKeyManager(template.GetTypeUrl()) to get the manager for each key (AEAD or Mac), then mgr.NewKeyData(template.GetValue()) to create each key.

But because I implemented MyKey using all of Tinks' patterns, including protobuf, the implementation of the methods that actually do encryption, decryption, and hashing needs to get at the underlying primitive set. I assume that anyone implementing their own key would need to so, as well.

@morambro
Copy link
Contributor

morambro commented Feb 5, 2025

Apologies for the delay.

But because I implemented MyKey using all of Tinks' patterns, including protobuf, the implementation of the methods that actually do encryption, decryption, and hashing needs to get at the underlying primitive set. I assume that anyone implementing their own key would need to so, as well.

Not necessarily, I think one would need this only to implement a new primitive (i.e., they need a <primitive>.New(kh) function). I tried the following, please let me know if this is close enough to your use case:

  • mykey.proto
message MyKeyFormat {
  // ...
}

message MyKey {
  uint32 version = 1;
  // ...
  // Stores the AEAD key material and configuration.
  google.crypto.tink.KeyData aead_key_data = 3;
  // Stores the MAC key material and configuration.
  google.crypto.tink.KeyData mac_key_data = 4;
}
  • mykey_key_manager.go (skipping details/error handling):
const typeURL  = "type.googleapis.com/google.crypto.tink.MyKey"

// ...

type keyManager struct{}

var _ registry.KeyManager = (*keyManager)(nil)

type mykeyPrimitive struct {
	// ...
}

var _ tink.AEAD = (*mykeyPrimitive)(nil)
var _ tink.MAC = (*mykeyPrimitive)(nil)

func (p *mykeyPrimitive) Encrypt(plaintext, associatedData []byte) ([]byte, error) { ... }

func (p *mykeyPrimitive) Decrypt(ciphertext, associatedData []byte) ([]byte, error) { ... }

func (p *mykeyPrimitive) ComputeMAC(data []byte) ([]byte, error) { ... }

func (p *mykeyPrimitive) VerifyMAC(mac, data []byte) error { ... }

func (km *keyManager) Primitive(serializedKey []byte) (any, error) {
	myKey := new(mykeypb.MyKey)
	err := proto.Unmarshal(serializedKey, myKey)
	if err != nil {
		return nil, err
	}
        // Marshall AEAD and MAC keys, get the key material, etc.
	ret := &mykeyPrimitive{
		// pass appropriate key material...
	}
	return ret, nil
}

func (km *keyManager) NewKey(serializedKeyFormat []byte) (proto.Message, error) {
	keyFormat := new(mykeypb.MyKeyFormat)
	if err := proto.Unmarshal(serializedKeyFormat, keyFormat); err != nil {
                  // err handling...
	}
	// ...create AEAD and MAC keys, and serialize them...
	k := &mykeypb.MyKey{}
	k.SetAeadKeyData(&tinkpb.KeyData{
		TypeUrl:         macURL,
		Value:             aeadKeyBytes,
		KeyMaterialType: tinkpb.KeyData_SYMMETRIC,
	})
	k.SetMacKeyData(&tinkpb.KeyData{
		TypeUrl:         aeadURL,
		Value:             macKeyBytes,
		KeyMaterialType: tinkpb.KeyData_SYMMETRIC,
	})
	return k, nil
}

func (km *keyManager) NewKeyData(serializedKeyFormat []byte) (*tinkpb.KeyData, error) {
	key, err := km.NewKey(serializedKeyFormat)
        // err handling...
	serializedKey, err := proto.Marshal(key)
        // err handling...
	return &tinkpb.KeyData{
		TypeUrl:         typeURL,
		Value:             serializedKey,
		KeyMaterialType: km.KeyMaterialType(),
	}, nil
}

func (km *keyManager) DoesSupport(typeURL string) bool { return typeURL == km.TypeURL() }

func (km *keyManager) TypeURL() string { return typeURL }

func (km *keyManager) KeyMaterialType() tinkpb.KeyData_KeyMaterialType {
	return tinkpb.KeyData_SYMMETRIC
}
  • In a test:
	km := &keyManager{}
	if err := registry.RegisterKeyManager(km); err != nil {
		t.Fatalf("Failed to register key manager: %v", err)
	}

	kh, err := keyset.NewHandle(createMyKeyTemplate(..., tinkpb.OutputPrefixType_TINK))
	if err != nil {
		t.Fatalf("Failed to create keyset handle: %v", err)
	}

	-, err := aead.New(kh)
	if err != nil {
		t.Fatalf("Failed to create AEAD: %v", err)
	}
  	_, err := mac.New(kh)
	if err != nil {
		t.Fatalf("Failed to create MAC: %v", err)
	}

The produced ciphertext and MAC will use the same output prefix.

@theory
Copy link
Author

theory commented Feb 12, 2025

That just might work. I'm trying it now, but one thing I didn't mention is that I have a method that calculates MACs for all of the keys in a Mac keyset, not just the primary key. For that, I've been using PrimitiveSet.EntriesInKeysetOrder. I presume I'll need to use Len() and Entry(), but until the Mac package supports that interface there is no way to iterate over all the keys aside from using the PrimitiveSet, yes?

@theory
Copy link
Author

theory commented Feb 12, 2025

but until the Mac package supports that interface there is no way to iterate over all the keys aside from using the PrimitiveSet, yes?

Sorry, that just applies to the need for key.Parameters and key.Key definitions for MAC keys; doesn't apply to using the keys.

In the meantime, I think I've confirmed that your proposal will work by mimicking the new 2.3.0 methods like so:

type mimicHandle struct {
	*keyset.Handle
}

func (h *mimicHandle) Primary() (*primitiveset.Entry, error) {
	ps, err := h.Primitives()
	if err != nil {
		return nil, err
	}

	return ps.Primary, nil
}

func (h *mimicHandle) Len() int {
	ps, err := h.Primitives()
	if err != nil {
		return 0
	}
	return len(ps.Entries)
}

func (h *mimicHandle) Entry(i int) (*primitiveset.Entry, error) {
	ps, err := h.Primitives()
	if err != nil {
		return nil, err
	}

	if i < 0 || i >= len(ps.EntriesInKeysetOrder) {
		return nil, fmt.Errorf("index %d out of range", i)
	}
	return ps.EntriesInKeysetOrder[i], nil
}

I think that approximates the new methods pretty well, and they work for my use case nicely. When I'm able to upgrade, I should be able to eliminate this wrapper struct and be good to go.

Have I overlooked anything?

Once there are key.Parameters and key.Key definitions for MAC keys I can try it for real.

@theory
Copy link
Author

theory commented Feb 13, 2025

I think that approximates the new methods pretty well, and they work for my use case nicely. When I'm able to upgrade, I should be able to eliminate this wrapper struct and be good to go.

That's assuming that Entry.Key() will allow me to use type assertion to get a MyKey (or AEAD or MAC). And that Entry.Key().Paramters() will let me get at params I have set up for my key implementation.

@theory
Copy link
Author

theory commented Feb 13, 2025

We are planning to add similar types for other primitives as well. Would that help?

Yes, I believe so, as long as it iterates over all keys, not just active keys. I assume it's a pretty straightforward interface to implement, yes?

I've started experimenting with this. Sadly, it looks like keyset.Entry lacks a method for the key URL.

@theory
Copy link
Author

theory commented Feb 13, 2025

That's assuming that Entry.Key() will allow me to use type assertion to get a MyKey (or AEAD or MAC). And that Entry.Key().Paramters() will let me get at params I have set up for my key implementation.

Sadly, it does not look like there is any way to get at the individual key to do the encryption or MAC generation. In the example above, one can get at the *aesgcm.Key, *aesgcmsiv.Key, etc. but the implementation does not provide Encrypt or Decrypt methods. Nor would the eventual key.Key implementation for MACs include ComputeMAC or VerifyMAC methods, I presume.

@morambro
Copy link
Contributor

morambro commented Feb 13, 2025

Thanks for the replies

I've started experimenting with this. Sadly, it looks like keyset.Entry lacks a method for the key URL.

Yes, one would have to use the type to distinguish the key type, e.g.:

switch entry.Key().(type) {
  case *aesgcm.Key:
    ...
  case *aesgcmsiv.Key:
    ...
}

Sadly, it does not look like there is any way to get at the individual key to do the encryption or MAC generation. In the example above, one can get at the *aesgcm.Key, *aesgcmsiv.Key, etc. but the implementation does not provide Encrypt or Decrypt methods. Nor would the eventual key.Key implementation for MACs include ComputeMAC or VerifyMAC methods, I presume.

One way to get a primitive for each key (without casting) is to import the key into a new keyset with only that key, and get the primitive out, for example:

macs := [][]byte{}
for i := 0; i < handle.Len(); i++ {
  entry, err := handle.Entry(i)
  // err handling
  km := keyset.NewManager()
  id, err := km.AddKey(entry.Key())
  // err handling
  err := km.SetPrimary(id)
  // err handling
  newHandle, err := km.Handle()
  // err handling
  m, err := mac.New(newHandle)
  // err handling
  res, err := m.ComputeMAC(...)
  // err handling
  macs := append(macs, res)
}

@theory
Copy link
Author

theory commented Feb 13, 2025

Yes, one would have to use the type to distinguish the key type, e.g.:

I guess that's do-able, but it means I'd have to map URLs to Tink-internal types. It would be nice if keys had all the same info easily available via KeysetInfo_KeyInfo was also provided by keyset.Entry.

One way to get a primitive for each key (without casting) is to import the key into a new keyset with only that key, and get the primitive out, for example:

Oh, okay, that works, though it's a bit fussy. Is there a more succinct way to do it via type coercion or assertion?

@theory
Copy link
Author

theory commented Feb 14, 2025

And I just realized an issue without access to the primitives: my code was using the Prefix for my key, not from the aead key. I can change that, but it really feels like there ought to be a way for key implementations outside the tink-go distribution to be able to get at their own primitives.

@theory
Copy link
Author

theory commented Feb 14, 2025

One last thing: how do I implement and integrate a key.Key for my key? I need a way to get at my params (field 2):

message MyKey {
  uint32 version = 1;
  // Tracks the AEAD and Mac schemes used by this key.
  HarkParams params = 2;
  // Stores the AEAD key material and configuration.
  google.crypto.tink.KeyData aead_key_data = 3;
  // Stores the MAC key material and configuration.
  google.crypto.tink.KeyData mac_key_data = 4;
}

@theory
Copy link
Author

theory commented Feb 15, 2025

I was able to get everything to work. However, to access the params in my key, I had to hack a public function into tink in theory/tink-go@e36b890. I could find no other way. I looked into implementing key.Key, but the interface requires internal APIs. This was the least invasive solution I could come up with.

@theory
Copy link
Author

theory commented Feb 16, 2025

One way to get a primitive for each key (without casting) is to import the key into a new keyset with only that key, and get the primitive out, for example:

As I said this works, but I was adding some tests and found that, because MyKey uses TINK prefix, both encrypted and hashed values have prefixes. That's fine for AEAD, but I don't want prefixes on the HASH digests. If I could use the underlying MAC key, which is configured for a RAW prefix, then I could do away with the prefix.

In the meantime I'm just going to snip it off.

@theory
Copy link
Author

theory commented Feb 18, 2025

I managed to get everything cleaned up and working perfectly using the aforementioned theory/tink-go@e36b890.

The upshot to all this, I think, is then need for key implementations without access to Tink internals to be able to access the primitives generated by their own managers. I'm not sure how to do this; maybe managers could be given an internalapi.Token when they register, which they can then use to call otherwise internal functions?

@morambro
Copy link
Contributor

I managed to get everything cleaned up and working perfectly using the aforementioned theory/tink-go@e36b890.

To access individual keys as tinkpb.Keyset_Key an option is to obtain the tinkpb.Keyset from the handle using insecurecleartextkeyset.KeysetMaterial, then iterate over protoKeyset.GetKey().

@theory
Copy link
Author

theory commented Mar 1, 2025

To access individual keys as tinkpb.Keyset_Key an option is to obtain the tinkpb.Keyset from the handle using insecurecleartextkeyset.KeysetMaterial, then iterate over protoKeyset.GetKey().

Ah-ha, that is exactly what I needed, thank you! For some reason I thought that API was private, or I would have used it before. Would have saved me quite a lot of aggravation (here and previously in #14).

All good now, thanks!

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

2 participants