Skip to content
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.

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

Kerberos smart card logon implementation #145

Merged
merged 26 commits into from
Aug 28, 2023

Conversation

TheBestTvarynka
Copy link
Collaborator

Hi,
In this PR I've added Kerberos smart card logon implementation. Finally 🎊 .

Changes overview:

  • I moved a lot of code from the Pku2u submodules into the pk-init module. Motivation: This code is reused in the Kerberos AS-messages exchange.
  • I moved PA-DATA (pre-authentication data) generation and session key extraction into a separate module.
  • Small builders refactoring. I wrote comments about it in the code.

Docs & references:

P.S. When this PR gets merged, I'll perform final dev testing and make sure I didn't break anything during the code review and refactoring. And after that, I'll create a final pull request from kerberos-smart-card-dev into the master branch.

@TheBestTvarynka TheBestTvarynka self-assigned this Aug 25, 2023
@CBenoit CBenoit self-assigned this Aug 25, 2023
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Question: I thought we intended to provide the smart card implementation itself in a separate standalone crate (winscard), with an API similar to the pcsc crate except it’s not powered by FFI bindings and is pure Rust instead. Did we change our mind? (cc @dbl-cross @awakecoding)

Otherwise, this is looking good to me.

@@ -232,7 +232,7 @@ pub fn generate_as_req_kdc_body(options: &GenerateAsReqOptions) -> Result<KdcReq
}

#[instrument(level = "debug", ret, skip_all)]
pub fn generate_as_req(pa_datas: &[PaData], kdc_req_body: KdcReqBody) -> AsReq {
pub fn generate_as_req(pa_datas: Vec<PaData>, kdc_req_body: KdcReqBody) -> AsReq {
Copy link
Member

Choose a reason for hiding this comment

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

What motivates this change? Maybe a to_owned or to_vec can be removed somewhere below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For every Kerberos AS exchange, we are using the pa_data::AsReqPaDataOptions::generate method to create needed PaDatas. They are never reused in our code. We generate new ones for every AS exchange. So, I changed this function signature to accept the owned PaDatas.

yes, .to_owned() can be removed below. I forgot about it during the refactoring.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed unneeded .to_owned()

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

@TheBestTvarynka
Copy link
Collaborator Author

You are right. We are still planning to replace the WinSCard API with our own implementation in Rust. The current Kerberos smart card logon implementation is just the initial one. Just to make the Kerberos work with smart cards.
I think we'll replace it with our winscard-rs when it's done

Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Got you! Looks good to me!

@CBenoit CBenoit merged commit 6ee6157 into kerberos-smart-card-dev Aug 28, 2023
40 checks passed
@CBenoit CBenoit deleted the kerberos-smart-card-logon branch August 28, 2023 15:53
CBenoit pushed a commit that referenced this pull request Aug 29, 2023
Adds Kerberos smart card logon implementation.

* Moves a lot of code from the Pku2u submodules into the `pk-init` module.
   Motivation: This code is reused in the Kerberos AS-messages exchange.
* Moves PA-DATA (pre-authentication data) generation and session key extraction
   into a separate module.
* Small builders refactoring. I wrote comments about it in the code.

References:

* RFC 4556: Public Key Cryptography for Initial Authentication in Kerberos (PKINIT):
   https://www.rfc-editor.org/rfc/rfc4556.html
* MS-CSSP TSCredentials:
   https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-cssp/94a1ab00-5500-42fd-8d3d-7a84e6c2cf03
* Winscard.h header - Win32 apps:
   https://learn.microsoft.com/en-us/windows/win32/api/winscard/
* Microsoft Base Smart Card Cryptographic Service Provider:
   https://learn.microsoft.com/en-us/previous-versions/windows/desktop/secsmart/microsoft-base-smart-card-cryptographic-service-provider
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants