-
Notifications
You must be signed in to change notification settings - Fork 144
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 support for SSH certificates #137
base: master
Are you sure you want to change the base?
Conversation
This is in preparation to support SSH certificates, where the signature type is different to the public key type.
At this time it is identical to pubKeyType. It will differ when SSH certificates are being used.
convertSignature was extended to convert certificate key types into their underlying key types.
getPublicSSH() will simply pass through the original key blob. getPublicPEM() will return raw public key data of the certificate's public key, without incorporating the additional metadata.
d9e9908
to
447a0c6
Compare
@mscdex Did you have time to take a look, yet? |
Would love to have this merged! Using @TimWolla's changes works great in our tests. |
looking forward for this PR is merged. |
Tested it -- works as expected with |
I am trying to use this for SSH Certificate Authority authentication using Hashicorp VAULT SSH CA. I have the appropriate VAULT SSH CA key on the OpenSSH configured as in sshd_config On the client side I have a private key
However I am unable to make work using this branch of SSH2. I am testing with the modified SSH2 example https://github.com/mscdex/ssh2#execute-uptime-on-a-server I get the error: Thank you for your work in this PR
|
switch (keyType) { | ||
case '[email protected]': | ||
case '[email protected]': | ||
case 'ecdsa-sha2-nistp256': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest this entire switch
statement is unnecessary and you could just perform the replace
blind. But this particular case
statement seems redundant as it doesn't end in the appropriate string to qualify for the replace call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact the two cases below are also identical. These should've been [email protected]
, [email protected]
, and [email protected]
.
Not going to fix it, because this PR realistically is not going to be merged due to the planned rewrite of this library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not going to fix it, because this PR realistically is not going to be merged due to the planned rewrite of this library.
Are this issues the same in mscdex/ssh2#808 (which also "primarily awaits an updated PR")?
see mscdex/ssh2#808
The biggest issue when debugging this was was that the code unconditionally used the key type as the signature type. For SSH certificates these do not match up. Apart from that the code already handled certificates well.