feat: Extended Root CA for client TLS connections (#181) #744

christian-roggia · 2021-06-08T13:19:43Z

See #706.

.schema/config.schema.json

docs/docs/api-access-rules.md

internal/certs/cert_manager.go

aeneasr · 2021-06-11T10:41:40Z

@christian-roggia marking this as a draft because you said it's WIP on slack :)

christian-roggia · 2021-06-15T21:15:16Z

@aeneasr this PR should be cleaned up and ready for review. I deployed this version to our cluster and works like a charm with self-signed TLS certificates. As soon as this is merged I will open a PR also for the Helm charts.

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

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

* 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

aeneasr · 2021-06-21T08:04:06Z

Thank you, this looks great! The CI is failing because some files are formatted incorrectly. To format them, run:

$ make format
$ git commit -a -m "styles: format code"
$ git push

Thank you!

christian-roggia · 2021-06-21T08:28:46Z

As I wrote on Slack, I run the command before but I didn't commit the two CHANGELOG files as I didn't touch them and the number of changed lines was huge. Looks like it worked tough now that I committed also those two files, I guess the linter was recently updated?

Thank you anyway for the help!

aeneasr · 2021-06-21T08:55:04Z

The problem was that I changed something in the CI which pushed an unformatted commit to master. Your PR includes those changes thus had issues in the linter. The issue is resolved in the CI but since then no update was triggered on Ory Oathkeeper.

I'll be reviewing your PR shortly!

aeneasr

Thank you for advancing this! From the documentation and overall implementation this looks very good!

To confirm that everything works as expected I think we definitely need to add some more tests.

aeneasr · 2021-06-22T09:35:41Z

internal/certs/cert_manager.go

+	"github.com/ory/oathkeeper/driver/configuration"
+)
+
+type CertManager struct {


Could you please add some tests to the certmanager? Also, what should we take care of here. For example, test different cert pools? How would this look like on Windows? :)

I haven't worked with x509 cert pools before. We should also generally ensure that:

The cache is used

The cache is invalidated properly

No untrusted certs are being used

The OS cert store is being used appropriately on the different OS'es (could be difficult for mac as we don't have a pipeline for that...)

aeneasr · 2021-06-22T09:36:31Z

docs/docs/api-access-rules.md

+serve:
+  proxy:
+    client_tls:
+      trusted_certificates:


If this is used for other upstream services (authenticators, ...) I think it would make sense to have client_tls as a root level config item

aeneasr · 2021-06-22T09:36:56Z

driver/configuration/provider.go

+	ProxyServeClientTLSTrustedCerts() []string
+	ProxyServeClientTLSCacheRefreshFrequency() int
+	ProxyServeClientTLSCacheTimeToLive() time.Duration


If we move the config to the root level we need to change this

aeneasr · 2021-06-22T09:36:59Z

driver/configuration/provider_viper.go

+	ViperKeyProxyClientTLSTrustedCerts          = "serve.proxy.client_tls.trusted_certificates"
+	ViperKeyProxyClientTLSCacheRefreshFrequency = "serve.proxy.client_tls.cache.refresh_frequency"
+	ViperKeyProxyClientTLSCacheTimeToLive       = "serve.proxy.client_tls.cache.ttl"


If we move the config to the root level we need to change this

aeneasr · 2021-06-22T09:37:37Z

internal/certs/round_tripper.go

+	tr *http.Transport
+}
+
+func NewRoundTripper(cm *CertManager) *RoundTripper {


Some tests would be great here as well

aeneasr · 2021-06-22T09:38:24Z

pipeline/authn/authenticator_bearer_token.go

@@ -8,9 +8,11 @@ import (
 	"github.com/tidwall/gjson"


Could we ensure in all the authenticators that the new TLS config is actually being used by adding tests for that case?

aeneasr · 2021-06-22T09:39:35Z

proxy/proxy_test.go

@@ -429,6 +431,61 @@ func TestConfigureBackendURL(t *testing.T) {
 	}
 }

+func TestUpstreamAppendCaCertToRootCa(t *testing.T) {


I think we could make this a generic test helper which could then be used in the different relevant places (authenticators, mutators, proxy, authorizers)

aeneasr · 2021-07-05T09:34:42Z

Just checking in if you need help with the tests :)

christian-roggia · 2021-07-07T17:25:38Z

I will be updating the configuration path to be at the root level but I will most definitely need help setting up tests!

aeneasr · 2021-07-12T12:24:09Z

Ok! Ping me when you're ready to work on it :) Meanwhile I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

aeneasr · 2021-12-26T17:42:05Z

As there has not been an update in the past 6 month, we'll consider this one stale and I'll close the PR. But please, do not feel discouraged in continuing the work if you have some time, the change is still appreciated! :)

