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

NTLM dependency package update to solve a duplicate headers bug upon NTLM authentication #2290

Merged
merged 6 commits into from
Dec 13, 2021

Conversation

Resousse
Copy link
Contributor

@Resousse Resousse commented Dec 9, 2021

Hello,

A new version of go-ntlmssp has been released following a PR I did, to solve a bug.
The bug is : NTLM authentication doesn't work when two Authorization/WWW-Authenticate headers are returned by the back-end server.
With the new version of the go-ntlmssp package, the number of header doesn't matter, and the most accurate will be taken.

@CLAassistant
Copy link

CLAassistant commented Dec 9, 2021

CLA assistant check
All committers have signed the CLA.

@mstoykov
Copy link
Contributor

mstoykov commented Dec 9, 2021

Hi @Resousse, thanks for the PR and the change upstream. I haven't checked either one of those, but you currently aren't committing the changes to the vendor directory, so you should do that as well ;)

@Resousse
Copy link
Contributor Author

Resousse commented Dec 9, 2021

Hi @Resousse, thanks for the PR and the change upstream. I haven't checked either one of those, but you currently aren't committing the changes to the vendor directory, so you should do that as well ;)

Oops, I didn't realize that dependency files were embedded in the git repo.

vendor directory has been updated with new files. Thanks @mstoykov

mstoykov
mstoykov previously approved these changes Dec 9, 2021
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for both contributions 🙇

We have some problems with testing this, so can you confirm that you have tested this experimentally? And if you have a setup that works okay on linux it will be really nice as we have been trying to get anything working for test purposes :(

@mstoykov mstoykov added this to the v0.36.0 milestone Dec 9, 2021
@Resousse
Copy link
Contributor Author

Resousse commented Dec 9, 2021

Hello,
Yes, tested on both Windows and MinGW environment, I can confirm it works.
I'm not able to test the NTLM connection on my personal Mac because it works in a secure VPN network, that is deployed on my windows Computer.

@mstoykov
Copy link
Contributor

mstoykov commented Dec 9, 2021

go mod verify is failing in the CI

@codecov-commenter
Copy link

codecov-commenter commented Dec 9, 2021

Codecov Report

Merging #2290 (f88ba56) into master (42f2e60) will increase coverage by 0.21%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2290      +/-   ##
==========================================
+ Coverage   72.71%   72.93%   +0.21%     
==========================================
  Files         184      184              
  Lines       14571    14762     +191     
==========================================
+ Hits        10596    10767     +171     
- Misses       3333     3351      +18     
- Partials      642      644       +2     
Flag Coverage Δ
ubuntu 72.88% <85.71%> (+0.23%) ⬆️
windows 72.58% <85.71%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/cloud.go 4.68% <0.00%> (-0.03%) ⬇️
cmd/login_cloud.go 0.00% <0.00%> (ø)
cmd/login_influxdb.go 0.00% <0.00%> (ø)
cmd/ui.go 20.61% <0.00%> (ø)
js/modules/modules.go 47.05% <ø> (-35.00%) ⬇️
lib/consts/consts.go 0.00% <ø> (ø)
lib/execution_segment.go 92.73% <ø> (ø)
lib/fsext/changepathfs.go 92.95% <0.00%> (-1.33%) ⬇️
lib/metrics/metrics.go 100.00% <ø> (ø)
lib/runtime_options.go 100.00% <ø> (ø)
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee8d9bd...f88ba56. Read the comment docs.

@Resousse
Copy link
Contributor Author

Resousse commented Dec 9, 2021

go mod verify is failing in the CI

Sorry @mstoykov , I forgot one minor label that has been updated prior my change. Tested ok with go mod vendor and go mod verify.

@mstoykov mstoykov merged commit f3b7cfa into grafana:master Dec 13, 2021
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.

5 participants