Skip to content

Conversation

@chibenwa
Copy link
Contributor

No description provided.

Copy link
Contributor

@felixauringer felixauringer left a comment

Choose a reason for hiding this comment

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

I have not built it myself but the code looks good. I like that aud validation is now part of the usual verification flow.

There is still an additional check in the introspection case here. Do you think that one is still necessary? I think the new check from this PR is also used in the introspection case as part of the local validation anyway.

@chibenwa
Copy link
Contributor Author

Thanks @felixauringer for the review I pushed a little commit to polish the doc

@felixauringer
Copy link
Contributor

Thanks @felixauringer for the review I pushed a little commit to polish the doc

Thanks, the docs look good to me now 🙂

I am still unsure about the double aud validation mentioned above.

@chibenwa
Copy link
Contributor Author

I am still unsure about the double aud validation mentioned above.

I fear I do not get you. What do you mean?

@felixauringer
Copy link
Contributor

When using introspection, there are currently two places with aud checks:

I am not sure whether the second is still needed. As far as I see it, the signature verification - which now also includes the aud check - is done in every code path anyway.

@chibenwa
Copy link
Contributor Author

I am not sure whether the second is still needed. As far as I see it, the signature verification - which now also includes the aud check - is done in every code path anyway.

Fair

@chibenwa
Copy link
Contributor Author

While I'm at it it seems less relevant to mandate introspect.

Would you agree relaxing it @felixauringer ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants