sds: additional support for symlink-based key rotation. #13721

htuch · 2020-10-23T00:07:50Z

There are a few limitations in our existing support for symlink-based
key rotation:

We don't atomically resolve symlinks, so a single snapshot might have
inconsistent symlink resolutions for different watched files.
Watches are on parent directories, e.g. for /foo/bar/baz on /foo/bar,
which doesn't support common key rotation schemes were /foo/new/baz
is rotated via a mv -Tf /foo/new /foo/bar.

The solution is to provide a structured WatchedDirectory for Secrets to
opt into when monitoring DataSources. SDS will used WatchedDirectory
to setup the inotify watch instead of the DataSource path. On update, it will
read key/cert twice, verifying file content hash consistency.

Risk level: Low (opt-in feature)
Testing: Unit and integration tests added.

Fixes #13663
Fixes #10979
Fixes #13370

Signed-off-by: Harvey Tuch [email protected]

repokitteh-read-only · 2020-10-23T00:07:56Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13721 was opened by htuch.

see: more, trace.

htuch · 2020-10-23T00:10:25Z

@mattklein123 @lizan @tsaarni can you take a look? This will address #13663, #10979 and #13370, I'd like to obtain consensus on the API before adding implementation and tests.

api/envoy/config/core/v3/base.proto

tsaarni

Regardless of working with only a subset of all DataSource use cases, I like this API.

This will work nicely with Kubernetes symlink swap as well, by leaving subdirectory optional 👍

api/envoy/config/core/v3/base.proto

api/envoy/extensions/transport_sockets/tls/v3/common.proto

mattklein123

At a high level LGTM with a few comments.

api/envoy/config/core/v3/base.proto

api/envoy/extensions/transport_sockets/tls/v3/common.proto

htuch · 2020-10-26T12:22:30Z

I've updated the API to avoid modifying DataSource, introducing a new concept of an opt-in WatchedDirectory. I've also added a draft implementation and a very basis integration test. I'd like to achieve agreement on API and design, at which point I can clean up the implementation and fill out the rest of the tests. @lizan @mattklein123 @tsaarni PTAL.

include/envoy/filesystem/filesystem.h

source/common/config/watched_directory.cc

api/envoy/config/core/v3/base.proto

htuch · 2020-10-27T02:24:16Z

This now uses hash-based comparison of files after reading twice and validated consistency. This is similar to what gRPC is doing.

tsaarni

Looks good to me after the unbound loop in SdsApi::onWatchUpdate() is resolved.

source/common/secret/sds_api.h

source/common/filesystem/posix/filesystem_impl.cc

api/envoy/config/core/v3/base.proto

mattklein123

Thanks agreed this seems like a better direction. Flushing out a few API questions/comments.

/wait

api/envoy/config/core/v3/base.proto

api/envoy/extensions/transport_sockets/tls/v3/common.proto

htuch · 2020-10-27T23:38:36Z

PTAL, if this makes sense now in terms of API and hash-based comparison implementation then I can fill in the missing tests/docs and move this out of draft.

mattklein123

From an API perspective this LGTM with one question. LMK when ready for review and I can take a look.

/wait

api/envoy/extensions/transport_sockets/tls/v3/common.proto

Signed-off-by: Harvey Tuch <[email protected]>

…. Fix format. Signed-off-by: Harvey Tuch <[email protected]>

Signed-off-by: Harvey Tuch <[email protected]>

mattklein123

LGTM with some API comments, thank you!

/wait

api/envoy/extensions/transport_sockets/tls/v3/common.proto

Signed-off-by: Harvey Tuch <[email protected]>

htuch · 2020-11-13T14:23:49Z

@mattklein123 to deal with failure scenarios I had to add some stats, which then forced me to fix SDS stats more generally, since they were broken before (they all ended up anonymously in the root namespace). PTAL and LMK if there's anything else needed around stats here.

mattklein123

Thanks LGTM with small questions.

/wait-any

mattklein123 · 2020-11-13T19:27:20Z

api/envoy/extensions/transport_sockets/tls/v3/common.proto

