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

Add in registry mutual tls #361

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Add in registry mutual tls #361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions pkg/imgpkg/cmd/registry_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ type RegistryFlags struct {
VerifyCerts bool
Insecure bool

MutualTLS bool
ClientCertificate string
ClientKey string

Username string
Password string
Token string
Expand All @@ -33,6 +37,10 @@ func (r *RegistryFlags) Set(cmd *cobra.Command) {
cmd.Flags().BoolVar(&r.VerifyCerts, "registry-verify-certs", true, "Set whether to verify server's certificate chain and host name")
cmd.Flags().BoolVar(&r.Insecure, "registry-insecure", false, "Allow the use of http when interacting with registries")

cmd.Flags().BoolVar(&r.MutualTLS, "registry-mutual-tls", false, "Set whether or not to use mutual TLS. If true set registry-client-cert-path and registry-client-key-path")
Copy link
Contributor

Choose a reason for hiding this comment

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

by using flags we are making it only possible to use this for a single registry -- this may work well if fetching a single image, but is not great for anything beyond that (bundles for example may be referencing images in different registries).

can you mention what use case you are going after? what are you fetching (single image?)?

Copy link
Author

Choose a reason for hiding this comment

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

ah I suppose yeah if we are pulling them from different registries. I suppose I could use a slice like we do for ca certs but then have to somehow keep the pairs together. I am fetching a bundle and pushing it to a single registry(that has mutual tls on it). Airgapped deployment of tanzu kubernetes grid in amazon-c2s

Copy link
Member

Choose a reason for hiding this comment

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

We might need to allow something similar to https://carvel.dev/imgpkg/docs/v0.27.0/auth/#via-environment-variables as well in order to get this working for multiple registries.
This will have consequences as well on the Registry and when we are creating the transports because we would need to attach these certificates not on the base transport but on the a particular transport that is used for a particular registry

Copy link
Author

Choose a reason for hiding this comment

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

@joaopapereira been searching around the codebase where is the _0, _1, _n for env support added for registry, password, and username. I only see it loading IMGPKG_USERNAME or IMGPKG_PASSWORD for example.

Also, are you saying updating the newHTTPTransport function for each registry will not fix this as that is the base transport?

Copy link
Member

Choose a reason for hiding this comment

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

Yes you are right, _0 and _1 are used to populate the keychain that is used when talking to the registry.

But in order to support MutalTLS for more than 1 registry we will need to extend the registry/transport.go structs to when they create a new round-tripper to select the appropriate keys based on the MutualTLS configuration.

Doesn't look very straightforward.

  1. The keychain would have to signal that it uses MutualTLS to the CreateRoundTripper function
  2. The baseRoundTripper attribute would have to be converted into a http.Transport or we can try to cast it on the fly so that the MutualTLS could be appropriately set on the transport provided to transport.NewWithContext
  3. Keychain would have to know how to read the _0 for the MutualTLS

Let me know if this makes sense, I am also free to try to talk this through a little more

cmd.Flags().StringVar(&r.ClientCertificate, "registry-client-cert-path", "", "Add Client Cert to authenticate against registry with (format: /tmp/foo)")
cmd.Flags().StringVar(&r.ClientKey, "registry-client-key-path", "", "Add Client Key to authenticate against registry with (format: /tmp/foo)")

cmd.Flags().StringVar(&r.Username, "registry-username", "", "Set username for auth ($IMGPKG_USERNAME)")
cmd.Flags().StringVar(&r.Password, "registry-password", "", "Set password for auth ($IMGPKG_PASSWORD)")
cmd.Flags().StringVar(&r.Token, "registry-token", "", "Set token for auth ($IMGPKG_TOKEN)")
Expand Down Expand Up @@ -71,6 +79,10 @@ func (r *RegistryFlags) AsRegistryOpts() registry.Opts {
VerifyCerts: r.VerifyCerts,
Insecure: r.Insecure,

MutualTLS: r.MutualTLS,
ClientCertificate: r.ClientCertificate,
ClientKey: r.ClientKey,

Username: r.Username,
Password: r.Password,
Token: r.Token,
Expand Down
24 changes: 21 additions & 3 deletions pkg/imgpkg/registry/registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ type Opts struct {

IncludeNonDistributableLayers bool

MutualTLS bool
ClientCertificate string
ClientKey string

Username string
Password string
Token string
Expand Down Expand Up @@ -453,9 +457,23 @@ func newHTTPTransport(opts Opts) (*http.Transport, error) {
clonedDefaultTransport := http.DefaultTransport.(*http.Transport).Clone()
clonedDefaultTransport.ForceAttemptHTTP2 = false
clonedDefaultTransport.ResponseHeaderTimeout = opts.ResponseHeaderTimeout
clonedDefaultTransport.TLSClientConfig = &tls.Config{
RootCAs: pool,
InsecureSkipVerify: opts.VerifyCerts == false,
if opts.MutualTLS {
cert, err := tls.LoadX509KeyPair(opts.ClientCertificate, opts.ClientKey)
if err != nil {
return nil, fmt.Errorf("Failed to read client certificate or key from '%s' and '%s': %s", opts.ClientCertificate, opts.ClientKey, err)
}
certs := make([]tls.Certificate, 0)
certs = append(certs, cert)
clonedDefaultTransport.TLSClientConfig = &tls.Config{
RootCAs: pool,
Certificates: certs,
InsecureSkipVerify: opts.VerifyCerts == false,
}
} else {
clonedDefaultTransport.TLSClientConfig = &tls.Config{
RootCAs: pool,
InsecureSkipVerify: opts.VerifyCerts == false,
}
}

return clonedDefaultTransport, nil
Expand Down