Allow external signer to be implemented on the host side#621
Allow external signer to be implemented on the host side#621j-baker wants to merge 2 commits intoEugeny:mainfrom
Conversation
Eugeny
left a comment
Eugeny
left a comment
There was a problem hiding this comment.
Thank you for the PR! There are some issues with hash alg handling but once that's fixed it should be good to go
| } | ||
|
|
||
| /// Creates a new key pair from an Arc-wrapped signer. | ||
| pub fn from_arc(signer: Arc<dyn PrivateKeySigner>) -> Self { |
There was a problem hiding this comment.
Not needed as there's already a corresponding From<>
| fn from(signer: Arc<dyn PrivateKeySigner>) -> Self { | ||
| KeyPair { | ||
| signer, | ||
| hash_alg: None, |
There was a problem hiding this comment.
hash_alg is never used anywhere except .algorithm() which itself is never used.
Host keys do not need a hash specification since russh assumes an RSA key can use any hash and automatically chooses the hash corresponding to the negotiated key algo (this PR also breaks that, see below).
Essentially, hash_alg needs to remain a parameter in PrivateKeySigner since the hash is only known by the time negotiation has already happened.
| /// let signer = SignerWrapper::new(agent, public_key, None); | ||
| /// let keypair = KeyPair::new(signer); | ||
| /// ``` | ||
| pub struct SignerWrapper<S> { |
There was a problem hiding this comment.
Since PrivateKeySigner is essentially a superset of Signer, I'd prefer the old trait to be completely replaced by PrivateKeySigner. You can keep it &self too and leave the responsibility of syncing to the user.
| pub async fn sign( | ||
| &self, | ||
| data: &[u8], | ||
| _signature_hash_alg: Option<HashAlg>, |
There was a problem hiding this comment.
signature_hash_alg should be used (see above)
| ) | ||
| .map_err(Into::into)?; | ||
| let signature = key | ||
| .sign(&hash, signature_hash_alg) |
There was a problem hiding this comment.
here, signature_hash_alg is the hash choice I meant above
|
|
||
| /// Returns the base algorithm without hash algorithm override. | ||
| /// This is useful for key compatibility checking. | ||
| pub fn base_algorithm(&self) -> Algorithm { |
There was a problem hiding this comment.
No need for this to exist since non-RSA algos are already equal to themselves (for is_key_compatible_with_algo) - Algorithm ignores hash_alg for non-RSA keys
| } | ||
|
|
||
| /// Returns whether this is an RSA key. | ||
| pub fn is_rsa(&self) -> bool { |
j-baker
force-pushed
the
jbaker/external-signer
branch
from
003cd47 to
185d068
Compare
2 participants
Fixes #618.
The only difference I made is that I didn't make this take a Signer because that takes an &mut. Instead I added a new trait which is a bit more strongly typed and added a wrapper so that type is compatible.