aeneasr reviewed Jun 9, 2021

View reviewed changes

.schema/config.schema.json Outdated Show resolved Hide resolved

docs/docs/api-access-rules.md Outdated Show resolved Hide resolved

docs/docs/api-access-rules.md Show resolved Hide resolved

internal/certs/cert_manager.go Show resolved Hide resolved

aeneasr mentioned this pull request Jun 11, 2021

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

Closed

5 tasks

aeneasr marked this pull request as draft June 11, 2021 10:41

christian-roggia changed the title ~~feat: Extended Root CA for upstream connections (#181)~~ feat: Extended Root CA for client TLS connections (#181) Jun 15, 2021

christian-roggia marked this pull request as ready for review June 15, 2021 21:14

christian-roggia mentioned this pull request Jun 15, 2021

feat: add support for client TLS certs ory/k8s#280

Closed

wraix and others added 16 commits June 20, 2021 16:59

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

818510e

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

Adds support for appending certificates to the Root CA

59307d2

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

Adds upstream certificate documentation

d01b48a

Remove unused code

c17465a

proxy: Tests append cert to root ca for upstream

0be3ef5

driver: Adds cacheing of Upstream Transport

5f98726

* 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

docs: Adds ca_refresh_frequency param

91c9d9c

feat: use atomic counter

ad8f3e7

feat: allow multiple CA certs and add cache TTL

8b11c65

feat: disable cache TTL if set to 0

d7d9758

fix: do not use system cert pool on windows

163401a

refactor: set certs for all HTTP requests

a5397a5

refactor: better support for TLS client connections

b321d46

fix: update docs

86c5e92

fix: rename functions and OAuth 2.0 import

89092df

fix: set default array value to null

c1110d1

christian-roggia force-pushed the fix_issue_118_trust_selfsigend_certs branch from e49e6b3 to c1110d1 Compare June 20, 2021 15:02

christian-roggia added 2 commits June 20, 2021 17:27

fix: do not refresh the transport every request

dfee24b

fix: linting and formatting

3db8369

styles: format code

a3645e3

aeneasr self-requested a review June 21, 2021 08:55

aeneasr self-assigned this Jun 21, 2021

aeneasr added the feat New feature or request. label Jun 21, 2021

aeneasr mentioned this pull request Jun 21, 2021

Add passthrough support for request headers in http requests from Oathkeeper. #752

Closed

aeneasr reviewed Jun 22, 2021

View reviewed changes

christian-roggia added 2 commits July 4, 2021 17:18

feat: add tls configuration to token endpoint

913ebe9

fix: set TLS context for token fetching

a389da2

aeneasr marked this pull request as draft July 12, 2021 19:51

aeneasr closed this Dec 26, 2021

aeneasr mentioned this pull request Jan 6, 2022

Lessons learned & ORY Oathkeeper NextGen #441

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Extended Root CA for client TLS connections (#181) #744

feat: Extended Root CA for client TLS connections (#181) #744

christian-roggia commented Jun 8, 2021

aeneasr commented Jun 11, 2021

christian-roggia commented Jun 15, 2021

aeneasr commented Jun 21, 2021

christian-roggia commented Jun 21, 2021

aeneasr commented Jun 21, 2021

aeneasr left a comment

aeneasr Jun 22, 2021

aeneasr Jun 22, 2021

aeneasr Jun 22, 2021

aeneasr Jun 22, 2021

aeneasr Jun 22, 2021

aeneasr Jun 22, 2021

aeneasr Jun 22, 2021

aeneasr commented Jul 5, 2021

christian-roggia commented Jul 7, 2021

aeneasr commented Jul 12, 2021

aeneasr commented Dec 26, 2021

feat: Extended Root CA for client TLS connections (#181) #744

feat: Extended Root CA for client TLS connections (#181) #744

Conversation

christian-roggia commented Jun 8, 2021

aeneasr commented Jun 11, 2021

christian-roggia commented Jun 15, 2021

aeneasr commented Jun 21, 2021

christian-roggia commented Jun 21, 2021

aeneasr commented Jun 21, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Jun 22, 2021

Choose a reason for hiding this comment

aeneasr Jun 22, 2021

Choose a reason for hiding this comment

aeneasr Jun 22, 2021

Choose a reason for hiding this comment

aeneasr Jun 22, 2021

Choose a reason for hiding this comment

aeneasr Jun 22, 2021

Choose a reason for hiding this comment

aeneasr Jun 22, 2021

Choose a reason for hiding this comment

aeneasr Jun 22, 2021

Choose a reason for hiding this comment

aeneasr commented Jul 5, 2021

christian-roggia commented Jul 7, 2021

aeneasr commented Jul 12, 2021

aeneasr commented Dec 26, 2021