Export V4AsymmetricPublicKey to paserk k4.public

[suttod]

Hi, I have started to work on an initial implementation of PASERK keys.
Currently only V4AsymmetricPublicKeys can be exported to k4.public paserk tokens.

Would you please review if the implementation concept matches your requirements.
If yes, I'd follow by implementing other plain exports and imports.

[aidantwoods]

Thanks for starting on the implementation here, looking great so far! Just had a couple of comments.

[aidantwoods]

My preference for this would be to return (KeyVersion, error), and then have the default: case return an error (rather than introducing a KeyVersionInvalid value into KeyVersion. And then similar comment for the similar bits of code, where an *Invalid variant is introduced.

[suttod]

Hi, it is quite simple to add a second return value with error type, but even that case something must be returned as KeyVersion as well. The following statement would be invalid:

default:
    return nil, fmt.Errorf("invalid key version string: %s", versionStr)

The return value must be an integer (which is the underlying type of KeyVersion). Default value of int is 0, which is the preferred return value on error.
So I could use:

default:
    return 0, fmt.Errorf("invalid key version string: %s", versionStr)

or equivalently (as const KeyVersionInvalid KeyVersion = 0

default:
    return KeyVersionInvalid, fmt.Errorf("invalid key version string: %s", versionStr)

So if you think, it is cleaner to return an error object as well, I could simply add it, just please let me know. I'm not very familiar with coding conventions in go.

[aidantwoods]

If we return a pointer to the key version, and an error (*KeyVersion, error), then we are able to return nil here for the key version (which is probably the better option for this case).

In Go, usually we return an explicit error for failure (which then reminds the caller to check for an error, handling where the returned err != nil).

[suttod]

Changing the signature to return a pointer would cause an error on valid cases. KeyVersion instances are currently constants, not variables, so the code below would be invalid:

	case "k1":
		return &KeyVersionV1

Should each const be changed to var then?

[aidantwoods]

Ah you're right, sorry I overlooked that.

Do we actually need this pattern at all if we set:

const KeyVersionV1 = "k1"

etc...?

If we do, I'd still prefer we return an error than rely on callers checking for the invalid value, as the error is more explicit from a function signature standpoint, so harder to forget. Either we can assign the const to a locally scoped var before returning (if using pointers), or return the invalid zero value (though maybe we don't need to actually export that invalid value const as part of the API).

[aidantwoods]

Rather than adding onto the PASETO vectors test here, I think probably we can just create a separate set of tests for PASREK (since most of the PASETO fields are going to be missing from parsed PASREK data).

[aidantwoods]

If possible without introducing dependency cycles, I think we can probably contain most of PASREK in its own submodule? (As opposed to including it directly in the main paseto package)?

[suttod]

I have tried to do it, after getting deeper in the paserk standard, and I see the following possibilities:

1. Use paserk package for defining encoders/decoders, and use the binary ([]byte) methods to export keys/create new ones. In this case paseto package would depend on paserk. An example (currently without error handling) would look like:

   key, err := paseto.V2SymmetricKeyFromHex("707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f")
   fmt.Println(key.ExportHex())
   password, memlimit, opslimit := "correct horse battery staple", 268435456, 3
   paserkStr, err := key.ToPaserk(paserk.EncodeWithPassword(password, memlimit, opslimit))
   fmt.Println(paserkStr)
   decodedKey, err := paseto.V2SymmetricKeyFromPaserk(paserkStr, paserk.DecodeWithPassword(password, memlimit, opslimit))
   fmt.Println(decodedKey.ExportHex())
   // Output:
   // 707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f
   // k2.local-pw.pO9bFwcQ4EvMgi05yZUZYAAAAAAQAAAAAAAAAwAAAAHQDD7woll221xBostsekagJ31WkLjCKzV1emeOK91FbTeyY-NRZAqnoLnGHs30MEbbL7SVqmtPhELmR8ZbrYIPOu6LJh2IQ4bs-xjkPk6UB0t5E9RcYhEt
   // 707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f

   For this v2_keys.go needs to be extended as follows:

   func (k V2SymmetricKey) ToPaserk(encoding paserk.Encoding) (string, error) {
       // we can simply pass the key to paserk as the key has a public ExportHex method and a Key interface is defined in paserk
       return encoding.Encode(&k)
   }
   
   func V2SymmetricKeyFromPaserk(paserkStr string, encoding paserk.Encoding) (V2SymmetricKey, error) {
       // Decoder can't return a V2SymmetricKey, as paserk can't use types from paseto package.
       // I can't se a way to hide it with an interface.
       data, err := encoding.Decode(paserkStr, paserk.KeyPurposeLocal, paserk.KeyVersionV2)
       if err != nil {
           // even though we return error, return a random key here rather than
           // a nil key
           return NewV2SymmetricKey(), errors.New("Key incorrect length")
       }
       return V2SymmetricKeyFromBytes(bytes)
   }

2. The previous solution may be implemented with even more specific encoders like:

   paserkStr, err = key.ToPaserk(paserk.V2EncodeLocalWithPassword(password, memlimit, opslimit))
   decodedKey, err := paseto.V2SymmetricKeyFromPaserk(paserkStr, paserk.V2DecodeLocalWithPassword(password, memlimit, opslimit))

   This may be useful for sealing, where encoder params depend on key version (ie. NIST vs ed curve keys).
3. Reverse the direction (ie. paserk package would depend on paseto)

   key, err := paseto.V2SymmetricKeyFromHex("707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f")
   fmt.Println(key.ExportHex())
   password, memlimit, opslimit := "correct horse battery staple", 268435456, 3
   encoder = paserk.V2PasswordEncoder(password, memlimit, opslimit)
   paserkStr, err := encoder.Encode(key)
   fmt.Println(paserkStr)
   decoder = paserk.V2PasswordDecoder(password, memlimit, opslimit)
   decodedKey, err := decoder.Decode(paserkStr)
   fmt.Println(decodedKey.ExportHex())
   // Output:
   // 707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f
   // k2.local-pw.pO9bFwcQ4EvMgi05yZUZYAAAAAAQAAAAAAAAAwAAAAHQDD7woll221xBostsekagJ31WkLjCKzV1emeOK91FbTeyY-NRZAqnoLnGHs30MEbbL7SVqmtPhELmR8ZbrYIPOu6LJh2IQ4bs-xjkPk6UB0t5E9RcYhEt
   // 707172737475767778797a7b7c7d7e7f808182838485868788898a8b8c8d8e8f    ```
   The latter seems to provide a cleaner API, and fits better to your core paseto implementation.

I would plan to implement the Marshaler/Unmarshaler interface to call the raw paserk encoding of the keys. Such a function (like func (k *V2SymmetricKey) UnmarshalJSON(jsonstr []byte) error) must be implemented in the paseto package because of the receiver. It can only be done of paseto package depends on paserk and not vica versa.

So which option would you prefer? Or can you come up with a different idea?

[aidantwoods]

Architecturally I think it probably makes the most sense for the paserk package to depend upon paseto (as in option 3). If you're using paserk, you'll be using it for paseto (but if you're using paseto, you might not use paserk). This would also matches the way the reference implementation is setup (not the only reason to do it, but makes things a bit simpler if we want to compare implementations).

I do also like the API simplicity of using paserk to first construct a key, and then using paseto to sign/encrypt the message. There's a nice separation of concerns there, and means that we can keep the key export options within the paseto package quite simple.

[aidantwoods]

Any reason not to use the strings corresponding to each variant here for simplicity?