search

go-ldap / ldap

Basic LDAP v3 functionality for the GO programming language.
Other
2.25k stars 355 forks source link

v3.4.3 Broken due to dependancy Update #381

Closed speatzle closed 2 years ago

speatzle commented 2 years ago

It seems that v3.4.3 (and probably most other releases) are broken. The Release fails to build because of a breaking change in https://github.com/Azure/go-ntlmssp (v0.0.0-20220621081337-cb9428e4ac1e) They Merged this PR https://github.com/Azure/go-ntlmssp/pull/32 in https://github.com/Azure/go-ntlmssp/commit/cb9428e4ac1e1edffb49d21e3b29a0a30c664c83

That PR changed the function signature of ntlmssp.ProcessChallenge to have an additional bool causing the following build failure:

# github.com/go-ldap/ldap/v3
../../go/pkg/mod/github.com/go-ldap/ldap/v3@v3.4.3/bind.go:500:96: not enough arguments in call to ntlmssp.ProcessChallenge
    have ([]byte, string, string)
    want ([]byte, string, string, bool)

https://github.com/Azure/go-ntlmssp does apparently not tag releases. Which is why it gets upgraded even tho it had a breaking change

iamcaje-psh commented 2 years ago

https://github.com/Azure/go-ntlmssp does apparently not tag releases. Which is why it gets upgraded even tho it had a breaking change

I had to downgrade to :

go get github.com/Azure/go-ntlmssp@v0.0.0-20211209120228-48547f28849e

to get my project to build. Added a comment to my go.mod letting me know not to upgrade:

require (
    // this version works - newer commits have breaking changes - do not update
    // github.com/Azure/go-ntlmssp v0.0.0-20211209120228-48547f28849e // indirect
    github.com/Azure/go-ntlmssp v0.0.0-20211209120228-48547f28849e // indirect
...
)
cpuschma commented 2 years ago

Thank you for opening an issue @speatzle. Did you run go get -u ./... beforehand? The working commit hash is referenced in our go.mod files: https://github.com/go-ldap/ldap/blob/a3dcdda8f1270b35e951a128288280e1eadf6814/go.mod#L1-L9

The build only failed when I manually updated the dependencies:

C:\Users\nevo\IdeaProjects\go-ldap>go get -v -u ./...
go: downloading github.com/Azure/go-ntlmssp v0.0.0-20220621081337-cb9428e4ac1e
go: downloading golang.org/x/crypto v0.0.0-20220525230936-793ad666bf5e
go: upgraded github.com/Azure/go-ntlmssp v0.0.0-20211209120228-48547f28849e => v0.0.0-20220621081337-cb9428e4ac1e
go: upgraded golang.org/x/crypto v0.0.0-20220331220935-ae2d96664a29 => v0.0.0-20220525230936-793ad666bf5e

C:\Users\nevo\IdeaProjects\go-ldap>go test -v ./...  
# github.com/go-ldap/ldap [github.com/go-ldap/ldap.test]
.\bind.go:521:96: not enough arguments in call to ntlmssp.ProcessChallenge
        have ([]byte, string, string)
        want ([]byte, string, string, bool)
FAIL    github.com/go-ldap/ldap [build failed]

Nevertheless, I hope the devs at Azure/go-ntlmssp take care of it. In case if they don't revert the change and all new releases stop working and to save others the frustration, we can add the dependencies with a working version to the repository and fix the latest release.. but older releases would still be affected when trying to update the dependencies.

If you're having trouble building your project: Ensure your go.mod file hasn't been updated and still references to the working version: 

github.com/Azure/go-ntlmssp v0.0.0-20211209120228-48547f28849e

If this is not the case, try the following steps to get it working again:

  1. Revert the go.mod + go.sum file to use correct / working commit
  2. Run go clean -modcache to clear your mod cache just in case
  3. In your project, download your dependencies again using go mod download
C:\Users\nevo\IdeaProjects\go-ldap>git checkout HEAD -- go.mod go.sum

C:\Users\nevo\IdeaProjects\go-ldap>go clean -modcache

C:\Users\nevo\IdeaProjects\go-ldap>go mod download

C:\Users\nevo\IdeaProjects\go-ldap>go test ./...
ok      ok      github.com/go-ldap/ldap 9.452s
speatzle commented 2 years ago

Our CI Pipeline automatically ran and did the go get -u and failed the step so it's fine for now.

tirupatibg commented 2 years ago

Hi @speatzle Could we fix this? based on the requirements asked by go-ntlmssp. I think adding new bool param is just a simple change here right?

speatzle commented 2 years ago

IMO https://github.com/Azure/go-ntlmssp should revert https://github.com/Azure/go-ntlmssp/pull/32 and then tag v1.0.0 after that they could merge https://github.com/Azure/go-ntlmssp/pull/32 again and tag v2.0.0

this would ensure that all old releases of go-ldap continue to work and would allow for breaking changes down the line without affecting go-ldap and other downstream projects.

go-ldap could then adopt the new function signature of https://github.com/Azure/go-ntlmssp/pull/32 in a new release

cpuschma commented 2 years ago

To clarify for both this issue and the PR over at Azure/go-ntlmssp#34, already released versions of go-ldap are still working unless you manually force an dependency update using go get -u. If Azure/go-ntlmssp do not revert their breaking change then we have no other choice but to:

  1. Update our functions to support the new function signature
  2. Maybe start vendoring our dependencies to avoid further issues
  3. Add a note to our README to inform users about potential problems when using older versions
abhinavdeep01 commented 2 years ago

For those not able to build in CI Pipeline add a flag -skipDepUpdate=true This worked for me.

Justin-W commented 2 years ago

I had to downgrade to :

Thanks to @speatzle and @iamcaje-psh for identifying the root cause of the problem. Instead of downgrading using @iamcaje-psh 's workaround, I added the following to my go.mod file, to prevent the broken Azure/go-ntlmssp API from getting upgraded even if/when you run go get -u ./... afterwards.

// github.com/Azure/go-ntlmssp@v0.0.0-20220621081337-cb9428e4ac1e introduced backwards-incompatible API changes
// SEE: https://github.com/Azure/go-ntlmssp/issues/33
// SEE: https://github.com/go-ldap/ldap/issues/381
replace (
    // NOTE: If this module adds any additional broken versions (without reverting the API breaking change), the aggressive replace will be needed.
    // However, if they fix the API in the next version, we can remove the aggressive replace and keep only the narrow replace (just in case).
    github.com/Azure/go-ntlmssp => github.com/Azure/go-ntlmssp v0.0.0-20211209120228-48547f28849e // aggressive: replaces EVERY version with the LKG (backwards-compatible) version
    github.com/Azure/go-ntlmssp v0.0.0-20220621081337-cb9428e4ac1e => github.com/Azure/go-ntlmssp v0.0.0-20211209120228-48547f28849e // narrow: replaces only the 1 currently-known-broken version
)

If https://github.com/Azure/go-ntlmssp/pull/34 gets merged as the next version, then the aggressive replace will become unnecessary. Or if https://github.com/go-ldap/ldap is instead updated to use the new https://github.com/Azure/go-ntlmssp API, then the replace directives can be removed to re-enable normal dependency upgrades.

cpuschma commented 2 years ago

If https://github.com/Azure/go-ntlmssp/pull/34 gets merged as the next version, then the aggressive replace will become unnecessary. Or if https://github.com/go-ldap/ldap is instead updated to use the new https://github.com/Azure/go-ntlmssp API, then the replace directives can be removed to re-enable normal dependency upgrades.

It is still unclear whether the PR at Azure/go-ntlmssp will be merged or not. I say we give them a week to decide, otherwise we might opt for vendoring or adopting their breaking change.