Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(client): Allow specifying grpc CallCredentials #1261

Merged
merged 1 commit into from
Oct 17, 2023

Conversation

mjameswh
Copy link
Contributor

What changed

  • Added ConnectionOptions.callCredentials, which can be used to register one or more grpc CallCredentials, eg. to add metadata based credential mechanism
  • If ConnectionOptions.credentials was specified (and not the tls property), then those credentials would not be passed to the gRPC client creation.

Why

@mjameswh mjameswh requested a review from a team as a code owner October 16, 2023 22:14
@bergundy
Copy link
Member

Do we need to expose this directly?
How is it better than a user setting the metadata directly which we already allow?

@mjameswh
Copy link
Contributor Author

Do we need to expose this directly?

We could technically avoid exposing this one, and tell people that if they need to register some CallCredentials, then they need to instantiate and provide a ChannelCredentials themself (rather than using the ConnectionOptions.tls property), and do combineChannelCredentials() themselves.

However, at some point in the near future, I would like to improve error reporting for some common TLS errors, but some assertions will only be possible if we have access to details of the TLS config. For that reason, I'd really prefer to discourage people from using the ConnectionOptions.credentials property. Anyway, there is almost no use case for ChannelCredentials that are not already supported by our ConnectionOptions.tls property.

How is it better than a user setting the metadata directly which we already allow?

The withMetadata function attach metadata to the async context. That makes it useful for metadata that applies in a given execution context (eg. in a web app, metadata that are specific to the current request context), but doesn't work for metadata that are dynamic yet relates to the connection itself (eg. OAuth2 bearer token representing the app itself).

I believe gRPC interceptors can technically be used to do pretty much the same as what CallCredentials would do, as shown here. However, looking at grpc-js' source, CallCredentials get involved much later than interceptors (ie. after picking sub channel), so that CallCredentials's metadata will be reevaluated in some cases where Interceptors would not.

So I really think that CallCredentials is what we should expose for people for people to use to support that type of auth headers.

@bergundy
Copy link
Member

I see, call credentials allows refreshing the metadata, that's much better experience.

Is there an expiration on those certs that you generated? Ideally CI won't fail when they expire.

@mjameswh
Copy link
Contributor Author

mjameswh commented Oct 17, 2023

Is there an expiration on those certs that you generated? Ideally CI won't fail when they expire.

10 years for CA, Client and Server certs, and RSA keys are 4096 bits. They shouldn't fail any time soon. CA's name clearly indicate that this is a test CA and shouldn't be trusted, and I destroyed the CA's private key, just in case.

For posterity, here are the commands I ran:

openssl genrsa -out test-ca.key 4096
openssl req -new -x509 -days 3650 -key test-ca.key -subj "/C=US/ST=WA/CN=Test Root CA - DO NOT TRUST" -out test-ca.crt -addext basicConstraints=critical,CA:TRUE

openssl req -newkey rsa:4096 -nodes -keyout test-server.key -subj "/CN=Server" -out test-server.csr
openssl x509 -req -extfile <(printf "subjectAltName=Server") -days 3650 -in test-server.csr -CA test-ca.crt -CAkey test-ca.key -CAcreateserial -out test-server.crt -extensions v3_ca -extfile <(echo "[v3_ca]"; echo "basicConstraints = critical,CA:TRUE")
cat test-server.crt test-ca.crt > test-server-chain.crt

openssl req -newkey rsa:4096 -nodes -keyout test-client.key -subj "/CN=Client" -out test-client.csr
openssl x509 -req -extfile <(printf "subjectAltName=Client") -days 3650 -in test-client.csr -CA test-ca.crt -CAkey test-ca.key -CAcreateserial -out test-client.crt -extensions v3_ca -extfile <(echo "[v3_ca]"; echo "basicConstraints = critical,CA:TRUE")
cat test-client.crt test-ca.crt > test-client-chain.crt

rm test-ca.key test-client.crt test-server.crt *.csr

@mjameswh mjameswh merged commit c4bb5cc into temporalio:main Oct 17, 2023
24 checks passed
@mjameswh mjameswh deleted the client-conn-credentials branch October 17, 2023 21:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Unable to provide Authorization header for a client Connection - CallCredentials does not appear to work
2 participants