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

fix(NODE-6372): correctly GSS wrap non-user data #206

Closed
wants to merge 1 commit into from

Conversation

dsullivan
Copy link

Description

Fix implementation of wrap function to work for use cases other than authentication.

What is changing?

Just an extra check for an empty string: if (user) becomes if (user && *user).

Without this, the wrap function was discarding the challenge/payload value and wrapping only the user option value, even if no such value was provided.

Is there new documentation needed for these changes?

No, this makes the wrap method work as per the existing documentation.

What is the motivation for this change?

Without this change, the wrap function works only for authentication. With this change, any payload can be GSS-wrapped.

Release Highlight

Allow Kerberos GSS wrap function to work on arbitrary challenge/payload data, not just user authentication.

Double check the following

  • Ran npm run check:lint script
  • Self-review completed using the steps outlined here
  • PR title follows the correct format: type(NODE-xxxx)[!]: description
    • Example: feat(NODE-1234)!: rewriting everything in coffeescript
  • Changes are covered by tests
  • New TODOs have a related JIRA ticket

@dsullivan
Copy link
Author

NOTE: I wasn't able to run the full test suite locally, given my Kerberos server configuration. I did run equivalent versions of the new tests in my application.

@nbbeeken nbbeeken added the Team Review Needs review from team label Sep 17, 2024
@nbbeeken nbbeeken self-assigned this Sep 17, 2024
Copy link
Collaborator

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution with tests!

@nbbeeken nbbeeken requested a review from addaleax September 17, 2024 19:42
@@ -468,7 +468,7 @@ gss_result authenticate_gss_client_wrap(gss_client_state* state,
input_token.length = len;
}

if (user) {
if (user && *user) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the check in L519 also be adjusted in the same way? The memory management code in this file is honestly a bit hard to follow, but it seems that way to me

Copy link
Author

Choose a reason for hiding this comment

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

I'm also having a hard time following.

My suspicion is that, since user is never null, that release is never being executed. Or at least has not been since the ToStringWithNonStringAsEmpty behavior was introduced.

Then again, I don't understand why we'd only need to release the buffer if there is no user. Maybe the right fix is to remove that condition altogether and just call the release if there is any data in the buffer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then again, I don't understand why we'd only need to release the buffer if there is no user

I assume that it corresponds to this condition here, hence my suggestion above – if this branch is taken, then input_token contains a stack-allocated reference to buf, rather than a dynamically allocated buffer

@nbbeeken
Copy link
Collaborator

Closing for inactivity, please feel free to re-open and continue this if there's interest!

@nbbeeken nbbeeken closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team Review Needs review from team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants