Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

crypto: accept ed25519 priv key as a value type #234

Closed

Conversation

areknoster
Copy link

@areknoster areknoster commented Jan 14, 2022

Since underlying type is []byte, the ed25519 key is generally passed as a value, not a pointer.

In particular, nowhere in std crypto package it is, nor any common parsing functions returns it this way.
The test itself shows the issue: whereas all other keys are passed as they are got from their respective generators,
ed25519 needs additional pointer retrieval. Also the behaviour is inconsistent, since in function PubKeyToStdKey in the case for Ed25519 a value to type with underlying []byte is returned, not a pointer.

This is a breaking change though, so if the lib is considered stable, the previous method should be deprecated and new one created.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. The only thing I'd be worried about is that this might break existing code.
I can't see any use of these two functions across the PL stack though.

@vyzo, wdyt?

Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

sounds reasonable, but lets keep compatibility and also accept pointers given by the user.

@areknoster
Copy link
Author

Ok, will adjust, so that both forms are accepted

@areknoster
Copy link
Author

areknoster commented Jan 15, 2022

Adjusted for backward compatibility. In PrivKeyToStdKey solution with adding additional switch case is not possible, since we're returning the pointer value, so I propose deprecation and new PrivKeyToStdCompatKey function

@Stebalien
Copy link
Member

Why not just accept both in a single function?

Since underlying type is []byte, the ed25519 key is generally passed as a value, not a pointer.

In particular, nowhere in std crypto package it is, nor any common parsing functions returns it this way.
The test itself shows the issue: whereas all other keys are passed as they are got from their respective generators,
ed25519 needs additional pointer mapping.  Also the behaviour is inconsistent, since in function PubKeyToStdKey in the case for Ed25519 a value to underlying []byte is returned, not a pointer.
@areknoster areknoster changed the title crypto: stop expecting ed25519 priv key as a pointer crypto: accept ed25519 priv key as a value type Jan 22, 2022
@areknoster
Copy link
Author

Sry for late reply, @Stebalien the changes I propose here in current version do extend KeyPairFromStdKey function to accept value type. I just realised, that I forgot to adjust commit and PR title to communicate the change, which I've just done.

As for the PrivKeyToStdKey, we can't adjust it without breaking changes (pointer to ed25519 priv key is returned), thus I propose to deprecate and have a new one in place.

@BigLep
Copy link

BigLep commented Aug 26, 2022

2022-08-26 triage note: Closing this at this point since it has been 8 months and it hasn't been merged yet. If it is still critical, please go ahead and open a PR in libp2p/go-libp2p since this repo is now archived.

@BigLep BigLep closed this Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P3 Low: Not priority right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants