Skip to content

Enabling mTLS between kraken services #390

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

Closed
hweawer wants to merge 18 commits into from
+11 −17
Closed

Enabling mTLS between kraken services #390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
65b9428
Enable mTLS
hweawer Dec 31, 2024
03c3aa5
Merge branch 'master' into KRAK-360
hweawer Jan 7, 2025
31184e9
Change config
hweawer Jan 8, 2025
532a99a
Revert
hweawer Jan 8, 2025
782bf6c
Stdout
hweawer Jan 8, 2025
4e9097d
Format
hweawer Jan 8, 2025
bf86006
Revert
hweawer Jan 9, 2025
dc4fda0
Fix binary path
hweawer Jan 13, 2025
a8654df
Remove skip verification on GET and HEAD
hweawer Jan 13, 2025
1f56a42
Http calls from the agent client
hweawer Jan 15, 2025
48de62c
Revert
hweawer Jan 15, 2025
ea21c12
Https in paths
hweawer Jan 16, 2025
d2dec56
Remove HTTP skip
hweawer Jan 17, 2025
8971b8e
Revert writing error logs to stdout
hweawer Jan 20, 2025
1ead1d8
HTTPS
hweawer Jan 24, 2025
38a48a7
HTTPS for registry requests
hweawer Jan 26, 2025
b8c49cc
Revert "HTTPS for registry requests"
hweawer Jan 27, 2025
f32eff9
Revert HTTPS in registry
hweawer Jan 27, 2025
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
6 changes: 3 additions & 3 deletions agent/agentclient/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
Expand Down Expand Up @@ -48,7 +48,7 @@ func New(addr string) *HTTPClient {
// GetTag resolves tag into a digest. Returns ErrTagNotFound if the tag does
// not exist.
func (c *HTTPClient) GetTag(tag string) (core.Digest, error) {
resp, err := httputil.Get(fmt.Sprintf("http://%s/tags/%s", c.addr, url.PathEscape(tag)))
resp, err := httputil.Get(fmt.Sprintf("https://%s/tags/%s", c.addr, url.PathEscape(tag)))
Copy link
Collaborator

@Anton-Kalpakchiev Anton-Kalpakchiev Jan 23, 2025

Choose a reason for hiding this comment

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

We have many hardcoded http requests in the code (search for http:// in the codebase). Why do we need to change this one to https and not the others?

if err != nil {
if httputil.IsNotFound(err) {
return core.Digest{}, ErrTagNotFound
Expand All @@ -72,7 +72,7 @@ func (c *HTTPClient) GetTag(tag string) (core.Digest, error) {
func (c *HTTPClient) Download(namespace string, d core.Digest) (io.ReadCloser, error) {
Copy link
Collaborator

@gkeesh7 gkeesh7 Jan 20, 2025

Choose a reason for hiding this comment

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

Shouldn't blob downloads from Agents be unauthenticated ?

Copy link
Collaborator Author

@hweawer hweawer Jan 23, 2025

Choose a reason for hiding this comment

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

Why they should? I thought that we should encrypt all the traffic, isn't it?

Copy link
Collaborator

@Anton-Kalpakchiev Anton-Kalpakchiev Jan 23, 2025

Choose a reason for hiding this comment

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

All traffic should be authenticated and encrypted. The only exception is that requests from localhost should not be required to authenticate themselves, i.e. only TLS will be enforced instead of mTLS.

However, I wonder if hardcoding the HTTP/HTTPS protocol like this makes sense. I'm not sure about this but I believe Kraken supports falling back to HTTP if https doesn't work and people outside of Uber might use this feature. This means that if we hardcode the HTTPS protocol, the new version might not work for them.
Check this comment and piece of code for more context:

kraken/utils/httputil/httputil.go

Line 317 in cfcda81

// Retry without tls. During migration there would be a time when the

resp, err := httputil.Get(
fmt.Sprintf(
"http://%s/namespace/%s/blobs/%s",
"https://%s/namespace/%s/blobs/%s",
c.addr, url.PathEscape(namespace), d))
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion lib/backend/httpbackend/http_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
Expand Down
2 changes: 1 addition & 1 deletion lib/backend/registrybackend/tagclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
Expand Down
8 changes: 4 additions & 4 deletions lib/backend/testfs/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (c *Client) Stat(namespace, name string) (*core.BlobInfo, error) {
return nil, fmt.Errorf("pather: %s", err)
}
resp, err := httputil.Head(
fmt.Sprintf("http://%s/files/%s", c.config.Addr, p))
fmt.Sprintf("https://%s/files/%s", c.config.Addr, p))
if err != nil {
if httputil.IsNotFound(err) {
return nil, backenderrors.ErrBlobNotFound
Expand All @@ -107,7 +107,7 @@ func (c *Client) Upload(namespace, name string, src io.Reader) error {
return fmt.Errorf("pather: %s", err)
}
_, err = httputil.Post(
fmt.Sprintf("http://%s/files/%s", c.config.Addr, p),
fmt.Sprintf("https://%s/files/%s", c.config.Addr, p),
httputil.SendBody(src))
return err
}
Expand All @@ -119,7 +119,7 @@ func (c *Client) Download(namespace, name string, dst io.Writer) error {
return fmt.Errorf("pather: %s", err)
}
resp, err := httputil.Get(
fmt.Sprintf("http://%s/files/%s", c.config.Addr, p))
fmt.Sprintf("https://%s/files/%s", c.config.Addr, p))
if err != nil {
if httputil.IsNotFound(err) {
return backenderrors.ErrBlobNotFound
Expand All @@ -145,7 +145,7 @@ func (c *Client) List(prefix string, opts ...backend.ListOption) (*backend.ListR
}

resp, err := httputil.Get(
fmt.Sprintf("http://%s/list/%s", c.config.Addr, path.Join(c.pather.BasePath(), prefix)))
fmt.Sprintf("https://%s/list/%s", c.config.Addr, path.Join(c.pather.BasePath(), prefix)))
if err != nil {
return nil, err
}
Expand Down
8 changes: 1 addition & 7 deletions nginx/config/default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
Expand All @@ -31,12 +31,6 @@ var _nameToDefaultTemplate = map[string]string{
const DefaultClientVerification = `
ssl_verify_client optional;
set $required_verified_client 1;
if ($scheme = http) {
Copy link
Collaborator

@gkeesh7 gkeesh7 Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

As mentioned it would be better if we can drive it via config.

Copy link
Collaborator

@Anton-Kalpakchiev Anton-Kalpakchiev Jan 23, 2025

Choose a reason for hiding this comment

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

What are the benefits of using config for this? The only benefit I see is if someone wants to use Kraken with HTTP instead of HTTPS. But I believe we should discuss internally whether we want to support HTTP communication for the future. We've already partially discussed this here: #379

set $required_verified_client 0;
}
if ($request_method ~ ^(GET|HEAD)$) {
set $required_verified_client 0;
}
if ($remote_addr = "127.0.0.1") {
set $required_verified_client 0;
}
Expand Down
2 changes: 1 addition & 1 deletion nginx/nginx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
Expand Down
Loading