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

[TLS - Revocation] Crl Provider #33786

Merged
merged 130 commits into from Oct 17, 2023
Merged

[TLS - Revocation] Crl Provider #33786

merged 130 commits into from Oct 17, 2023

Conversation

gtcooke94
Copy link
Contributor

@gtcooke94 gtcooke94 commented Jul 20, 2023

The basic APIs for the CRL Reloading features.
This adds external types to represent CRL Providers, CRLs, and CertificateInfo.
Internally we will use CrlImpl - this layer is needed to hide OpenSSL details from the user.

TODO - link to associated gRFC.

Things Done

  • Add external API for CrlProvider, Crl, CertInfo (CertInfo is used during CRL lookup rather than passing the entire certificate).
  • Add code paths in ssl_transport_security to utilize CRL providers
  • Add StaticCrlProvider
  • Refactor crl_ssl_transport_security_test.cc so it is more extensible and can be used with providers

Copy link
Member

@markdroth markdroth 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 like a good start!

Please let me know if you have any questions.

include/grpc/grpc_crl_provider.h Outdated Show resolved Hide resolved
include/grpc/grpc_crl_provider.h Show resolved Hide resolved
include/grpc/grpc_crl_provider.h Outdated Show resolved Hide resolved
include/grpc/grpc_crl_provider.h Outdated Show resolved Hide resolved
@gtcooke94 gtcooke94 marked this pull request as ready for review July 21, 2023 15:24
@gtcooke94 gtcooke94 changed the title Crl reloading [TLS - Revocation] Crl reloading Jul 21, 2023
@gtcooke94 gtcooke94 added area/security release notes: no Indicates if PR should not be in release notes labels Jul 21, 2023
include/grpc/grpc_crl_provider.h Show resolved Hide resolved
include/grpc/grpc_crl_provider.h Outdated Show resolved Hide resolved
include/grpc/grpc_crl_provider.h Outdated Show resolved Hide resolved
test/core/security/grpc_crl_provider_test.cc Outdated Show resolved Hide resolved
test/core/security/grpc_crl_provider_test.cc Outdated Show resolved Hide resolved
test/core/security/grpc_crl_provider_test.cc Outdated Show resolved Hide resolved
test/core/security/grpc_crl_provider_test.cc Outdated Show resolved Hide resolved
@@ -32,6 +32,8 @@ using grpc_core::experimental::
CreateStaticCrlProvider; // NOLINT(misc-unused-using-decls)
using grpc_core::experimental::Crl; // NOLINT(misc-unused-using-decls)
using grpc_core::experimental::CrlProvider; // NOLINT(misc-unused-using-decls)
using grpc_core::experimental::
grpc_tls_credentials_options_set_crl_provider; // NOLINT(misc-unused-using-decls)
Copy link
Member

Choose a reason for hiding this comment

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

We definitely do not want this using statement. The grpc_tls_credentials_options_set_crl_provider() method should not be in the grpc_core::experimental namespace to begin with; it should be at the top level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've taken it out now

Comment on lines 1064 to 1084
"//src/core/tsi/test_creds:ca.pem",
"//src/core/tsi/test_creds:client.key",
"//src/core/tsi/test_creds:client.pem",
"//src/core/tsi/test_creds:server1.key",
"//src/core/tsi/test_creds:server1.pem",
"//test/core/tsi/test_creds/crl_data:ca.pem",
"//test/core/tsi/test_creds/crl_data:intermediate_ca.key",
"//test/core/tsi/test_creds/crl_data:intermediate_ca.pem",
"//test/core/tsi/test_creds/crl_data:leaf_and_intermediate_chain.pem",
"//test/core/tsi/test_creds/crl_data:leaf_signed_by_intermediate.key",
"//test/core/tsi/test_creds/crl_data:leaf_signed_by_intermediate.pem",
"//test/core/tsi/test_creds/crl_data:revoked.key",
"//test/core/tsi/test_creds/crl_data:revoked.pem",
"//test/core/tsi/test_creds/crl_data:valid.key",
"//test/core/tsi/test_creds/crl_data:valid.pem",
"//test/core/tsi/test_creds/crl_data/crls:ab06acdd.r0",
"//test/core/tsi/test_creds/crl_data/crls:b9322cac.r0",
"//test/core/tsi/test_creds/crl_data/crls:current.crl",
"//test/core/tsi/test_creds/crl_data/crls:intermediate.crl",
"//test/core/tsi/test_creds/crl_data/crls_missing_intermediate:ab06acdd.r0",
"//test/core/tsi/test_creds/crl_data/crls_missing_root:b9322cac.r0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like file path is not supported for ios rules.

A quick fix would be only add files that are actually used in your test, looks like they passes with fe118ac in #34707 which I only removed the ones that conflicts. From what I see in you test, only 6 of those files are used.

--
@sampajano We probably can just remove job grpc_bazel_cpp_ios_event_engine_experiment_tests.cfg as per our previous discussion, 0 test was actually executed on iOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That'll work for this PR, but there's some coming work where directory structure for CRLs matters - I'm not 100% sure what this is doing with the flattening and bundling, but I suppose we'll just figure it out as it happens?

Copy link
Contributor

Choose a reason for hiding this comment

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

@HannahShiSFB I'm not against removing grpc_bazel_cpp_ios_event_engine_experiment_tests.cfg since the tests are not really effective :)

@gtcooke94 gtcooke94 merged commit 0f0396a into grpc:master Oct 17, 2023
68 checks passed
apolcyn added a commit to apolcyn/grpc that referenced this pull request Oct 17, 2023
ctiller pushed a commit that referenced this pull request Oct 17, 2023
gtcooke94 added a commit to gtcooke94/grpc that referenced this pull request Oct 17, 2023
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security bloat/medium imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core lang/ruby per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants