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.

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

feat: support for vc+sd-jwt #273

Draft
wants to merge 34 commits into
base: dev_sdjwt-bk
Choose a base branch
from
Draft

Conversation

Zicchio
Copy link
Collaborator

@Zicchio Zicchio commented Sep 19, 2024

This pull request closes #269 and #255.
Proposals and modifications are very welcomed.

The followings steps where done:

  1. define a new interface for general purpose VP token (which is not intended to be a definitive and final iteration; open for review if/when we handle mdoc-cbor tokens); previous iteration "kinda" assume that the only possible VP tokens were vp-envolpes.
  2. new interface is intended to be decoupled from trust-layer and direct post processing as much possible for extensibility purposes.
  3. define an implementation for that interface for vc+sd-jwt tokens - In this PR only PIDs are obtained this way.
  4. integrate the usage of the new interface in the response_handler.py, with some care regarding extensibility and possible modifications at the trust layers

The final iteration resulted in more modifcations that intended - special care should be taken before pulling in @peppelinux .

Note that in the process we "lost" VP-envolpe; which was expclicity against the intent of this pull request. This is because the envolpe hypotesys was spread around in multiple methods and I found it hard to salvage (I don't deny that it might be a skill issue). Hopelly, the new interface should promote a new implementation in short time.

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.

Good achievements, anyway we have decided to continue in this PR these two core components:

pyeudiw/openid4vp/vp_sd_jwt_kb.py Outdated Show resolved Hide resolved
pyeudiw/openid4vp/vp_sd_jwt_kb.py Outdated Show resolved Hide resolved
pyeudiw/openid4vp/vp_sd_jwt_kb.py Outdated Show resolved Hide resolved
pyeudiw/openid4vp/vp_sd_jwt_kb.py Outdated Show resolved Hide resolved
# _verify_kb_jwt_payload_sd_hash(sdjwt)
_verify_kb_jwt_signature(kbjwt.jwt, cnf_jwk)


Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# TODO: sd-jwt-python already does this check, however it would be space for us to have it more explicit in our code

pyeudiw/satosa/default/response_handler.py Outdated Show resolved Hide resolved
pyeudiw/satosa/default/response_handler.py Outdated Show resolved Hide resolved
@@ -71,7 +71,7 @@ def __init__(
holder_key,
sign_alg,
add_decoy_claims,
serialization_format
serialization_format,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
serialization_format,
serialization_format

class VcSdJwtHeaderSchema(BaseModel):
typ: str
alg: str
kid: str
Copy link
Member

Choose a reason for hiding this comment

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

this is optional

Comment on lines 93 to 99
class PidVcSdJwtPayloadSchema(VcSdJwtPayloadSchema):
given_name: Optional[str] = None
family_name: Optional[str] = None
birth_date: Optional[Any] = None # TODO: date is dd-mm-yyyy but I'm not sure if libraries parses them as str or a native format
unique_id: Optional[str] = None
tax_id_code: Optional[str] = None

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class PidVcSdJwtPayloadSchema(VcSdJwtPayloadSchema):
given_name: Optional[str] = None
family_name: Optional[str] = None
birth_date: Optional[Any] = None # TODO: date is dd-mm-yyyy but I'm not sure if libraries parses them as str or a native format
unique_id: Optional[str] = None
tax_id_code: Optional[str] = None

@Zicchio Zicchio marked this pull request as draft September 20, 2024 06:41
@Zicchio
Copy link
Collaborator Author

Zicchio commented Sep 25, 2024

dev note: I rebased the branch based on the changes of PR #270 and PR #272 and handles some conflicts (mostly in the ingration test hanges with PR #272).
no further changes are introduced

"url_scheme": "haip", # haip://
"scopes": ["pid-sd-jwt:unique_id+given_name+family_name"],
"default_acr_value": "https://www.spid.gov.it/SpidL2",
"expiration_time": 5, # minutes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"expiration_time": 5, # minutes
"expiration_time": 5 # minutes

Comment on lines +51 to +58
"httpc_params": {
"connection": {
"ssl": True
},
"session": {
"timeout": 6
}
}
Copy link
Member

Choose a reason for hiding this comment

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

please use __init__.DEFAULT_DIRECT_TRUST_PARAMS

@peppelinux peppelinux changed the base branch from dev to dev_sdjwt-bk October 3, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Satosa] [Bug] Errore verifica corretteza in integration test
3 participants