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

tls1prf: require callers to pass in the result buffer #116

Merged
merged 3 commits into from
Sep 15, 2023
Merged

Conversation

qmuntal
Copy link
Collaborator

@qmuntal qmuntal commented Sep 8, 2023

This PR updates TLS1PRF to accept a byte slice parameter where to write the output. This avoids allocating a new slice on each function call and integrates better with the standard library, which expects the PRF to update an already existing slice: https://github.com/golang/go/blob/2f0b28da1900909a2c3ddf646bb508fc7effb8f2/src/crypto/tls/prf.go#L68.

To make is clear, the current code would have to be integrated like this:

func prf12(hashFunc func() hash.Hash) func(result, secret, label, seed []byte) error {
	return func(result, secret, label, seed []byte) error {
		if backend.Enabled && backend.SupportsTLS1PRF() {
			out, err := backend.TLS1PRF(secret, label, seed, len(result), hashFunc)
			if err != nil {
				return fmt.Errorf("crypto/tls: prf12: %v", err)
			}
			copy(result, out)
			return nil
		}
                ...
	}
}

While with the new approach, it would like this:

func prf12(hashFunc func() hash.Hash) func(result, secret, label, seed []byte) error {
	return func(result, secret, label, seed []byte) error {
		if backend.Enabled && backend.SupportsTLS1PRF() {
			err := backend.TLS1PRF(result, secret, label, seed, len(result), hashFunc)
			if err != nil {
				return fmt.Errorf("crypto/tls: prf12: %v", err)
			}
			return nil
		}
                ...
	}
}

@qmuntal qmuntal requested review from ueno and dagood September 8, 2023 11:14
Copy link
Collaborator

@ueno ueno left a comment

Choose a reason for hiding this comment

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

Makes sense to me; I thought it might be good to align with other KDF API (e.g., either crypto/pbkdf2 or crypto/hkdf), though TLS1PRF is purely for internal use, so it would be better to align with the actual usage in crypto/tls.

@qmuntal qmuntal requested a review from gdams September 12, 2023 13:59
tls1prf.go Outdated Show resolved Hide resolved
Co-authored-by: Davis Goodin <[email protected]>
@qmuntal qmuntal merged commit 1ee02b5 into v2 Sep 15, 2023
16 checks passed
@qmuntal qmuntal deleted the tls1prgret branch September 15, 2023 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants