Enabling mTLS between kraken services #390
Conversation
That's a great idea! Could you incorporate a config setting for this change into this PR itself? |
| const DefaultClientVerification = ` | ||
| ssl_verify_client optional; | ||
| set $required_verified_client 1; | ||
| if ($scheme = http) { |
There was a problem hiding this comment.
As mentioned it would be better if we can drive it via config.
There was a problem hiding this comment.
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
|
|
||
| // Download returns the blob of d. Callers should close the returned ReadCloser | ||
| // when done reading the blob. | ||
| func (c *HTTPClient) Download(namespace string, d core.Digest) (io.ReadCloser, error) { |
There was a problem hiding this comment.
Shouldn't blob downloads from Agents be unauthenticated ?
There was a problem hiding this comment.
Why they should? I thought that we should encrypt all the traffic, isn't it?
There was a problem hiding this comment.
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
|
|
||
| // Download returns the blob of d. Callers should close the returned ReadCloser | ||
| // when done reading the blob. | ||
| func (c *HTTPClient) Download(namespace string, d core.Digest) (io.ReadCloser, error) { |
There was a problem hiding this comment.
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
| const DefaultClientVerification = ` | ||
| ssl_verify_client optional; | ||
| set $required_verified_client 1; | ||
| if ($scheme = http) { |
There was a problem hiding this comment.
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
| // 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))) |
There was a problem hiding this comment.
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?
What
Change Kraken setting to enforce TLS in the communication between services.
Why
We want to onboard Kraken to secure proxy or use mTLS, this PR, will allow enforcing certificates verification if the certificates are provided in the request, and Kraken services are already providing them in our infra.
How
Remove optional verification between services.
Tested in the devzone.
Things we might want to consider
Have a config to setting for this changes.