sigstore: refactor, use IdentityToken everywhere

[woodruffw]

Key changes:

• sigstore._internal.oidc.Identity is now stabilized as sigstore.oidc.IdentityToken, and provides attributes similar to the ones originally requested/proposed by @jku in Signing should support issuer and identity arguments too  #567
• The top level Signer.sign(...) API now takes an IdentityToken rather than a raw str
• The refactored IdentityToken class now uses the correct IdentityError exception type (the first party one, rather than id.IdentityError
• The (internal) Fulcio CSR API now takes an IdentityToken rather than a raw str
• The (internal) CLI helpers now use and return IdentityToken rather raw str objects
• The (internal) CLI helpers now include _die, which improves our type-checking of the CLI

See #567.

CC @jku

[jku]

I think this looks great from my perspective, apart from one thing: I am interested in the certificate identity issuer which is not necessarily "iss" in the JWT.

So as an example, in this example JWT below the certificate identity details I'm looking for are email and federated_claims.connector_id. I assume in other cases iss is really the certificate issuer.

{
  'iss': 'https://oauth2.sigstore.dev/auth', 
  'sub': '...', 
  'aud': 'sigstore',
  'exp': 1682590933,
  'iat': 1682590873, 
  'nonce': '...', 
  'at_hash': '...',
  'email': '[email protected]',
  'email_verified': True,
  'federated_claims': {
    'connector_id': 'https://github.com/login/oauth',
    'user_id': '...'
  }
}

[tetsuo-cpp]

LGTM! I think just the point about federated_connector_id needs to be addressed. But otherwise, the refactor looks great.

[woodruffw]

I've added IdentityToken.federated_issuer in 45ab029.

[woodruffw]

With regards to naming, it would be good if Fulcio, or Sigstore as a whole system, had an agreed name for this concept so that different clients can use the same name.

Yep, strongly agreed. I wonder if that's something that should go in the client spec, or maybe somewhere else? CC @znewman01

[znewman01]

Likely the Fulcio spec—CC @haydentherapper

[haydentherapper]

Ack, added a TODO to the spec doc to include this somewhere

[woodruffw]

Thanks all! I think this is good to go, pending final review.

[tnytown]

New changes LGTM!

[jku]

Looks like a really good improvement to me, thanks for the patient tweaking.

Left one log content comment and have two other notes but none of these should IMO block this PR:

• The attribute name does not feel great but your reasoning about it makes sense and I don't have better suggestions. It does feel like identity is a lot like expected_certificate_subject but only one of them has "expected" in the name...
• IdentityTokens identity and expected_certificate_subject attributes also operate slightly differently: one is pre-computed and the other is not. I don't think this matters much

[woodruffw]

• The attribute name does not feel great but your reasoning about it makes sense and I don't have better suggestions. It does feel like identity is a lot like expected_certificate_subject but only one of them has "expected" in the name...

Yeah, fully agreed -- I'm vacillating between expected and effective, but I think both are confusing/non-ideal in ways that the other doesn't address 🙂

[tetsuo-cpp]

The newest changes LGTM!