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

Fixes MAISTRA-2324: Backport multi-root support for Envoy #92

Merged
merged 6 commits into from
May 10, 2021

Conversation

dmitri-d
Copy link
Contributor

@dmitri-d dmitri-d commented May 5, 2021

This is a port of tls: separate out cert validation logic from ContextImpl (#14757) (the original PR: envoyproxy/envoy#14757).

The next PR in the queue is envoyproxy/envoy#14884. It's going to be tricky, upstream relies on what is an OpenSSL internal data-structure in their spiffe validator implementation.

@dgn, @oschaaf.

@dmitri-d dmitri-d changed the title [WIP] Fixes MAISTRA-2324: Backport multi-root support for Envoy [WIP, Do not merge] Fixes MAISTRA-2324: Backport multi-root support for Envoy May 5, 2021
@dmitri-d dmitri-d force-pushed the multi-root-support branch from 057f294 to 9bca8bc Compare May 7, 2021 00:43
@dmitri-d
Copy link
Contributor Author

dmitri-d commented May 7, 2021

envoyproxy/envoy#14884 ported.

@dgn, @oschaaf

@oschaaf: fixed the comparison you pointed out the other day; The main changes in this batch are here: https://github.com/maistra/envoy/pull/92/files#diff-067aae04811b0f9ec5191388ebd58efe08012c3bc75e0951c028448ec87e2d70R164. I'm quite certain we are ok (see the comment), an alternative would be to open up internal OpenSSL structures (something I try to do sparingly) and just use upstream code without changes.

@dmitri-d dmitri-d force-pushed the multi-root-support branch from 9bca8bc to fa297b0 Compare May 7, 2021 16:10
Dmitri Dolguikh and others added 5 commits May 7, 2021 10:05
port of tls: separate out cert validation logic from ContextImpl (#14757)
cherry-pick of tls: implement SPIFFE Certificate Validator for independent multiple trust domain support (#14884)

Adds the extension point for certificate validations, and its first implementation for SPIFFE multi trust domain support in a single listener or cluster. Resolves envoyproxy/envoy#14614 and envoyproxy/envoy#9284.
Risk Level: low (only adding the new extension point and one implementation for it)
Testing: unit tests and integration tests.
Docs Changes:
Release Notes: tls: implement SPIFFE Certificate Validator for independent multiple trust domain support.

Signed-off-by: Takeshi Yoneda <[email protected]>
tls: fix flaky integration test of SPIFFE validator (#15301)

Previously these tests flake due to early counter check before the counter is actually incremented, and fail occasionally around 1~3%.

I tried this with --runs_per_test=10000 and never failed.

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Add separate SPIFFE integeration test build target. (#15324)

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
tls: enable allow_expired_certificate for SPIFFE validator (#15426)

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
@dmitri-d dmitri-d force-pushed the multi-root-support branch from fa297b0 to 9b72e59 Compare May 7, 2021 18:05
@dmitri-d
Copy link
Contributor Author

dmitri-d commented May 7, 2021

Last batch of cherry-picks, minimal to no changes this time. This is good to merge provided CI is happy and changes are approved.

@dgn, @oschaaf

@dmitri-d dmitri-d changed the title [WIP, Do not merge] Fixes MAISTRA-2324: Backport multi-root support for Envoy Fixes MAISTRA-2324: Backport multi-root support for Envoy May 7, 2021
Copy link
Contributor

@oschaaf oschaaf left a comment

Choose a reason for hiding this comment

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

One comment, other then that LGTM

bssl::UniquePtr<X509_STORE_CTX> verify_ctx(X509_STORE_CTX_new());
// We make a copy of X509_VERIFY_PARAMs in the store_ctx that we received as a parameter.
// This is a precaution mostly, as Envoy doesn't configure any X509_VERIFY_PARAMs.
// Note that there's no api to copy crls from one store_ctx to another; the assumption is that
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence is slightly hard to grok because of the double negation; if it is semantically correct probably not worth iterating on, but if this is accidental perhaps it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, indeed, this is hard to understand, and I wrote it. Fixed.

tls: enable match_subject_alt_names option in SPIFFE validator (#15509)

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
@dmitri-d dmitri-d force-pushed the multi-root-support branch from 9b72e59 to 41c3342 Compare May 10, 2021 19:34
@maistra-bot maistra-bot merged commit 2911486 into maistra:maistra-2.1 May 10, 2021
@dmitri-d dmitri-d deleted the multi-root-support branch May 10, 2021 23:05
@dmitri-d dmitri-d restored the multi-root-support branch May 19, 2021 19:29
dmitri-d pushed a commit to dmitri-d/maistra-envoy that referenced this pull request May 19, 2021
* Fixes MAISTRA-2324:

port of tls: separate out cert validation logic from ContextImpl (#14757)

* Fixes MAISTRA-2324:

cherry-pick of tls: implement SPIFFE Certificate Validator for independent multiple trust domain support (#14884)

Adds the extension point for certificate validations, and its first implementation for SPIFFE multi trust domain support in a single listener or cluster. Resolves envoyproxy/envoy#14614 and envoyproxy/envoy#9284.
Risk Level: low (only adding the new extension point and one implementation for it)
Testing: unit tests and integration tests.
Docs Changes:
Release Notes: tls: implement SPIFFE Certificate Validator for independent multiple trust domain support.

Signed-off-by: Takeshi Yoneda <[email protected]>

* Fixes MAISTRA-2324: cherry-pick of

tls: fix flaky integration test of SPIFFE validator (#15301)

Previously these tests flake due to early counter check before the counter is actually incremented, and fail occasionally around 1~3%.

I tried this with --runs_per_test=10000 and never failed.

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>

* Fixes MAISTRA-2324: cherry-pick of

Add separate SPIFFE integeration test build target. (#15324)

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>

* Fixes MAISTRA-2324: cherry-pick of

tls: enable allow_expired_certificate for SPIFFE validator (#15426)

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>

* Fixes MAISTRA-2324: cherry-pick of

tls: enable match_subject_alt_names option in SPIFFE validator (#15509)

Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>

Co-authored-by: Takeshi Yoneda <[email protected]>
jwendell added a commit to jwendell/envoy-1 that referenced this pull request Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants