Add HKDF funcs for Key Scheduling - DTLS v1.3

[philipch07]

Description

This adds the HKDF related functions for the key scheduling feature in accordance with DTLS v1.3 section 5.9. Note that it links to TLS 1.3 section 7.1.

This aims to add:

• HKDF-Expand-Label
• HKDF-Extract
• Derive-Secret

Note that the architecture is still a WIP (see #738) so the current file structures are subject to change.

Reference issue

Closes #740

[codecov]

Codecov Report

❌ Patch coverage is 76.47059% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.54%. Comparing base (accee59) to head (70b7fe2).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/crypto/keyschedule/keyschedule.go	76.47%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #737      +/-   ##
==========================================
- Coverage   82.57%   82.54%   -0.04%     
==========================================
  Files         116      117       +1     
  Lines        6650     6684      +34     
==========================================
+ Hits         5491     5517      +26     
- Misses        760      764       +4     
- Partials      399      403       +4     

Flag	Coverage Δ
go	82.54% <76.47%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[theodorsm]

Thanks for starting on this!

I think this PR should be scoped to implement the HKDF functions: HKDF-Expand-Label and Derive-Secret as you have started on. This will be one part of multiple to complete issue #736.

[philipch07]

Thanks for starting on this!

I think this PR should be scoped down to adding the HKDF functions HKDF-Expand-Label and Derive-Secret as you have started on. This will be one part of multiple to complete issue #736.

No problem! I agree, this seems to be a rather large thing and due to the current block that you mentioned #736 (comment), I think it would make sense to tackle #736 in multiple parts. Just to clarify, I should still include hkdfExtract though, correct?

[theodorsm]

@philipch07, yes we should also export a HkdfExtract function, good catch.

[theodorsm]

I have made some changes to the draft to make better use of the standard crypto library and should be more in-line with the TLS 1.3 implementation.

I also think we should add some tests that verify the byte output of Expand/Derive functions.

Note: the crypto/hkdf library requires go version 1.24, which is a higher minimum version than we currently have. I think the usage of this library justifies the bump in minimum version, but I am unsure of how much this would break for our users. Looking for input on this!

[JoTurk]

@theodorsm

Note: the crypto/hkdf library requires go version 1.24, which is a higher minimum version than we currently have. I think the usage of this library justifies the bump in minimum version,

Sadly we can't upgrade to 1.24, unless we're shipping dtls 1.3 now. many users are still on 1.22, 1.23, And upgrading dtls to 1.24 will force to upgrade pion/webrtc too and many other libraries.

Maybe we can keep this in a branch until we upgrade? we should upgrade to 1.24 before dtls 1.3 is ready anyway.

[theodorsm]

@joeturki, I agree that we should wait to merge this until we are closer to a WIP for DTLS 1.3. Unfortunately, this blocks much of the implementation, so we should provide a similar mock API meanwhile.

[adrianosela]

@theodorsm @JoTurk

Maybe a dumb idea but as a band aid solution to the problem… can we copy and paste the crypto/hkdf into this repo? So we dont have to manage multiple branches :) then when we bump to 1.24 we get rid of the copy.

[JoTurk]

I think it's safe to upgrade to 1.24 once 1.26 is released in a few days :)

[theodorsm]

We finally bumped the go version, so this PR is unblocked!

I have rebased and made some touch ups.

[adrianosela]

LGTM! Feel free to ignore the nits.

I think we should add some tests for HkdfExpandLabel and DeriveSecret with known-good expected outputs. Maybe we can get some from https://github.com/openssl/openssl/blob/master/test/recipes/30-test_evp_data/evpkdf_tls13_kdf.txt (though these are for tls not dtls, we'll need to adapt them. I think we should be able to just change the prefixes from tls13 to dtls13)

^^ but this can be done in a separate PR/later. Ship it.

[adrianosela]

Here's one taken from openssl's tls13 tests. All the hex strings are the same except for expected:

// TestHKDFExpandLabel_WithKnownOutput validates HKDF-Expand-Label produces correct output.
// Adapted from OpenSSL TLS13-KDF test vectors (modified for DTLS 1.3 "dtls13" prefix)
// https://github.com/openssl/openssl/blob/5b310281af9a77d180a81c6dc0a093022931b1cb/test/recipes/30-test_evp_data/evpkdf_tls13_kdf.txt#L27-L34
func TestHKDFExpandLabel_WithKnownOutput(t *testing.T) {
	secret, _ := hex.DecodeString("153b6394a9c03cf3f5accc6e455a7693281138a1bcfa3803c26735dd1194d216")
	label, _ := hex.DecodeString("6320652074726166666963")
	data, _ := hex.DecodeString("7c92f68bd5bf3638ea338a6494722e1b44127e1b7e8aad535f2322a644ff22b3")
	expected, _ := hex.DecodeString("acaf1b1b0471e0f601be88d9aa96b8974e76dcd1b55d83a222073752c4baeced")

	actual, err := HkdfExpandLabel(sha256.New, secret, fmt.Sprintf(" %s", label), data, 32)
	assert.NoError(t, err)
	assert.Equal(t, expected, actual, "HkdfExpandLabel output mismatch")
}

The expected value I generated with a standalone hkdfExpandLabelDTLS13... since there is no authoritative reference/program to do it... I guess you could argue the utility of checking go we wrote with go we wrote... but in case you are interested:

import (
	"crypto/sha256"
	"fmt"

	"golang.org/x/crypto/cryptobyte"
	"golang.org/x/crypto/hkdf"
)

func hkdfExpandLabelDTLS13(secret []byte, label string, context []byte, length int) ([]byte, error) {
	var builder cryptobyte.Builder

	builder.AddUint16(uint16(length))
	builder.AddUint8LengthPrefixed(func(b *cryptobyte.Builder) { b.AddBytes([]byte("dtls13" + label)) })
	builder.AddUint8LengthPrefixed(func(b *cryptobyte.Builder) { b.AddBytes(context) })

	hkdfLabel, err := builder.Bytes()
	if err != nil {
		return nil, err
	}

	return hkdf.Expand(sha256.New, secret, string(hkdfLabel), length)
}

Or if we wanted we could refactor the code into a function hkdfExpandLabel that takes the prefix as an argument. And we could test that with the exact values from openssl's tests.

Feel free to ignore all this too... this PR is good to go as is IMO.

[theodorsm]

@adrianosela, thanks for the review! I have added some of the nits;)

Regarding the tests, I chose to not add them:

1. The are no ACVP vectors for DTLS 1.3 similar to what openssl's tls13 tests use: https://pages.nist.gov/ACVP/#kdfs. I checked boringSSL and wolfSSL without finding any similar values there either for DTLS. I leaning towards interoptability testing later for verification.
2. hkdfExpandLabelDTLS13 is just a wrapper around Expand from "golang.org/x/crypto/hkdf". I assume we can trust it as they do testing of the function: https://cs.opensource.google/go/go/+/master:src/crypto/hkdf/hkdf_test.go;drc=93b6475e1de0f055139bd0926048ee8e36aa7c42;l=318

[adrianosela]

Ship it!

[JoTurk]

Happy to see this finally merged, it took ages waiting for us to upgrade to 24 :)

[philipch07]

@theodorsm thank you for cleaning up this PR (and the key share extension), and apologies for not getting around to them. The work that both you and @adrianosela have been doing in DTLS has been amazing!! :))