+  // If specified, updates of a file-based *trusted_ca* source will be triggered
+  // by this watch. This allows explicit control over the path watched, by
+  // default the parent directory of the filesystem path in *trusted_ca* is
+  // watched if this field is not specified. This only applies when a
+  // *CertificateValidationContext* is delivered by SDS with references to
+  // filesystem paths.
+  config.core.v3.WatchedDirectory watched_directory = 11;


I still don't understand how this does anything if there is only a single file above? What's the utility?

Let's say you have a trusted CA files /foo/bar/baz/ca.pem. By default, envoy watches /foo/bar/baz. If you want to reload when baz is atomically moved, you need to set the watch on /foo/bar. This is what this field allows.

OK I think having a concrete example might help for this stuff (or ref link back to the docs where you have the example) as it was pretty hard for me to understand the difference.

source/common/secret/sds_api.cc

mattklein123

LGTM. Feel free to do any doc updates as a follow up.

htuch · 2020-11-13T21:23:02Z

Sure, will merge as some folks are depending on this, but I'll take an AI to do the docs followups next week. Thanks.

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <[email protected]>

Some followup docs tweaks to #13721. Signed-off-by: Harvey Tuch <[email protected]>

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <[email protected]>

…3721) There are a few limitations in our existing support for symlink-based key rotation: We don't atomically resolve symlinks, so a single snapshot might have inconsistent symlink resolutions for different watched files. Watches are on parent directories, e.g. for /foo/bar/baz on /foo/bar, which doesn't support common key rotation schemes were /foo/new/baz is rotated via a mv -Tf /foo/new /foo/bar. The solution is to provide a structured WatchedDirectory for Secrets to opt into when monitoring DataSources. SDS will used WatchedDirectory to setup the inotify watch instead of the DataSource path. On update, it will read key/cert twice, verifying file content hash consistency. Risk level: Low (opt-in feature) Testing: Unit and integration tests added. Fixes envoyproxy#13663 Fixes envoyproxy#10979 Fixes envoyproxy#13370 Signed-off-by: Harvey Tuch <[email protected]> Signed-off-by: Qin Qin <[email protected]>

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <[email protected]> Signed-off-by: Qin Qin <[email protected]>

repokitteh-read-only bot added the api label Oct 23, 2020

htuch assigned lizan and mattklein123 Oct 23, 2020

htuch commented Oct 23, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

tsaarni reviewed Oct 23, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Show resolved Hide resolved

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

mattklein123 reviewed Oct 23, 2020

View reviewed changes

htuch force-pushed the sds-symlink branch from b843f06 to d2b2957 Compare October 26, 2020 12:20

htuch commented Oct 26, 2020

View reviewed changes

include/envoy/filesystem/filesystem.h Outdated Show resolved Hide resolved

htuch mentioned this pull request Oct 26, 2020

Symlink-atomic reads for private key/certificate pairs #13370

Closed

tsaarni reviewed Oct 26, 2020

View reviewed changes

source/common/config/watched_directory.cc Outdated Show resolved Hide resolved

markdroth reviewed Oct 26, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

tsaarni reviewed Oct 27, 2020

View reviewed changes

source/common/secret/sds_api.h Outdated Show resolved Hide resolved

source/common/filesystem/posix/filesystem_impl.cc Outdated Show resolved Hide resolved

markdroth reviewed Oct 27, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

mattklein123 requested changes Oct 27, 2020

View reviewed changes

api/envoy/config/core/v3/base.proto Outdated Show resolved Hide resolved

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added waiting and removed waiting labels Oct 27, 2020

mattklein123 requested changes Oct 28, 2020

View reviewed changes

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Oct 28, 2020

htuch added 6 commits November 9, 2020 18:12

baseline from GH

08d0c51

Signed-off-by: Harvey Tuch <[email protected]>

Fine grained WatchedPath control, hash-based file atomic reads.

d3c88a5

Signed-off-by: Harvey Tuch <[email protected]>

Revert filesystem changes.

ce847fd

Signed-off-by: Harvey Tuch <[email protected]>

Always load secrets inline, even with non-watched path. Bound retries…

cbe018b

…. Fix format. Signed-off-by: Harvey Tuch <[email protected]>

