fix(#19314): allow ssh/altssh subdomains in repo URLs to match webhook payload #19315

mtbennett-godaddy · 2024-07-30T23:16:46Z

ℹ️ Edit: There was a request to make things reusable between the Application and ApplicationSet webhooks. See #19315 (comment) for notes on those refactors. Original PR description follows.

Fixes #19314.

Updates the Application and ApplicationSet webhooks to allow an optional ssh or altssh subdomain to exist in the resource’s repo URL that doesn’t appear in the webhook payload’s repo URL.
- GitHub supports ssh subdomain over 443: https://docs.github.com/en/authentication/troubleshooting-ssh/using-ssh-over-the-https-port
- GitLab supports altssh subdomain over 443: https://about.gitlab.com/blog/2016/02/18/gitlab-dot-com-now-supports-an-alternate-git-plus-ssh-port/#how-to-use-the-alternate-ssh-connection-on-gitlab.com
- Bitbucket supports altssh subdomain over 443: https://confluence.atlassian.com/bbkb/port-22-is-blocked-on-local-network-1168865232.html
- Azure DevOps does not support such an alternative: https://developercommunity.visualstudio.com/t/Can-I-use-SSH-on-a-port-other-than-22-in/10699062?sort=active&topics=fixed+in%3A+visual+studio+2019+version+16.6+preview+2
Updates ApplicationSet webhook regular expression logic to match Application’s:
- Uses extended usernameRegex instead of just \w+.
- Calls regexp.QuoteMeta on hostname and path.

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
The title of the PR states what changed and the related issues number (used for the release note).
The title of the PR conforms to the Toolchain Guide
I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
~~I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.~~
~~Does this PR require documentation updates?~~
~~I've updated documentation as required by this PR.~~
I have signed off all my commits as required by DCO
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My build is green (troubleshooting builds).
~~My new feature complies with the feature status guidelines.~~
I have added a brief description of why this PR is necessary and/or what this PR solves.
~~Optional. My organization is added to USERS.md.~~
Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Signed-off-by: Matthew Bennett <[email protected]>

bunnyshell · 2024-07-30T23:16:50Z

❗ Preview Environment delete from Bunnyshell failed

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to try again to remove the environment

bunnyshell · 2024-07-30T23:16:51Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

codecov · 2024-07-31T00:10:02Z

Codecov Report

Attention: Patch coverage is 68.64686% with 95 lines in your changes missing coverage. Please review.

Project coverage is 55.90%. Comparing base (49431b9) to head (cedbd1e).
Report is 382 commits behind head on master.

Files with missing lines	Patch %	Lines
server/webhookhandler/webhookhandler.go	72.72%	38 Missing and 7 partials ⚠️
util/webhook/webhook.go	46.15%	41 Missing and 1 partial ⚠️
applicationset/webhookhandler/webhookhandler.go	82.35%	5 Missing and 1 partial ⚠️
server/server.go	86.66%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #19315      +/-   ##
==========================================
- Coverage   55.95%   55.90%   -0.05%     
==========================================
  Files         322      323       +1     
  Lines       44546    44530      -16     
==========================================
- Hits        24924    24895      -29     
- Misses      17055    17075      +20     
+ Partials     2567     2560       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

Signed-off-by: Matthew Bennett <[email protected]>

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

mtbennett-godaddy · 2024-08-05T16:08:07Z

applicationset/webhook/webhook.go

+// https://www.rfc-editor.org/rfc/rfc3986#section-3.2.1
+// https://github.com/shadow-maint/shadow/blob/master/libmisc/chkname.c#L36
+const usernameRegex = `[a-zA-Z0-9_\.][a-zA-Z0-9_\.-]{0,30}[a-zA-Z0-9_\.\$-]?`


Copied directly from https://github.com/argoproj/argo-cd/blob/7746506/util/webhook/webhook.go#L41-L43.

Should we instead move this to a common util package?

I had the same thought but wasn’t sure if I should dig in that much. (Wasn’t sure if the 2 webhooks were being allowed to diverge intentionally for some reason.) There’s another new feature in the Application webhook (maxPayloadSizeMB) that isn’t here either that I wanted to pull in.

I’ll see what I can do about reusing as much as possible between the 2 webhooks.

I have a similar PR open where the thing with the util package is already done (#16292). Maybe this can be used as an inspiration to these two PRs can be combined somehow.

mtbennett-godaddy · 2024-08-05T16:08:53Z

applicationset/webhook/webhook.go

+	regexEscapedHostname := regexp.QuoteMeta(urlObj.Hostname())
+	regexEscapedPath := regexp.QuoteMeta(urlObj.EscapedPath()[1:])
+	regexpStr := fmt.Sprintf(`(?i)^(http://|https://|%s@|ssh://(%s@)?((alt)?ssh\.)?)%s(:[0-9]+|)[:/]%s(\.git)?$`,
+		usernameRegex, usernameRegex, regexEscapedHostname, regexEscapedPath)


QuoteMeta and Sprintf formatting copied from https://github.com/argoproj/argo-cd/blob/7746506/util/webhook/webhook.go#L348-L351.

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

crenshaw-dev · 2024-08-07T14:09:00Z

applicationset/webhook/webhook.go

+// https://www.rfc-editor.org/rfc/rfc3986#section-3.2.1
+// https://github.com/shadow-maint/shadow/blob/master/libmisc/chkname.c#L36
+const usernameRegex = `[a-zA-Z0-9_\.][a-zA-Z0-9_\.-]{0,30}[a-zA-Z0-9_\.\$-]?`


Should we instead move this to a common util package?

applicationset/webhook/testdata/gitlab-event.json

crenshaw-dev · 2024-08-07T14:10:33Z

applicationset/webhook/webhook.go

+	regexEscapedHostname := regexp.QuoteMeta(urlObj.Hostname())
+	regexEscapedPath := regexp.QuoteMeta(urlObj.EscapedPath()[1:])
+	regexpStr := fmt.Sprintf(`(?i)^(http://|https://|%s@|ssh://(%s@)?((alt)?ssh\.)?)%s(:[0-9]+|)[:/]%s(\.git)?$`,
+		usernameRegex, usernameRegex, regexEscapedHostname, regexEscapedPath)


Ditto with this, if it's identical to the appset webhook handling logic, can we just move it to a shared location?

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

Signed-off-by: Matthew Bennett <[email protected]>

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

Signed-off-by: Matthew Bennett <[email protected]>

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

Signed-off-by: Matthew Bennett <[email protected]>

mtbennett-godaddy · 2024-08-13T17:47:16Z

util/webhook/webhook.go

+func GetApiUrlRegex(originalUrl string) (*regexp.Regexp, error) {
+	return getUrlRegex(originalUrl, `(?i)^https?://%[2]s(:[0-9]+)?/?$`)


The original regex used for this (here) allowed for SSH URLs which, AFAIK, aren’t valid for an API URL. If I’m mistaken on that, I can easily expand this to account for that. (There were no tests for this regex before. I added basic positive and negative cases in new Test_GetApiUrlRegex.)

mtbennett-godaddy · 2024-08-13T17:49:20Z

@crenshaw-dev

The common webhook handling code (HTTP- and provider-related bits and the queue logic) has been split from the use case-specific webhook payload handling (to make the former reusable across instances of the latter):

Webhook
- util/webhook was refactored to have all the reusable parts of the webhook handling.
- has a payloadHandler field of type WebhookPayloadHandler, which is an interface for the different webhook payload handlers to implement.
- has a HandleRequest function that does all the provider-specific payload parsing and authentication and then adds the payload to a queue.
- has a worker pool running to pull payloads off the queue and call its payload handler’s HandlePayload function.
WebhookPayloadHandler
- has a HandlePayload function that handles the payload for the handler’s particular use case.
- server/webhookhandler is new and contains the ApplicationWebhookPayloadHandler, which implements WebhookPayloadHandler for applications.
- applicationset/webhook was refactored to applicationset/webhookhandler to be the ApplicationSetWebhookPayloadHandler, which implements WebhookPayloadHandler for application sets.
server.go creates a Webhook with an ApplicationWebhookPayloadHandler instance and assigns the webhook’s HandleRequest function to the /api/webhook endpoint.
applicationset_controller.go creates a Webhook with an ApplicationSetWebhookPayloadHandler instance and assigns the webhook’s HandleRequest function to the /api/webhook endpoint.
Tests
- The URL regex tests stayed in util/webhook since that functionality is still there. (The existing tests only covered web URLs. I added tests for API URLs.)
- The remaining tests that were in util/webhook moved to server/webhookhandler with minimal refactors. (server/webhookhandler/webhookhandler_test.go shows as a new file in the diff but it is almost entirely just a copy/paste of the original util/webhook/webhook_test.go file.)
- The applicationset/webhookhandler tests required minimal refactors.

Signed-off-by: Matthew Bennett <[email protected]>

mtbennett-godaddy · 2024-08-14T02:30:03Z

util/webhook/webhook.go

+func GetWebUrlRegex(originalUrl string) (*regexp.Regexp, error) {
+	return getUrlRegex(originalUrl, `(?i)^((https?|ssh)://)?(%[1]s@)?((alt)?ssh\.)?%[2]s(:[0-9]+)?[:/]%[3]s$`)


@robinlieb As far as I can tell, this regex covers the changes you introduced in #16292.

The updated regex matches the following sequence of parts:

optional: protocol (http, https, or ssh) followed by ://

optional: username followed by @

previously, the regex did not allow for usernames in http, https, or protocol-less URLs.

optional: ssh or altssh subdomain

this is the tiny change I originally set out to introduce 😅

required: hostname parsed from original URL

optional: : followed by port number

required: : or /

required: path parsed from original URL followed by optional .git

Feel free to suggest any additional test cases in webhook_test.go and I’ll incorporate them and make sure the regex works against them.

Signed-off-by: Matthew Bennett <[email protected]>

mtbennett-godaddy · 2024-12-20T17:34:47Z

Closing in favor of much smaller #21227.

allow ssh/altssh subdomains in repo URLs to match webhook payload

801aae4

Signed-off-by: Matthew Bennett <[email protected]>

mtbennett-godaddy requested a review from a team as a code owner July 30, 2024 23:16

mtbennett-godaddy added 4 commits August 1, 2024 10:37

Merge branch 'master' of github.com:argoproj/argo-cd into allow-alt-s…

3f8958f

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

test nit

c79b60d

Signed-off-by: Matthew Bennett <[email protected]>

Merge branch 'master' of github.com:argoproj/argo-cd into allow-alt-s…

858d6df

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

Merge branch 'master' of github.com:argoproj/argo-cd into allow-alt-s…

7746506

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

mtbennett-godaddy commented Aug 5, 2024

View reviewed changes

Merge branch 'master' of github.com:argoproj/argo-cd into allow-alt-s…

cf8ce08

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

crenshaw-dev requested changes Aug 7, 2024

View reviewed changes

mtbennett-godaddy added 12 commits August 7, 2024 21:48

Merge branch 'master' of github.com:argoproj/argo-cd into allow-alt-s…

1948699

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

Merge branch 'master' of github.com:argoproj/argo-cd into allow-alt-s…

7c6a3ab

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

wip

eb25202

Signed-off-by: Matthew Bennett <[email protected]>

Merge branch 'master' of github.com:argoproj/argo-cd into allow-alt-s…

c4665d3

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

unify return types and allow for nil webhook

a58f88a

Signed-off-by: Matthew Bennett <[email protected]>

lint fixes

dba501e

Signed-off-by: Matthew Bennett <[email protected]>

missing import

40a16ce

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base and push argoCdSettings init down

5857b05

Signed-off-by: Matthew Bennett <[email protected]>

Merge branch 'master' of github.com:argoproj/argo-cd into allow-alt-s…

e3c9acb

…sh-subdomain Signed-off-by: Matthew Bennett <[email protected]>

use numbered inputs so caller can provide regex format

f4e0773

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

30cc282

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

f9da773

Signed-off-by: Matthew Bennett <[email protected]>

mtbennett-godaddy commented Aug 13, 2024

View reviewed changes

mtbennett-godaddy added 2 commits August 13, 2024 13:04

resolve with base

27d62e2

Signed-off-by: Matthew Bennett <[email protected]>

tweak URL regular expressions

7ba2875

Signed-off-by: Matthew Bennett <[email protected]>

mtbennett-godaddy commented Aug 14, 2024

View reviewed changes

mtbennett-godaddy added 27 commits August 18, 2024 17:17

resolve with base

0f36a7d

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

fb09082

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

7369ba2

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

10909de

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

3bd6d61

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

0520145

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

d70f91f

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

601d857

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

08e0a5f

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

6208575

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

1fc1257

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

ddade23

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

ac206d3

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

735fbc3

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

f359d34

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

12e827f

Signed-off-by: Matthew Bennett <[email protected]>

fix test data file indentation

27ef474

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

046b24e

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

ae3e907

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

e9cee07

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

68309fc

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

6b55fe3

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

207dccf

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

ac143ad

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

d8438f0

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

c3396ab

Signed-off-by: Matthew Bennett <[email protected]>

resolve with base

cedbd1e

Signed-off-by: Matthew Bennett <[email protected]>

mtbennett-godaddy closed this Dec 20, 2024

mtbennett-godaddy deleted the allow-alt-ssh-subdomain branch December 20, 2024 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#19314): allow ssh/altssh subdomains in repo URLs to match webhook payload #19315

fix(#19314): allow ssh/altssh subdomains in repo URLs to match webhook payload #19315

mtbennett-godaddy commented Jul 30, 2024 •

edited

Loading

bunnyshell bot commented Jul 30, 2024 •

edited

Loading

bunnyshell bot commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 31, 2024 •

edited

Loading

mtbennett-godaddy Aug 5, 2024

crenshaw-dev Aug 7, 2024

mtbennett-godaddy Aug 7, 2024

robinlieb Aug 7, 2024

mtbennett-godaddy Aug 5, 2024

crenshaw-dev Aug 7, 2024

crenshaw-dev Aug 7, 2024

mtbennett-godaddy Aug 13, 2024

mtbennett-godaddy commented Aug 13, 2024

mtbennett-godaddy Aug 14, 2024

mtbennett-godaddy commented Dec 20, 2024

		func GetApiUrlRegex(originalUrl string) (*regexp.Regexp, error) {
		return getUrlRegex(originalUrl, `(?i)^https?://%[2]s(:[0-9]+)?/?$`)

		func GetWebUrlRegex(originalUrl string) (*regexp.Regexp, error) {
		return getUrlRegex(originalUrl, `(?i)^((https?\|ssh)://)?(%[1]s@)?((alt)?ssh\.)?%[2]s(:[0-9]+)?[:/]%[3]s$`)

fix(#19314): allow ssh/altssh subdomains in repo URLs to match webhook payload #19315

fix(#19314): allow ssh/altssh subdomains in repo URLs to match webhook payload #19315

Conversation

mtbennett-godaddy commented Jul 30, 2024 • edited Loading

bunnyshell bot commented Jul 30, 2024 • edited Loading

❗ Preview Environment delete from Bunnyshell failed

bunnyshell bot commented Jul 30, 2024 • edited Loading

❌ Preview Environment deleted from Bunnyshell

codecov bot commented Jul 31, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtbennett-godaddy commented Aug 13, 2024

Choose a reason for hiding this comment

mtbennett-godaddy commented Dec 20, 2024

mtbennett-godaddy commented Jul 30, 2024 •

edited

Loading

bunnyshell bot commented Jul 30, 2024 •

edited

Loading

bunnyshell bot commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 31, 2024 •

edited

Loading