-
Notifications
You must be signed in to change notification settings - Fork 10
chore: Update BDRS to upstream 0.15.1 #338
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
Conversation
728d7e8 to
cd5c99c
Compare
9740de3 to
5c98899
Compare
|
Hi @ndr-brt , would be nice, if you could have a look, I hope, I did not miss a thing concerning the update to 0.15.x. The audience thing is a bit difficult, as the token used by a bdrs client contains for all three claims, issue, subject, and audience the callers did, at least for the audience I would have expected the bdrs operators did, but this is not really specified, as it is a heuristic, that the issuers did is also the did of the BDRS operator. I assume that was the reason for that decision. So I faked the audience check, as it uses the value missing some other source for the value. |
| .onFailure(f -> monitor.warning("Error validating BDRS client VP: %s".formatted(f.getFailureDetail()))) | ||
| .succeeded(); | ||
| } | ||
| String audience = getAudienceFromToken(token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the audience used to validate shouldn't be the one in the token, otherwise it will always pass, because the validator also extracts the audience from the token for comparison.
The audience passed should be the own did of the bdrs service, so the configured one.
All the parse logic here can be avoided then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem is, that this is not the case, see the way the token is generated in the connector:
var claims = Map.<String, Object>of(
JWT_ID, UUID.randomUUID().toString(),
ISSUER, ownDid,
SUBJECT, ownDid,
AUDIENCE, ownDid
);Source: BdrsClientImpl.java
My hypothesis is, that when the BDRS was created, this part was done in a rush and the setup with a proper DID on the BDRS side was overlooked. If you look at the charts, there is the following comment:
- name: "EDC_IAM_ISSUER_ID"
value: "required but not really needed"A EDC_PARTICIPANT_ID is not even required for the service.
The way, the authentication is handled in BDRS is not the target solution, imho, but it is as it is today, which means, that the audience is not really used properly. That is, why this code is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with your solution them
Signed-off-by: Lars Geyer-Blaumeiser <[email protected]>
Signed-off-by: Lars Geyer-Blaumeiser <[email protected]>
Signed-off-by: Lars Geyer-Blaumeiser <[email protected]>
Signed-off-by: Lars Geyer-Blaumeiser <[email protected]>
Signed-off-by: Lars Geyer-Blaumeiser <[email protected]>
a23a6b6 to
1c1eebe
Compare
WHAT
Update the BDRS repo to the latest upstream version 0.15.1.
WHY
To stay with upstream development
To fix the openapi issue which is based on a mismatch between the autodoc version used and the latest upstream workflows in use for openapi creation.
Closes #325