cockroach icon indicating copy to clipboard operation
cockroach copied to clipboard

ccl/jwtauthccl: Implement JWT based auth for CRDB SSO

Open kpatron-cockroachlabs opened this issue 3 years ago • 10 comments
trafficstars

In order to enable future work to allow customers to log into CRDB instances using SSO, this change implements a new type of auth based on JWTs. Its enablement and configuration is controlled by cluster settings instead of being added as an hba_conf option.

In particular, this change introduces three cluster settings: server.jwt_authentication.enable, server.jwt_authentication.jwks, and server.jwt_authentication.issuers. If during login, the custom option --jwt.auth=true is sent, the JWT code path is activated and the contents of the "password" field are analyzed as a token, if the enabled cluster setting is set to true.

The token signature is validated with the jwks cluster setting. The token's expiration time is validated, the subject field is matched to the username requested, the audience is field is matched against the cluster's ID and the issuer is matched to the value of the issuers cluster setting.

Finally, if the token passes all of those checks, CRDB ensures that an enterprise license is active.

Release note (enterprise change): A new JWT based auth method has been introduced as an option. This work lays the ground work for users to have SSO based login into their CockroachDB Dedicated clusters.

kpatron-cockroachlabs avatar Aug 11 '22 16:08 kpatron-cockroachlabs

This change is Reviewable

cockroach-teamcity avatar Aug 11 '22 16:08 cockroach-teamcity

Could I get some initial feedback on the approach I'm taking here for the CRDB SSO work, before I move forward with tests, etc?

Some things I would appreciate more eyes on in particular:

  • How does CC get access to the cluster ID of a CRDB dedicated/serverless instance? I checked the CC database and it looks like the only cluster IDs there are generated by CC itself. Is there an alternative way we can pull this data? If not, should we consider making the audience field be checked against something else? What are other values that both the CRDB and CC instances would know about?
  • Do CRDB dedicated/serverless instances have enterprise licenses enabled by default?
  • Did I implement the separation of CCL code correctly? The approach I took feels a little...not great but tries to emulate the approach taken for DB Console OIDC.
  • Is there any preference for using custom vs non-custom options? For now I picked custom because it required less code.
  • Bazel seems to be unhappy about a transitive dependency on github.com/decred/dcrd/crypto/blake256, which is stopping me from running mirror. I need to investigate why this is happening more but if anyone knows off-hand it would be appreciated.

kpatron-cockroachlabs avatar Aug 11 '22 16:08 kpatron-cockroachlabs

That comment has to do with how options are represented in the CRDB code base. As far as I can tell, the options you pass from the client get populated into both the SessionDefaults and the CustomOptionSessionDefaults properties on the SessionArgs object. The difference appears to be that those with a . in them get put into CustomOptionSessionDefaults. Additionally, the SessionDefaults need to be part of a varGen map in vargs.go, while the CustomOptionSessionDefaults don't. Originally I thought that the distinction was a postgres vs CRDB setting but I'm seeing settings that appear to be CRDB specific (such as default_trasaction_priority. Now I'm not sure what the distinction is. However, my guess was that this one had to go into the custom one because it is CCL licensed.

What you're describing are custom session options. Non-custom session options are in sql/vars.go. However, this PR is adding cluster settings, which don't have any notion of "custom settings." The only allowed cluster settings are those registered with the settings registry.

rafiss avatar Aug 15 '22 16:08 rafiss

Kyle was asking about the custom pgwire startup option. This one is indeed special: it's a startup parameter but not meant to become a session variable.

I'd say please handle it in a similar way to crdb:remote_addr.

knz avatar Aug 15 '22 16:08 knz

also maybe rename the custom option from jwt.auth to crdb:jwt-auth or something similar (i.e. with a crdb prefix so that low-level protocol dumps make it clear who consumes what when a LB is involved)

knz avatar Aug 15 '22 16:08 knz

In case I wasn't clear we have to use options IMHO, and my point before was that the name of the option should be crdb:xxx

so the psql URL would be postgres://....?...&options=--crdb:jwt-auth=1

knz avatar Aug 16 '22 15:08 knz

I tried to add tests to auth_test.go but since this feature requires CCL the runs failed saying that it couldn't use JWT auth because it needs CCL features. I'm not sure whether we want to include CCL code in the auth path to allow this to be tested in that location or not.

kpatron-cockroachlabs avatar Aug 23 '22 19:08 kpatron-cockroachlabs

You can put your tests under pkg/ccl somewhere. Pls ask rafi for help, I'm off for the week

knz avatar Aug 24 '22 14:08 knz

I think this should be ready for a review. The failing tests seem like they are probably unrelated.

kpatron-cockroachlabs avatar Aug 24 '22 21:08 kpatron-cockroachlabs

don't forget to get a review from your team as well.

knz avatar Sep 20 '22 20:09 knz

you might need to do rm -rf vendor; git checkout vendor; git submodule update --init --recursive

rafiss avatar Sep 23 '22 14:09 rafiss

you might need to do rm -rf vendor; git checkout vendor; git submodule update --init --recursive

Guess someone pushed a new vendor update again. I fixed it yesterday but I guess there's another one.

kpatron-cockroachlabs avatar Sep 23 '22 14:09 kpatron-cockroachlabs

bors r+

kpatron-cockroachlabs avatar Sep 23 '22 15:09 kpatron-cockroachlabs

Build succeeded:

craig[bot] avatar Sep 23 '22 16:09 craig[bot]

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 165f67a8cce69ca71accd9a09065bdc099184a72 to blathers/backport-release-22.2-85965: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

blathers-crl[bot] avatar Sep 23 '22 16:09 blathers-crl[bot]