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.

Sign up for GitHub

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

Jump to bottom

tls: allow cert validation by only leaf trusted CA's CRL #18289

Merged
merged 16 commits into from
Nov 2, 2021
Merged

tls: allow cert validation by only leaf trusted CA's CRL #18289

merged 16 commits into from
Nov 2, 2021

Conversation

Shikugawa
Copy link
Member

Signed-off-by: Shikugawa rei@tetrate.io

Commit Message: Allow cert validation by only leaf trusted CAs CRL
Additional Description: Close #18268. In the previous implementation, we don't have availability to validate certs when all trusted CAs don't have their own CRLs if any trusted CAs have that. This feature allows validating even if all trusted CAs don't have CRLs.
Risk Level: Low
Testing: Unit
Docs Changes: Required
Release Notes: Required
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

cc @incfly

@Shikugawa
tls: allow cert validation by only leaf trusted CA's CRL
5faafc3 
Signed-off-by: Shikugawa <rei@tetrate.io>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #18289 was opened by Shikugawa.

see: more, trace.

mattklein123
mattklein123 requested changes Sep 28, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks flushing out some comments. cc @ggreenway to provide a quick sanity check of the feature.

/wait

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
docs/root/version_history/current.rst Outdated Show resolved Hide resolved
test/extensions/transport_sockets/tls/ssl_socket_test.cc Outdated Show resolved Hide resolved
@incfly
Copy link
Contributor

incfly commented Sep 28, 2021

Thanks for the quick change! @Shikugawa

@Shikugawa
fix
3d4f9d7 
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
Merge branch 'main' of https://github.com/envoyproxy/envoy into leaf-crl
24baa17 
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
fix
0fd74a0 
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
fix
faf39f1 
Signed-off-by: Shikugawa <rei@tetrate.io>
mattklein123
mattklein123 requested changes Sep 29, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Flushing some more API comments. I will defer to @lizan and others for the testing review to make sure we have good coverage, thanks.

/wait

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
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
docs/root/version_history/current.rst Outdated Show resolved Hide resolved
@Shikugawa
fix
dc9081c 
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
Copy link
Member Author

@lizan Could you take a look?

incfly
incfly reviewed Sep 30, 2021
View reviewed changes
Copy link
Contributor

@incfly incfly left a comment

Choose a reason for hiding this comment

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

LGTM

mattklein123
mattklein123 previously approved these changes Sep 30, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

API LGTM thanks. I will defer to @lizan for the TLS/test review. Thank you!

@mattklein123
Copy link
Member

Needs a main merge. Ping @lizan PTAL.

/wait

lizan
lizan suggested changes Oct 6, 2021
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

one more about naming, and please resolve conflicts.

api/envoy/extensions/transport_sockets/tls/v3/common.proto Outdated Show resolved Hide resolved
@Shikugawa
fix
79f987e 
Signed-off-by: Shikugawa <rei@tetrate.io>
@mattklein123
Copy link
Member

Needs a main merge.

/wait

@repokitteh-read-only
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 @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18289 was synchronize by Shikugawa.

see: more, trace.

@Shikugawa
fix
4a20681 
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
fix
bc7e61a 
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18289 (comment) was created by @Shikugawa.

see: more, trace.

@Shikugawa
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18289 (comment) was created by @Shikugawa.

see: more, trace.

@Shikugawa
Kick CI
5059098 
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
Merge branch 'main' of https://github.com/envoyproxy/envoy into leaf-crl
0413ec9 
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18289 (comment) was created by @Shikugawa.

see: more, trace.

@Shikugawa
Copy link
Member Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18289 (comment) was created by @Shikugawa.

see: more, trace.

mattklein123
mattklein123 approved these changes Nov 2, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. @lizan @incfly can you quickly check the test coverage? Thank you.

@repokitteh-read-only repokitteh-read-only bot removed the api label Nov 2, 2021
incfly
incfly reviewed Nov 2, 2021
View reviewed changes
Copy link
Contributor

@incfly incfly left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks @Shikugawa for the change, and @mattklein123 for the headsup

@lizan lizan merged commit 56e8c45 into envoyproxy:main Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@incfly incfly incfly left review comments

@lizan lizan lizan approved these changes

@mattklein123 mattklein123 mattklein123 approved these changes

@asraa asraa Awaiting requested review from asraa

@ggreenway ggreenway Awaiting requested review from ggreenway ggreenway is a code owner

Assignees

@lizan lizan

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow CRL verification to not require all level CA's CRL.
4 participants
@Shikugawa @incfly @mattklein123 @lizan