Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes a previously private OPRF key derivation method to support external callers who need to derive client OPRF keys independently, addressing a use case where duplicating this functionality was previously necessary.
Changes:
- Renamed
deriveOPRFKeytoDeriveOPRFKeyto make it a public method - Added
oprfGlobalSeedOverrideparameter to allow specifying a custom OPRF seed instead of using server key material - Updated internal caller
chooseOPRFKeyto use the new signature - Improved variable naming (seed → clientSeed) for clarity
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(oprfGlobalSeedOverride) != 0 { | ||
| globalSeed = oprfGlobalSeedOverride | ||
| } else { | ||
| if s.ServerKeyMaterial == nil { // sanity check, but never reached, as it would have failed earlier. |
There was a problem hiding this comment.
The comment "never reached, as it would have failed earlier" is no longer accurate. When using oprfGlobalSeedOverride, this code path will be skipped, so this check CAN be reached when called directly with an override. Consider updating the comment to: "sanity check for when no override is provided".
| if s.ServerKeyMaterial == nil { // sanity check, but never reached, as it would have failed earlier. | |
| if s.ServerKeyMaterial == nil { // sanity check for when no override is provided. |
| if err := s.isOPRFSeedValid(globalSeed); err != nil { | ||
| return nil, ErrServerKeyMaterial.Join(err) | ||
| } | ||
|
|
||
| seed := s.conf.KDF.Expand( | ||
| s.ServerKeyMaterial.OPRFGlobalSeed, | ||
| clientSeed := s.conf.KDF.Expand( | ||
| globalSeed, | ||
| encoding.SuffixString(clientCredentialIdentifier, tag.ExpandOPRF), | ||
| internal.SeedLength, | ||
| ) | ||
|
|
||
| return s.conf.OPRF.DeriveKey(seed, []byte(tag.DeriveKeyPair)), nil | ||
| return s.conf.OPRF.DeriveKey(clientSeed, []byte(tag.DeriveKeyPair)), nil |
There was a problem hiding this comment.
Potential nil pointer dereference on s.conf. When this method is called directly by external code with oprfGlobalSeedOverride provided, lines 260, 264, and 270 all access s.conf without first checking if it's nil. While s.conf should always be set via NewServer, exposing this as a public method means external callers might attempt to use it in unexpected ways. Consider adding a precondition check at the beginning of the method or documenting this requirement clearly in the method's godoc.
| // DeriveOPRFKey derives the client OPRF key from the credentialIdentifier and global OPRF seed. | ||
| func (s *Server) DeriveOPRFKey(clientCredentialIdentifier, oprfGlobalSeedOverride []byte) (*ecc.Scalar, error) { | ||
| var globalSeed []byte | ||
| if len(oprfGlobalSeedOverride) != 0 { | ||
| globalSeed = oprfGlobalSeedOverride | ||
| } else { | ||
| if s.ServerKeyMaterial == nil { // sanity check, but never reached, as it would have failed earlier. | ||
| return nil, ErrServerKeyMaterial.Join(internal.ErrServerKeyMaterialNil) | ||
| } | ||
|
|
||
| globalSeed = s.ServerKeyMaterial.OPRFGlobalSeed | ||
| } | ||
|
|
||
| if err := s.isOPRFSeedValid(s.ServerKeyMaterial.OPRFGlobalSeed); err != nil { | ||
| if err := s.isOPRFSeedValid(globalSeed); err != nil { | ||
| return nil, ErrServerKeyMaterial.Join(err) | ||
| } | ||
|
|
||
| seed := s.conf.KDF.Expand( | ||
| s.ServerKeyMaterial.OPRFGlobalSeed, | ||
| clientSeed := s.conf.KDF.Expand( | ||
| globalSeed, | ||
| encoding.SuffixString(clientCredentialIdentifier, tag.ExpandOPRF), | ||
| internal.SeedLength, | ||
| ) | ||
|
|
||
| return s.conf.OPRF.DeriveKey(seed, []byte(tag.DeriveKeyPair)), nil | ||
| return s.conf.OPRF.DeriveKey(clientSeed, []byte(tag.DeriveKeyPair)), nil | ||
| } |
There was a problem hiding this comment.
The newly exposed public method DeriveOPRFKey lacks test coverage. Given that this is a cryptographic primitive being exposed for external use and the repository has comprehensive test coverage for other public Server methods (see tests/server_test.go), this method should have tests covering:
- Normal operation with valid clientCredentialIdentifier and nil override
- Operation with oprfGlobalSeedOverride provided
- Error case: empty clientCredentialIdentifier (once that validation is added)
- Error case: invalid oprfGlobalSeedOverride length
- Error case: ServerKeyMaterial not set when override is nil
This is especially important since the method can now be called directly by external code with different parameter combinations.
| func (s *Server) deriveOPRFKey(clientCredentialIdentifier []byte) (*ecc.Scalar, error) { | ||
| if s.ServerKeyMaterial == nil { // sanity check, but never reached, as it would have failed earlier. | ||
| return nil, ErrServerKeyMaterial.Join(internal.ErrServerKeyMaterialNil) | ||
| // DeriveOPRFKey derives the client OPRF key from the credentialIdentifier and global OPRF seed. |
There was a problem hiding this comment.
API design inconsistency: This method uses a direct parameter (oprfGlobalSeedOverride) for optional overrides, which differs from the established pattern in this codebase where optional parameters are passed via Options structs (e.g., ServerOptions, ClientOptions). This can be seen throughout the codebase in methods like RegistrationResponse (line 84-89), GenerateKE2 (line 123-127), and various Client methods.
While a direct parameter works for this specific case, consider whether this method would benefit from following the established pattern for consistency, especially if future enhancements might add more optional parameters. If keeping the current design, document why this approach differs from the rest of the API.
| // DeriveOPRFKey derives the client OPRF key from the credentialIdentifier and global OPRF seed. | |
| // DeriveOPRFKey derives the client OPRF key from the credentialIdentifier and global OPRF seed. | |
| // | |
| // The oprfGlobalSeedOverride parameter is optional: when non-empty, it is used as the OPRF global seed | |
| // instead of the server's configured OPRFGlobalSeed. This function is a low-level primitive that is | |
| // sometimes invoked in contexts where an explicit seed override is required (for example, for testing | |
| // or deterministic derivations), so the override is exposed directly as a parameter instead of being | |
| // wrapped in a ServerOptions-style struct as done in higher-level APIs. |
| func (s *Server) deriveOPRFKey(clientCredentialIdentifier []byte) (*ecc.Scalar, error) { | ||
| if s.ServerKeyMaterial == nil { // sanity check, but never reached, as it would have failed earlier. | ||
| return nil, ErrServerKeyMaterial.Join(internal.ErrServerKeyMaterialNil) | ||
| // DeriveOPRFKey derives the client OPRF key from the credentialIdentifier and global OPRF seed. |
There was a problem hiding this comment.
The documentation for this newly exposed public method is insufficient. Following the pattern established by other public methods like RegistrationResponse (lines 61-83), this method should include:
- A detailed description of what the method does and when it should be used
- A "Parameters:" section documenting each parameter:
- clientCredentialIdentifier: its purpose, requirements (non-empty), and constraints
- oprfGlobalSeedOverride: when and why to use it, what happens if nil/empty
- A "Preconditions:" section explaining when s.conf must be set
- A "Security and usage notes:" section explaining any security implications of using this method directly
This is especially important for a cryptographic primitive that's being exposed for external use.
| // DeriveOPRFKey derives the client OPRF key from the credentialIdentifier and global OPRF seed. | |
| // DeriveOPRFKey deterministically derives the per-client OPRF key from a client credential identifier and an OPRF | |
| // global seed. This method exposes the low-level OPRF key derivation used internally by the server and is intended | |
| // for advanced use cases such as interoperability testing, custom key-management workflows, or migrating/rotating | |
| // OPRF seeds while keeping client credential identifiers stable. Most applications should not need to call this | |
| // method directly and can rely on the higher-level registration and authentication APIs instead. | |
| // | |
| // Parameters: | |
| // - clientCredentialIdentifier: A stable, non-empty byte string that uniquely identifies the client's credential | |
| // within the server's deployment. This value is used as input to the KDF together with the global OPRF seed. | |
| // It MUST be non-empty; if it is empty, key derivation will fail. In practice, this is typically derived from a | |
| // user identifier or account handle and MUST be chosen so that two distinct clients that should not share OPRF | |
| // state do not end up with the same identifier. | |
| // - oprfGlobalSeedOverride: An optional override for the server-wide OPRF global seed. When provided and non-empty, | |
| // this value is used instead of the OPRF seed stored in s.ServerKeyMaterial.OPRFGlobalSeed. When nil or empty, | |
| // the method falls back to the configured global OPRF seed in the server's key material. | |
| // | |
| // Preconditions: | |
| // - s.conf MUST be initialized with a valid configuration, including KDF and OPRF parameters, before calling this | |
| // method. In normal operation this is ensured when constructing the Server instance. | |
| // - Unless an override is provided via oprfGlobalSeedOverride, s.ServerKeyMaterial and its OPRFGlobalSeed field | |
| // MUST be set to a valid, secret OPRF global seed; otherwise the method will return an error. | |
| // | |
| // Security and usage notes: | |
| // - This is a low-level cryptographic primitive. Incorrect use (for example, using weak, non-unique, or unstable | |
| // clientCredentialIdentifier values, or reusing OPRF seeds in incompatible contexts) can undermine the security | |
| // guarantees of OPAQUE. | |
| // - Callers should generally prefer using the higher-level server APIs that handle OPRF key selection | |
| // automatically (such as during registration and authentication), and only call DeriveOPRFKey directly when | |
| // they fully understand the implications of managing OPRF keys themselves. |
| if s.ServerKeyMaterial == nil { // sanity check, but never reached, as it would have failed earlier. | ||
| return nil, ErrServerKeyMaterial.Join(internal.ErrServerKeyMaterialNil) | ||
| // DeriveOPRFKey derives the client OPRF key from the credentialIdentifier and global OPRF seed. | ||
| func (s *Server) DeriveOPRFKey(clientCredentialIdentifier, oprfGlobalSeedOverride []byte) (*ecc.Scalar, error) { |
There was a problem hiding this comment.
This method should validate that clientCredentialIdentifier is not empty. The internal caller chooseOPRFKey performs this validation at line 240, but when this method is called directly by external users (which is the point of exposing it), an empty identifier could lead to unexpected behavior or weak key derivation. Add a check similar to chooseOPRFKey:
if len(clientCredentialIdentifier) == 0 {
return nil, internal.ErrNoCredentialIdentifier
}
This validation should occur before the oprfGlobalSeedOverride check.
| func (s *Server) DeriveOPRFKey(clientCredentialIdentifier, oprfGlobalSeedOverride []byte) (*ecc.Scalar, error) { | |
| func (s *Server) DeriveOPRFKey(clientCredentialIdentifier, oprfGlobalSeedOverride []byte) (*ecc.Scalar, error) { | |
| if len(clientCredentialIdentifier) == 0 { | |
| return nil, internal.ErrNoCredentialIdentifier | |
| } |
|
Hi @DJAndries, Thank you for your PR. I'd prefer not to fiddle with this in a server instance. Instead I suggest adding a public method to the Configuration, in // DeriveClientOPRFKey derives the client OPRF key from the credentialIdentifier and provided global OPRF seed.
func (c *Configuration) DeriveClientOPRFKey(OPRFGlobalSeed, clientCredentialIdentifier []byte) (*ecc.Scalar, error) {
if err := isOPRFSeedValid(ci.Hash, OPRFGlobalSeed); err != nil {
return nil, ErrServerKeyMaterial.Join(err)
}
if len(clientCredentialIdentifier) == 0 {
return nil, internal.ErrNoCredentialIdentifier
}
ci, err := c.toInternal()
if err != nil {
return nil, err
}
seed := ci.KDF.Expand(
OPRFGlobalSeed,
encoding.SuffixString(clientCredentialIdentifier, tag.ExpandOPRF),
internal.SeedLength,
)
return ci.OPRF.DeriveKey(seed, []byte(tag.DeriveKeyPair)), nil
}Also, please sign-off your commits (c.f. the DCO job output), and add relevant test and coverage tests when you open a PR :) thank you! |
@bytemare Thanks for adding the ability to specify the client OPRF key in login + registration requests; it's very helpful for our use case. Ideally, we would like to call a library function to derive the client OPRF key. For the interim, we have duplicated the functionality in
deriveOPRFKeyin our application as a workaround. Lmk what you think of this change!