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

Add possibility to disable SPIFFE cert validation per envoy instance #3014

Merged

Conversation

StupidScience
Copy link
Contributor

@StupidScience StupidScience commented Apr 29, 2022

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Description of change

Added functionality that disables SPIFFE cert validation per instance of envoy by checking presence and value of disable_spiffe_cert_validation key in envoy node metadata.

Which issue this PR fixes

fixes #3010

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Excellent, @StupidScience. Just a few small comments. Thank you for this contribution!

@@ -321,6 +321,8 @@ support for the [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/en
extension, which is only available starting with Envoy 1.18.
The default name is configurable (see `default_all_bundles_name` under [SDS Configuration](#sds-configuration).

SPIFFE Certificate Validator enabled by default and can be disabled by setting `disable_spiffe_cert_validation` to `true` in [SDS Configuration](#sds-configuration) for all instances or per instance in envoy's node metadata.
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
SPIFFE Certificate Validator enabled by default and can be disabled by setting `disable_spiffe_cert_validation` to `true` in [SDS Configuration](#sds-configuration) for all instances or per instance in envoy's node metadata.
The [SPIFFE Certificate Validator](https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto) configures Envoy to perform SPIFFE authentication. It is used by default but can be disabled by setting `disable_spiffe_cert_validation` to `true` in [SDS Configuration](#sds-configuration). Alternatively, to disable for an individual envoy instance, the `disable_spiffe_cert_validation` key can be configured and set to `false` in the Envoy node metadata. When not used, Envoy will perform standard X.509 certificate chain validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit not correct but I've changed so PTAL once again

pkg/agent/endpoints/sdsv3/handler.go Outdated Show resolved Hide resolved
Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Excellent, thank you @StupidScience.

@azdagron azdagron added this to the 1.3.0 milestone May 2, 2022
@azdagron azdagron changed the title Add posibility to disable SPIFFE cert validation per envoy instance Add possibility to disable SPIFFE cert validation per envoy instance May 2, 2022
@azdagron
Copy link
Member

azdagron commented May 2, 2022

Hi @StupidScience. We're ready to merge this but require PRs to be up to date with the latest main. Can you merge main into this PR, or better yet grant us permissions to push commits on the PR branch? The latter makes it easier for us to manage the merge queue.

@azdagron azdagron merged commit dc6325a into spiffe:main May 3, 2022
@StupidScience
Copy link
Contributor Author

@azdagron sorry, was on holiday so didn't react in time.

@StupidScience StupidScience deleted the disable-spiffe-validator-per-instance branch May 3, 2022 08:16
@azdagron
Copy link
Member

azdagron commented May 3, 2022

It's not a problem! We worked around it :)

azdagron pushed a commit that referenced this pull request May 3, 2022
…3014)

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
@azdagron azdagron mentioned this pull request May 3, 2022
azdagron pushed a commit to azdagron/spire that referenced this pull request May 3, 2022
…piffe#3014)

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
@azdagron azdagron mentioned this pull request May 3, 2022
azdagron pushed a commit that referenced this pull request May 4, 2022
…3014)

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
azdagron pushed a commit that referenced this pull request May 4, 2022
…3014)

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
azdagron pushed a commit to azdagron/spire that referenced this pull request May 13, 2022
…piffe#3014)

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
@azdagron azdagron mentioned this pull request May 13, 2022
azdagron pushed a commit to azdagron/spire that referenced this pull request May 13, 2022
…piffe#3014)

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
azdagron pushed a commit that referenced this pull request May 13, 2022
…3014)

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023
…piffe#3014)

Signed-off-by: Anton Kaymakchi <anton.kaymakchi@transferwise.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabling custom validation per request/envoy-instance
2 participants