Reviewing the Go implementation

[thaidn]

We want to release a Go implementation in 1.2.0. Aside from missing features that we're working hard to implement, there's also a couple of things in the code base that I'm not sure what we should do. I want to use this ticket to get some help or comments from the Go community.

First thing first, does the Go API make sense?

We copied the Java API, so I'm not entirely sure that it fits Go's style and philosophy. For example, our Lint tool warns us that https://github.com/google/tink/blob/master/go/aead/aead_config.go#L39 and several other places return unexported type *aead.config, which can be annoying to use. Is this problem important and we should fix it?

Secondly, I don't know any good way to implement classes consisting of pure static methods like CleartextKeysetHandle. To call a function in this class Go users have to initialize an object, while in Java users can just call the function directly (e.g., CleartextKeysetHandle().blah() versus CleartextKeysetHandle.blah()).

Aside from these immediate issues, we'd love to hear any feedback from the Go community on the API, the structure of the code base, coding style, features, etc.

Thanks!

@gdbelvin @dgryski

[dgryski]

/cc @FiloSottile

[dgryski]

In the case of returning unexported types, you can also create a public interface with no methods, or export the type while leaving all the methods and members unexported.

Pure static methods can just be exported methods in a package. If there's no state, there's no reason to tie them to an object. For example, in the case of the cleartext keyset functions, you can just have the tink package export the three methods (ParseSerializedKeyset, ParseKeyset, GenerateNew). If you wanted to group them, you could create a cleartextkeyset package with those three methods (assuming it won't introduce any circular dependencies.)

[ise-crypto]

Thanks for the suggestion @dgryski.

In the case of returning unexported types, you can also create a public interface with no methods, or export the type while leaving all the methods and members unexported.

It looks like I can get rid of these types. https://github.com/google/tink/blob/master/go/aead/aead_config.go just consists of static methods and (in the near future) constants, so it looks like I can merge them to the aead package.

Pure static methods can just be exported methods in a package. If there's no state, there's no reason to tie them to an object. For example, in the case of the cleartext keyset functions, you can just have the tink package export the three methods (ParseSerializedKeyset, ParseKeyset, GenerateNew). If you wanted to group them, you could create a cleartextkeyset package with those three methods (assuming it won't introduce any circular dependencies.)

These methods need to stay in the same package as keyset_handle.go. Otherwise they cannot call the newKeysetHandle method. We want to restrict access to that method because we want to control who can read cleartext keysets (which is a bad practice).

I think I have two choices:

1/ Moving both keyset_handle.go and cleartext_keyset_handle.go to tink/keyset_handle package, or

2/ Keep them in tink.

I prefer 1/.

[Saqibm128]

How long would a full refactor of the golang methods take? From what I can tell, most of the functions are becoming static context?

[LMMilewski]

Hi Thai,

if you're not working with anyone on this already then perhaps I could help. Thank you for having Go version :)
I added a few general comments below. Let me know if that's helpful. There's no expectation on my part that you'd do any of my suggestions. I'm just trying to help.

First thing first, does the Go API make sense?

I think that it makes sense.
The API surface is large though and the smaller the API the easier it is to choose the correct function. Also, the fewer promises you make in the API the easier it'll be to maintain it.
It's also clear that the API is inspired by Java and not a Go-first design. I don't necessarily see this as a bad thing -- if you have to maintain multiple versions of an API, then it makes sense to keep them somewhat similar. It may be better to have a complete Go version of a Java API than a Go design that lags behind its Java counterpart.

I'd say that if most common use-cases are easy to get right and less common use-cases are supported then the API makes sense. I don't know what those use-cases are though. It would really be very helpful if you provided examples of what you consider the most common intended usage of this API. We could decide what Go code we'd want to write for them and then derive the API from that. We should also include those examples in the documentation (you'd rather have people copy-paste your examples than code written by others and we both know that something will get copy-pasted).

I've seen your Java examples. It's unclear to me how representative of actual usage those are.

I'd typically focus on the high-level API first but since I'm not sure how far from Java we could go and what problem we're solving (code examples), I also listed below a few easy-to-fix things that I noticed. That is, less API design, more Go readability.

Protos vs Go types:
Keyset and KeysetHandle types seem a little confusing (at least they were to me). Perhaps use "Keyset" type? Something like:

  ks, err := tink.KeysetFromProto(pb) // returns an error if pb is not a valid keyset
  pb, err := ks.ToProto()
  ks.{Encrypt,Decrypt}

Keyset also looks like something that could get its own package. It looks like there are a lot of keyset operations. You could have keyset.{Decrypt,Encrypt,Encrypted,ToProto,FromProto}.

In general, it might be good to hide protos from users.

Naming:

• Initialisms should use consistent case. For example: s/tink.Aead/tink.AEAD/
  https://github.com/golang/go/wiki/CodeReviewComments#initialisms

• We typically drop "Get" prefix unless it creates a conflict. For example, s/GetPrimitive/Primitive/ or s/GetPrimitive/PrimitiveForKeyset/
  https://golang.org/doc/effective_go.html#Getters

• Go names are typically shorter than in other languages. It's good to drop unnecessary context. For example: s/tink.KeysetHandle/tink.Keyset/.

• Avoid stutter in names. For example, s/aead.primitiveSetAead/aead.primitives/ (or aead.primitiveSet)

• Similarly, avoid stutter with the type or function name. For example:
  Replace:

  func (km *KeysetManager) RotateWithTemplate(keyTemplate *tinkpb.KeyTemplate) error

With:

  func (km *KeysetManager) RotateWithTemplate(kt *tinkpb.KeyTemplate) error

In general, put enough context in variable names that it's clear what they mean. Don't include context that's obvious from the code around.

• Interfaces with a single method have typically the same name as the method with "er" suffix. For example, s/PublicKeySign/Signer/ and s/PublicKeyVerify/Verifier/

• Some names lack context. For example "ValidateVarsion" should probably be "ValidateKeyVersion" (although it's not clear what "key version" is -- is it the same as "keyId"?)

Line breaks:
There's no line length limit in Go style. It's good to split lines that become unreadable, but it's much more common to see unnecessary line breaks. For example:

  func DecryptKeyset(encryptedKeyset *tinkpb.EncryptedKeyset,
	masterKey Aead) (*tinkpb.Keyset, error) {

would be better as

  func DecryptKeyset(encryptedKeyset *tinkpb.EncryptedKeyset, masterKey Aead) (*tinkpb.Keyset, error) {

• don't rename imports unless it's necessery due to conflicts (except for protos). For example, don't rename "github.com/golang/protobuf/proto" as "proto". That doesn't do anything.

Setters/Getters:
Some of them seem unnecessary. For example:

  // SetKeyTemplate sets the key template of the manager.
  func (km *KeysetManager) SetKeyTemplate(template *tinkpb.KeyTemplate) {
    km.keyTemplate = template
  }
  // KeyTemplate returns the key template of the manager.
  func (km *KeysetManager) KeyTemplate() *tinkpb.KeyTemplate {
    return km.keyTemplate
  }

It would be better to just export keyTemplate.

Comments:

• I think that this comment: "Package tink defines interfaces for the crypto primitives that Tink supports." doesn't provide enough information. For example, look at the regexp package https://godoc.org/regexp for inspiration. Ideally this document would be self contained. It references "primitives that Tink supports" but it's not clear what those are. Provide more information, perhaps a link to general Tink documentation and a couple examples of common use.

• Comments should state what's not obvious from the code. For example this comment is redundant:

// newPrimitiveSetAead creates a new instance of primitiveSetAead
func newPrimitiveSetAead(ps *tink.PrimitiveSet) *primitiveSetAead {

All exported names should be documented, but this name is unexported and this comment could be dropped.

• Avoid decoration in docstrings. For example Aead.Encrypt says

	// Encrypt encrypts {@code plaintext} with {@code additionalData} as additional

godoc doesn't recognize {@code ...} syntax and you end up with:
https://godoc.org/github.com/google/tink/go/tink#Aead
It's better to do:

	// Encrypt encrypts plaintext with additionalData as additional

• Instead of "X is an implementation of Y interface" I'd probably say "X implements Y".

• You can preview documentation with godoc (either locally or at godoc.org). For example, "* Constants and convenience.." renders poorly.

• We typically avoid "thread-safe" because Go doesn't have threads. It's better to say "it's safe for concurrent use".

• Go doesn't "throw" anything. For example, replace "@throws error if the prefix type of {@code key} is unknown." with "returns an error if the prefix type of the key is unknown".

• Make sure that only existing code is referenced. For example, "{@code key.KeyId}" references the "key" package. It's not clear where it is. In fact, it looks like you use uint32 instead of KeyId (btw. Go name would be KeyID, not KeyId).

• Some comments state the obvious but don't include actually useful information. For example

    // Registry creates an instance of registry if there isn't and returns the instance.

I still don't know what this registry is and why I should care (the "registry" type is unexported and godoc doesn't show its documentation).
FWIW. I read the documentation for "type registry" and I still don't know why I'd use it. It sounds like this is an internal detail that I, as a user, shouldn't worry about.

Function signatures:

Consider:

   Encrypt(plaintext []byte, additionalData []byte) ([]byte, error)

• You can collapse multiple arguments with a single type. It could become

  Encrypt(plaintext, additionalData []byte) ([]byte, error)

• I'd be worried that people will get the order of arguments wrong. It may be worth to consider introducing types for plaintext, additional data, ciphertext. The tradeoff isn't clear to me here though. This is just something to consider, not necessarily a recommendation. I point it out because you say "Tink is a multi-language, cross-platform library that provides a simple and misuse-proof API for common cryptographic tasks."

Internal packages:
You could "hide" the subtle package if you like: https://docs.google.com/document/d/1e8kOo3r51b2BWtTs_1uADIA5djfXhPT36s6eHVRIvaU/edit

Interfaces:

In Go, you'd typically define an interface when:

• you need to work with values of multiple types (i.e. you "consume" the interface), or
• you want to define a standard for interop between libraries (e.g. the "io" package)
  It's different than in Java where you define interface when you want to return a value implementing an interface.

The purpose of the "KeyManager" interfaces isn't clear to me. It's large so it's hard to implement and most callers won't care about all methods. It's never used except to assert that types implement it. I suggest dropping it. If someone needs to use two different KeyManagers then they can define their own interface. Most likely they'll use a subset of methods.
Note that changing an interface (e.g. adding methods) is hard, in Go, as it breaks everyone who implements it.

I don't understand the Aead interface either. It seems that there's only one implementation and it's unclear that users are expected to provide their own. Same for tink.Mac, PublicKeySign,PublicKeyManager,PublicKeyVerify.

As to returning unexported types. We could talk about that but it really depends on what kind of help you expect here. I think that this problem arises because the API is shaped after the Java API. Ideally we would have an API that returns exported structs instead.

[thaidn]

Thanks so much for your comments! I'll address the readability issues first, then I'll schedule a meeting to discuss the API design.

I made the changes in 92c541b several days ago, before I saw your comment today. In that change, I made most of the functions static, i.e., replacing tink.Registry().blah() with tink.blah(). BTW the registry is just a map of key types to their implementations; we need it because we want to make Tink modular, i.e., users can include only key types that they need or they can add their own key types.

[LMMilewski]

sounds good

[dgryski]

What's left to do for this issue? Can this be closed? Are there substantial missing components from the Go implementation that we're waiting on? If so, should they have their own tracking issues?

[thaidn]

This is almost done. You can see the new structure at https://godoc.org/github.com/google/tink/go.

We're on track to include Go as part of 1.3.0 release.

[thaidn]

Golang support was included in 1.3.0 RC1. If you have some free time, please play with it (instruction is https://github.com/google/tink/blob/master/docs/GOLANG-HOWTO.md) and file bugs if you encounter anything undesirable.

This historical bug can now be closed. Thanks everybody for your insights!