roughenough icon indicating copy to clipboard operation
roughenough copied to clipboard

draft08 interop

Open cjpatton opened this issue 10 months ago • 3 comments

Hi! My coworker @lukevalenta has begun interop testing https://github.com/cloudflare/roughtime against your code and found they're incompatible:

# Google-Roughtime
./target/release/roughenough-client -p 0 roughtime.cloudflare.com 2003
Apr 11 2024 16:32:05 -04:00
# IETF-Roughtime
./target/release/roughenough-client -p 1 roughtime.cloudflare.com 2003
Timeout waiting for response

Likewise, our client can't talk to the int08h server via IETF Roughtime:

% ./getroughtime -ping roughtime.int08h.com:2002 -ping-version Google-Roughtime -pubkey AW5uAoTSTDfG5NfY1bTh08GUnOqlRb+HVhbJ3ODJvsE=
Ping response: 2024-04-11 16:49:39.613237 -0400 EDT ±1s (in 89ms)
% ./getroughtime -ping roughtime.int08h.com:2002 -ping-version IETF-Roughtime -pubkey AW5uAoTSTDfG5NfY1bTh08GUnOqlRb+HVhbJ3ODJvsE=
Ping error: no reply

We're going to look into this, but we thought we'd let you know in case you want to get a jump on interop testing.

Right off the bat, from your readme it sounds as though you've made a slight change to draft08 that would already break compatibility. Can you tell us your thinking here? https://github.com/int08h/roughenough/blob/5b59d8a5ab4d70f3ca54e064fc58bbf4deec27d5/README.md?plain=1#L20-L22

Relatedly, like you we wanted to run both the Google and IETF versions from the same port. How did you accomplish this? We use the "ROUGHTIM" prefix from the IETF version to distinguish it from the Google version: https://github.com/cloudflare/roughtime/blob/39613b31da8489364a13c71ba76b734a946339c8/protocol/protocol.go#L265-L269

cjpatton avatar Apr 11 '24 21:04 cjpatton

EDITED ~The roughenough client does not appear to implement the VER tag properly:~ cloudflare/roughtime does not implement the VER tag properly.

To reproduce:

Run the server:

[~/github.com/cloudflare/roughtime]$ go run ./cmd/testserver

Run the client (copy the public key from the server's output):

[~/github.com/int08h/roughenough]$ cargo run --bin roughenough-client -- -p 1 -k  pi5WxhBfmTtEmiJzcd+27kEkXWLEEwU4tUXbRVVv7QA= 127.0.0.1 2002

Expected server output:

main.go:64: Root public key: pi5WxhBfmTtEmiJzcd+27kEkXWLEEwU4tUXbRVVv7QA=
main.go:97: Error while handling request: missing VER tag
exit status 1

cjpatton avatar Apr 11 '24 23:04 cjpatton

I've confirmed interop for batch size of 1 between int08h/roughenough and cloudflare/roughtime, modulo the following discrepancies:

  1. int08h/roughenough uses a non-standard version number: should be 0x80000008
  2. cloudflare/roughtime incorrectly implements the VER tag: https://github.com/cloudflare/roughtime/pull/51
  3. I noticed that the draft appears to have generated the ZZZZ tag incorrectly: https://github.com/wbl/roughtime-draft/issues/4. Both of our implementations do the same thing.

I'm not able to conveniently test batched signing, as our test server doesn't support this(https://github.com/cloudflare/roughtime/tree/master/cmd/testserver). If your server implements this, it would be great if you could try it out.

cjpatton avatar Apr 12 '24 19:04 cjpatton

Hello @cjpatton , thanks so much for kicking off the interop testing!

I'll take a look at your suggestions and work on getting the two implementations working together.

int08h avatar May 12 '24 01:05 int08h

Hi @cjpatton,

Relatedly, like you we wanted to run both the Google and IETF versions from the same port. How did you accomplish this? We use the "ROUGHTIM" prefix from the IETF version to distinguish it from the Google version:

I do the same thing (match on the framing bytes ROUGHTIM: https://github.com/int08h/roughenough/blob/master/src/request.rs#L25).

Right off the bat, from your readme it sounds as though you've made a slight change to draft08 that would already break compatibility. Can you tell us your thinking here?

My thinking was 0x1 corresponds to the value of "Roughtime version 1" that the protocol is going to use according to the draft RFC, so I just went ahead and used it.

That is, 0x1 is not in use today but will be once the RFC is adopted (if I understand correctly). Instead of using the "Reserved for private use" scheme of 0x80000000 + draft revision number, waiting for the RFC to be adopted, then changing to 0x1.

int08h avatar May 17 '24 22:05 int08h

@cjpatton I made some changes to this implementation to support private prefixed versions, specifically to send VER value0x80000008 for Draft8 to be compatible with the latest Cloudflare implementation.

This client interops with the testServer from github.com/cloudflare/roughtime in Google/Classic mode, and once I realized my mistake, it now interops in RfcDraft8 mode too. Our implementations can talk to each other 😄.

# Roughenough client talking RFC Draft8 to Cloudflare testServer
$ target/debug/roughenough-client -p 8 127.0.0.1 2002  -d               
Request = RtMessage|3|{
  VER(4) = 08000080
  NONC(32) = 2507a070b98c61cd3fe2696230c794151fbc64d39d4c79e95e1ce954897e0f7f
  ZZZZ(964) = 00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000...0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
}

Response = RtMessage|6|{
  SIG(64) = 180c168352eb62935728c19ac4186a2413090051e20d474ea218314b887f6792984f16c4d582a362420a9b7c58de04f49db42d105243ac5d630fd1c0379cc60b
  VER(4) = 08000080
  PATH(0) = 
  SREP(68) = RtMessage|3|{
    RADI(4) = 01000000
    MIDP(8) = 70674a6600000000
    ROOT(32) = 31b8d600dd8e8611e4b0e6e9be67ef30528e5f9d8d38f10126630e24374bdce9
  }
  CERT(152) = RtMessage|2|{
    SIG(64) = 8efc119d3f39bdbea40b6f3c6d564697ad3a1cf081bc6a0a6ece31228e2ec47517acfb3cc81b3e2cb28b684096591050ca2239893bb3cca228e8e2d62acfcc0e
    DELE(72) = RtMessage|3|{
      PUBK(32) = 212244f315620769192b4e8a0ecdeefeaaa8a3d320b71cba3007d3f358e047f9
      MINT(8) = e315496600000000
      MAXT(8) = e3b84b6600000000
    }
  }
  INDX(4) = 00000000
}

May 19 2024 15:56:16 -05:00

I confused myself and forgot that the contents of the VER tag would be serialized little endian (least significant byte first). https://github.com/cloudflare/roughtime/blob/master/protocol/version.go#L28 clearly has the correct value, and I had to trace our implementations to discover I had it wrong (https://github.com/int08h/roughenough/commit/8682e3aec2512ac64be032df2e4e157bdb6e2ab3).

  • Added a -p 8 command line parameter to roughenough-client to select RFC Draft8 protocol (-p 0 selects Google/Classic and -p 1 selects the anticipated RFC version).

int08h avatar May 19 '24 20:05 int08h

Note there may yet be future draft versions before RFC, though I'm hopeful we're close to the final draft as most of the chatter on the NTP mailing list has died down.

BTW we have deployed our server, if you'd like to try your client against a live server: https://github.com/cloudflare/roughtime/blob/2814185b05e12ca627f78a7aefcbc5ab3c88ba5e/ecosystem.json#L3-L14

cjpatton avatar May 20 '24 09:05 cjpatton

@cjpatton closing this issue now that draft8 is interop'ing. Tracking your draft10 implementation and this one's in #40.

int08h avatar Jun 13 '24 19:06 int08h

We're aiming to add draft 10 support to our server before IETF 120.

cjpatton avatar Jun 13 '24 19:06 cjpatton