Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: user claims in the ID Token #55

Merged
merged 12 commits into from
Oct 14, 2022
Merged

feat: user claims in the ID Token #55

merged 12 commits into from
Oct 14, 2022

Conversation

fmarino-ipzs
Copy link
Collaborator

@fmarino-ipzs fmarino-ipzs commented Sep 9, 2022 •
edited by peppelinux
Loading

Attributi utente disponibili nell'ID Token

Content

  • Gli attributi utente sono presenti nell'ID Token oltre che nella response dello userinfo endpoint.
  • Per ragioni di confidenzialità, gli RP possono decidere di richiedere che l'ID Token sia cifrato. Se il RP inserisce il parametro id_token_encrypted_response_alg allora l'OP dovrà restituire un Nested JWT come avviene per lo userinfo.

Prossimo Avviso SPID

  • estensione dei metadata di RP con id_token_encrypted_response_alg
  • estensione struttura dell'id_token, presenza degli user claim
  • Tutti gli attributi utente richiesti saranno sempre anche nell'id_token
  • presenza di header cty valorizzato a JWT

Questa PR risolve
#54

Review

  • Ensure your files are written following RST specs (not MD!)
  • Italian version
  • English version
  • Example files
  • Ask for review

@fmarino-ipzs
feat: user claims in the ID Token
4b64291 
added support for ecrypted ID Token
@vercel
Copy link

vercel bot commented Sep 9, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
spid-cie-oidc-docs ✅ Ready (Inspect) Visit Preview Oct 13, 2022 at 9:58AM (UTC)

@gitguardian
Copy link

gitguardian bot commented Sep 9, 2022 •
edited
Loading

⚠️ GitGuardian has uncovered 6 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
- Bearer Token 4b64291 docs/en/userinfo_endpoint.rst View secret
- Bearer Token 4b64291 docs/en/userinfo_endpoint.rst View secret
- Bearer Token 4b64291 docs/it/userinfo_endpoint.rst View secret
- Bearer Token a7be856 docs/en/userinfo_endpoint.rst View secret
- Bearer Token a7be856 docs/en/userinfo_endpoint.rst View secret
- Bearer Token a7be856 docs/it/userinfo_endpoint.rst View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

peppelinux
peppelinux approved these changes Sep 14, 2022
View reviewed changes
Copy link
Member

@peppelinux peppelinux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questa PR dovrebbe essere unita nel momento in cui AgID definisce i contenuti del prossimo avviso SPID

@fmarino-ipzs
Copy link
Collaborator Author

Ho creato dei box di disambiguazione per evidenziare il diverso funzionamento di CIEid e SPID rispetto alla questione degli attributi utente nellID Token.
@peppelinux
@agcolella
@nunzionapoli
@damikael

@peppelinux
Copy link
Member

Per la nostra revisione dobbiamo considerare le implicazioni di criptare l'id_token anche quando sono in esso presenti i grant tokens delle attribute authorities

peppelinux
peppelinux requested changes Sep 30, 2022
View reviewed changes
docs/en/userinfo_endpoint.rst Outdated Show resolved Hide resolved
@peppelinux
Copy link
Member

peppelinux commented Oct 10, 2022 •
edited
Loading

Durante la riunione del 10 Ottobre è stata discussa questa PR e le proposte pendenti:

  1. IPZS: propone di ottenere sempre i claim utente in id token e opzionalmente criptare l'id_token
  2. DTD: Ok ottenere i claim sempre nell'id_token. Se il RP contiene nel metadata id_token_encrypted_response_alg il OP DOVREBBE criptare l'id_token
  3. AgID: ok sul MUST solo se id_token_encrypted_response_alg presente nel metadata RP, questo aspetto viene poi controllato nei QaD tests e durante le fasi di onboarding, rendendo di conseguenza la criptazione dell'id_token in SPID obbligatoria

peppelinux
peppelinux requested changes Oct 10, 2022
View reviewed changes
docs/en/metadata_oidc_rp.rst Outdated Show resolved Hide resolved
docs/en/metadata_oidc_rp.rst Show resolved Hide resolved
docs/en/token_endpoint.rst Outdated Show resolved Hide resolved
docs/it/metadata_oidc_rp.rst Outdated Show resolved Hide resolved
docs/it/metadata_oidc_rp.rst Show resolved Hide resolved
docs/it/token_endpoint.rst Outdated Show resolved Hide resolved
docs/it/userinfo_endpoint.rst Outdated Show resolved Hide resolved
@peppelinux peppelinux self-assigned this Oct 10, 2022
@peppelinux peppelinux mentioned this pull request Oct 11, 2022
Closed
14 tasks
@damikael
Copy link
Member

