diff --git a/changelog/32000.txt b/changelog/32000.txt new file mode 100644 index 00000000000..c6bc3c8d3f1 --- /dev/null +++ b/changelog/32000.txt @@ -0,0 +1,3 @@ +```release-note:improvement +identity/oidc: Omit claims with a null or empty string value from OIDC provider ID tokens and UserInfo responses, in line with the OIDC specification. Empty lists and objects are preserved. +``` diff --git a/vault/identity_store_oidc.go b/vault/identity_store_oidc.go index 7ff7a8a5b74..671e3efb9af 100644 --- a/vault/identity_store_oidc.go +++ b/vault/identity_store_oidc.go @@ -1168,6 +1168,8 @@ func (tok *idToken) generatePayload(logger hclog.Logger, templates ...string) ([ // mergeJSONTemplates will merge each of the given JSON templates into the given // output map. It will simply merge the top-level keys of the unmarshalled JSON // templates into output, which means that any conflicting keys will be overwritten. +// Claims with a null or empty string value are omitted, in line with the OIDC +// specification. func mergeJSONTemplates(logger hclog.Logger, output map[string]interface{}, templates ...string) error { for _, template := range templates { var parsed map[string]interface{} @@ -1176,11 +1178,21 @@ func mergeJSONTemplates(logger hclog.Logger, output map[string]interface{}, temp } for k, v := range parsed { - if !strutil.StrListContains(reservedClaims, k) { - output[k] = v - } else { + if strutil.StrListContains(reservedClaims, k) { logger.Warn("invalid top level OIDC template key", "template", template, "key", k) + continue } + + // Omit claims with a null or empty string value. Per the OIDC + // specification, a claim that is not returned SHOULD be omitted + // rather than included with a null or empty string value. Empty + // lists and objects are preserved, since an empty collection is a + // meaningful claim value. + if v == nil || v == "" { + continue + } + + output[k] = v } } diff --git a/vault/identity_store_oidc_test.go b/vault/identity_store_oidc_test.go index ff91179eec9..644e40dac98 100644 --- a/vault/identity_store_oidc_test.go +++ b/vault/identity_store_oidc_test.go @@ -27,6 +27,50 @@ import ( "github.com/stretchr/testify/require" ) +// TestOIDC_mergeJSONTemplates verifies that claims with a null or empty string +// value are omitted from merged OIDC templates, while claims with a meaningful +// value (including empty lists and objects) are preserved. +func TestOIDC_mergeJSONTemplates(t *testing.T) { + template := `{ + "name": "Brian Candler", + "organization": "", + "nickname": null, + "groups": [], + "metadata": {}, + "login_count": 0, + "active": false + }` + + output := map[string]interface{}{ + "sub": "entity-id", + } + + if err := mergeJSONTemplates(hclog.NewNullLogger(), output, template); err != nil { + t.Fatalf("unexpected error merging templates: %v", err) + } + + // Claims with a null or empty string value must be omitted. + for _, claim := range []string{"organization", "nickname"} { + if _, ok := output[claim]; ok { + t.Errorf("expected claim %q to be omitted, but it was present", claim) + } + } + + // Claims with a meaningful value, including empty lists and objects, must + // be preserved. + expected := map[string]interface{}{ + "sub": "entity-id", + "name": "Brian Candler", + "groups": []interface{}{}, + "metadata": map[string]interface{}{}, + "login_count": float64(0), + "active": false, + } + if diff := deep.Equal(output, expected); diff != nil { + t.Errorf("unexpected merged claims: %v", diff) + } +} + // TestOIDC_Path_OIDC_RoleNoKeyParameter tests that a role cannot be created // without a key parameter func TestOIDC_Path_OIDC_RoleNoKeyParameter(t *testing.T) {