cockroach
cockroach copied to clipboard
ccl/jwtauthccl: Implement JWT based auth for CRDB SSO
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.
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.
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
SessionDefaultsand theCustomOptionSessionDefaultsproperties on theSessionArgsobject. The difference appears to be that those with a.in them get put intoCustomOptionSessionDefaults. Additionally, theSessionDefaultsneed to be part of avarGenmap invargs.go, while theCustomOptionSessionDefaultsdon'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 asdefault_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.
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.
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)
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
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.
You can put your tests under pkg/ccl somewhere. Pls ask rafi for help, I'm off for the week
I think this should be ready for a review. The failing tests seem like they are probably unrelated.
don't forget to get a review from your team as well.
you might need to do rm -rf vendor; git checkout vendor; git submodule update --init --recursive
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.
bors r+
Encountered an error creating backports. Some common things that can go wrong:
- The backport branch might have already existed.
- There was a merge conflict.
- 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.