Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

webtransport: use deterministic TLS certificates #1833

Merged
merged 18 commits into from
Nov 14, 2022

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented Oct 17, 2022

Closes #1824

@MarcoPolo MarcoPolo marked this pull request as draft October 17, 2022 19:05
@MarcoPolo MarcoPolo marked this pull request as ready for review October 17, 2022 19:47
p2p/transport/webtransport/crypto.go Outdated Show resolved Hide resolved
runs := 1000
for i := 0; i < runs; i++ {
t.Run(fmt.Sprintf("Run=%d", i), func(t *testing.T) {
zeroSeed := [32]byte{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a random value? Otherwise, what's the point of running 1000 runs?

Copy link
Contributor Author

@MarcoPolo MarcoPolo Oct 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't matter. The test is to check if the signing process itself is non-deterministic. This seed is fed into the hkdf so the signing process is oblivious to this.

Imagine the signing process changes in the future and it randomly reads a byte from /dev/urandom to mix in 10% of the time. Then a single run would succeed 81% of the time. Doing more runs decreases the chance of success iteratively (0.81*0.81*...). The 1000 runs is to detect if there's any chance on non-determinism.

runs := 1000
for i := 0; i < runs; i++ {
t.Run(fmt.Sprintf("Run=%d", i), func(t *testing.T) {
zeroSeed := [32]byte{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

p2p/transport/webtransport/cert_manager.go Outdated Show resolved Hide resolved
@@ -77,7 +86,7 @@ func TestCertRenewal(t *testing.T) {
}
return false
}, 100*time.Millisecond, 10*time.Millisecond)
cl.Add(2 * time.Second)
cl.Add(clockSkewAllowance + 2*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which change in the logic made this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the skew here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of. In this test we have this setup

 v-- start here-ish
|---------|
        |---------|

with cl.Set(m.currentConfig.End().Add(-(2*clockSkewAllowance + time.Second))), we advance the clock:

        v-- clock is here now (minus one second)
|---------|
        |---------|

Remember the transition point is one clock skew before the end.

        v-- clock is here now (minus one second)
|---------|
         | <- transition point
        |---------|

So we need to move one clock skew forward and a couple seconds

         v-- clock is here now (plus one second)
|---------|
         | <- transition point
        |---------|

Maybe this makes more sense if I change it to only advance the clock to one clock skew before the end?
with cl.Set(m.currentConfig.End().Add(-(clockSkewAllowance + time.Second))), we advance the clock:

@marten-seemann marten-seemann changed the title Use deterministic TLS certificates for webtransport webtransport: use deterministic TLS certificates Oct 19, 2022
@MarcoPolo MarcoPolo force-pushed the marco/deterministic-certhashes branch from c387a22 to 92bcb04 Compare October 20, 2022 19:04
@MarcoPolo MarcoPolo marked this pull request as draft October 20, 2022 23:23
@MarcoPolo MarcoPolo marked this pull request as ready for review October 21, 2022 12:06
for i := 0; i < runs; i++ {
t.Run(fmt.Sprintf("Run=%d", i), func(t *testing.T) {
cl := clock.NewMock()
priv, _, err := test.RandTestKeyPair(crypto.Ed25519, 256)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use a zero byte seed. Otherwise we can randomly get to an edge case around the hour (with random offset)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand. Isn’t there the chance we hit the offset for any key?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test makes sure that if we reboot one hour in the future then we get the same key assuming we aren't near the boundary. The problem with the random key is that it means we have a random offset (offset is defined by the key). So by setting the key to a predetermined one we can uphold the assumption. The zero seed is just one such key, but any key that doesn't put us near the bounds is fine.

p2p/transport/webtransport/cert_manager.go Outdated Show resolved Hide resolved
// We want to add a random offset to each start time so that not all certs
// rotate at the same time across the network. The offset represents moving
// the bucket start time some `offset` earlier.
offset := (time.Duration(binary.LittleEndian.Uint16(pubkeyBytes)) * time.Minute) % (certValidity - clockSkewAllowance)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this module certValidity minus a single clockSkewAllowance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was including the clockSkew allowance into the offset, but that was wrong (and also tricky). I've done the simpler thing of subtracting the start I pass to getCurrentBucketStartTime by clockSkewAllowance. This works because getCurrentBucketStartTime will never give a time later than start + certValidity - 2*clockSkew.

p2p/transport/webtransport/cert_manager_test.go Outdated Show resolved Hide resolved
p2p/transport/webtransport/cert_manager_test.go Outdated Show resolved Hide resolved
@@ -77,7 +86,7 @@ func TestCertRenewal(t *testing.T) {
}
return false
}, 100*time.Millisecond, 10*time.Millisecond)
cl.Add(2 * time.Second)
cl.Add(clockSkewAllowance + 2*time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the skew here?

@@ -100,3 +110,66 @@ func TestCertRenewal(t *testing.T) {
// check that the 2nd certificate from the beginning was rolled over to be the 1st certificate
require.Equal(t, second[1].Value(), third[0].Value())
}

func TestDeterministicCertsAcrossReboots(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to skip on OSX?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MacOS issue was around listening, this should be okay (after using a deterministic seed, since otherwise we have random offsets and adding an hour might get us over the border).

for i := 0; i < runs; i++ {
t.Run(fmt.Sprintf("Run=%d", i), func(t *testing.T) {
cl := clock.NewMock()
priv, _, err := test.RandTestKeyPair(crypto.Ed25519, 256)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand. Isn’t there the chance we hit the offset for any key?

p2p/transport/webtransport/transport_test.go Outdated Show resolved Hide resolved
func TestServerSendsBackValidCert(t *testing.T) {
var maxTimeoutErrors = 10

require.NoError(t, quick.Check(func(timeSinceUnixEpoch time.Duration, keySeed int64, randomClientSkew time.Duration) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to repeat this test 100 time or so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marten-seemann
Copy link
Contributor

Looks like one of these tests is failing:

 === RUN   TestServerSendsBackValidCert
  2022/10/22 14:53:07 failed to sufficiently increase receive buffer size (was: 208 kiB, wanted: 2048 kiB, got: 416 kiB). See https://github.com/lucas-clemente/quic-go/wiki/UDP-Receive-Buffer-Size for details.
  Error: CRYPTO_ERROR (0x12a): Times are not valid: server_now=2062-11-24 13:43:26.165 +0000 UTC client_now=2062-11-24 13:25:30.7370715 +0000 UTC certstart=2062-11-24 13:30:00 +0000 UTC certend=2062-12-08 13:30:00 +0000 UTC
      transport_test.go:598: 
          	Error Trace:	/home/runner/work/go-libp2p/go-libp2p/p2p/transport/webtransport/transport_test.go:598
          	Error:      	Received unexpected error:
          	            	#54: failed on input -6053665406165266670, -6699227244707765831, -642676675427928500
          	Test:       	TestServerSendsBackValidCert
  --- FAIL: TestServerSendsBackValidCert (0.76s)

@marten-seemann marten-seemann mentioned this pull request Oct 31, 2022
34 tasks
@MarcoPolo MarcoPolo force-pushed the marco/deterministic-certhashes branch from e7455e4 to e913a1b Compare October 31, 2022 22:21
@MarcoPolo MarcoPolo merged commit a0432e7 into master Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

webtransport: deterministically generate certificates, to survive host reboots
2 participants