update the encryption and signing specs to reflect the field order we shipped

[oconnor663]

The Go msgpack encoder we're using has a counterintuitive order for
struct fields when serializing as arrays. We meant to put the final flag
at the end, but it's actually at the beginning (though not for
signcryption mode). Update the v2 specs to reflect this unfortunate
reality.

r? @mlsteele @akalin-keybase

[coveralls]

Coverage remained the same at 84.746% when pulling e64539d on jack/flag_order into 72dc788 on master.

[akalin-keybase]

Oh, man, this is probably my fault, sorry. :( Can you point me to the docs about the struct order?

Why is it too late to fix? I'm guessing we've started writing sigchain stuff with saltpack v2?

[oconnor663]

@akalin-keybase, we turned on V2-by-default for keybase encrypt and keybase sign last week. There are probably documents in the wild using the current behavior, that would break if we changed it without supporting the current version for all of time. So...we could fix it in v3?

[akalin-keybase]

LGTM, makes sense re. fixing it in v3. I'm interested in how you found this out, though...I couldn't find any mention in https://godoc.org/github.com/ugorji/go/codec about this behavior.

[oconnor663]

@akalin-keybase I assume it's a low level detail of how Go reflects over struct fields, and not a specific decision the codec makes?

I ran into it first when I was starting to bring https://github.com/keybase/saltpack-python up to speed with v2, though I kind of glossed over it at the time. Then @gherynos ran into it updating https://github.com/Gherynos/libsaltpack, and he messaged me over Keybase chat.

[akalin-keybase]

Ah, I see. It's a bit worrisome, since we serialize to arrays in a few other places in KBFS.

Digging into the ugorji/codec source, it looks like it relies on reflect.Value.Field ( https://golang.org/pkg/reflect/#Value.Field ) which doesn't mention any guarantees as to ordering, or whether it may change (although that would probably break a lot of stuff).

https://stackoverflow.com/questions/32392311/reflect-type-field-order doesn't mention any other official sources.

I'll ask around on the golang lists to see what the official word is. I'll also probably file an issue with ugorji/codec to clarify its documentation, too.

[oconnor663]

At least in the saltpack case we should have some tests that fail now if the order changes. But yes, I agree it's worrisome.

[akalin-keybase]

ILT replied to my query: https://groups.google.com/d/msg/golang-nuts/_he7g1TL0K8/8BBCFB0tCgAJ

I'll try to repro, and see if this is a bug in ugorji, or maybe our old fork of it.

[oconnor663]

Hmm, looks like reflection doesn't inline the fields of the embedded struct:

package main

import (
	"fmt"
	"reflect"
)

type S1 struct {
	Foo int
	Bar int
}

type S2 struct {
	S1
	Baz int
}

func main() {
	mystruct := S2{}

	t := reflect.TypeOf(mystruct)

	for i := 0; i < t.NumField(); i++ {
		fmt.Printf("Found field: %s\n", t.Field(i).Name)
	}
}

Prints:

Found field: S1
Found field: Baz

[akalin-keybase]

Yeah, I came to the same conclusion. Must be a bug in ugorji/codec, looking...

[akalin-keybase]

It's a bug in our fork of (an old version of) ugorji/codec: see https://play.golang.org/p/IiMPOFv9kF for a program that behaves differently depending on whether ugorji/codec or keybase/go-codec/codec is used.

sigh We should probably update our fork. But now we'll have to be super-careful doing so, and probably do it a repo at a time, so that we don't mess up anything else relying on this old behavior.

[oconnor663]

Thanks Fred. If we get to this update for saltpack, I'd say we should just stop using inlining for our array structs and hardcode the current behavior, to work around the back-compat issue.

[akalin-keybase]

Yeap, SGTM.