Skip to content

Support OpenSSH public key format. #29

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.

Sign up for GitHub

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

Merged
Lukasa merged 1 commit into from
Jun 4, 2020
Merged

Support OpenSSH public key format. #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 38 additions & 7 deletions Sources/NIOSSH/Keys And Signatures/NIOSSHPublicKey.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
//===----------------------------------------------------------------------===//

import Crypto
import Foundation
Copy link
Contributor

@artemredkin artemredkin Jun 4, 2020

Choose a reason for hiding this comment

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

what do we need it for?

Copy link
Contributor Author

@Lukasa Lukasa Jun 4, 2020

Choose a reason for hiding this comment

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

Base64, sadly.

Copy link
Contributor Author

@Lukasa Lukasa Jun 4, 2020

Choose a reason for hiding this comment

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

It's not a meaningful addition: Crypto depends on Foundation anyway, so we already have the dependency.

import NIO

/// An SSH public key.
Expand All @@ -29,6 +30,31 @@ public struct NIOSSHPublicKey: Hashable {
internal init(backingKey: BackingKey) {
self.backingKey = backingKey
}

/// Create a `NIOSSHPublicKey` from the OpenSSH public key string.
public init(openSSHPublicKey: String) throws {
// The OpenSSH public key format is like this: "algorithm-id base64-encoded-key comments"
//
// We split on spaces, no more than twice. We then check if we know about the algorithm identifier and, if we
// do, we parse the key.
var components = ArraySlice(openSSHPublicKey.split(separator: " ", maxSplits: 2, omittingEmptySubsequences: true))
Copy link
Contributor

@Joannis Joannis Jun 4, 2020

Choose a reason for hiding this comment

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

Is there a specification for this format. Or is this a non-written standard? A quick search resulted in references to the protocol format, but not towards the format specified above.

The implementation uses this format as a String. Would it be helpful to split up the public initialiser to accept two parameters? One for the algorithm, and one for the base64 encoded bytes? Or would it be worth considering just decoding the base64, since the only purpose of the algorithm-id is to verify it against the neighbouring base64 contents?

What's the use case for this public implementation? Where does the string originate from?

Copy link
Contributor Author

@Lukasa Lukasa Jun 4, 2020

Choose a reason for hiding this comment

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

I'm not aware of a formal specification: this format is used by OpenSSH, not by the SSH protocol itself, so any specification would be documented there. However, I don't see one.

The implementation uses this format as a String. Would it be helpful to split up the public initialiser to accept two parameters?

We could, but I don't think it's terribly useful. Most users will have the content as a single whole string, as I'll show below.

What's the use case for this public implementation? Where does the string originate from?

This is the format of public keys in the .ssh directory, or /etc/ssh. This is useful, then, in cases where we'd like to load public keys from those locations. We can do that when we're trying to offer keys to remote peers, or when we're trying to evaluate the keys presented by those peers.

There is an associated private key format, but I wanted to keep it out of this diff for now. We can evaluate adding that as well.

guard let keyIdentifier = components.popFirst(), let keyData = components.popFirst() else {
throw NIOSSHError.invalidOpenSSHPublicKey(reason: "invalid number of sections")
}
guard let rawBytes = Data(base64Encoded: String(keyData)) else {
throw NIOSSHError.invalidOpenSSHPublicKey(reason: "could not base64-decode string")
}

var buffer = ByteBufferAllocator().buffer(capacity: rawBytes.count)
buffer.writeBytes(rawBytes)
guard let key = try buffer.readSSHHostKey() else {
throw NIOSSHError.invalidOpenSSHPublicKey(reason: "incomplete key data")
}
guard key.keyPrefix.elementsEqual(keyIdentifier.utf8) else {
throw NIOSSHError.invalidOpenSSHPublicKey(reason: "inconsistent key type within openssh key format")
}
self = key
}
}

