Allow associating an ID with a biscuit's root key

[seh]

In order to accommodate biscuit issuers with multiple key pairs in use, whether concurrently or in an ongoing rotation cycle, biscuits can record and expose an identifier for the root private key used to sign its authority block. Allow issuers to associate such an identifier with the private key when creating a new biscuit.

Introduce the option function WithRootKeyID to supply such an identifier at composition time, and the (*Biscuit).RootKeyID method to query this identifier later.

Contributes to #150.

[divarvel]

LGTM, given the current state of the library.

An interesting thing to note, however is that we have moved away from this design in other libs, especially biscuit rust.

In biscuit rust, we now have two kinds of builders:

• one for a biscuit block
• one for a biscuit itself (composed of a block builder for the autority block, plus a field for the root key id)

With this setup, there is no need for separate add_fact / add_authority_fact distinctions, everything uses the same code for adding facts, rules and checks.

This also makes the root key id part of the builder, so that there is no need to modify the build() function, the builder can be extended without affecting the rest of the codebase.

This is clearly a big change and this PR improves the situation, but at some point i’d like to have this structure in the go lib as well.

[divarvel]

would returning a pointer instead of a tuple also work to signal optionality?

[seh]

I vacillated over this decision for a while before implementing this new method. I'm glad you asked about it.

Yes, it would work to return a pointer, though it's fairly typical in Go to use this style, especially when the primary return value's type has a sensible zero value.

This paired value style comes from and is adapted somewhat unjustly from a few of Go's statements and expressions that benefit from it:

• Channel receive statements that can include an optional second untyped boolean value to check whether the channel is closed.
• Map index expressions that can use an optional second untyped boolean value to check whether the given key is present in the map.
• Type assertions can use an optional second untyped boolean value to check whether the interface value's dynamic type matches the asserted type.

In each of these cases, callers are free to omit the assignment of this second value, and I don't mean just not assigning it to a named variable. Callers can omit mention of the second value entirely if they don't care about it. By contrast, if a map index expression returned by pointer to indicate whether or not the key was bound to a value in the map, all callers would have to confront the possibility of absence.

My use of that idiom here doesn't benefit from that same treatment, though. A caller of my (*Biscuit).RootKeyID cannot ignore the presence of that second return value. They are free to ignore its value, though, and use the first value unconditionally, since the zero value for uint32 is, well, zero, which is a valid key ID.

Given that, I'm now coming back around to thinking that returning by pointer would be a safer choice here. Please allow me to make that change, and then you can decide if you like that by-pointer approach better.

[divarvel]

i also think returning a pointer would be a bit safer. The rust implementation distinguishes between no id and 0. Since biscuit uses protobuf 2, I think the serialization also makes that distinction.

[seh]

Please see commit 9b7f488 for this change. I'll squash the commits before merging.

[divarvel]

looks good, thanks! in can also squash-merge the PR if that’s simpler for you

[seh]

Yes, that's fine with if you squash the commits. I recommend preserving the message from the first one.

After we merge this, I'll carry on with addressing the rest of #150—selecting a public key in calls to the (b *Biscuit).Authorizer method, or a new sibling method, per #150 (comment).