Back to WatchedDirectory.

6ae5d80

Signed-off-by: Harvey Tuch <[email protected]>

Merge remote-tracking branch 'upstream/master' into sds-symlink

8a2056e

htuch changed the title ~~[WiP] sds/base: additional support for symlink-based key rotation.~~ sds: additional support for symlink-based key rotation. Nov 10, 2020

htuch added 4 commits November 11, 2020 20:36

Release notes and docs.

2282367

Signed-off-by: Harvey Tuch <[email protected]>

Typo.

f7b7d94

Signed-off-by: Harvey Tuch <[email protected]>

Fix clang-tidy.

da64842

Signed-off-by: Harvey Tuch <[email protected]>

fails_on_windows tag added (after consulting @wrowe)

2dae3be

Signed-off-by: Harvey Tuch <[email protected]>

mattklein123 requested changes Nov 12, 2020

View reviewed changes

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 12, 2020

htuch added 2 commits November 12, 2020 17:21

Strengthen docs.

6b5f720

Signed-off-by: Harvey Tuch <[email protected]>

Fix SDS stats and avoid crashing on empty directory rotations.

4ef3f13

Signed-off-by: Harvey Tuch <[email protected]>

repokitteh-read-only bot removed the waiting label Nov 13, 2020

mattklein123 reviewed Nov 13, 2020

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Nov 13, 2020

mattklein123 approved these changes Nov 13, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Nov 13, 2020

htuch merged commit 122257e into envoyproxy:master Nov 13, 2020

htuch added a commit to htuch/envoy that referenced this pull request Nov 15, 2020

sds: improve watched directory documentation.

b927378

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <[email protected]>

htuch mentioned this pull request Nov 15, 2020

sds: improve watched directory documentation. #14029

Merged

htuch added a commit that referenced this pull request Nov 16, 2020

sds: improve watched directory documentation. (#14029)

dac6e58

Some followup docs tweaks to #13721. Signed-off-by: Harvey Tuch <[email protected]>

andreyprezotto pushed a commit to andreyprezotto/envoy that referenced this pull request Nov 24, 2020

sds: improve watched directory documentation. (envoyproxy#14029)

54af113

Some followup docs tweaks to envoyproxy#13721. Signed-off-by: Harvey Tuch <[email protected]>

ggreenway mentioned this pull request May 13, 2021

Support reloading TlsCertificate files #10387

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sds: additional support for symlink-based key rotation. #13721

sds: additional support for symlink-based key rotation. #13721

htuch commented Oct 23, 2020 •

edited

Loading

repokitteh-read-only bot commented Oct 23, 2020

htuch commented Oct 23, 2020

tsaarni left a comment

mattklein123 left a comment

htuch commented Oct 26, 2020

htuch commented Oct 27, 2020

tsaarni left a comment •

edited

Loading

mattklein123 left a comment

htuch commented Oct 27, 2020

mattklein123 left a comment

mattklein123 left a comment

htuch commented Nov 13, 2020 •

edited

Loading

mattklein123 left a comment

mattklein123 Nov 13, 2020

htuch Nov 13, 2020

mattklein123 Nov 13, 2020

mattklein123 left a comment

htuch commented Nov 13, 2020

sds: additional support for symlink-based key rotation. #13721

sds: additional support for symlink-based key rotation. #13721

Conversation

htuch commented Oct 23, 2020 • edited Loading

repokitteh-read-only bot commented Oct 23, 2020

htuch commented Oct 23, 2020

tsaarni left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

htuch commented Oct 26, 2020

htuch commented Oct 27, 2020

tsaarni left a comment • edited Loading

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

htuch commented Oct 27, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

htuch commented Nov 13, 2020 • edited Loading

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 Nov 13, 2020

Choose a reason for hiding this comment

htuch Nov 13, 2020

Choose a reason for hiding this comment

mattklein123 Nov 13, 2020

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

htuch commented Nov 13, 2020

htuch commented Oct 23, 2020 •

edited

Loading

tsaarni left a comment •

edited

Loading

htuch commented Nov 13, 2020 •

edited

Loading