search

strimzi / strimzi-kafka-oauth

OAuth2 support for Apache Kafka® to work with many OAuth2 authorization servers
Apache License 2.0
140 stars 89 forks source link

Oauth client assertion support #211

Closed robbertvanwaveren closed 7 months ago

robbertvanwaveren commented 10 months ago

This PR adds support for Kafka clients to obtain an access token by authenticating to the authorization server with client assertion as specified by https://www.rfc-editor.org/rfc/rfc7523 and https://www.rfc-editor.org/rfc/rfc7521. The assertion can be provided by an external mechanism and available as a file on the file system or it can be explicitly set through OAuth configuration before running the Kafka client.

In addition, the static access token or refresh token can now be provided as a file on the local filesystem.

The following new configuration options have been added by this PR:

robbertvanwaveren commented 10 months ago

this PR now has DCO signoff and grants maintainer edit rights

mstruk commented 10 months ago

I added a better description.

scholzj commented 10 months ago

@mstruk Same as the other PR. The Travis tests do not seem to run here.

mstruk commented 9 months ago
  1. the README should recommend the use of filesystem permissions to protect tokens on disk.

Good point, I'll add some docs to address this.

  1. we should consider, when the OS is unix, checking the file permissions and failing if they token files are readable by all, or the group. Obviously this doesn't provide any protection on non-unix, but it's better than nothing in my opinion.

Failing is a harsh reaction to this. Especially if an external mechanism takes care of mounting a filesystem so it's all automatic and possibly out of your control. I would prefer to log a warning, but not fail. WDYT?

tombentley commented 9 months ago

Failing is a harsh reaction to this. Especially if an external mechanism takes care of mounting a filesystem so it's all automatic and possibly out of your control. I would prefer to log a warning, but not fail. WDYT?

The 'possibly out of your control point' is a good one. So I guess logging a warning is reasonable.

mstruk commented 9 months ago

The 'env:ENV_VARIABLE_NAME' feature doesn't seem to add any capability that couldn't be performed before. Also rather than conflating such a feature with the current PR it is better to introduce it separately if at all.

Also, there is a possibility of unexpected behaviour, for example using oauth.client.secret=\"env:Hfs67vfv.M8a\" would automatically trigger a lookup of Hfs67vfv.M8a env variable which in this case was not the intention.

It is already the case that when a configuration option is resolved the resolution tries several different override possibilities in a specific order as implemented here and documented here.

First, if the system property exists with the same name it overrides the JAAS configured attribute. Second, if the env variable with the same name exists that is used. Third, if the env variable exists with the name equal to configuration key converted to all caps and underscores then that one is used. Only if there are no overrides via System properties and env vars the config parameter as configured via JAAS is used.

According to these rules using oauth.client.assertion.location="env:TOKEN_FILE" can be achieved by setting the following env variable in the client's execution environment: OAUTH_CLIENT_ASSERTION_LOCATION="$TOKEN_FILE" rather than set as a JAAS parameter.

robbertvanwaveren commented 9 months ago

I don’t see how the logic you refer to performs the substitution of the TOKEN_FILE env variable for its actual content.

The original intent of this PR is support for MS Azure Workload Identity which injects certain env vars into the execution environment. Which is the main (only) practical use-case for this PR.

Note that this means that these env var names are not configurable so these should be piped towards this libs configs parama/vars.

mstruk commented 9 months ago

@robbertvanwaveren Can you give an example or provide a failing test?

Substitution happens in your kafka-client.yaml (or whatever you call the yaml file that you use) when setting environment vars for the pod. Or in your bash startup script used as an entry point for your client.

mstruk commented 8 months ago

I'm thinking of renaming the oauth.*.location config options to oauth.*.path. The reasoning is that location is too generic, it can mean a file on the local filesystem or a url or something completely different as one of the values from a set of supported values. By saying path it makes it super clear (at least to me) that it is the path to the file on the local filesystem.

I wonder what others think?

mstruk commented 8 months ago

Actually that won't work as there are existing previous options that refer to files and are called oauth.*.location (e.g. oauth.ssl.truststore.location) and I prefer to keep consistency in the naming.

mstruk commented 7 months ago

@tombentley Do you think this needs another review (specifically, the file permissions check)?

mstruk commented 7 months ago

Travis CI integration not working again. My local run of the full testsuite has been passing on this one for a long time. If some Travis CI issues appear during release of the next version we'll fix it then.

This PR has been a long time in the making. Many thanks to everyone who contributed, and especially @robbertvanwaveren for the initial development and continued engagement.