-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fields defined in SD-JWT specification are not verified #236
Comments
#227 Maybe this can be combine with integration of zod for validation of objects, but i think there's a lot of unexpected security issues that can arise from not validating these things. For now I've added a lot of extra validation in Credo, but ideally we handle it here. |
@TimoGlastra you are correct, at the According to the spec we should at least validate the From my point of view validating the time based fields is fine. But we should only do this after allowing to pass the other validations (or passing a parameter allowing to validate "not in time" credentials. Also checking the types of the fields COULD be relevant. However we need to respect all possible types an object can have. Also what other fields beside the ones I mentioned to you think are worth to validate? How to e.g. validate |
I think we can look at did-jwt for a good example on configuration options, where you can pass:
In terms of issuer, i think it would be great to have a jwkResolver callback that will be called to fetch the public keys. It could be useful to also implement a default type metadata fetcher (which can be retrieved optionally during verification). Really like this pattern as it gives you full contril without having to understand everything. By default everything is verified, but you can disable certain verifications (i think being strict by default is very important for security as people |
I understand to validate credentials that are not in the present. But we need to be careful since statuslist does not allow to fetch older versions of a statuslist. So I can not check with this "was the credential valid 2 weeks ago". Your mentioned policies step sounds good to me. But as mentioned before I would love that I pass my rules to the verify function and the verify function returns the decoded payload or throws an error in case something went wrong. The seems to be the normal behavior when dealing with jwt libraries. |
I think we can provide separate validate function for SD-JWT. User can use it optionally. |
What if it throws a structured error where we can extract the validation result? I'm fine with it being an error, the problem is the library knows which validations succeeded / failed. If it just throws an error message it means we have to redo all validations to find out what failed / succeeded, at which point it's easier to use a custom/different SD-JWT VC implementation. I'm hoping we can accommodate both our requirements, but we're pretty set on the requirement that we need to know which validations succeeded/failed. |
According to the docs the As discussed in another issue we want to define the error message as constants, allowing you to handle errors like:
This of course forces us to define good error messages so the developer knows what failed. For use cases like API endpoint validation a parallel validation of the values in the object makes sense, so the sender can fix all problems in the next try. But I do not see this case when dealing with JWT or SD-JWTs. So is returning detailed and unique error messages fine for your or do you need explicit to have a report of all validations like when running a unit-test? I would not like to open the box which validations in the function should run in sequence and which in parallel. Like when the signature is wrong, I would like to stop the validation since I can not trust the payload inside it. |
I was thinking a custom error class, with an instanceof check we can then check if it's a validation error and then extract e.g. validations property. Parsing error messages seems more prone to errors. |
Re the validations in sequence vs parallel, maybe we should split this up:
We would use the multiple lower level functions to build our validation object, you can use the top-level validation function So then the parallel vs sequential isn't a problem. We just want to provide helpful error messages in Paradym (our SSI developer platform) for the SD-JWT validation. One thing we see as helpful is not just an error, but a structured error where you can use the result of every check separately. This way users can look at the validation result as a whole (isValid) or look at why the validation failed (and this can have multiple reasons, not just one). But I think we'll just go a level deeper and use the lower level utilities from this library to build our own validation pipeline, maybe our requirements are too custom |
How about adding a new verification function like We would also not break current implementations or have breaking changes. Could this approach work for you? This would also allow us to compare different usages and maybe later drop the current approach when everyone is favouring yours 😉 |
Yes, a new method sounds good to me :) |
Just wanted to also link to section 13. Of this new revocation spec, that outlines why checking revocation is a decision made on the business layer: https://peppelinux.github.io/draft-demarco-oauth-status-attestations/draft-demarco-oauth-status-attestations.html#name-considerations-on-revocatio |
Yes, revocation check should be done via a flag, we already have this for the key binding. We only have to decide the default value, I would suggest to validate if it's present, but it can be skipped by configuration (security by design) |
Such as the
exp
,iss
, et.. fields. I would expect these fields to be validated when using an SD-JWT library.It seems that verifying an SD-JWT VC mainly focuses on verifying the signature (and now also the status list), but no the fields in the SD-JWT VC itself.
Is that intended with this library? Is the main focus SD-JWT and not SD-JWT VC?
The text was updated successfully, but these errors were encountered: