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

fix(gitprovider): use provided scheme for API calls #3530

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

hiddeco
Copy link
Contributor

@hiddeco hiddeco commented Feb 20, 2025

Follow up on #3320 (review)

This ensures that if explicitly defined, a connection over HTTP can still be made instead of silently enforcing HTTPS. This is particularly useful when e.g. running a self-hosted Gitea or GitLab instance in a homelab environment without a proper TLS configuration.

Copy link

netlify bot commented Feb 20, 2025

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit 112cb33
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/67b753e9fd4ce0000852ccc3
😎 Deploy Preview https://deploy-preview-3530.docs.kargo.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@hiddeco hiddeco force-pushed the fix-hardcoded-https branch from 6c0109e to 112cb33 Compare February 20, 2025 16:10
@hiddeco hiddeco enabled auto-merge February 20, 2025 16:29
@krancour
Copy link
Member

This LGTM on its own, but with what I think is a safe assumption that APIs of all these Git hosting providers don't permit any unauthenticated access (or at least won't allow opening a PR without authenticating), this change might not have any practical effect unless we address #2198 also.

// If we are dealing with an insecure HTTP endpoint (of any type),
// refuse to return any credentials
if strings.HasPrefix(repoURL, "http://") {
logging.LoggerFromContext(ctx).Info(
"refused to get credentials for insecure HTTP endpoint",
"repoURL", repoURL,
)
return credentials.Credentials{}, false, nil
}

I remain of the opinion that sending credentials over an insecure protocol is playing with fire, but I've come around to the idea of an option in the chart that allows operators to opt into that, as long as it also comes with a sufficiently stern warning.

I'd be equally supportive of this PR being amended with that change or addressing it in a follow-up instead.

For now, LGTM'ing this as is on its own merits.

@hiddeco hiddeco added this pull request to the merge queue Feb 21, 2025
@krancour krancour removed this pull request from the merge queue due to a manual request Feb 21, 2025
@hiddeco
Copy link
Contributor Author

hiddeco commented Feb 21, 2025

Will tackle credentials for HTTP in a separate PR.

@hiddeco hiddeco added this pull request to the merge queue Feb 21, 2025
Merged via the queue into akuity:main with commit 8df5d9e Feb 21, 2025
15 checks passed
@hiddeco hiddeco deleted the fix-hardcoded-https branch February 21, 2025 18:38
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.

2 participants