extension NIOSSHPublicKey {
Expand Down Expand Up @@ -184,23 +210,28 @@ extension ByteBuffer {
mutating func readSSHHostKey() throws -> NIOSSHPublicKey? {
try self.rewindOnNilOrError { buffer in
// The wire format always begins with an SSH string containing the key format identifier. Let's grab that.
guard var keyIdentifierBytes = buffer.readSSHString() else {
guard let keyIdentifierBytes = buffer.readSSHString() else {
return nil
}

// Now we need to check if they match our supported key algorithms.
let bytesView = keyIdentifierBytes.readableBytesView
if bytesView.elementsEqual(NIOSSHPublicKey.ed25519PublicKeyPrefix) {
return try buffer.readPublicKeyWithoutPrefixForIdentifier(keyIdentifierBytes.readableBytesView)
}
}

mutating func readPublicKeyWithoutPrefixForIdentifier<Bytes: Collection>(_ keyIdentifierBytes: Bytes) throws -> NIOSSHPublicKey? where Bytes.Element == UInt8 {
try self.rewindOnNilOrError { buffer in
if keyIdentifierBytes.elementsEqual(NIOSSHPublicKey.ed25519PublicKeyPrefix) {
return try buffer.readEd25519PublicKey()
} else if bytesView.elementsEqual(NIOSSHPublicKey.ecdsaP256PublicKeyPrefix) {
} else if keyIdentifierBytes.elementsEqual(NIOSSHPublicKey.ecdsaP256PublicKeyPrefix) {
return try buffer.readECDSAP256PublicKey()
} else if bytesView.elementsEqual(NIOSSHPublicKey.ecdsaP384PublicKeyPrefix) {
} else if keyIdentifierBytes.elementsEqual(NIOSSHPublicKey.ecdsaP384PublicKeyPrefix) {
return try buffer.readECDSAP384PublicKey()
} else if bytesView.elementsEqual(NIOSSHPublicKey.ecdsaP521PublicKeyPrefix) {
} else if keyIdentifierBytes.elementsEqual(NIOSSHPublicKey.ecdsaP521PublicKeyPrefix) {
return try buffer.readECDSAP521PublicKey()
} else {
// We don't know this public key type.
let unexpectedAlgorithm = keyIdentifierBytes.readString(length: keyIdentifierBytes.readableBytes) ?? "<unknown algorithm>"
let unexpectedAlgorithm = keyIdentifierBytes.count > 0 ? String(decoding: keyIdentifierBytes, as: UTF8.self) : "<unknown algorithm>"
throw NIOSSHError.unknownPublicKey(algorithm: unexpectedAlgorithm)
}
}
Expand Down
9 changes: 9 additions & 0 deletions Sources/NIOSSH/NIOSSHError.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ extension NIOSSHError {
internal static func invalidHostKeyForKeyExchange(expected: Substring, got actual: String.UTF8View) -> NIOSSHError {
NIOSSHError(type: .invalidHostKeyForKeyExchange, diagnostics: "Expected \(String(expected)), got \(String(actual))")
}

@inline(never)
Copy link
Contributor

@Joannis Joannis Jun 4, 2020

Choose a reason for hiding this comment

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

What is the purpose of this @inline(never)?

Copy link
Contributor Author

@Lukasa Lukasa Jun 4, 2020

Choose a reason for hiding this comment

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

We believe that the case of throwing an error here will be very rare. For this reason, we can't expect to gain any performance from inlining this method: it'll cost binary size, and potentially prevent the compiler inlining something else that would be more useful to performance. For that reason we just force the compiler not to do that.

Copy link
Contributor

@Joannis Joannis Jun 4, 2020

Choose a reason for hiding this comment

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

Interesting micro optimisation!

internal static func invalidOpenSSHPublicKey(reason: String) -> NIOSSHError {
NIOSSHError(type: .invalidOpenSSHPublicKey, diagnostics: reason)
}
}

// MARK: - NIOSSHError CustomStringConvertible conformance.
Expand Down Expand Up @@ -167,6 +172,7 @@ extension NIOSSHError {
case missingGlobalRequestResponse
case remotePeerDoesNotSupportMessage
case invalidHostKeyForKeyExchange
case invalidOpenSSHPublicKey
}

private var base: Base
Expand Down Expand Up @@ -258,6 +264,9 @@ extension NIOSSHError {

/// The peer has sent a host key that does not correspond to the one negotiated in key exchange.
public static let invalidHostKeyForKeyExchange: ErrorType = .init(.invalidHostKeyForKeyExchange)

/// The OpenSSH public key string could not be parsed.
public static let invalidOpenSSHPublicKey: ErrorType = .init(.invalidOpenSSHPublicKey)
}
}

Expand Down
50 changes: 50 additions & 0 deletions Tests/NIOSSHTests/HostKeyTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,4 +297,54 @@ final class HostKeyTests: XCTestCase {
XCTAssertEqual((error as? NIOSSHError).map { $0.type }, .unknownSignature)
}
}

func testLoadingEd25519KeyFromFileRoundTrips() throws {
let keyData = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIJfkNV4OS33ImTXvorZr72q4v5XhVEQKfvqsxOEJ/XaR [email protected]"
XCTAssertNoThrow(try self.roundTripKey(keyData: keyData, label: "ssh-ed25519", comment: " [email protected]"))
}

func testLoadingP256KeyFromFileRoundTrips() throws {
let keyData = "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBIZS1APJofiPeoATC/VC4kKi7xRPdz934nSkFLTc0whYi3A8hEKHAOX9edgL1UWxRqRGQZq2wvvAIjAO9kCeiQA= [email protected]"
XCTAssertNoThrow(try self.roundTripKey(keyData: keyData, label: "ecdsa-sha2-nistp256", comment: " [email protected]"))
}

func testLoadingP384KeyFromFileRoundTrips() throws {
let keyData = "ecdsa-sha2-nistp384 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAAABhBJPOgAXHijSxoZBiyhSDOR3eUELUoc+hqh/SY1Wq4/562jThf6Q+tjVzZTMWZMAP4S6DD2qZswsRvisxXkcZDOw5bvyk0WmezYvjUP6TZII/0BDVTotCf4SxukEtcqBZqg== [email protected]"
XCTAssertNoThrow(try self.roundTripKey(keyData: keyData, label: "ecdsa-sha2-nistp384", comment: " [email protected]"))
}

func testLoadingP521KeyFromFileRoundTrips() throws {
let keyData = "ecdsa-sha2-nistp521 AAAAE2VjZHNhLXNoYTItbmlzdHA1MjEAAAAIbmlzdHA1MjEAAACFBACkfM3aZf9sgjAkncWtK6A295sdghn1GG1BKJ+hQfD2VBIJxSQDnPOocNIQQZEo3zs1kvwUXOIgWANJqbOiv77tCACxWRRYmAvM3hzgcEOhPROROG+KGvuDAWW6ZuCkaW0QnseR7Yn0+q/+/jai3tNNDWrbVLDesDj5Aq5xq1yrKDHGEA== [email protected]"
XCTAssertNoThrow(try self.roundTripKey(keyData: keyData, label: "ecdsa-sha2-nistp521", comment: " [email protected]"))
}

func testMissingCommentIsTolerated() throws {
let keyData = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIJfkNV4OS33ImTXvorZr72q4v5XhVEQKfvqsxOEJ/XaR"
XCTAssertNoThrow(try self.roundTripKey(keyData: keyData, label: "ssh-ed25519", comment: ""))
}

func testDripFeedingKey() throws {
let keyData = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIJfkNV4OS33ImTXvorZr72q4v5XhVEQKfvqsxOEJ/XaR"
for index in keyData.indices.dropLast() {
XCTAssertThrowsError(try NIOSSHPublicKey(openSSHPublicKey: String(keyData[..<index]))) { error in
XCTAssertEqual((error as? NIOSSHError)?.type, .invalidOpenSSHPublicKey)
}
}
}

func testKeyLiesAboutItsType() throws {
// Secretly P384
let keyData = "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAzODQAAAAIbmlzdHAzODQAAABhBJPOgAXHijSxoZBiyhSDOR3eUELUoc+hqh/SY1Wq4/562jThf6Q+tjVzZTMWZMAP4S6DD2qZswsRvisxXkcZDOw5bvyk0WmezYvjUP6TZII/0BDVTotCf4SxukEtcqBZqg== [email protected]"
XCTAssertThrowsError(try NIOSSHPublicKey(openSSHPublicKey: keyData)) { error in
XCTAssertEqual((error as? NIOSSHError)?.type, .invalidOpenSSHPublicKey)
}
}

private func roundTripKey(keyData: String, label: String, comment: String) throws {
let key = try assertNoThrowWithValue(NIOSSHPublicKey(openSSHPublicKey: keyData))
var keyBuffer = ByteBufferAllocator().buffer(capacity: 1024)
keyBuffer.writeSSHHostKey(key)
let expectedKeyData = "\(label) \(keyBuffer.readData(length: keyBuffer.readableBytes)!.base64EncodedString())\(comment)"
XCTAssertEqual(keyData, expectedKeyData)
}
}