Conversation
Closes #96 Removes the regex validation of the SPIFFE IDs for access to the Connect API. Envoy will still validate the presented cert, but then which identities can do what is handled within Connect application code. On the xds service the validation remains to only allow the Cofide agent to connect to it, but the regex is updated to allow SPIFFE IDs both with and without cluster names in them. Once Connect is updated to always include the cluster name in this identity the old one can be removed.
alec-w
requested review from
jsnctl,
markgoddard,
mattbates,
sd109 and
yoctozepto
Summary of ChangesHello @alec-w, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the flexibility of Cofide identities by adjusting SPIFFE ID validation within Envoy configurations. It removes strict regex validation for the Connect API, shifting identity control to the application layer, and updates the xDS service to support Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the SPIFFE ID validation in the Envoy configuration to allow for an optional cluster name, ensuring backward compatibility while introducing a new identity format. However, a high-severity vulnerability has been identified: the removal of explicit validation from the Connect API endpoint at the Envoy proxy level significantly weakens access control for a sensitive API endpoint by removing a critical defense-in-depth security control. It is strongly recommended to reinstate this control. Additionally, consider consolidating the two new regular expressions into a single, more maintainable one.
I am having trouble creating individual review comments. Click here to see my feedback.
charts/cofide-connect/templates/configmap-envoy.yaml (228-245)
The default_validation_context, which enforces SPIFFE ID-based authorization, has been removed from the connect_agent_api filter chain. Previously, this control ensured that only specific, authorized client identities could connect to this endpoint. Its removal means that any client with a valid certificate from the trust domain can now establish a connection.
This change constitutes a security regression as it eliminates a critical defense-in-depth layer. While the PR description mentions that authorization is handled within the application, relying solely on application-level checks increases the risk. If any authorization bypass vulnerabilities exist in the application, they are now exposed to a much wider range of potential attackers. The principle of secure design recommends layered defenses, and this change removes one such layer.
charts/cofide-connect/templates/configmap-envoy.yaml (51-60)
For better maintainability and conciseness, you can combine the two regular expressions for matching SPIFFE IDs into a single one using an optional group for the cluster name. This avoids duplicating the san_type and matcher blocks.
# cofide-agent identity with or without cluster name
- san_type: URI
matcher:
safe_regex:
regex: spiffe://[^/]+(/cluster/[^/]+)?/ns/cofide/sa/cofide-agent
jsnctl
left a comment
jsnctl
left a comment
There was a problem hiding this comment.
Looks good and makes sense intuitively to me. Have you been able to test with the Cofide workloads to see if they still auth as expected?
Just need to supply the validation context being used now there aren't multiple.
I forgot there wasn't an automated test in CI here. Found one error - rather than using a combined validation context with an empty default, we just need to supply the one validation context being used in that case. With that my cofide-observer is working as expected (can reach Connect). Pushed second commit with that fix in. thanks @jsnctl 🙂 |
3 participants
Closes #96
Part of https://github.com/cofide/cofide/issues/175
Removes the regex validation of the SPIFFE IDs for access to the Connect API. Envoy will still validate the presented cert, but which identities can do what is handled within Connect application code.
On the xds service the validation remains to only allow the Cofide agent to connect to it, but the regex is updated to allow SPIFFE IDs both with and without cluster names in them. Once Connect is updated to always include the cluster name in this identity the old one can be removed.