refactor: move hardware-independent code out of scard feature #260
Conversation
probakowski
changed the title
|
@TheBestTvarynka this is (much leaner and more correct probably) version of #259 if you have some time for a review |
probakowski
force-pushed
the
probakowski/scard-rework
branch
from
f91c85b to
65f6b76
Compare
I'll review it today |
| sign_data: Box::new(move |data_to_sign| { | ||
| let mut sha1 = Sha1::new(); | ||
| sha1.update(data_to_sign); | ||
| let hash = sha1.finalize().to_vec(); | ||
| let private_key = PrivateKey::from_pem_str(&private_key_pem)?; | ||
| let rsa_private_key = RsaPrivateKey::try_from(&private_key)?; | ||
| Ok(rsa_private_key.sign(Pkcs1v15Sign::new::<Sha1>(), &hash)?) | ||
| }), |
There was a problem hiding this comment.
I'm concerned a bit with removing the smartcard layer and just using the simple RSA signature generation. But I think it's not a problem for us. Because of two reasons:
- Our
winscardcrate does the same. - Anyway, we don't support system-provided smart cards in our current sspi implementation. But we will in the next releases and I'll look into this code and refactor it in any case.
There was a problem hiding this comment.
Thank you for the heads-up! If you’re going to look into it afterwards, I’m fine with that 👍
There was a problem hiding this comment.
The aim was to cut the dependency on winscard crate and to avoid including unused libraries for system-provided smart cards when it's not needed. So yeah, the code is basically copied over from emulated part of winscard crate.
| @@ -54,6 +54,7 @@ impl SmartCard { | |||
| }) | |||
| } | |||
|
|
|||
| #[allow(dead_code)] | |||
There was a problem hiding this comment.
question: Why this code is dead? Can you add a comment explaining why we should keep the code around? Thank you!
There was a problem hiding this comment.
this one is also related to my comment above:
...removing the smartcard layer...
There was a problem hiding this comment.
Indeed! In general, we avoid keeping dead code around so if we decide to do so here, I would like a FIXME comment so we don’t forget forever about it. In this case, I believe you’ll end up re-using these functions later so it’s fine.
There was a problem hiding this comment.
I've added FIXME
|
@CBenoit I merged |
CBenoit
force-pushed
the
probakowski/scard-rework
branch
from
1d8ea49 to
0aff841
Compare
CBenoit
changed the title
This PR aims to move code that doesn't depend on real smart card reader out of
scardfeature.