Smart card credentials handling #206
Conversation
…and Clone for SmartCardContext
…uireCredentialHandle[W,A] functions
src/kerberos/mod.rs
Outdated
| // the original flag is | ||
| // GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG | GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG | ||
| // we want to be able to turn of sign and seal, so we leave confidentiality and integrity flags out | ||
| let flags = builder.context_requirements.into(); | ||
| let mut checksum_value = ChecksumValues::default(); | ||
| checksum_value.set_flags(flags); | ||
| // let flags = builder.context_requirements.into(); | ||
| let checksum_value = ChecksumValues::default(); | ||
| // checksum_value.set_flags(flags); |
There was a problem hiding this comment.
We have a problem here. During the dev testing, I had a problem with this code. When I uncomment these flags, then the CredSSP server returns an IvalidToken error. I'm not sure why, but when commented on it, then authentication succeeded.
There was a problem hiding this comment.
That’s suspicious.
For now, can you leave a FIXME at this place? We’ll have to address this before merging into master.
There was a problem hiding this comment.
I added a comment here.
Can I take this issue today? I want to merge smart card implementation into a master quickly.
| // remove null | ||
| let new_len = password.as_ref().len() - 2; | ||
| password.as_mut().truncate(new_len); |
There was a problem hiding this comment.
nitpick: I recommend writing "proper sentences" to create a more appropriate frame of mind when writing comments. The expectation is to write down more of the context you had in mind when writing this code.
For instance, in this case what I really want to know as someone not familiar with the smart card protocol is "why we need to remove the null terminator at this place?" or "why is there a null terminator", etc. It’s okay to not always explain when it’s just a C-style UTF-16 string → Rust string conversion, but this is not what it is. (And if it was, using the appropriate function would be explicit enough.)
This comment can be applied at several places, but I leave you decide if and how much you do it.
(All that being said, the code you wrote is generally well documented, thank you again.)
There was a problem hiding this comment.
I agree with you. Can you check it again? I updated some comments
There was a problem hiding this comment.
It’s looking much better! For this place specifically, I would like something more like that:
The "pin" buffer of the SmartCardIdentityBuffers structure must not be NULL-terminated because / as specified in […] (protocol spec reference? empirical observation?).
Since the password data returned by CredUnPackAuthenticationBufferW is a NULL-terminated wide C string, we need to remove the last two bytes.
The conversion itself is a detail, and what is important is the surroundings, the things we can’t see just by looking at the code.
(Again, I’m really being pedantic here.)
There was a problem hiding this comment.
Again, I’m really being pedantic here.
I can understand you and I like it 😃.
I added more comprehensive explanations
There was a problem hiding this comment.
LGTM!
I’ll approve the changes as-is.
The flag problem mentioned here must be addressed before we merge into master, but it’s okay to merge this PR into the winscard dev branch when you are ready @TheBestTvarynka
If you can add a FIXME just to not forget about it before, that would be perfect.
Thank you!
Hi,
In this PR I implemented the smart card credentials handling on the SSPI side. How it works: if the provided username is
NT_ENTERPRISEand all needed environment variables with smart card credentials are present, then the SSPI will try to authenticate using an emulated smart card.Also, I made a small refactoring in the ./ffi/src/sspi module.
All needed env vars are listed in this module of the
winscardcrate: https://github.com/Devolutions/sspi-rs/blob/winscard-rs-impl/crates/winscard/src/env.rs.This is the last PR with a new implemented logic in the scope of this winscard implementation and integration.
Here is a small demo:
20240206005148.mp4
It took a username and PIN code (password) from the UI and the rest parameters (container name, reader name, certificate, private file, and so on) from environment variables.
cc @awakecoding