Add basic support for SSH certificates. #31
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Quite a chunk of information/code to process, so it'll take me a while. I just wanted to give you my compliments on the the great motivation you've written. It's incredibly helpful to get started with review. |
Joannis
reviewed
Jun 11, 2020
| /// Verifies that a given `NIOSSHSignature` was created by the holder of the private key associated with this | ||
| /// public key. | ||
| internal func isValidSignature<DigestBytes: Digest>(_ signature: SSHSignature, for digest: DigestBytes) -> Bool { | ||
| internal func isValidSignature<DigestBytes: Digest>(_ signature: NIOSSHSignature, for digest: DigestBytes) -> Bool { |
Contributor
There was a problem hiding this comment.
Wouldn't for be better as forPublicKey?
Contributor
Author
There was a problem hiding this comment.
In this case the DigestBytes are the bytes of the digest of a message. The naming here is definitely a bit awkward but I followed CryptoKit's construction, which does PublicKey.isValidSignature(_ signature:for digest:).
We could invert this and place the method on NIOSSHSignature instead, which would give us signature.isValid(for digest:with key:) but I don't think that's honestly much better. What do you think?
Contributor
There was a problem hiding this comment.
I agree, I guess there isn't a clearer way to represent this functionality.
| let unexpectedAlgorithm = keyIdentifierBytes.count > 0 ? String(decoding: keyIdentifierBytes, as: UTF8.self) : "<unknown algorithm>" | ||
| throw NIOSSHError.unknownPublicKey(algorithm: unexpectedAlgorithm) | ||
| // We don't know this public key type. Maybe the certified keys do. | ||
| return try buffer.readCertifiedKeyWithoutKeyPrefix(keyIdentifierBytes).map { NIOSSHPublicKey($0) } |
Contributor
There was a problem hiding this comment.
Shouldn't this become .map(NIOSSHPublicKey.init)? Or does that not fit the style guide?
Contributor
Author
There was a problem hiding this comment.
Yup, good idea, thanks.
| /// | ||
| /// NIOSSH supports only ECC-based signatures, either with ECDSA or Ed25519. | ||
| internal struct SSHSignature: Equatable { | ||
| /// This type is intentionally highly opaque: we don't expect users to do anything with this directly. Instead, |
Contributor
There was a problem hiding this comment.
Nit: Instead, could be on the next comment line.
| } | ||
|
|
||
| extension NIOSSHCertifiedPublicKey { | ||
| /// `NIOSSHCertifiedPublicKey` is a struct backed by a class. We do this for a few reasons: |
Contributor
|
I was trying to sniff out a missing test. Considering the amount of usecases I would've expected something to be overlooked, but as far as I'm aware you caught it all. Awesome! |
Motivation: Traditionally, SSH public key authentication uses a rudimentary full-mesh solution to key distribution. If a user wishes to log in to a machine with a public key (or validate a host key), they need to distribute their public key ahead of time, out-of-band. This approach has the advantage of simplicity, making it a great fit for small-scale operation, with small numbers of users and machines. However, as organisations grow it begins to represent a limitation: working out how to distribute keys and associate policy with those keys becomes a substantial technical challenge. To address this, SSH has support for a simplified certificate authority model. This allows servers and clients to delegate their authority to one or more certificate authorities, which become responsible for authenticating keys and providing policies for the keyholders. In larger organisations, this lets servers and clients be configured with a small number of CA keys, and then to apply policy to the certificates they receive. Certificate authorities can restrict the validity of certificates or their capabilities, allowing them to restrict the authority of keyholders. This provides a powerful policy engine that is extremely valuable in larger enterprises. There are two good reasons for swift-nio-ssh to want to build out support for SSH certificates. The first is a simple compatibility issue: SSH certificates present themselves as a somewhat specialised type of public key. In this case, we'd like to be able to work with those kinds of keys, if in no more complex way than by treating them as straightforward public keys. The more important reason is that SSH certificates unlock a bunch of powerful tools for management of SSH in larger organisations. These tools are useful to be able to work with programmatically, and so we can unlock more user value by supporting these key types. This will require further investment going forward, as this initial patch is somewhat limited in scope. Modifications: This patch adds support for parsing/serialising SSH certificates to the wire format, as well as parsing of the OpenSSH public key format for SSH certificates. It also reworks the `NIOSSHPrivateKey` type to be able to encapsulate a certificate. As part of this work, we also promote `SSHSignature` to become a public type, renaming it to `NIOSSHSignature` in the process. Finally, we add a bunch of tests for the basic functionality, including validation. This patch does not contain the following (fairly important) functionality: 1. No support for generating and signing certificates. 2. No support for serializing the OpenSSH public key format for certificates. 3. No proper support for using certificates generated by other tools, as we don't support parsing the OpenSSH private key format. This functionality will come along in later patches. Results: The basic SSH certificate infrastructure is in place. This provides us a jumping off point for implementing the rest of the functionality. In particular, we are highly unlikely to need new public types from this point onward: we'd just be extending the types defined here.
artemredkin
approved these changes
Jun 11, 2020
Joannis
approved these changes
Jun 11, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Traditionally, SSH public key authentication uses a rudimentary
full-mesh solution to key distribution. If a user wishes to log in to a
machine with a public key (or validate a host key), they need to distribute
their public key ahead of time, out-of-band.
This approach has the advantage of simplicity, making it a great fit for
small-scale operation, with small numbers of users and machines.
However, as organisations grow it begins to represent a limitation:
working out how to distribute keys and associate policy with those keys
becomes a substantial technical challenge.
To address this, SSH has support for a simplified certificate authority
model. This allows servers and clients to delegate their authority to
one or more certificate authorities, which become responsible for
authenticating keys and providing policies for the keyholders.
In larger organisations, this lets servers and clients be configured
with a small number of CA keys, and then to apply policy to the
certificates they receive. Certificate authorities can restrict the
validity of certificates or their capabilities, allowing them to
restrict the authority of keyholders. This provides a powerful policy
engine that is extremely valuable in larger enterprises.
There are two good reasons for swift-nio-ssh to want to build out
support for SSH certificates. The first is a simple compatibility issue:
SSH certificates present themselves as a somewhat specialised type of
public key. In this case, we'd like to be able to work with those kinds
of keys, if in no more complex way than by treating them as
straightforward public keys.
The more important reason is that SSH certificates unlock a bunch of
powerful tools for management of SSH in larger organisations. These
tools are useful to be able to work with programmatically, and so we can
unlock more user value by supporting these key types. This will require
further investment going forward, as this initial patch is somewhat
limited in scope.
Modifications:
This patch adds support for parsing/serialising SSH certificates to the
wire format, as well as parsing of the OpenSSH public key format for SSH
certificates. It also reworks the
NIOSSHPrivateKeytype to be able toencapsulate a certificate. As part of this work, we also promote
SSHSignatureto become a public type, renaming it toNIOSSHSignaturein the process.
Finally, we add a bunch of tests for the basic functionality, including
validation.
This patch does not contain the following (fairly important)
functionality:
certificates.
we don't support parsing the OpenSSH private key format.
This functionality will come along in later patches.
Results:
The basic SSH certificate infrastructure is in place. This provides us a
jumping off point for implementing the rest of the functionality. In
particular, we are highly unlikely to need new public types from this
point onward: we'd just be extending the types defined here.