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

spiffe: add support for spiffe bundle format #36190

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

briansonnenberg
Copy link
Contributor

@briansonnenberg briansonnenberg commented Sep 18, 2024

Commit Message: Adds alternative to "trust_domains" config for the spiffe validator—"trust_bundle_map".

Additional Description:

#35567
trust_bundle_map points to a local file containing a SPIFFE bundle map. A file watcher is set up to trigger refreshes to the SPIFFE data when this file is modified. SPIFFE refresh hint and sequence number are currently ignored.

Risk Level: medium
Testing: WIP
Docs Changes: TBD
Release Notes: TBD

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #36190 was opened by briansonnenberg.

see: more, trace.

@jmarantz
Copy link
Contributor

/wait

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. some new comments to the API to start the review. And please address the comment from @markdroth .

@markdroth
Copy link
Contributor

/lgtm api

@kyessenov
Copy link
Contributor

Please merge main.
/wait

@alyssawilk
Copy link
Contributor

/wait on CI

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and patience. And some comments are added.

@wbpcode
Copy link
Member

wbpcode commented Oct 17, 2024

Please also check the CI :)

Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
@RyanTheOptimist
Copy link
Contributor

@RyanTheOptimist
Copy link
Contributor

/wait

Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
@briansonnenberg
Copy link
Contributor Author

@wbpcode @alyssawilk @markdroth

Finally have the CI passing. 😅

Would you folks mind taking another look?

@markdroth
Copy link
Contributor

I'm still not thrilled that we're doing this instead of implementing the certificate provider framework, but at least this doesn't preclude us from doing that later.

/lgtm api

@adisuissa
Copy link
Contributor

@wbpcode seems that latest comments were addressed, PTAL.

@wbpcode
Copy link
Member

wbpcode commented Dec 18, 2024

Will take a look before tomorrow night.

Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks for the update and the long time investment. It's much better now. I add some comments new but should be easy to address. Thanks again.

Signed-off-by: Brian Sonnenberg <bsonnenberg@google.com>
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

LGTM overall now. Thanks so much for your update, It's near there, only some non-major comments are added.

And merry Christmas!!! 🎄

/wait

Comment on lines 126 to 130
// json_object::iterate seems to always return ok(), so this check is
// redundant...
if (!status.ok() || !parsing_status.ok()) {
return parsing_status;
}
Copy link
Member

Choose a reason for hiding this comment

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

I agree the check may be redundant. But there is a two-symbolism and may have potential problem. If the parsing_status is ok and status is not ok because the implementation change, what will happen?

Suggested change
// json_object::iterate seems to always return ok(), so this check is
// redundant...
if (!status.ok() || !parsing_status.ok()) {
return parsing_status;
}
// json_object::iterate seems to always return ok(), so this check is
// redundant...
RETURN_IF_NOT_OK_REF(status);
RETURN_IF_NOT_OK_REF(parsing_status);

Comment on lines +155 to +156
ENVOY_LOG(error, "Failed to load SPIFFE bundle map from '{}': '{}'",
trust_bundle_file_name_, new_trust_bundle.status());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ENVOY_LOG(error, "Failed to load SPIFFE bundle map from '{}': '{}'",
trust_bundle_file_name_, new_trust_bundle.status());
ENVOY_LOG(error, "Failed to load SPIFFE bundle map from '{}': '{}'",
trust_bundle_file_name_, new_trust_bundle.status().message());

Comment on lines +195 to +197
if (!parse_result.ok()) {
throw EnvoyException(
fmt::format("Failed to load SPIFFE Bundle map: {}", parse_result.status()));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!parse_result.ok()) {
throw EnvoyException(
fmt::format("Failed to load SPIFFE Bundle map: {}", parse_result.status()));
THROW_IF_NOT_OK_REF(parse_result);

@@ -19,6 +24,7 @@ envoy_extension_cc_test(
],
data = [
"//test/common/tls/test_data:certs",
"//test/extensions/transport_sockets/tls/cert_validator/spiffe:trust_bundles",
Copy link
Member

Choose a reason for hiding this comment

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

Seems you only move the generated file to this directory. Sorry for the chaos, maybe keep your previous design is better if we cannot split all these out from the tls/test_data cleanly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants