fix pubkey interning test #104
Conversation
There was a problem hiding this comment.
Overall LGTM. Most of the comments are actually questions to make sure I did not miss anything.
The PR is selling itself short, as it's not just the test that required fixing, it seems :-)
There's a minor refactoring suggestion for algorithm serialization.
Apart from that and it's not related to the PR itself, but I think the spec conformance suite should come with 3rd party request / contents samples that should be parsed / used by implementations with the resulting token checked against an expected result (not byte-for-byte, but at least contents)
| @@ -180,7 +180,7 @@ public String print_scope(final Scope scope) { | |||
| return pk.get().toString(); | |||
| } | |||
| } | |||
| return "?"; | |||
| return "<"+ scope.publicKey+"?>"; | |||
There was a problem hiding this comment.
shouldn't this throw instead?
There was a problem hiding this comment.
probably, but then it would trigger changs all over the API. I'll attempt that in a follow up PR
| @@ -88,10 +86,14 @@ public static Biscuit make(final SecureRandom rng, final KeyPair root, final Int | |||
| * @param authority authority block | |||
| * @return Biscuit | |||
| */ | |||
| static private Biscuit make(final SecureRandom rng, final KeyPair root, final Option<Integer> root_key_id, final Block authority) throws Error.FormatError { | |||
| static Biscuit make(final SecureRandom rng, final KeyPair root, final Option<Integer> root_key_id, final Block authority) throws Error.FormatError { | |||
There was a problem hiding this comment.
why was this private / how was it supposed to be called previously?
There was a problem hiding this comment.
this one was used from other overloaded make static methods, I forgot to move it back to private. In the next major version, all the make static methods will be private, I'll remove them from the API
| ArrayList<Block> blocks = new ArrayList<>(); | ||
|
|
||
| KeyPair next = new KeyPair(rng); | ||
| KeyPair next = KeyPair.generate(root.public_key().algorithm, rng); |
There was a problem hiding this comment.
so by default, it will use the same algorithm as the root key for attenuation. That's interesting, i think this should be discussed in the ECDSA PR (and in the spec at some point)
There was a problem hiding this comment.
yes, that's something that should be discussed
There was a problem hiding this comment.
I'm moving it back to hardcoded Ed25519 for now
| @@ -48,7 +48,7 @@ public Block(SymbolTable base_symbols) { | |||
| this.scopes = new ArrayList<>(); | |||
| this.publicKeys = new ArrayList<>(); | |||
| this.externalKey = Option.none(); | |||
| this.version = SerializedBiscuit.MAX_SCHEMA_VERSION; | |||
| this.version = SerializedBiscuit.MIN_SCHEMA_VERSION; | |||
There was a problem hiding this comment.
no, it is not used, the block version is derived when serializing. I'm removing the field.
| @@ -18,15 +18,13 @@ | |||
| import java.util.*; | |||
|
|
|||
| public class Block { | |||
| long index; | |||
There was a problem hiding this comment.
do you know why it was there in the first place?
There was a problem hiding this comment.
because the builder API had methods that create "the block at index i" instead of creating a generic block builder that can then be appended. There's so much in this API we have to fix
| sgr.initVerify(externalKey.key); | ||
|
|
||
| sgr.update(blockResponse.payload); | ||
| ByteBuffer algo_buf = ByteBuffer.allocate(4).order(ByteOrder.LITTLE_ENDIAN); |
There was a problem hiding this comment.
this serialization logic might be better encapsulated in a method rather than inlined here (and i assume in other places)
There was a problem hiding this comment.
something to look at after the ECDSA PR has merged, I think, otherwise we'll get lots of conflicts
There was a problem hiding this comment.
I tried abstract this into a function for my remote_signer branch however these new third party tests are the only ones which are failing when trying to rebase my branch. Still digging into it, not sure whether subtle nuance or not.
There was a problem hiding this comment.
Think I see my issue, around the externalSignature which appears to be specific to third party.
| } | ||
|
|
||
| long pkIndex = symbols.insert(externalKey); | ||
| // if (copiedBiscuit.publicKeyToBlockId. |
There was a problem hiding this comment.
this can be removed i assume
It is automatically derived when serializing
This implements 3rd party block generation and appending to existing tokens. It also fixes existing issues with public key interning which made deserialization of tokens with 3rd party blocks incorrect in some cases. Co-authored-by: Geoffroy Couprie <[email protected]>
Implement 3rd party block creation (#104)
No description provided.