openid / sharedsignals

OpenID Shared Signals Working Group Repository
47 stars 12 forks source link

Simple & Complex Subject Identifiers: why not use "aliases" construct defined in secevents draft spec? #85

Open scvenema opened 1 year ago

scvenema commented 1 year ago

A recent comment by Phil Hunt a few days ago in PR #82

The format value is an IANA registered format to avoid conflicts. The term "complex" is probably too generic. You may want to prefix it with the spec name (e.g. caep, risc, ssf) For example "caep_complex". Registration info is in section 8.1 of the subject identifier draft Other than that looks good.

...caused to wonder why we are using this "Simple" and "Complex construct when the aliases subject ID type is already defined in draft-ietf-secevent-subject-identifiers-18 §3.2.8 and seems to fit our use case?

If we used aliases, then the Figure 6 example in the current draft of the Framework spec would instead look like:

{
  "iss": "https://idp.example.com/",
  "jti": "756E69717565206964656E746966696572",
  "iat": 1520364019,
  "aud": "636C69656E745F6964",
  "events": {
    "https://schemas.openid.net/secevent/caep/event-type/session-revoked": {
      "sub_id": {
          "format": "aliases",
          "identifiers": [
              "user": {
                  "format": "iss_sub",
                  "iss": "https://idp.example.com/3957ea72-1b66-44d6-a044-d805712b9288/",
                  "sub": "jane.smith@example.com"
              },
              "device": {
                  "format": "iss_sub",
                  "iss": "https://idp.example.com/3957ea72-1b66-44d6-a044-d805712b9288/",
                  "sub": "e9297990-14d2-42ec-a4a9-4036db86509a"
          ],
      },
      "initiating_entity": "policy",
      "reason_admin": "Policy Violation: C076E82F",
      "reason_user": "Landspeed violation.",
      "event_timestamp": 1600975810
    }
  }
}

I thought I brought this up on the 2022-06-20 call, but it somehow morphed into the "simple"/"complex" approach and I can't remember why. I recall @timcappalli mentioning some issue about nesting the array format being more difficult to process. I see that the aliases spec specifically precludes nesting; is nesting needed for some use case? From a (de)serializing perspective, it seems like we already need to deal with this aliases type to conform with the draft IETF spec linked above so it should make things easier rather than more difficult for implementation.

Other observations:

I'm happy to do edits on the draft and open a PR if there is any consensus on these questions.

FragLegs commented 1 year ago

From the discussion today, here is the reformatted version of Steve's proposed SET:

{
  "iss": "https://idp.example.com/",
  "jti": "756E69717565206964656E746966696572",
  "iat": 1520364019,
  "aud": "636C69656E745F6964",
  "sub_id": {
      "format": "aliases",
      "identifiers": [
          {
              "format": "user",
              "user_id": "foo"
          },
          {
              "format": "device",
              "device_id": "bar"
           }
      ],
  },
  "events": {
    "https://schemas.openid.net/secevent/caep/event-type/session-revoked": {
      "initiating_entity": "policy",
      "reason_admin": "Policy Violation: C076E82F",
      "reason_user": "Landspeed violation.",
      "event_timestamp": 1600975810
    }
  }
}
tulshi commented 5 months ago

@FragLegs , what is the status of this issue?

FragLegs commented 5 months ago

I think that once we realized that the Aliases format does not carry the same semantic information as the Complex format, the issue was dropped. That is, you could put all of the Simple subjects that make up a Complex subject into an Aliases format instead, but you would no longer know whether each Simple subject referred to a user, a group, a tenant, etc.

What about @independentid 's original comment that sparked this thread? Should we consider changing complex format to ssf-complex to make it less likely to conflict with something else?