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

L46: C-core: New TLS Credentials API #422

Open
wants to merge 52 commits into
base: master
Choose a base branch
from

Conversation

gtcooke94
Copy link

@gtcooke94 gtcooke94 commented Mar 22, 2024

This PR is subsuming the previous PR of the same title (#205), and attempting to bring the content to the current state with the goal of formally merging this and moving these API from experimental to stable.

This is still under active work and is not ready for final review.

@gtcooke94 gtcooke94 marked this pull request as draft March 22, 2024 17:42
Copy link
Author

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

Did a rework of the cert provider section, still need to address the verification side

L46-core-tls-credential-API.md Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Show resolved Hide resolved
L46-core-tls-credential-API.md Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
// (in the one-side TLS scenario, the server is not required to provide root
// certs). We don't support default root certs on server side.
void watch_root_certs();
// Sets the name of root certificates being watched, if |watch_root_certs| is

Choose a reason for hiding this comment

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

Pinging this so we don't lose it.

L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
~TlsCustomVerificationCheckRequest() {}

absl::string_view target_name() const;
absl::string_view peer_cert() const;

Choose a reason for hiding this comment

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

I think that comment is another signal that this API is not useful for anyone since they cannot really predict what the return value should be. I think we should probably deprecate + delete this API and replace it with APIs that return predictable and explicit values.

L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Show resolved Hide resolved
// Sets the certificate verifier. The certificate verifier performs checks on
// the peer certificate chain after the chain has been (cryptographically)
// verified to chain up to a trusted root.
// If unset, this will default to the `HostNameCertificateVerifier` detailed

Choose a reason for hiding this comment

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

Should it only do this default on the client-side?

Copy link
Author

Choose a reason for hiding this comment

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

That's a good question - I think it depends on other defaults, and if we are doing MTLS vs. TLS? I'll double check, but I don't believe this is called unless it is configured to do MTLS.

Choose a reason for hiding this comment

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

I'm not sure that's correct. For example, we want it to default to the HostnameVerifier on the client-side even when we are doing (normal) TLS only.

L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
// identity certificates(single side TLS).
class TlsChannelCredentialsOptions final : public TlsCredentialsOptions {
public:
// Sets the decision of whether to do a crypto check on the server certs.

Choose a reason for hiding this comment

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

Despite the other comment, I think we need to clean up this comment so that it is more precise. :)

~TlsCustomVerificationCheckRequest() {}

absl::string_view target_name() const;
absl::string_view peer_cert() const;

Choose a reason for hiding this comment

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

How do we get a verified chain on resumed handshakes?

L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Outdated Show resolved Hide resolved
L46-core-tls-credential-API.md Show resolved Hide resolved
@gtcooke94 gtcooke94 marked this pull request as ready for review June 11, 2024 17:13
Copy link

@matthewstevenson88 matthewstevenson88 left a comment

Choose a reason for hiding this comment

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

LGTM, modulo the newly-added comments.

// Sets the certificate verifier. The certificate verifier performs checks on
// the peer certificate chain after the chain has been (cryptographically)
// verified to chain up to a trusted root.
// If unset, this will default to the `HostNameCertificateVerifier` detailed

Choose a reason for hiding this comment

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

I'm not sure that's correct. For example, we want it to default to the HostnameVerifier on the client-side even when we are doing (normal) TLS only.

~TlsCustomVerificationCheckRequest() {}

absl::string_view target_name() const;
absl::string_view peer_cert() const;

Choose a reason for hiding this comment

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

Ya that's what I was hinting at. Resumed handshakes skip the cryptographic verification, but I'm not sure if they skip the post-handshake verification. If that's correct, we need to explain that some of these fields may not be available on resumed handshakes.

```c++


class UnverifiedPeerChain {

Choose a reason for hiding this comment

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

micro-nit: s/PeerChain/PeerCertificateChain



class UnverifiedPeerChain {
std::vector<absl::string_view> peer_cert_chain_der;

Choose a reason for hiding this comment

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

nit: absl::Span instead of std::vector?

public:
virtual ~CustomChainBuilderInterface() = default;

// Performs custom chain building. gRPC invokes this function. When Chain

Choose a reason for hiding this comment

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

nit: Consider omitting "gRPC invokes this function.", as I don't think it adds anything.

public:
virtual ~CustomChainBuilderInterface() = default;

// Performs custom chain building. gRPC invokes this function. When Chain

Choose a reason for hiding this comment

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

micro-nit: s/Chain/chain

```

The default behavior for chain building is to defer to the underlying SSL library, which has a built-in chain building API that has been hardened over many years of use with the web PKI. For example, [X509_verify_cert](https://www.openssl.org/docs/man1.1.1/man3/X509_verify_cert.html) is the implementation in OpenSSL that builds a chain and verifies that it is trusted by one of the root certificates.
One use case for the custom chain building feature is to enable SPIFFE federation (TODO: ref the SPIFFE spec), where the set of root certificates to use is determined based on the SPIFFE trust domain in the peer's leaf certificate.

Choose a reason for hiding this comment

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

nit: Please address this TODO.

FileWatcherCertificateProvider(absl::string_view private_key_path,
absl::string_view identity_certificate_path,
absl::string_view root_cert_path,
unsigned int refresh_interval_sec);

Choose a reason for hiding this comment

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

micro-nit: Prefer absl::Duration here and below rather than using an unsigned int to represent time.

public:
// Sets the decision of whether to do a crypto check on the server certificates.
// The default is true.
void set_verify_server_certificates(bool verify_server_certs);

Choose a reason for hiding this comment

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

We need e.g. "insecure" in the name here.

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.

None yet

4 participants