Skip to content

Feat/generic webhook ntlm auth#4316

Merged
Skarlso merged 2 commits intoexternal-secrets:mainfrom
yifongau:feat/generic-webhook-ntlm-auth
May 7, 2025
Merged

Feat/generic webhook ntlm auth#4316
Skarlso merged 2 commits intoexternal-secrets:mainfrom
yifongau:feat/generic-webhook-ntlm-auth

Conversation

@yifongau
Copy link
Copy Markdown
Contributor

@yifongau yifongau commented Jan 21, 2025 •
edited
Loading

Problem Statement

This pull requests adds support for NTLM authentication for the webhook provider. Useful for legacy environments.

Related Issue

No issue opened (if necessary I can create one, let me know).

Proposed Changes

The ntlm authentication is achieved through 3 changes:

  1. If NTLM is used, add wrapper around Transport when creating the client through GetHTTPClient(). This wrapper is provided by the following library: https://github.com/Azure/go-ntlmssp. I tested two Go NTLM implementations, and found this one most suitable as it has the ability to respond both "Negotiate" and "NTLM" www-authenticate headers. There is another library (vadimi/go-http-ntlm) which is also good, but unfortunately only responds to "NTLM" www-authenticate headers. Unfortunately this does not fit our use case, and I suspect many other use cases.

  2. If NTLM is used, set the authentication header when creating the request in GetWebhookData(). When performing the request, the wrapper reads the Authentication header, and uses it in the challenge-response action with the server.

  3. I also made the changes for the types for webhook provider, such that users can use the NTLM authentication in the following way:

apiVersion: external-secrets.io/v1beta1
kind: SecretStore
metadata:
  name: webhook-backend
spec:
  provider:
    webhook:
      url: "http://httpbin.org/get?parameter={{ .remoteRef.key }}"
      result:
        jsonPath: "$.args.parameter"
      auth:
        ntlm:
            usernameSecret:
              name: webhook-credentials
              key: username
              namespace: externalsecrets
            passwordSecret:
              name: webhook-credentials
              key: password
              namespace: externalsecrets
---
apiVersion: v1
kind: Secret
metadata:
  name: webhook-credentials
  namespace: externalsecrets
data:
  username: dGVzdA== # "test"
  password: dGVzdA== # "test"

Please let me know if any other changes/improvements are necessary. Thank you!

Checklist

  • I have read the contribution guidelines
  • All commits are signed with git commit --signoff
  • My changes have reasonable test coverage
  • All tests pass with make test
  • I ensured my PR is ready for review with make reviewable

@yifongau yifongau requested a review from a team as a code owner January 21, 2025 01:21
@yifongau yifongau requested a review from moolen January 21, 2025 01:21
@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Jan 21, 2025

Hello.

Could you please squash those 50 commits and sign it? :) Thank you!

@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Jan 21, 2025

I am not sure these changes will work with webhook generators. Can you please add tests to them as well?

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jan 21, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@yifongau
Copy link
Copy Markdown
Contributor Author

yifongau commented Jan 21, 2025

Hi there! I squashed and rebuild my commits.

I am not sure these changes will work with webhook generators. Can you please add tests to them as well?

I am not sure what you mean, could yo clarify? I did not touch the generator code.

@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Jan 21, 2025

The changes you made to common/webhook were there to support both generators webhook and SecretStore webhook.

Because you also changed structures there, I’m not sure this would really work for that.

I think it will actually panic

@yifongau
Copy link
Copy Markdown
Contributor Author

yifongau commented Jan 22, 2025 •
edited
Loading

The changes you made to common/webhook were there to support both generators webhook and SecretStore webhook.

Because you also changed structures there, I’m not sure this would really work for that.

I think it will actually panic

Thanks for your response. We do not use the Generator feature, so I actually was not aware that code was shared between them. I will investigate.

@rpardini
Copy link
Copy Markdown

rpardini commented Jan 31, 2025

👍 Excellent work @yifongau 🥇 -- I've built this and tested -- it enables me to use NTLM-protected services (provided by Microsoft IIS) just fine.

It seems recent commits landed in main branch require this to be rebased (and make reviewable ran).

I've not noticed any panics as mentioned by reviewers here.

Skarlso
Skarlso reviewed Feb 1, 2025
View reviewed changes
Skarlso
Skarlso reviewed Feb 1, 2025
View reviewed changes
Skarlso
Skarlso reviewed Feb 1, 2025
View reviewed changes
@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Feb 1, 2025

Please fix the sonarcloud issue as well that now popped up because of the additional complexity. :)

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Feb 1, 2025

@gusfcarvalho I looked at the generator webhook and the tests are fine. Plus, because of the nil checks on the Auth stuff, it doesn't look like the generator could panic. Do you have anything else in mind?

@yifongau
Copy link
Copy Markdown
Contributor Author

yifongau commented Mar 4, 2025

Please fix the sonarcloud issue as well that now popped up because of the additional complexity. :)

I'll review the sonarcloud and make appropriate changes! thx!

@yifongau
Copy link
Copy Markdown
Contributor Author

yifongau commented Mar 5, 2025

Please fix the sonarcloud issue as well that now popped up because of the additional complexity. :)

I refactored the method that was flagged by SonarCloud.

@yifongau
Copy link
Copy Markdown
Contributor Author

yifongau commented Mar 5, 2025

make reviewable throws some error at the helm templating step:

Error: YAML parse error on external-secrets/templates-crds/clustersecretstore.yaml: error converting YAML to JSON: yaml: line 23: mapping values are not allowed in this context.

Not sure what caused this, as this seems to be a generated file. Looking at line 23 it is not tripped by anything I modified.

@yifongau yifongau requested a review from Skarlso March 6, 2025 09:39
gusfcarvalho
gusfcarvalho reviewed Mar 24, 2025
View reviewed changes
gusfcarvalho
gusfcarvalho reviewed Mar 24, 2025
View reviewed changes
@gusfcarvalho
Copy link
Copy Markdown
Member

gusfcarvalho commented Mar 26, 2025

/ok-to-test sha=cd33836c0692ab9c91d9a56b3424641108043206

@eso-service-account-app
Copy link
Copy Markdown
Contributor

eso-service-account-app bot commented Mar 26, 2025

[Bot] - ✅ e2e for cd33836c0692ab9c91d9a56b3424641108043206 passed

@yifongau
Copy link
Copy Markdown
Contributor Author

yifongau commented Mar 27, 2025

Heey! I see the dependency license check is failing (it seems to erroring out). Does this need to be addressed before it can be merged?

If there is anything else I can do to expedite the merge, please let me know!

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Apr 6, 2025

@yifongau don't worry about it. Sadly, it keeps failing...

@yifongau
Copy link
Copy Markdown
Contributor Author

yifongau commented Apr 8, 2025

@yifongau don't worry about it. Sadly, it keeps failing...

Good to know... thanks!

Skarlso
Skarlso reviewed Apr 25, 2025
View reviewed changes
Skarlso
Skarlso reviewed Apr 25, 2025
View reviewed changes
Skarlso
Skarlso reviewed Apr 25, 2025
View reviewed changes
@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented Apr 25, 2025

Just have a couple of questions and I see there are linter and unit test errors. Could you please take care of those, then this is LGTM and we can merge it. :)

@Skarlso
Copy link
Copy Markdown
Contributor

Skarlso commented May 7, 2025

@yifongau This is almost done now. :) Just a quick couple of things and I can merge it. :) If you finish it today, I can get it into the release. :)

@yifongau
Copy link
Copy Markdown
Contributor Author

yifongau commented May 7, 2025

@yifongau This is almost done now. :) Just a quick couple of things and I can merge it. :) If you finish it today, I can get it into the release. :)

Thanks for your comments. I just made the change you suggested and resolved those linting errors (had to copy some stuff to from esv1beta1 to esv1 and change the corresponding references).

yifongau added 2 commits May 7, 2025 10:03
@yifongau
Add support for ntlm authentication for webhook provider
e651453 
Signed-off-by: Yi Fong Au <yi-fong.au@stater.nl>

refactored executeRequest method
@yifongau
rm switch case + use esv1
6a0e24e 
regenerated docs

Signed-off-by: Yi Fong Au <yi-fong.au@stater.nl>
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented May 7, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@Skarlso Skarlso merged commit ec12919 into external-secrets:main May 7, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gusfcarvalho gusfcarvalho gusfcarvalho left review comments

@Skarlso Skarlso Skarlso approved these changes

@moolen moolen Awaiting requested review from moolen moolen was automatically assigned from external-secrets/maintainers

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yifongau @Skarlso @gusfcarvalho @rpardini