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

Add label parameter to TLS1PRF #109

Merged
merged 1 commit into from
Aug 25, 2023
Merged

Add label parameter to TLS1PRF #109

merged 1 commit into from
Aug 25, 2023

Conversation

qmuntal
Copy link
Collaborator

@qmuntal qmuntal commented Aug 24, 2023

This PR updates the TLS1PRF signature to accept the PRF label as a separate parameter:

func TLS1PRF(secret, label, seed []byte, keyLen int, h func() hash.Hash) ([]byte, error)`

It is common for TLS PRF functions to accept the label as a separate parameter instead of prepending it to the seed. This better match the crypto/tls PRF signature: https://github.com/golang/go/blob/d5c5808534f0ad97333b1fd5fff81998f44986fe/src/crypto/tls/prf.go#L48

Copy link
Collaborator

@dagood dagood left a comment

Choose a reason for hiding this comment

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

My only thought is: maybe the performance will be different if we append the seed on the Go side rather than call into cgo twice to have OpenSSL do it.

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.

+1

@qmuntal
Copy link
Collaborator Author

qmuntal commented Aug 25, 2023

My only thought is: maybe the performance will be different if we append the seed on the Go side rather than call into cgo twice to have OpenSSL do it.

If that slowdown is relevant, then we can always implement part of the function in C while still having a better API 😸

@qmuntal qmuntal merged commit 8cbc4d7 into v2 Aug 25, 2023
10 checks passed
@qmuntal qmuntal deleted the tlsprf branch August 25, 2023 06:17
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