fix: expose ALPN in TLS handshake

[howardjohn]

New versions of gRPC-go are enforcing the h2 ALPN to be presented during the TLS handshake. See
https://pkg.go.dev/google.golang.org/grpc/internal/envconfig#pkg-variables GRPC_ENFORCE_ALPN_ENABLED. The TLS server here isn't automatically getting this set due to usage of GetConfigForClient.

This properly sets it.

Without this, istio-csr will be incompatible with Istio 1.24, which upgrades the gRPC version. Note this can be worked around by setting GRPC_ENFORCE_ALPN_ENABLED=false on the proxy container, which Istio is able to do -- so there is an escape hatch for users.

The Istio logs look like
"transport: authentication handshake failed: credentials: cannot check peer: missing selected ALPN property"

[cert-manager-prow]

Hi @howardjohn. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[howardjohn]

cc @inteon

[ThatsMrTalbot]

/ok-to-test

[ThatsMrTalbot]

Hiya! Thanks for contributing this fix.

Looks like the golangci-lint check is failing.

Specifically its complaining about the indentation now you have added the comment block, running golangci-lint run --fix gave me this diff:

                Certificates: []tls.Certificate{tlsCert},
                // Advertise ALPN, required in modern gRPC versions
                // Typically gRPC sets this for us, but since this tls.Config ultimately gets returned in GetConfigForClient it doesn't.
-               NextProtos:   []string{"h2"},
-               ClientAuth:   tls.VerifyClientCertIfGiven,
-               ClientCAs:    peerCertVerifier.GetGeneralCertPool(),
+               NextProtos: []string{"h2"},
+               ClientAuth: tls.VerifyClientCertIfGiven,
+               ClientCAs:  peerCertVerifier.GetGeneralCertPool(),
                VerifyPeerCertificate: func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
                        err := peerCertVerifier.VerifyPeerCert(rawCerts, verifiedChains)
                        if err != nil {

[inteon]

Thank you for the bug report and fix.

Could you add a tests/ modify our tests to make sure GRPC_ENFORCE_ALPN_ENABLED now works & will keep working in the future?

Also, do you know where in code this is normally auto-set when we are not using GetConfigForClient?

[howardjohn]

Also, do you know where in code this is normally auto-set when we are not using GetConfigForClient?

credentials.NewTLS() from gRPC auto adds it

Could you add a tests/ modify our tests to make sure GRPC_ENFORCE_ALPN_ENABLED now works & will keep working in the future?

Should we just add a test for Istio 1.24 directly? Can use a pre-release in the meantime

[inteon]

Adding an Istio 1.24 test sounds like a great solution.

[inteon]

@howardjohn In order to test istio 1.24, we will have to add a file here: https://github.com/cert-manager/istio-csr/tree/main/make/config/istio.

We can locally test the test using the following command:
ISTIO_VERSION="1.24.0-alpha.0" make test-e2e

[howardjohn]

@howardjohn In order to test istio 1.24, we will have to add a file here: https://github.com/cert-manager/istio-csr/tree/main/make/config/istio.

We can locally test the test using the following command: ISTIO_VERSION="1.24.0-alpha.0" make test-e2e

Thanks, added it and also https://github.com/cert-manager/testing/pull/1066/files

[ThatsMrTalbot]

Can confirm, on this branch:

Ran 14 of 14 Specs in 109.715 seconds
SUCCESS! -- 14 Passed | 0 Failed | 0 Pending | 0 Skipped

But if I copy the istio config and run the tests on the main branch then they just eventually time out. Whats interesting is I cant see errors in the logs, but I can see a loop of it constantly creating certificate-requests.

[howardjohn]

The errors were in the istio-proxy logs in my experience

[inteon]

This worked for me:

ISTIO_VERSION="1.24.0-alpha.0" make test-e2e
...
Ran 14 of 14 Specs in 75.084 seconds
SUCCESS! -- 14 Passed | 0 Failed | 0 Pending | 0 Skipped
PASS

[inteon]

/approve
/lgtm

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [inteon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[SgtCoDFish]

Adding even though this has merged: I saw these errors in istio-proxy in the e2e tests without this change:

2024-10-25T10:13:04.992535Z     error   citadelclient   failed to sign CSR: create certificate: rpc error: code = Unavailable desc = connection error: desc = "transport: authentication handshake failed: credentials: cannot check peer: missing selected ALPN property"