Skip to content

[jwk] Change ecdh importer/exporter#1315

Merged
lestrrat merged 8 commits intodevelop/v3from
gh-1265
Mar 12, 2025
Merged

[jwk] Change ecdh importer/exporter#1315
lestrrat merged 8 commits intodevelop/v3from
gh-1265

Conversation

@lestrrat
Copy link
Collaborator

@lestrrat lestrrat commented Mar 12, 2025

Supersedes #1266
fixes #1265

OP in #1265 didn't respond, but I'm reviving this, and will merge once all checks pass.

@lestrrat lestrrat requested a review from Copilot March 12, 2025 23:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the JWK package to improve the ECDH key importer/exporter mechanism while adding a new test case to validate the changes.

  • Introduces a new test case (TestGH1262) for ECDH key conversion in jwk_test.go.
  • Refactors key importer registrations and adds dedicated conversion functions for ECDH keys in jwk/convert.go.
  • Enhances key conversion support in jwk/ecdsa.go by handling both ECDSA and ECDH key types in the conversion routines.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
jwk/jwk_test.go Adds a test to validate correct handling of ECDH key conversions
jwk/convert.go Refactors importer registrations and adds new conversion functions for ECDH keys
jwk/ecdsa.go Updates raw key conversion to support ECDH alongside ECDSA
Comments suppressed due to low confidence (2)

jwk/convert.go:158

  • [nitpick] Consider updating the error message here to reflect that the conversion is intended for an ECDH key rather than an OKP key, to improve clarity for future debugging.
return nil, fmt.Errorf(`cannot convert key type '%T' to OKP jwk.Key`, src)

jwk/ecdsa.go:148

  • [nitpick] The removal of a catch-all interface{} case in the type switch for the destination object might cause valid inputs to unexpectedly trigger an error. Please verify that the stricter type check aligns with the intended usage scenarios.
case ecdsa.PublicKey, *ecdsa.PublicKey:

@lestrrat lestrrat requested a review from Copilot March 12, 2025 23:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the key importer/exporter logic for ECDH support by revising key registration in the init function, adding dedicated conversion functions for ECDH keys, and extending public/private key conversion in the ECDSA-related code.

  • Splits ECDH key registrations from OKP key registrations
  • Adds conversion functions (ecdhPrivateKeyToJWK, ecdhPrivateKeyToECJWK, ecdhPublicKeyToJWK, ecdhPublicKeyToECJWK) for ECDH keys
  • Extends ECDSA JWK-to-raw conversion to support ECDH keys via new helper functions

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
jwk/jwk_test.go Adds a test case demonstrating the updated ECDH importer/exporter logic
jwk/convert.go Refactors key importer registration and adds conversion logic for ECDH keys
jwk/ecdsa.go Introduces helper functions for building ECDH keys and updates conversion
Comments suppressed due to low confidence (2)

jwk/jwk_test.go:2008

  • [nitpick] The test name 'TestGH1262' is not descriptive. Consider renaming it to something that clearly indicates its purpose, such as 'TestECDHImporterExporter'.
func TestGH1262(t *testing.T) {

jwk/ecdsa.go:107

  • [nitpick] Consider adding a brief comment explaining the expected uncompressed point format (e.g. the 0x04 prefix) used in constructing the ECDH public key to improve clarity for future maintainers.
func buildECDHPublicKey(alg jwa.EllipticCurveAlgorithm, xbuf, ybuf []byte) (*ecdh.PublicKey, error) {

@lestrrat lestrrat merged commit 7805e8a into develop/v3 Mar 12, 2025
23 checks passed
@lestrrat lestrrat deleted the gh-1265 branch March 12, 2025 23:49
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.

[v3] wrong curve when marshaling ecdh keypair

2 participants