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

Extend TLS support for external scaler #3565

Closed
ahmelsayed opened this issue Aug 18, 2022 · 11 comments
Closed

Extend TLS support for external scaler #3565

ahmelsayed opened this issue Aug 18, 2022 · 11 comments
Assignees
Labels
feature-request All issues for new features that have not been committed to help wanted Looking for support from community needs-discussion stale All issues that are marked as stale due to inactivity

Comments

@ahmelsayed
Copy link
Contributor

ahmelsayed commented Aug 18, 2022

Current State:

Today external scaler metadata has the following:

  metadata:
    scalerAddress: external-scaler-service:8080
    tlsCertFile : /path/to/tls/cert.pem

the Grpc connection is created using:

if metadata.tlsCertFile != "" {
	creds, err := credentials.NewClientTLSFromFile(metadata.tlsCertFile, "")
	...
	return grpc.Dial(metadata.scalerAddress, grpc.WithTransportCredentials(creds))
}

return grpc.Dial(metadata.scalerAddress, grpc.WithTransportCredentials(insecure.NewCredentials()))

This means that is tlsCertFile is provided, the grpc connection will be tls. tlsCertFile is expected to be the CA file the client expect to validate the server cert with credentials.NewClientTLSFromFile()

If no tlsCertFile is defined, KEDA creates the grpc connection using insecure.NewCredentials() which doesn't do auth or encryption (https).

Proposal:

  1. deprecate tlsCertFile (only because the naming is confusing considering what it does.
  2. introduce new metadata properties:
  metadata:
    scalerAddress: external-scaler-service:8080
    caCert : /path/to/tls/ca.pem # optional
    tlsClientCert: /path/to/tls/cert.pem # optional
    tlsClientKey: /path/to/tls/key.pem # optional
    forceTls: false # optional
    skipInsecureVerify: false # optional

If caCert, tlsClientCert, and/or tlsClientKey are defined, the connection must be TLS.
If they are not defined, by default KEDA will do HTTP connection, but with forceTls it'll use tls.

The code will look like:

// tlsCertFile behavies the same way, but is deprecated
if metadata.tlsCertFile != "" {
	creds, err := credentials.NewClientTLSFromFile(metadata.tlsCertFile, "")
	// handle err
	return grpc.Dial(metadata.scalerAddress, grpc.WithTransportCredentials(creds))
}

// Build a full tls.Config{} based on supplied caCert, tlsClientCert, tlsClientKey, and insecureSkipVerify
tlsConfig := &tls.Config{}
if metadata.caCert != nil {
	tlsConfig.RootCAs = metadata.caCert
}
if metadata.tlsClientCert != nil && metadata.tlsClientKey != nil {
	tlsCert, err := tls.X509KeyPair(metadata.tlsClientCert, metadata.tlsClientKey)
	// handle err
	tlsConfig.Certificates = []tls.Certificate{ tlsCert }
}
if metadata.insecureSkipVerify {
	tlsConfig.InsecureSkipVerify = metadata.insecureSkipVerify
}

// if there is a CA cert set, or a client cert, or forceTls, use the generated tls.Config{}
if metadata.forceTls || len(tlsConfig.Certificates) > 0 || tlsConfig.RootCAs != nil {
	return grpc.Dial(metadata.scalerAddress, grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig)))
}

// fall back to currect behavior of no auth, no encryption. 
return grpc.Dial(metadata.scalerAddress, grpc.WithTransportCredentials(insecure.NewCredentials()))
  1. support TriggerAuthentication to resolve these values from a secret, rather than needing to mount them on KEDA pod
apiVersion: keda.sh/v1alpha1
kind: TriggerAuthentication
metadata:
  name: keda-external-trigger-auth
spec:
  secretTargetRef:
  - parameter: caCert
    name: external-scaler-secrets
    key: ca.pem
  - parameter: tlsClientCert
    name: external-scaler-secrets
    key: cert.pem
  - parameter: tlsClientKey 
    name: external-scaler-secrets
    key: key.pem

Use-Case

Today it's not possible to use mtls or just https without a ca file. This will enable these scenarios for external scalers.

Anything else?

No response

@ahmelsayed ahmelsayed added needs-discussion feature-request All issues for new features that have not been committed to labels Aug 18, 2022
@philomory
Copy link
Contributor

philomory commented Sep 7, 2022

This would be so nice. Needing to mount the CA certificate into the KEDA pod is a bit of a pain (it's also not terribly well documented). Frankly, I'd like to see support for this in all scalers that are capable of using TLS.

@zroubalik
Copy link
Member

Make sense, I would vote for using exclusively TriggerAuthentication for this and don't use trigger metadata to hold secrets (3.).

@stale
Copy link

stale bot commented Nov 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Nov 8, 2022
@philomory
Copy link
Contributor

I'd hope this could be kept open, personally.

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Nov 11, 2022
@zroubalik
Copy link
Member

@philomory willing to contribute this?

@doctorpangloss
Copy link
Contributor

If you are trying to connect to a secure / TLS gRPC endpoint terminated by something else like LetsEncrypt, AWS ALB, nginx with ACME, etc., use the following trigger metadata:

apiVersion: keda.sh/v1alpha1
kind: ScaledObject
...
spec:
...
  triggers:
    - type: external-push
...
      metadata:
...
        tlsCertFile: /etc/ssl/certs/ca-certificates.crt

This corresponds to ca-certificates on the distroless static image used by Keda Operator.

@stale
Copy link

stale bot commented Feb 11, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Feb 11, 2023
@stale
Copy link

stale bot commented Feb 18, 2023

This issue has been automatically closed due to inactivity.

@stale stale bot closed this as completed Feb 18, 2023
@zroubalik zroubalik reopened this Feb 20, 2023
@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Feb 20, 2023
@zroubalik zroubalik added the help wanted Looking for support from community label Feb 20, 2023
@dttung2905
Copy link
Contributor

If you dont mind, I could take a look at thsi issue @zroubalik

@stale
Copy link

stale bot commented May 20, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label May 20, 2023
@zroubalik
Copy link
Member

Fixed in #4407

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request All issues for new features that have not been committed to help wanted Looking for support from community needs-discussion stale All issues that are marked as stale due to inactivity
Projects
Archived in project
Development

No branches or pull requests

5 participants