damikael commented Oct 12, 2022 •
edited
Loading

Please consider that in:
https://openid.net/specs/openid-connect-core-1_0.html
5.5. Requesting Claims using the "claims" Request Parameter
is specified:

The top-level members of the Claims request JSON object are:

userinfo
OPTIONAL. Requests that the listed individual Claims be returned from the UserInfo Endpoint. If present, the listed Claims are being requested to be added to any Claims that are being requested using scope values. If not present, the Claims being requested from the UserInfo Endpoint are only those requested using scope values.
When the userinfo member is used, the request MUST also use a response_type value that results in an Access Token being issued to the Client for use at the UserInfo Endpoint.
id_token
OPTIONAL. Requests that the listed individual Claims be returned in the ID Token. If present, the listed Claims are being requested to be added to the default Claims in the ID Token. If not present, the default ID Token Claims are requested, as per the ID Token definition in Section 2 and per the additional per-flow ID Token requirements in Sections 3.1.3.6, 3.2.2.10, 3.3.2.11, and 3.3.3.6.

I think that the merge of claims requested in userinfo or in id_token is not standard.

@peppelinux
Copy link
Member

Attendiamo che IPZS e AgID forniscano le diciture esatte da includere all'interno delle note

immagine

@peppelinux peppelinux mentioned this pull request Oct 12, 2022
Closed
fmarino-ipzs
fmarino-ipzs commented Oct 12, 2022
View reviewed changes
Copy link
Collaborator Author

@fmarino-ipzs fmarino-ipzs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ho aggiunto i miei commenti relativi alla parte CIE.

damikael
damikael reviewed Oct 13, 2022
View reviewed changes
docs/en/authorization_endpoint.rst
@@ -195,57 +172,35 @@ When the **scope** parameter is used, the following values are supported:
- *email*;
- *email_verified*.

The attributes requested by the parameter **scope** are available both in the ID Token and in the response to the Userinfo Endpoint.
.. admonition:: |spid-icon|

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for SPID: if a scope is requested other than 'openid', the RP metadata SHOULD contains id_token_encrypted_response_alg and id_token_encrypted_response_enc and their value SHOULD NOT be 'none'.
If a scope is requested other than 'openid' and the RP metadata do not contains id_token_encrypted_response_alg and id_token_encrypted_response_enc or their value is 'none', none user attributes are returned in the id_token from token endpoint response.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep this comment and try to include it after the next commits and revisions, so please keep this on hold

Copy link
Member

@peppelinux peppelinux Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just put in the metadata parameter definition that the allowed values MUST be the same as the ones adopted for encrypting userinfo ... just a pointer without a re-definition

do you agree?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, as in SPID Notice 41

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here my proposal

#82

so if you agree we may merge it as it is and then create a separate PR for that

damikael
damikael reviewed Oct 13, 2022
View reviewed changes
docs/en/authorization_endpoint.rst Outdated

If the parameter **claims** is not present or has no value and the only scope openid has been requested, only the claim *sub* is returned in the response to the *userinfo endpoint*.

.. admonition:: |spid-icon|

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for SPID:
Only if the RP metadata contains id_token_encrypted_response_alg and id_token_encrypted_response_enc and their value is not 'none', the user attributes requested by the id_token object into the claims parameter will be available into the id_token from the token endpoint, added to any claims that are being requested using scope values.

The user attributes requested by the userinfo object into the claims parameter will be available only into the response from the userinfo endpoint, added to any Claims that are being requested using scope values.

@nunzionapoli please check if this is compliant to OIDC Core.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here my proposal

#82

so if you agree we may merge it as it is and then create a separate PR for that

damikael
damikael reviewed Oct 13, 2022
View reviewed changes
docs/en/token_endpoint.rst
@@ -225,6 +225,15 @@ ID Token
++++++++

The ID Token is a JSON Web Token (JWT) that contains information on the user that has executed the authentication. The RPs MUST validate the ID Token.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only for SPID: if some user attributes are requested with scope or claim parameter, the RP metadata SHOULD contains id_token_encrypted_response_alg and id_token_encrypted_response_enc and their value SHOULD NOT be 'none'.
If some user attributes are requested with scope or claim and the RP metadata does not contains id_token_encrypted_response_alg and id_token_encrypted_response_enc or their value is 'none', none user attributes are returned in the id_token from token endpoint response.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here my proposal

#82

so if you agree we may merge it as it is and then create a separate PR for that

damikael
damikael reviewed Oct 13, 2022
View reviewed changes
docs/it/authorization_endpoint.rst Outdated

.. admonition:: |spid-icon|

Gli attributi richiesti tramite il parametro **scope** sono disponibili nella risposta allo *userinfo endpoint*.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solo per SPID: se sono richiesti scope ulteriori a 'openid', il metadata del RP DOVREBBE contenere id_token_encrypted_response_alg e id_token_encrypted_response_enc ed il loro valore dovrebbe essere diverso da 'none'. Se sono richiesti scope ulteriori a 'openid' e il metadata del RP non contiene id_token_encrypted_response_alg e id_token_encrypted_response_enc o il loro valore è 'none', nessun attributo utente viene restituito nell'id_token nella risposta dall'endpoint token.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here my proposal

#82

so if you agree we may merge it as it is and then create a separate PR for that

damikael
damikael reviewed Oct 13, 2022
View reviewed changes
docs/it/authorization_endpoint.rst Outdated

La tabella seguente mostra alcuni esempi di utilizzo.
.. admonition:: |spid-icon|

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solo per SPID:

Solo se il metadata del RP contiene id_token_encrypted_response_alg e id_token_encrypted_response_enc ed il loro valore è diverso da 'none', gli attributi utente richiesti tramite l'oggetto id_token del parametro claims saranno disponibili nell'id_token restituito dall'endpoint token, in aggiunta agli altri claim eventualmente richiesti tramite il parametro scope.

Gli attributi utente richiesti tramite l'oggetto userinfo del parametro claims saranno disponibili solo nella risposta ottenuta dall'endpoint userinfo, in aggiunta agli altri claim eventualmente richiesti tramite il parametro scope.

@nunzionapoli gentilmente verifica se tale comportamento è conforme a OIDC Core.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here my proposal

#82

so if you agree we may merge it as it is and then create a separate PR for that

damikael
damikael reviewed Oct 13, 2022
View reviewed changes
docs/it/token_endpoint.rst
@@ -224,6 +224,11 @@ ID Token
++++++++

L'ID Token è un JSON Web Token (JWT) che contiene informazioni sull'utente che ha eseguito l'autenticazione. I RP DEVONO eseguire la validazione dell'ID Token.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Solo per SPID: se sono richiesti attributi utente tramite il parametro scope o tramite il parametro claims, il metadata del RP DOVREBBE contenere id_token_encrypted_response_alg e id_token_encrypted_response_enc ed il loro valore dovrebbe essere diverso da 'none'. Se sono richiesti attributi utente tramite il parametro scope o tramite il parametro claims ed il metadata del RP non contiene id_token_encrypted_response_alg e id_token_encrypted_response_enc oppure il loro valore è 'none', nessun attributo dell'utente sarà presente nell'id_token restituito dall'endpoint token.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here my proposal

#82

so if you agree we may merge it as it is and then create a separate PR for that

damikael
damikael reviewed Oct 13, 2022
View reviewed changes
Copy link
Member

@damikael damikael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inseriti commenti per gli attributi utente in id_token per SPID

@peppelinux
Copy link
Member

peppelinux commented Oct 13, 2022 •
edited
Loading

TODO:

  • aggiungere una sezione con gli algoritmi supportati (e rimarcare l'esclusione di none)
  • linkare i metadata claim interessati a questa sezione unica
  • aggiungere un box per rimarcare le proprieta dell'id token in spid, nel caso in cui vi fossero attributi utente

@peppelinux peppelinux mentioned this pull request Oct 14, 2022
Closed
3 tasks
@peppelinux peppelinux merged commit a514f3a into italia:versione-corrente Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peppelinux peppelinux peppelinux requested changes

@damikael damikael damikael left review comments

@nunzionapoli nunzionapoli Awaiting requested review from nunzionapoli

@agcolella agcolella Awaiting requested review from agcolella

Assignees

@peppelinux peppelinux

Labels
avviso AgID
Projects
SPID CIE OpenID Connect docs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fmarino-ipzs @peppelinux @damikael