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

Client: Allow explicitely specifying a publicKey #808

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TimWolla
Copy link

@TimWolla TimWolla commented Jul 21, 2019

(see mscdex/ssh2-streams#137)

This is to support SSH certificates. As before the privateKey will
be used for the publicKey (i.e. the derived publicKey) if nothing
is given.

The given publicKey is checked to match the given privateKey.

Closes #551


Example:

const ssh = require('ssh2');
const fs = require('fs');

const conn = new ssh.Client();
conn
.on('ready', function() {
	console.log('Client :: ready');
	conn.exec('ls /', function(err, stream) {
		if (err) throw err;
		stream.on('close', function(code, signal) {
			console.log('Stream :: close :: code: ' + code + ', signal: ' + signal);
			conn.end();
		}).on('data', function(data) {
			console.log('STDOUT: ' + data);
		}).stderr.on('data', function(data) {
			console.log('STDERR: ' + data);
		});
		stream.write('foo');
	});
}).connect({
	// debug: console.log.bind(console),
	username: 'root',
	host: 'xxx',
	privateKey: fs.readFileSync('node'),
	publicKey: fs.readFileSync('node-cert.pub')
})
$ ssh-keygen -s ca -I test_cert -V +60m -O "force-command=env" -O no-agent-forwarding -O no-port-forwarding -O no-pty -O no-user-rc -O no-x11-forwarding node.pub
Signed user key node-cert.pub: id "test_cert" serial 0 valid from 2019-07-21T17:10:00 to 2019-07-21T18:11:34
$ docker run -it --rm -v (pwd):(pwd) -v (pwd):/pwd --workdir /pwd timwolla/node index.js
Client :: ready
STDOUT: SHELL=/bin/bash
PWD=/root
LOGNAME=root
XDG_SESSION_TYPE=tty
HOME=/root
LANG=en_US.UTF-8
SSH_ORIGINAL_COMMAND=ls /
SSH_CONNECTION=xxx 58216 xxx 22
XDG_SESSION_CLASS=user
USER=root
SHLVL=0
XDG_SESSION_ID=36
XDG_RUNTIME_DIR=/run/user/0
SSH_CLIENT=xxx 58216 22
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
MAIL=/var/mail/root
_=/usr/bin/env

Stream :: close :: code: 0, signal: undefined

This is to support SSH certificates. As before the privateKey will
be used for the publicKey (i.e. the derived publicKey) if nothing
is given.

The given publicKey is checked to match the given privateKey.
@SaviourSelf
Copy link

Can this get merged in?

@jisack
Copy link

jisack commented Mar 5, 2020

confirm this is worked for me with Vault module Signed SSH Certificates

@aadityabhatia
Copy link

Tested this out with the latest mscdex/ssh2:master and two sets of user certificates (ssh-rsa-cert-v01@openssh.com and ecdsa-sha2-nistp521-cert-v01@openssh.com). Upon calling conn.connect, I receive the following error:

publicKey does not belong to the private key

Removed the check publicKey.getPublicPEM() !== privateKey.getPublicPEM() on lib/client.js:262 and was still unable to connect:

Error: All configured authentication methods failed

Upon closer examination, publicKey.getPublicPEM() output for an ecdsa certificate looks off (31 bytes and ends in nistp521), so I will look at mscdex/ssh2-streams#137 and run additional tests. Please let me know if I got something wrong in the process.

@aadityabhatia
Copy link

I will look at mscdex/ssh2-streams#137 and run additional tests

I did not realize that mscdex/ssh2-streams#137 was still open. Merged those changes into my local workspace and everything works perfectly. Please disregard the last comment.

@travelton
Copy link

Ping: Any chance we can get this merged soon?

lib/client.js Show resolved Hide resolved
@mscdex
Copy link
Owner

mscdex commented May 29, 2021

I haven't forgotten about this feature, however the PR will need to be updated to match the internal changes in v1.0.0.

@TimWolla
Copy link
Author

TimWolla commented Jul 18, 2021

I haven't forgotten about this feature, however the PR will need to be updated to match the internal changes in v1.0.0.

Whoops. Appears I missed the email notification about this comment.

@mscdex Will you handle this, please? You know the internals better than I do and it's almost 2 years since I last touched it myself.

@mast
Copy link

mast commented Aug 29, 2022

Is there any chance this to be merged soon? I'm considering using this module, but it's lacking authentication with signed public key.

@david-autonoma
Copy link

Hi all,

We also need this feature urgently.
@mscdex or @TimWolla: could one of you implement that within the following days? We would be open to fund it with 200€ if handled timely.

Email me via david.gumpinger@autonoma.cloud if you are interested. Thanks!

@TimWolla
Copy link
Author

@david-autonoma This PR is way outdated and would likely need to be adapted to the 1.0 release of the library. This PR was developed on company time, where we currently don't have a need for this feature (as we've worked around it internally). I'm afraid the costs of relearning how the current version of this library works after 4 years alone would likely exceed the offered compensation.

@david-autonoma
Copy link

Hi @TimWolla, we understand the difficulty. Could you maybe describe how you worked around it internally?

Though, we also have heard, that @mscdex might have a solution for it. We would be very glad if we could have a quick chat or feedback via email. Thank you.

@TimWolla
Copy link
Author

Could you maybe describe how you worked around it internally?

I don't remember the details after 4 years. I believe we used a regular public / private key pair, because while using certificates would have been nicer, the cost of maintaining a fork of this library would not have been worth it.

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.

Feature Request: support certificate authentication.
10 participants