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

Update errorhandling for Wallets, SIOP QR Codes #33

Merged
merged 11 commits into from
Feb 1, 2024
Merged

Conversation

nodh
Copy link
Contributor

@nodh nodh commented Jan 30, 2024

Implements improvements for Wallet Apps:

  • Passes exceptions from native crypto service implementations up to callers of the JwsService
  • Adds QR Code use case for SIOPv2
  • Store entries are serializable

In my opinion we should release version 3.4.0 afterwards.

@nodh nodh requested a review from JesusMcCloud January 30, 2024 18:33
@nodh nodh self-assigned this Jan 30, 2024
Copy link
Collaborator

@JesusMcCloud JesusMcCloud left a comment

Choose a reason for hiding this comment

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

Looks good except for the remnants of hardcoded P-256-based algos. But that's a separate issue and should be done in a separate PR (but still before 3.4.0, becuase now we have a CryptoService that supports all curves, but other pieces of the puzzle are hardcoded to only fit P-256.

@@ -95,6 +95,48 @@ class OidcSiopVerifier(
)
}

private val containerJwt = FormatContainerJwt(algorithms = arrayOf(JwsAlgorithm.ES256.identifier))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the algo hardcoded here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed that in recent commits, for several usages of hardcoded algorithms in the OID4VCI/SIOP implementations.

@@ -231,9 +235,10 @@ class OidcSiopWallet(
)
val jwsPayload = idToken.serialize().encodeToByteArray()
val jwsHeader = JwsHeader(algorithm = JwsAlgorithm.ES256)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we not take this opportunity to get rid of all the hardcoded stuff and at least make it a constructor property, that defaults to ES256? I'm not saying this PR should do it, but before we release 3.4.0, those hardcoded values should go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in JwsHeader, the algorithm parameter is required, i.e. non-null.

Copy link
Collaborator

@JesusMcCloud JesusMcCloud left a comment

Choose a reason for hiding this comment

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

Looks good except for the remnants of hardcoded P-256-based algos. But that's a separate issue and should be done in a separate PR (but still before 3.4.0, becuase now we have a CryptoService that supports all curves, but other pieces of the puzzle are hardcoded to only fit P-256.

@nodh nodh requested a review from JesusMcCloud January 31, 2024 08:12
Copy link
Collaborator

@JesusMcCloud JesusMcCloud left a comment

Choose a reason for hiding this comment

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

Are the hardcoded SHA-256 digests a problem?

@nodh nodh merged commit c5d939e into main Feb 1, 2024
3 checks passed
@nodh nodh deleted the feature/errorhandling branch February 1, 2024 15:06
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.

2 participants