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.

Sign up for GitHub

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

Jump to bottom

OCI GetChart #2323

Merged
merged 6 commits into from
Feb 8, 2021
Merged

OCI GetChart #2323

merged 6 commits into from
Feb 8, 2021

Conversation

andresmgot
Copy link
Contributor

Description of the change

Follow up of #2311

Now it's time to implement the actual interface for the OCI case.

Possible drawbacks

As explained in a TODO, using a CA cert won't work here. The only alternative with ORAS is to use an insecure flag but I didn't included that (yet?).

Applicable issues

absoludity
absoludity approved these changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@absoludity absoludity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lack of support for encrypted traffic to a private registry is a bit worrying? Keen to know if this is known, intentional (in oras/helm) or an issue they're working on?

pkg/chart/chart.go

// InitClient returns an HTTP client based on the chart details loading a
// custom CA if provided (as a secret)
// TODO(andresmgot): Using a custom CA cert is not supported by ORAS (neither helm), only using the insecure flag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why (as in, is this intentional to require public tls certs)? What are the implications for private registries?

pkg/helm/test/utils.go
)

// CheckHeader verifies that the given puller contains the given header
func CheckHeader(t *testing.T, puller helm.ChartPuller, key, value string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice move :)

Base automatically changed from chartv1 to chartOCI February 8, 2021 09:54
@andresmgot
Copy link
Contributor Author

The lack of support for encrypted traffic to a private registry is a bit worrying? Keen to know if this is known, intentional (in oras/helm) or an issue they're working on?

This is the main issue for Helm: helm/helm#6324 and the related implementation in ORAS: oras-project/oras#109 but in none of them there are mentions to support a custom CA. In any case, the traffic is encrypted with TLS but the certs are not validated.

@andresmgot andresmgot merged commit 56f3ca4 into chartOCI Feb 8, 2021
@andresmgot andresmgot deleted the ociClient branch February 8, 2021 10:17
andresmgot pushed a commit that referenced this pull request Feb 8, 2021
ChartClient internal refactor (#2310)
31c39ee 
* Refactor chart utils interface

* Refactor chart utils interface - part 2

* Add missing pkg

* self review

* Fix namespace selection

* Review

* Review

* Remove code related to charts v1 (#2311)

* OCI GetChart (#2323)
@andresmgot andresmgot mentioned this pull request Feb 8, 2021
Merged
@absoludity
Copy link
Contributor

The lack of support for encrypted traffic to a private registry is a bit worrying? Keen to know if this is known, intentional (in oras/helm) or an issue they're working on?

This is the main issue for Helm: helm/helm#6324 and the related implementation in ORAS: deislabs/oras#109 but in none of them there are mentions to support a custom CA. In any case, the traffic is encrypted with TLS but the certs are not validated.

I just created oras-project/oras#217 so we can at least be moving toward something not susceptible to machine-in-the-middle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@absoludity absoludity absoludity approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andresmgot @absoludity