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

Enhancement to present KeyPair.getKeyTypeString() method #259

Merged
merged 1 commit into from
Jan 3, 2023

Conversation

Alex-Vol-Amz
Copy link
Contributor

The getSshName() method was available for most KeyPair implementations but inconsistently and not as part of the KeyPair class interface. This makes the method part of the KeyPair class interface and makes each implementation provide the correct value to the caller. I found this to be useful when importing keys from unknown files and expecting the KeyPair class to provide a hint as to the loaded key type in a way that can be used by code logic. I prefer this to doing class type inspection as it is much easier to get it right expecially for the ECDSA KeyPair which can have different SshName based on the key size.

@norrisjeremy
Copy link
Contributor

There's already a getKeyType() method: is there a reason that isn't sufficient?

@Alex-Vol-Amz
Copy link
Contributor Author

Alex-Vol-Amz commented Jan 3, 2023

The key type only gives a coarse level type. ECDSA keys for example have an SSH name that includes the key size encoded in them. I find this useful to have in my use cases. Since it is already available I see no harm in the added public method. It does not cause any backwards compatibility issues as far as I know and it is a convenient property to have.

@norrisjeremy
Copy link
Contributor

If we're going to promote this as a public API method, I'd like to name it something better than getSshName().
I originally added it only to KeyPairEdDSA as an internal method to allow easy abstraction between Ed25519 & Ed448 and didn't consider it for use as a public API method.

How about naming it getKeyTypeString()? And then we can just add it directly to KeyPair.java as a wrapper around getKeyTypeName(), avoiding any changes to implementation classes:

public String getKeyTypeString() {
  return Util.byte2str(getKeyTypeName());
}

@Alex-Vol-Amz
Copy link
Contributor Author

Alex-Vol-Amz commented Jan 3, 2023

I appreciate your suggestion. I will make the change to the new name and see if the getKeyTypeName() wrapper works.

I only used the getSshName() as I noted it was already present.

I will also add Javadoc to the new method which I did not in the first revision.

The package private method `getKeyTypeName()` is a convenient way to get
the standard key name for SSH protocol keys. This method gives access to
it as a String and makes it common to all KeyPair types.
@Alex-Vol-Amz Alex-Vol-Amz force-pushed the enhance-KeyPair-with-getSshName branch from e00b984 to 4b94250 Compare January 3, 2023 14:55
@Alex-Vol-Amz Alex-Vol-Amz changed the title Enhancement to present KeyPair.getSshName() method Enhancement to present KeyPair.getKeyTypeString() method Jan 3, 2023
@mwiede mwiede merged commit a907690 into mwiede:master Jan 3, 2023
@Alex-Vol-Amz Alex-Vol-Amz deleted the enhance-KeyPair-with-getSshName branch January 3, 2023 15:39
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.

None yet

3 participants