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

feat(sspi): add SECBUFFER_READONLY_WITH_CHECKSUM flag support #357

Merged
merged 15 commits into from
Feb 5, 2025

Conversation

TheBestTvarynka
Copy link
Collaborator

Hi,
I added SECBUFFER_READONLY and SECBUFFER_READONLY_WITH_CHECKSUM flags support. It is related to #120

This PR affects many files because I changed the security buffer structure to add those flags.

Docs & reference:

@TheBestTvarynka TheBestTvarynka self-assigned this Feb 4, 2025
Cargo.toml Outdated Show resolved Hide resolved
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.

Nice work!

Comment on lines 22 to 23
/// A special security buffer type is used for the data decryption. Basically, it's almost the same
/// as `OwnedSecurityBuffer` but for decryption.
Copy link
Member

Choose a reason for hiding this comment

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

thought: What about SecurityBuffer for the owned version, and SecurityBufferRef for the borrowing version? It’s a common naming I’m seeing more and more. (Follow up in a separate PR.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea! Your naming suggestions are good as always. Somewhat I didn't think about the -Ref postfix.
Done

#[non_exhaustive]
pub enum SecurityBuffer<'data> {
pub enum SecurityBufferType<'data> {
Copy link
Member

Choose a reason for hiding this comment

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

naming: What do you think of UntaggedSecurityBuffer? Does it make sense in this context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, it is tagged because each enum variant represents a different buffer type. It lacks only buffer flags, so I renamed it to UnflaggedSecurityBuffer. What do you think?

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

}

/// Returns the vector of immutable references to the [SecurityBuffer] with specified buffer type.
pub fn buffers_with_type<'a>(
Copy link
Member

Choose a reason for hiding this comment

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

naming: of instead of with is clearer. Same suggestion for the other filtered iterators.

Suggested change
pub fn buffers_with_type<'a>(
pub fn buffers_of_type<'a>(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@TheBestTvarynka TheBestTvarynka marked this pull request as ready for review February 5, 2025 16:23
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.

LGTM!

@CBenoit CBenoit merged commit 07d61a7 into dev/dpapi Feb 5, 2025
42 checks passed
@CBenoit CBenoit deleted the feat/readonly-secbuffer-support branch February 5, 2025 17:22
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