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 client certificate and grpc backend builder options #133

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Aug 20, 2024

No description provided.

@dgryski dgryski force-pushed the dgryski/client-cert branch 3 times, most recently from d9962c7 to 603fa2b Compare August 20, 2024 20:48
@dgryski dgryski force-pushed the dgryski/client-cert branch 6 times, most recently from 4930f94 to 77b981c Compare August 21, 2024 17:29
@dgryski dgryski changed the title WIP: add client cert to backend add client certificate and grpc backend builder options Aug 21, 2024
@dgryski dgryski marked this pull request as ready for review August 21, 2024 17:30
@@ -279,6 +280,19 @@ func (b *BackendOptions) SNIHostname(host string) *BackendOptions {
return b
}

// ClientCertificate sets the client certificate to be provided to the server as part of the SSL handshake.
func (b *BackendOptions) ClientCertificate(certificate string, key secretstore.Secret) *BackendOptions {
b.abiOpts.UseSSL(true)
Copy link
Member

Choose a reason for hiding this comment

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

Should other SSL-related things (CertHostname, CACert, Ciphers, SNIHostname, SSLMaxVersion, SSLMinVersion) also imply UseSSL(true)? Feels like either all of them should or none of them should, and we should make it clear in the docs if so.

Copy link
Member Author

Choose a reason for hiding this comment

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

This behaviour was copied from the Rust SDK.

    /// Provide the given client certificate to the server as part of the SSL handshake.
    //
    /// Setting this will enable SSL for the connection as a side effect. Both
    /// the certificate and the key to use should be in standard PEM format;
    /// providing the information in another format will lead to an error. We
    /// suggest that (at least the) key should be held in something like the
    /// Fastly secret store for security, with the handle passed to this function
    /// without unpacking it via [`Secret::plaintext`]; the certificate can
    /// be held in a less secure medium.
    ///
    /// (If it is absolutely necessary to get the key from another source, we
    /// suggest the use of [`Secret::from_bytes`]).
    pub fn provide_client_certificate(
        mut self,
        pem_certificate: impl ToString,
        pem_key: Secret,
    ) -> Self {
        self.use_ssl = true;
        self.client_certificate_info = Some((pem_certificate.to_string(), pem_key));
        self
    }

The other SSL-related calls also set use_ssl = true in the Rust SDK. I'll update our implementation as well.

Copy link
Collaborator

@cee-dub cee-dub left a comment

Choose a reason for hiding this comment

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

This LGTM.

Copy link
Member

@joeshaw joeshaw left a comment

Choose a reason for hiding this comment

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

Doc suggestions below. Otherwise LGTM.

fsthttp/backend.go Show resolved Hide resolved
fsthttp/backend.go Show resolved Hide resolved
fsthttp/backend.go Show resolved Hide resolved
fsthttp/backend.go Show resolved Hide resolved
fsthttp/backend.go Show resolved Hide resolved
fsthttp/backend.go Show resolved Hide resolved
fsthttp/backend.go Show resolved Hide resolved
@dgryski dgryski merged commit a95214e into main Aug 22, 2024
11 checks passed
@dgryski dgryski deleted the dgryski/client-cert branch August 22, 2024 15:53
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.

3 participants