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

feat: Extended Root CA for upstream connections (#181) #706

Closed
wants to merge 12 commits into from

Conversation

wraix
Copy link

@wraix wraix commented Apr 25, 2021

This allows for appending a certificate file to the Root CA without altering the system Root CA. This is useful for allowing self-signed certificates on the upstream connections

Related issue

Proposed changes

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

This allows for appending a certificate file to the Root CA without altering the system Root CA. This is useful for allowing self-signed certificates on the upstream connections
@CLAassistant
Copy link

CLAassistant commented Apr 25, 2021

CLA assistant check
All committers have signed the CLA.

This adds support for appending certificates to the Root CA of the proxy process on upstreams. This does not alter the entire system.
@wraix
Copy link
Author

wraix commented Apr 30, 2021

I got a notification from github saying: "Windows go test #412 - No test was run". Not sure what that means?

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you! This looks great! Could you please add a test for this? I think you could use httptest.NewTLSServer and extract the certificate from it

@wraix
Copy link
Author

wraix commented May 3, 2021

I have added a test case to test that appending a certificate file to root ca works for TLS on upstream connections.

Enjoy.

@aeneasr
Copy link
Member

aeneasr commented May 8, 2021

Thank you! Hectic week...will review on Monday!

@aeneasr aeneasr self-requested a review May 8, 2021 10:37
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

This looks really good. I think we need to improve the TLS set up so that it doesn't load on every request. Also it would be great if you could document this new feature as a new section Upstream Configuration.

Thanks!

driver/registry_memory.go Outdated Show resolved Hide resolved
@aeneasr
Copy link
Member

aeneasr commented May 21, 2021

Are you still up for the changes? 🧐 If you need any help, let us know!

@christian-roggia
Copy link
Contributor

+1 this is extremely useful for internal load balancers that communicate over HTTPS.

@wraix
Copy link
Author

wraix commented May 27, 2021

Yes i will try and find some time for this. Might need some guidance on where to put the cached transport, but will give it a go.

wraix added 3 commits May 29, 2021 14:41
* Adds cacheing of UpstreamTransport to avoid excessive IO ops at scale
* Adds configuration option `ca_refresh_frequency` for periodic checks of certificate file changes
* Adds new Upstream Configuration section
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! This looks much better. Just one more thing :)

// UpstreamTransport decides the transport to use for the upstream for the request.
func (r *RegistryMemory) UpstreamTransport(req *http.Request) (http.RoundTripper, error) {

r.upstreamRequestCount++
Copy link
Member

Choose a reason for hiding this comment

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

Since we probably have highly concurrent system this might cause issues. I think it would make sense to use atomic.Uint instead ( https://golang.org/pkg/sync/atomic/ ) to count up here :)

@aeneasr
Copy link
Member

aeneasr commented Jun 11, 2021

@christian-roggia is continuing the work done here in #744 :) So I'll be marking this as a draft!

@aeneasr aeneasr marked this pull request as draft June 11, 2021 10:08
@aeneasr
Copy link
Member

aeneasr commented Sep 18, 2021

Closing this as it is superseded by #744

@aeneasr aeneasr closed this Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants