-
Notifications
You must be signed in to change notification settings - Fork 599
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 authentication with DSA & RSA user certificates (#153) #319
Conversation
Updates: - KeyType.java - add support for two certificate key types ssh-rsa-cert-v01@openssh.com ssh-dsa-cert-v01@openssh.com - Buffer.java - allow uint64s that overflow Long.MAX_VALUE, otherwise we break on certificates with serial numbers greater Long.MAX_VALUE - OpenSSHKeyFile, KeyProviderUtil - prefer public key files that end "-cert.pub" if they exist Added new class Certificate, which represents certificate key Reference: https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.certkeys?annotate=HEAD
Hi @chqr, Thanks for the PR. Looks pretty good already. |
Hi @hierynomus, Thanks for the fast response! Looks like MPInts are encoded differently on the wire from UInt64s, so they can't be used interchangeably. But I could add new methods readUInt64AsBigInteger() and putUInt64(BigInteger uint64), and revert the changes to the existing put/readUInt64 methods. Will look at the Codacy issues. |
byte[] signature; | ||
|
||
try { | ||
nonce = buf.readStringAsBytes(); |
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.
Can we immediately build the builder instead of first defining all the fields, then assigning them, and then passing them all into the builder?
critOptions = unpackMap(buf.readStringAsBytes()); | ||
extensions = unpackMap(buf.readStringAsBytes()); | ||
buf.readString(); // reserved | ||
signatureKey = buf.readStringAsBytes(); |
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.
Why not readBytes()
?
extensions = unpackMap(buf.readStringAsBytes()); | ||
buf.readString(); // reserved | ||
signatureKey = buf.readStringAsBytes(); | ||
signature = buf.readStringAsBytes(); |
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.
Why not readBytes()
if (f.exists()) | ||
// try cert key location first | ||
File pubKey = new File(location + "-cert.pub"); | ||
if (! pubKey.exists()) { |
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.
formatting, there should be no space between !
and statement.
.putString(packMap(certificate.getCritOptions())) | ||
.putString(packMap(certificate.getExtensions())) | ||
.putString("") // reserved | ||
.putString(certificate.getSignatureKey()) |
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.
putBytes
(also for other places). Esp. signatures and keys are not strings but rather byte[]
.
Though the implementation in the buffer doesn't differ, the semantics are different.
Had a bit more in-depth look this morning, few questions/issues left. |
Thanks for the great contribution! |
First pass at support for RSA & DSA certificate authentication. Thanks for maintaining this awesome library!
Updates:
KeyType.java - add support for two certificate key types
ssh-rsa-cert-v01@openssh.com
ssh-dsa-cert-v01@openssh.com
Buffer.java - allow uint64s that overflow Long.MAX_VALUE, otherwise
we break on certificates with serial numbers greater Long.MAX_VALUE
OpenSSHKeyFile, KeyProviderUtil - prefer public key files that end
"-cert.pub" if they exist
Added new class Certificate, which represents certificate key
Reference:
https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.certkeys?annotate=HEAD