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

Protoype of kerberos support (non parameterized with TODOs) #3267

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

blackentropy
Copy link

No description provided.


// TODO: probably a better way to handle this.
if (token == null) {
this.emit('error', 'Received null GSSAPI token on continue')
Copy link
Author

@blackentropy blackentropy Jun 26, 2024

Choose a reason for hiding this comment

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

Just a quick note; the token can be null if the negotiation has already been successfully completed. I suspect the right thing to do is to do something like:

if (inToken === 'oRQwEqADCgEAoQsGCSqGSIb3EgECAg==') {
  return; // negotiation successful, no need to do anything further.
}

see: https://github.com/jcmturner/gokrb5/blob/master/spnego/http.go#L198 for where that magic string came from.

@albertchang
Copy link

@brianc We'd ideally like to get this merged and released through the main package. Could you let us know what the procedure would be? There's unfortunately not a lot of documentation for this flow, and it's not well trodden.

@brianc
Copy link
Owner

brianc commented Jul 3, 2024

We'd ideally like to get this merged and released through the main package. Could you let us know what the procedure would be?

I can do that! Its very easy on my side to release new versions. Ideally all features should have tests along side them. I realize testing against a kerberos system is.... unpleasant ... do you have any way you could include tests? Even the pg-protocol stuff typically ships with unit tests. Its very high likelyhood without tests sometime within the next 10 years this code will change and/or break without them.

@brianc
Copy link
Owner

brianc commented Jul 3, 2024

@albertchang note also: CI is running on this PR now & lint is failing

@albertchang
Copy link

albertchang commented Jul 3, 2024

We'd ideally like to get this merged and released through the main package. Could you let us know what the procedure would be?

I can do that! Its very easy on my side to release new versions. Ideally all features should have tests along side them. I realize testing against a kerberos system is.... unpleasant ... do you have any way you could include tests? Even the pg-protocol stuff typically ships with unit tests. Its very high likelyhood without tests sometime within the next 10 years this code will change and/or break without them.

Cool, I'll try to take a stab at writing some tests for this change and circle back afterwards. Thanks!

Also, is CI not configured to run automatically on changes to the branch?

@brianc
Copy link
Owner

brianc commented Jul 4, 2024

Also, is CI not configured to run automatically on changes to the branch?

Boo i need to set that up. having to keep re-approving it is no bueno!

@albertchang
Copy link

@brianc Could you approve the workflow run for CI? Thanks!

@albertchang
Copy link

@brianc Would also appreciate a review when you get a chance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants