-
Notifications
You must be signed in to change notification settings - Fork 65
Feature/insecure flag #785
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
Feature/insecure flag #785
Conversation
…S connections The --insecure flag now provides dual functionality for registry authentication: 1. Allows insecure HTTPS connections by skipping certificate verification 2. Enables plain HTTP connections when HTTPS is not available This change improves compatibility with both: - Self-signed/invalid HTTPS certificates - Legacy registries that only support HTTP Technical changes: - Updated URL handling in registry authentication - Modified GetRegistryFromRef to properly handle HTTP/HTTPS schemes - Enhanced login flow to support fallback to HTTP when HTTPS fails - Improved error handling for connection attempts Breaking changes: None Signed-off-by: Tanner Jones <tanner@testifysec.com>
Add test cases to verify the behavior of the --insecure flag in registry authentication: - Test successful HTTP connections when --insecure is used - Test successful HTTPS connections with self-signed certificates when --insecure is used - Test failure of HTTPS connections with invalid certificates when --insecure is not used - Test failure of HTTP connections when --insecure is not used These tests ensure that the --insecure flag properly handles both: 1. Allowing plain HTTP connections 2. Skipping certificate verification for HTTPS connections The test suite uses a local test registry with both HTTP and HTTPS (self-signed) endpoints to verify the authentication behavior in different security contexts. Signed-off-by: Tanner Jones <tanner@testifysec.com>
Signed-off-by: Tanner Jones <tanner@testifysec.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tannerjones4075 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/ok-to-test |
Any movement? I don't see any additional tests being ran. |
ok-to-test is not actually needed here, sorry. Also, I apologize for the delay in reviewing this PR (we have been busy with the upcoming release). Still, I want to assure you that this PR is under our radar. Thank tyou |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds an --insecure flag to the registry auth basic command to allow plain HTTP connections and skip TLS certificate verification.
- Updates the OCI auth client to support
InsecureSkipVerifyvia a newWithInsecureoption. - Enhances the basic login flow to detect
http:///https://schemes and apply insecure or fallback behavior. - Binds the new flag in the CLI, updates utilities for scheme stripping, and adds tests for various insecure scenarios.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/oci/authn/client.go | Added Insecure option and configures transport to skip TLS verify. |
| internal/utils/validate.go | Strips http:///https:// prefixes before extracting registry name. |
| internal/login/basic/basic.go | Implements insecure flag logic: scheme detection, fallback, and errors. |
| cmd/registry/auth/basic/basic.go | Binds --insecure, passes WithInsecure() to client creation. |
| cmd/registry/auth/basic/basic_test.go | Tests covering HTTP/HTTPS success and failure with/without --insecure. |
Comments suppressed due to low confidence (4)
cmd/registry/auth/basic/basic.go:29
- The import
oras.land/oras-go/v2/registry/remote/authis unused in this file and should be removed to keep imports clean.
"oras.land/oras-go/v2/registry/remote/auth",
cmd/registry/auth/basic/basic.go:84
- [nitpick] The flag description only mentions SSL certs; consider clarifying that
--insecurealso enables plain HTTP and skips TLS verification for self-signed certificates.
--insecure allow connections to SSL registry without certs
internal/login/basic/basic.go:61
- [nitpick] This error message could be more informative by including the registry hostname, e.g.,
cannot use plain HTTP for %q without --insecure flag.
return fmt.Errorf("cannot use plain HTTP without --insecure flag")
internal/utils/validate.go:26
- The
strings.TrimPrefixcalls require importing thestringspackage. Addimport "strings"to avoid a compile error.
ref = strings.TrimPrefix(strings.TrimPrefix(ref, "http://"), "https://")
remove duplicative r.CheckConnection(ctx) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Tanner Jones <78619684+tannerjones4075@users.noreply.github.com>
…s4075/falcoctl into feature/insecure-flag Signed-off-by: Tanner Jones <tanner@testifysec.com>
28a2768 to
0f153fe
Compare
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/lifecycle stale |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh with Rotten issues close after an additional 30d of inactivity. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle rotten |
|
/remove-lifecycle rotten |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh with Stale issues rot after an additional 30d of inactivity and eventually close. If this issue is safe to close now please do so with Provide feedback via https://github.com/falcosecurity/community. /lifecycle stale |
|
/remove-lifecycle stale |
|
Hi @tannerjones4075, First of all, thanks for this PR, since the However, commits are missing DCO sign-off, which is required for merging. Moreover, since this PR has been open for a while without activity, I'm going to close it for now to keep our backlog tidy. Please don't take this personally. Your contribution is appreciated! Feel free to reopen this PR (or open a fresh one) if you have time to revisit it. We're happy to help if you have any questions. Thanks again 🙏 |
|
/close |
|
@leogr: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What type of PR is this?
feature
Any specific area of the project related to this PR?
cli
tests
What this PR does / why we need it:
Added --insecure flag to registry auth basic command
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I tested this by created a local private registry and authenticated to it with the insecure tag and I was able to pull and pull from the registry:
docker run -d -p 5001:5000 --name registry1 registry:2 ./falcoctl registry auth basic --insecure -u testuser -p testpass --config ~/.falcoctl/config.yaml http://localhost:5001