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

support insecure registries #461

Closed
cgwalters opened this issue Apr 5, 2024 · 6 comments · Fixed by #580
Closed

support insecure registries #461

cgwalters opened this issue Apr 5, 2024 · 6 comments · Fixed by #580
Labels
area/updates Related to upgrading between versions enhancement New feature or request

Comments

@cgwalters
Copy link
Collaborator

I'd resisted doing this a lot, but there are enough local dev/test scenarios where it'd be useful to be able to fetch from an insecure registry.

This would probably be a simple new insecure: true flag in the host image spec which we end up passing down into the ostree-ext/skopeo stack.

@cgwalters cgwalters added enhancement New feature or request area/updates Related to upgrading between versions labels Apr 5, 2024
@jmarrero
Copy link
Member

@ravanelli is going to look at this during the sprint. Let us know if that is ok.

@cgwalters
Copy link
Collaborator Author

Awesome 🎉 Happy to join a call and talk about it/mentor

@ravanelli
Copy link

Thanks @cgwalters, I'm totally new to bootc and also to Rust. I was looking around and seems you mentioned this part: https://github.com/containers/bootc/blob/main/lib/src/spec.rs#L63
Is it correct?
If so, I will take the time to do the change and understand how to properly test it.

@cgwalters
Copy link
Collaborator Author

cgwalters commented Apr 10, 2024

That's signature verification which is different from TLS. I stubbed this out to start

diff --git a/lib/src/deploy.rs b/lib/src/deploy.rs
index 3eb31a8..c35a001 100644
--- a/lib/src/deploy.rs
+++ b/lib/src/deploy.rs
@@ -121,6 +121,7 @@ pub(crate) async fn pull(
     quiet: bool,
 ) -> Result<Box<ImageState>> {
     let repo = &sysroot.repo();
+    // MODIFY HERE to eventually pass things down to the `skopeo` process with --src-tls-verify=false
     let ostree_imgref = &OstreeImageReference::from(imgref.clone());
     let mut imp = new_importer(repo, ostree_imgref).await?;
     let prep = match imp.prepare().await? {
diff --git a/lib/src/spec.rs b/lib/src/spec.rs
index 5f6df93..fbf2d44 100644
--- a/lib/src/spec.rs
+++ b/lib/src/spec.rs
@@ -74,6 +74,9 @@ pub struct ImageReference {
     /// Signature verification type
     #[serde(skip_serializing_if = "Option::is_none")]
     pub signature: Option<ImageSignature>,
+    /// Skip TLS and certificate verification; this is very insecure and
+    /// should only be used in testing environments.
+    pub disable_tls_verification: bool,
 }
 
 /// The status of the booted image

This will also likely require a change to...oh no wait we already have https://github.com/containers/containers-image-proxy-rs/blob/28155f45bf635edcbaf5b4e3540f3e3c54a13bd2/src/imageproxy.rs#L127

(Edit yeah let's also call it insecure_disable_tls_verification here too to emphasize)

@ravanelli
Copy link

@cgwalters I wonder if you could clarify if the path I'm going is the right one?
Seems we don't need to change the the pull function as you mentioned here:

diff --git a/lib/src/deploy.rs b/lib/src/deploy.rs
index 3eb31a8..c35a001 100644
--- a/lib/src/deploy.rs
+++ b/lib/src/deploy.rs
@@ -121,6 +121,7 @@ pub(crate) async fn pull(
     quiet: bool,
 ) -> Result<Box<ImageState>> {
     let repo = &sysroot.repo();
+    // MODIFY HERE to eventually pass things down to the `skopeo` process with --src-tls-verify=false
     let ostree_imgref = &OstreeImageReference::from(imgref.clone());
     let mut imp = new_importer(repo, ostree_imgref).await?;
     let prep = match imp.prepare().await? {

If I understood it, I need to pass the new insecure_disable_tls_verification to OstreeImageReference that eventually will be used to ImageProxyConfig. The issue I'm getting now is: I probably need to get it added as part of the ostree-res-ext first, around here.
Can I say is it the right path to go?
You can see what I already did in my branch here

ravanelli added a commit to ravanelli/bootc that referenced this issue Apr 22, 2024
- Introduce 'insecure-disable-tls-verification' parameter for
enabling TLS verification skipping

Signed-off-by: Renata <rravanel@redhat.com>
ravanelli added a commit to ravanelli/bootc that referenced this issue Apr 22, 2024
- Introduce 'insecure-disable-tls-verification' parameter for
enabling TLS verification skipping

Signed-off-by: Renata <rravanel@redhat.com>
ravanelli added a commit to ravanelli/bootc that referenced this issue Apr 22, 2024
- Introduce 'insecure-disable-tls-verification' parameter for
enabling TLS verification skipping
- Fix Issue: containers#461

Signed-off-by: Renata <rravanel@redhat.com>
ravanelli added a commit to ravanelli/bootc that referenced this issue Apr 22, 2024
- Introduce 'insecure-disable-tls-verification' parameter for
skipping TLS verification;
- Fix Issue: containers#461.

Signed-off-by: Renata <rravanel@redhat.com>
@mangelajo
Copy link

Hey, has any progress happened on this topic?

We are looking into local insecure registries for E2E testing a fleet management system based on bootc.

cgwalters added a commit to cgwalters/bootc that referenced this issue Jun 3, 2024
We don't have a `--tls-verify=false` flag because:

- It's a bad idea to make it too easy to do
- When you *do* do it, you want it to be persistent/global anyways
  so a global config file is righ

Closes: containers#461

Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters added a commit to cgwalters/bootc that referenced this issue Jun 3, 2024
We don't have a `--tls-verify=false` flag because:

- It's a bad idea to make it too easy to do
- When you *do* do it, you want it to be persistent/global anyways
  so a global config file is righ

Closes: containers#461

Signed-off-by: Colin Walters <walters@verbum.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/updates Related to upgrading between versions enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants