[WIP] allow server cert rotation without a restart #1672

krasi-georgiev · 2019-10-21T20:30:10Z

I did a quick test and reading the file from disk on every request is ok sinse the kernel puts this in the pagecache so in practice it reads the file from memory when not changed and reads it from disk when it is replaced with a new one.

tests
refactor to reduce the performance overhead
try to implement something similar for the CA rotation.
the CA rotation works only for the client CAs and not for the server CAs. Missing GetServerConfig as mentioned in proposal: Add GetClientCAs to tls.Config golang/go#16066 (comment)
Tracking issue: proposal: crypto/tls: add GetConfigForServer callback to *tls.Config golang/go#22836
should be able to use VerifyPeerCertificate for this - proposal: crypto/tls: add GetConfigForServer callback to *tls.Config golang/go#22836 (comment)

related modules that use the same idea.
https://godoc.org/golang.org/x/crypto/acme/autocert
https://github.com/johanbrandhorst/certify

I did a quick test and reading the same file from disk is not a problem as the kernel caches it in the memory when the file is not changed.

Test for reading the same file again doesn't hit the disk

func Test_io(t *testing.T) {
	pid := os.Getpid()
	for {
		_, err := ioutil.ReadFile("env")
		testutil.Ok(t, err)

		io, err := ioutil.ReadFile("/proc/" + strconv.Itoa(pid) + "/io")
		testutil.Ok(t, err)

		fmt.Println(string(io))
		time.Sleep(500 * time.Millisecond)
	}
}

povilasv

🥇

brancz · 2019-10-22T12:56:47Z

This reads and parses the certificates on every request. I feel it would be appropriate to do this periodically instead and atomically swap them.

krasi-georgiev · 2019-10-22T16:10:34Z

This reads and parses the certificates on every request. I feel it would be appropriate to do this periodically instead and atomically swap them.

Yep, and it was the second item on my to do list in the PR description 😸

refactor to reduce the performance overhead

I already refactored it to compare mod times and reload only when different, but will think how to improve even more.
I think this is only important with the receiver which receives many requests per second all other components should be fine even with this approach right?
I am thinking to add some delay arg which would be used only by the receiver.

by the way currently the Prometheus scrape client does this with every request which I agree should be improved.
https://github.com/prometheus/common/blob/2afc453a9de43c46dadd462998d096e52cd4e4e3/config/http_config.go#L348

brancz · 2019-10-23T11:04:39Z

Sorry was too hasty. All good :)

GiedriusS · 2019-10-28T20:19:21Z

pkg/tls/tls.go

+			return nil, errors.Wrap(err, "building client CA certificate")
+		}
+
+		// ????? test if true...... This is a pointer it will update the underlying tls config pool.


I guess this is still a work in progress because of these comments?

yep one of the items in my todo list

test if the CA pool gets updated with the ponter

bwplotka · 2019-10-29T10:06:33Z

Do we have a story for compatible rotation? e.g how this will work between server and client? (: e.g does the rotation has to happen in the same time for both srv and client?

krasi-georgiev

Do we have a story for compatible rotation? e.g how this will work between server and client? (: e.g does the rotation has to happen in the same time for both srv and client?

good point. I think Thanos should only be responsible for rotating the certs when new ones are presented the rest should be handled by the config management.

All that said at the moment the setup allows a single CA and maybe it should be changed to allow multiple CA files to allow an easier rotation.

brancz · 2019-10-29T17:04:44Z

Even if a single file, CAs can usually be loaded as bundles so in reality there are multiple CAs in one file.

krasi-georgiev · 2019-11-01T15:18:05Z

now after writing the tests I realized that there is another problem.
getConfigForClient and getCertificate are called only with the tls handshake and NOT when sending or receiving data which means that for long running data flow it will never reload the certs as it will reuse the same connection and no new tls handshake will happen.
In this PR I have also added a cli flag so users can set the keep alive setting with MaxConnectionAge which enforces a new TLS hanshake when MaxConnectionAge expires.
New tls handshake is initiated when the connection is interrupted or when the MaxConnectionAge expires.

So with this new info I think there are 3 options:

keep the current implementation with a cli flag for MaxConnectionAge
run some loop which will restart the server gracefully or drop all connections when it detects tls cert files changes.
provide a reload endpoint and leave reloading to external config management.

Again I think this is important mainly for the thanos receiver as I imagine that with all other components a new connection and a new tls handshake would be created with every request so it will reload the certs as expected.

cc @bwplotka, @s-urbaniak

krasi-georgiev · 2019-11-07T01:52:59Z

@squat , @bwplotka @s-urbaniak waiting for some feedback on which options do you think is best for this implementation.

s-urbaniak · 2019-11-26T08:45:59Z

@krasi-georgiev Sorry for the late reply. I am voting for option "1. keep the current implementation with a cli flag for MaxConnectionAge"

I believe this is a sensible setting and less disruptive than the other options. Also, it does not require external coordination (someone has to implement that timeout).

Finally cert rotation happens in timespans of minutes, maybe even hours so a MaxConnectionAge in that timespan sounds sensible to me too.

@bwplotka @brancz Do you have additional thoughts or objections?

bwplotka · 2019-11-26T14:47:02Z

the CA rotation works only for the client CAs and not for the server CAs. Missing GetServerConfig as mentioned in golang/go#16066 (comment)

How did we work around this one?

So I think MaxConnectionAge here might be too short by default (1m), as kind of defies any KeepAlive logic.. but maybe something longer would make sense. Also something I would like to check if MaxConnectionAge is actually accounted for during long-running requests, although I think we don't do those longer than X minutes.

What's the requirement for those cert rotations in terms of delay? I think ~minutes is ok, so maybe we could start with this, but larger MaxConnectionAge

s-urbaniak · 2019-11-26T15:17:53Z

What's the requirement for those cert rotations in terms of delay? I think ~minutes is ok, so maybe we could start with this, but larger MaxConnectionAge

agreed, the ~minutes ballpark sounds good to me.

Signed-off-by: Krasi Georgiev <[email protected]>

krasi-georgiev · 2019-12-09T12:03:40Z

After a bit of a struggle the tls rotation is implemented with full e2e tests.

As mentioned in the first PR comment server CA rotation is missing becasue it is yet not added in the golang std library so I left a TODO note to add it when it is added in the golang std library. I don't think that this is a blocker for this PR and I will follow up the tracking golang issues for updates.

@squat I also noticed that there are no e2e tests for the reciever TLS, should I add it to this PR or you prefer to open another PR after this one is merged?

Signed-off-by: Krasi Georgiev <[email protected]>

squat

Looks quite good so far!

pkg/tls/tls.go

test/e2e/tls_test.go

pkg/tls/tls.go

Signed-off-by: Krasi Georgiev <[email protected]>

krasi-georgiev · 2019-12-09T13:31:30Z

@squat thanks for the review, addressed all comments.

didn't answer my original question though 😺

@squat I also noticed that there are no e2e tests for the reciever TLS, should I add it to this PR or you prefer to open another PR after this one is merged?

squat · 2019-12-09T13:40:50Z

let's do it in a new PR to keep the concerns separate :) I can make the PR

krasi-georgiev · 2020-01-02T16:03:25Z

@brancz @bwplotka any other comments for this PR?

krasi-georgiev · 2020-01-09T16:58:06Z

ping @bwplotka

GiedriusS · 2020-01-16T20:56:45Z

@krasi-georgiev are there any numbers on the performance impact of the current implementation? 👀 does Windows exhibit the same behavior with caching?

krasi-georgiev · 2020-01-17T09:10:07Z

@krasi-georgiev are there any numbers on the performance impact of the current implementation?

probably, but only for single syscall to get the ModTime() so I would say negligible. If anyone wants to disable this can set a high enough --grpc-server-max-connection-age so that the connection is persistent and it never checks for new certs.

does Windows exhibit the same behavior with caching?

I would say yes, but is windows even suported by Thanos?
Last time I tried to run the tests under windoes these failed in many places.

xunleii · 2020-03-05T20:53:41Z

Thanks for this PR, it is awesome to automatically reload certificates when they are modified.

It works perfectly on Kubernetes with certificates automatically renewed by Cert-Manager 👍
I have tested this PR with a Thanos querier and a Thanos sidecar, using DNS SRV discovery, GRPC TLS and client auth by certiifcates, with all certificates renewed each 5 minutes.

EDIT: Sorry guys, I edit this comment because of huge pebcak on my side ... I have tested this PR and it seems working perfectly !

stale · 2020-04-04T22:35:21Z

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

bwplotka · 2020-04-05T09:18:44Z

Still much needed. Looking for more time to review it properly. Help wanted.

…

On Sat, 4 Apr 2020 at 23:35, stale[bot] ***@***.***> wrote: This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1672 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O4MJTA5B7WPDWLTIYLRK6Y3NANCNFSM4JDEVZAA> .

stale · 2020-06-04T09:50:00Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

bwplotka · 2020-06-04T10:27:30Z

It is indeed (:

…

On Thu, 4 Jun 2020 at 10:50, stale[bot] ***@***.***> wrote: Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1672 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3OYE7SATENMESBDUAWTRU5U5NANCNFSM4JDEVZAA> .

stale · 2020-08-03T10:38:08Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

bwplotka · 2020-08-03T10:53:43Z

still valid (:

…

On Mon, 3 Aug 2020 at 11:38, stale[bot] ***@***.***> wrote: Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1672 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O7WWUZXGRE2P4OWMI3R62HR3ANCNFSM4JDEVZAA> .

stale · 2020-10-02T13:54:35Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

bwplotka · 2020-10-02T14:29:47Z

For review, help wanted. Kind Regards, Bartek Płotka (@bwplotka)

…

On Fri, 2 Oct 2020 at 14:54, stale[bot] ***@***.***> wrote: Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1672 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVA3O5UKITRY25SMPKGGYDSIXLSXANCNFSM4JDEVZAA> .

krasi-georgiev · 2020-10-13T09:53:29Z

stale, someone else needs to take over.

Tachone · 2022-06-10T09:17:28Z

now after writing the tests I realized that there is another problem. getConfigForClient and getCertificate are called only with the tls handshake and NOT when sending or receiving data which means that for long running data flow it will never reload the certs as it will reuse the same connection and no new tls handshake will happen. In this PR I have also added a cli flag so users can set the keep alive setting with MaxConnectionAge which enforces a new TLS hanshake when MaxConnectionAge expires. New tls handshake is initiated when the connection is interrupted or when the MaxConnectionAge expires.

So with this new info I think there are 3 options:

keep the current implementation with a cli flag for MaxConnectionAge

run some loop which will restart the server gracefully or drop all connections when it detects tls cert files changes.

provide a reload endpoint and leave reloading to external config management.

Again I think this is important mainly for the thanos receiver as I imagine that with all other components a new connection and a new tls handshake would be created with every request so it will reload the certs as expected.

cc @bwplotka, @s-urbaniak

Why do we need to set MaxConnectionAge, Is there any problem with the long connection not being closed? the old certificate is no longer used on the established connection, am i wrong?

povilasv approved these changes Oct 22, 2019

View reviewed changes

krasi-georgiev force-pushed the cert-rotation branch from 50afeaf to 41aeb30 Compare October 22, 2019 16:14

GiedriusS reviewed Oct 28, 2019

View reviewed changes

krasi-georgiev commented Oct 29, 2019

View reviewed changes

kakkoyun mentioned this pull request Oct 30, 2019

.*: Introduce graceful shutdown for gRPC Servers #1687

Merged

2 tasks

krasi-georgiev force-pushed the cert-rotation branch from 37a00c4 to 445a68d Compare December 9, 2019 11:46

rebased

7da85ca

Signed-off-by: Krasi Georgiev <[email protected]>

krasi-georgiev force-pushed the cert-rotation branch from 445a68d to 7da85ca Compare December 9, 2019 11:50

regeareted the docs

998bffd

Signed-off-by: Krasi Georgiev <[email protected]>

comments format

c14ff48

Signed-off-by: Krasi Georgiev <[email protected]>

squat requested changes Dec 9, 2019

View reviewed changes

nits

f0f6ac5

Signed-off-by: Krasi Georgiev <[email protected]>

krasi-georgiev changed the title ~~allow server cert rotation without a restart~~ [WIP] allow server cert rotation without a restart Feb 12, 2020

stale bot added the stale label Apr 4, 2020

stale bot removed the stale label Apr 5, 2020

krasi-georgiev mentioned this pull request Apr 7, 2020

Verify TLS observatorium/api#24

Merged

stale bot added the stale label Jun 4, 2020

stale bot removed the stale label Jun 4, 2020

stale bot added the stale label Aug 3, 2020

stale bot removed the stale label Aug 3, 2020

stale bot added the stale label Oct 2, 2020

stale bot removed the stale label Oct 2, 2020

krasi-georgiev closed this Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] allow server cert rotation without a restart #1672

[WIP] allow server cert rotation without a restart #1672

krasi-georgiev commented Oct 21, 2019 •

edited

Loading

povilasv left a comment

brancz commented Oct 22, 2019

krasi-georgiev commented Oct 22, 2019 •

edited

Loading

brancz commented Oct 23, 2019

GiedriusS Oct 28, 2019

krasi-georgiev Oct 29, 2019

bwplotka commented Oct 29, 2019 •

edited

Loading

krasi-georgiev left a comment •

edited

Loading

brancz commented Oct 29, 2019

krasi-georgiev commented Nov 1, 2019 •

edited

Loading

krasi-georgiev commented Nov 7, 2019 •

edited

Loading

s-urbaniak commented Nov 26, 2019

bwplotka commented Nov 26, 2019

s-urbaniak commented Nov 26, 2019

krasi-georgiev commented Dec 9, 2019

squat left a comment

krasi-georgiev commented Dec 9, 2019

squat commented Dec 9, 2019

krasi-georgiev commented Jan 2, 2020

krasi-georgiev commented Jan 9, 2020

GiedriusS commented Jan 16, 2020

krasi-georgiev commented Jan 17, 2020

xunleii commented Mar 5, 2020 •

edited

Loading

stale bot commented Apr 4, 2020

bwplotka commented Apr 5, 2020 via email

stale bot commented Jun 4, 2020

bwplotka commented Jun 4, 2020 via email

stale bot commented Aug 3, 2020

bwplotka commented Aug 3, 2020 via email

stale bot commented Oct 2, 2020

bwplotka commented Oct 2, 2020 via email

krasi-georgiev commented Oct 13, 2020

Tachone commented Jun 10, 2022

[WIP] allow server cert rotation without a restart #1672

[WIP] allow server cert rotation without a restart #1672

Conversation

krasi-georgiev commented Oct 21, 2019 • edited Loading

povilasv left a comment

Choose a reason for hiding this comment

brancz commented Oct 22, 2019

krasi-georgiev commented Oct 22, 2019 • edited Loading

brancz commented Oct 23, 2019

GiedriusS Oct 28, 2019

Choose a reason for hiding this comment

krasi-georgiev Oct 29, 2019

Choose a reason for hiding this comment

bwplotka commented Oct 29, 2019 • edited Loading

krasi-georgiev left a comment • edited Loading

Choose a reason for hiding this comment

brancz commented Oct 29, 2019

krasi-georgiev commented Nov 1, 2019 • edited Loading

krasi-georgiev commented Nov 7, 2019 • edited Loading

s-urbaniak commented Nov 26, 2019

bwplotka commented Nov 26, 2019

s-urbaniak commented Nov 26, 2019

krasi-georgiev commented Dec 9, 2019

squat left a comment

Choose a reason for hiding this comment

krasi-georgiev commented Dec 9, 2019

squat commented Dec 9, 2019

krasi-georgiev commented Jan 2, 2020

krasi-georgiev commented Jan 9, 2020

GiedriusS commented Jan 16, 2020

krasi-georgiev commented Jan 17, 2020

xunleii commented Mar 5, 2020 • edited Loading

stale bot commented Apr 4, 2020

bwplotka commented Apr 5, 2020 via email

stale bot commented Jun 4, 2020

bwplotka commented Jun 4, 2020 via email

stale bot commented Aug 3, 2020

bwplotka commented Aug 3, 2020 via email

stale bot commented Oct 2, 2020

bwplotka commented Oct 2, 2020 via email

krasi-georgiev commented Oct 13, 2020

Tachone commented Jun 10, 2022

krasi-georgiev commented Oct 21, 2019 •

edited

Loading

krasi-georgiev commented Oct 22, 2019 •

edited

Loading

bwplotka commented Oct 29, 2019 •

edited

Loading

krasi-georgiev left a comment •

edited

Loading

krasi-georgiev commented Nov 1, 2019 •

edited

Loading

krasi-georgiev commented Nov 7, 2019 •

edited

Loading

xunleii commented Mar 5, 2020 •

edited

Loading