ccl/oidcccl: support principal matching on list claims

[cameronnunez]

Previously, matching on ID token claims was not possible if the claim key
specified had a corresponding value that was a list, not a
string. With this change, matching can now occur on claims that are list valued
in order to add login capabilities to DB Console. It is important to note that
this change does NOT offer the user the ability to choose between possible
matches; it simply selects the first match to log the user in.

This change also adds more verbose logging about ID token details.

Epic: none
Fixes: #97301, #97468

Release note (enterprise change): The cluster setting
server.oidc_authentication.claim_json_key for DB Console SSO
now accepts list-valued token claims.

Release note (general change): Increasing the logging verbosity
is more helpful with troubleshooting DB Console SSO issues.

[cockroach-teamcity]

This change is 

[kpatron-cockroachlabs]

Reviewed 1 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cameronnunez and @dhartunian)

---

pkg/ccl/oidcccl/claim_match.go line 12 at r1 (raw file):

)

const claimGroups = "groups"

Out of curiosity why are we special casing claims named "groups" name instead of doing something like: always parsing the string representation so users can write their own regex on the larger string (["user1", "user2"]) or determining whether to treat it as a string or a group based on how it deserializes? AFAIK "groups" is not a reserved name when it comes to JWTs/OIDC.

[cameronnunez]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian and @kpatron-cockroachlabs)

---

pkg/ccl/oidcccl/claim_match.go line 12 at r1 (raw file):

Previously, kpatron-cockroachlabs (Kyle) wrote…

Out of curiosity why are we special casing claims named "groups" name instead of doing something like: always parsing the string representation so users can write their own regex on the larger string (["user1", "user2"]) or determining whether to treat it as a string or a group based on how it deserializes? AFAIK "groups" is not a reserved name when it comes to JWTs/OIDC.

Good point, fixed.

[kpatron-cockroachlabs]

although I'm no CRDB dev to be stamping

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cameronnunez and @dhartunian)

---

pkg/ccl/oidcccl/claim_match.go line 29 at r2 (raw file):

	var principal string
	if err := json.Unmarshal(targetClaim, &principal); err != nil {
		// Try parsing assuming the claim value is a list and not a string.

Nit: it might be worth adding an info log for this case so people can get the error message from this unmarshalling if neither succeeds.

---

pkg/ccl/oidcccl/claim_match.go line 58 at r2 (raw file):

}

func matchOnListClaim(

Nit: is there any way to combine this case with the one above? Maybe by making a slice with exactly the principal if it's a string and not an array? It feels a little weird that these have so much in common.

[cameronnunez]

thanks for the review!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dhartunian and @kpatron-cockroachlabs)

---

pkg/ccl/oidcccl/claim_match.go line 29 at r2 (raw file):

Previously, kpatron-cockroachlabs (Kyle) wrote…

Nit: it might be worth adding an info log for this case so people can get the error message from this unmarshalling if neither succeeds.

Done.

---

pkg/ccl/oidcccl/claim_match.go line 58 at r2 (raw file):

Previously, kpatron-cockroachlabs (Kyle) wrote…

Nit: is there any way to combine this case with the one above? Maybe by making a slice with exactly the principal if it's a string and not an array? It feels a little weird that these have so much in common.

Good call done.

[dhartunian]

Reviewed 2 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cameronnunez and @kpatron-cockroachlabs)

---

-- commits line 16 at r3:
include a separate release note about the OIDC feature that's being added.

---

pkg/ccl/oidcccl/claim_match.go line 79 at r3 (raw file):

		log.Infof(ctx,
			"check OIDC cluster settings: %s, %s",
			OIDCPrincipalRegexSettingName, OIDCClaimJSONKeySettingName,

can we include the name and value of the claim in a log statement as well when we can't find any matches? that's one of the specific things customers have trouble with because they don't know what they're trying to match on.

---

pkg/ccl/oidcccl/authentication_oidc_test.go line 310 at r3 (raw file):

			claims: map[string]json.RawMessage{
				"email": json.RawMessage(`"myfakeemail@example.com"`),
			},

is it possible to have a test with a slightly higher level of abstraction so that we can test the whole claim decode flow from the JSON string?

[cameronnunez]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dhartunian and @kpatron-cockroachlabs)

---

-- commits line 16 at r3:

Previously, dhartunian (David Hartunian) wrote…

include a separate release note about the OIDC feature that's being added.

Done.

---

pkg/ccl/oidcccl/claim_match.go line 79 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

can we include the name and value of the claim in a log statement as well when we can't find any matches? that's one of the specific things customers have trouble with because they don't know what they're trying to match on.

The values are included in a log entry shortly before this one, see line 352 in authentication_oidc.go

---

pkg/ccl/oidcccl/authentication_oidc_test.go line 310 at r3 (raw file):

Previously, dhartunian (David Hartunian) wrote…

is it possible to have a test with a slightly higher level of abstraction so that we can test the whole claim decode flow from the JSON string?

might need some more clarification on what you mean. I think the testing covers the added functionality, extractUsernameFromClaims, which captures the flow you mentioned, including checking the target claim in the payload and matching on it. what happens before the call is just unmarshalling the token payload into a map

[dhartunian]

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cameronnunez and @kpatron-cockroachlabs)

---

pkg/ccl/oidcccl/claim_match.go line 79 at r3 (raw file):

Previously, cameronnunez (Cameron Nunez) wrote…

The values are included in a log entry shortly before this one, see line 352 in authentication_oidc.go

that log statement only outputs the values of the cluster settings. that's easily visible to the customer already. what's not visible is the rest of the token in case they don't know what claim to use, or the value that's been extracted if there was a match.

---

pkg/ccl/oidcccl/authentication_oidc_test.go line 310 at r3 (raw file):

Previously, cameronnunez (Cameron Nunez) wrote…

might need some more clarification on what you mean. I think the testing covers the added functionality, extractUsernameFromClaims, which captures the flow you mentioned, including checking the target claim in the payload and matching on it. what happens before the call is just unmarshalling the token payload into a map

Gotcha. I agree, this is adequate. Thanks.

[cameronnunez]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dhartunian and @kpatron-cockroachlabs)

---

pkg/ccl/oidcccl/claim_match.go line 79 at r3 (raw file):

that log statement only outputs the values of the cluster settings. that's easily visible to the customer already.

"easily visible" is a bit misleading given that the customer requested for the values of the cluster settings to be logged in order to reduce the time spent investigating. To quote the customer, "Minimally, log the client_json_key element, and the pattern match sequence." They want to be able to identify if the cluster settings are indeed the problem without having to manually check the values.

I agree that it's helpful to also log the other token claims but I don't think that's what you originally expressed

[dhartunian]

Thanks again for picking this up.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @cameronnunez and @kpatron-cockroachlabs)

---

pkg/ccl/oidcccl/claim_match.go line 79 at r3 (raw file):

Previously, cameronnunez (Cameron Nunez) wrote…

that log statement only outputs the values of the cluster settings. that's easily visible to the customer already.

"easily visible" is a bit misleading given that the customer requested for the values of the cluster settings to be logged in order to reduce the time spent investigating. To quote the customer, "Minimally, log the client_json_key element, and the pattern match sequence." They want to be able to identify if the cluster settings are indeed the problem without having to manually check the values.

I agree that it's helpful to also log the other token claims but I don't think that's what you originally expressed

Agree that this is not what was originally expressed. We can leave it as-is, no worries.

[cameronnunez]

thanks

bors r=dhartunian

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

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 496d069 to blathers/backport-release-22.2-98522: 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.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[mikeCRL]

@cameronnunez Do we still expect that this will be backported?