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

picky-krb: CredSSP credentials structures #233

Merged
merged 4 commits into from
Aug 18, 2023

Conversation

TheBestTvarynka
Copy link
Collaborator

Hello,
During the Kerberos smart card logon, the smart card credentials are transferred using the TsSmartCardCreds and TsCspDataDetail structures.

The password-based credentials are encoded using the ber module in the sspi-rs. But now we have picky-* crates for the asn1 encoding. So, I decided to add CredSSP-related constants and credentials structures to the picky-krb crate.

Docs and reference:

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.

This is looking good to me!

I see you authored a rather clean history on your branch so if you want, we can "rebase and merge" the PR, but there is small details I want fixed by rebasing interactively first:

  • The commit adding tests should either be part of the commit adding the feature, or alternatively the conventional commits type "test" should be used instead of "feat"
  • We don’t really scope commits by module
  • We don’t end the summary with ;
  • The summary line should be no longer than 50 characters (and at the very most 72 characters), so that it doesn’t get wrapped onto the next line like here:
    image
  • Rebase interactively my "suggestion commit" into the relevant commit (using the fixup rebase command)

This would give the following history:

  • feat(picky-krb): add credssp password and smartcard structs
  • feat(picky-krb): add creds and key spec constants
  • test(picky-krb): unit tests for credssp structs
  • feat(picky-asn1-x509): add USER_PRINCIPAL_NAME oid

Following this convention helps me when generating the changelogs.

Otherwise I usually squash the whole PR into a single commit for simplicity.

@TheBestTvarynka
Copy link
Collaborator Author

@CBenoit
Changed commit messages according to your guideline. I'll keep it in mind for future commits.

@CBenoit CBenoit self-assigned this Aug 18, 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.

Thank you! @TheBestTvarynka
I’ll rebase and merge into master 😄
FYI, I’m wrapping up some other work after what I’ll cut a release for a few picky crates.

@CBenoit CBenoit enabled auto-merge (rebase) August 18, 2023 20:18
@CBenoit CBenoit merged commit 19838e7 into master Aug 18, 2023
10 checks passed
@CBenoit CBenoit deleted the credssp-credentials-structures branch August 18, 2023 20:31
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