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: GSSAPI client (with unix support) #449

Merged
merged 5 commits into from Mar 14, 2024
Merged

Conversation

levkohimins
Copy link
Contributor

@levkohimins levkohimins commented Jul 22, 2023

Refactoring the code from this PR to implement ldap.GSSAPIClient interface.

// NewClientWithKeytab creates a new client from a keytab credential.
gssapi.NewClientWithKeytab(username, realm, keytabPath, krb5confPath)

// NewClientWithPassword creates a new client from a password credential.
gssapi.NewClientWithPassword(username, realm, password, krb5confPath)

// NewClientFromCCache creates a client from a populated client cache.
gssapi.NewClientFromCCache(ccachePath, krb5confPath)

VRStalker pushed a commit to VRStalker/ldap that referenced this pull request Dec 15, 2023
patryk4815 added a commit to patryk4815/ldap that referenced this pull request Mar 9, 2024
@patryk4815
Copy link

works fine ;) @t2y why not merged yet?

@t2y
Copy link
Contributor

t2y commented Mar 12, 2024

I don't have permission to merge. I think @cpuschma can review it.

@cpuschma
Copy link
Member

There are open merge conflicts that need to be resolved first but first and foremost we/I need to review the function and structs, as there already GSSAPI related functions available coming from PR #402 .

@Macmod
Copy link

Macmod commented Mar 14, 2024

My two cents...

Client from PR #449 implements GSSAPIClient from PR #402 (but is not Windows-only), therefore the "Bind" functions from PR #402 should work fine with Clients instead of SSPIClients, since they rely on the interface instead of the old concrete type.

The only functions from PR #402 that reference the old concrete type are the NewSSPIClient* functions, which allow for backwards compatibility if the user still wants to use the Windows-only implementation instead of migrating to the improved version.

IMO right now this should be enough to guarantee that this PR only adds functionality to go-ldap that will allow for lots of new features in projects that use it and should not cause any issues.

The alternative is to introduce breaking changes by removing the Windows-only types and methods and adding extra code for the interface and the bind to this PR, which does not sound ideal. Another possibility is to rename "Client" to "SSPIClient", remove the old code from PR #402, and rename other functions from this PR accordingly to match the old PR - this would be a much more sensitive operation, involving removing the dependency on alexbrainman/sspi and changing some type signatures...

@cpuschma
Copy link
Member

My two cents...

Client from PR #449 implements GSSAPIClient from PR #402 (but is not Windows-only), therefore the "Bind" functions from PR #402 should work fine with Clients instead of SSPIClients, since they rely on the interface instead of the old concrete type.

The only functions from PR #402 that reference the old concrete type are the NewSSPIClient* functions, which allow for backwards compatibility if the user still wants to use the Windows-only implementation instead of migrating to the improved version.

IMO right now this should be enough to guarantee that this PR only adds functionality to go-ldap that will allow for lots of new features in projects that use it and should not cause any issues.

The alternative is to introduce breaking changes by removing the Windows-only types and methods and adding extra code for the interface and the bind to this PR, which does not sound ideal. Another possibility is to rename "Client" to "SSPIClient", remove the old code from PR #402, and rename other functions from this PR accordingly to match the old PR - this would be a much more sensitive operation, involving removing the dependency on alexbrainman/sspi and changing some type signatures...

The pull request as such is definitely a good addition and I am grateful for all contributions, especially because time is currently tight due to work (but the situation should ease soon). This was the reason for the first message, I haven't had time to review the PR and check if the functions and structs differ too much from the existing ones.

We can and should avoid breaking changes whenever possible. I'll review and merge the PR today, only an addition of the examples would be helpful if someone finds the time and has a system with Kerberos integration at hand.

@cpuschma cpuschma merged commit 83a306c into go-ldap:master Mar 14, 2024
21 of 22 checks passed
@patryk4815 patryk4815 mentioned this pull request Mar 14, 2024
gustavoluvizotto pushed a commit to gustavoluvizotto/ldap-fork that referenced this pull request Apr 11, 2024
* feat: unix gssapi client

* fix: lint errors

* fix: comment grammar

---------

Co-authored-by: Levko Burburas <levko.burburas@biptec.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants