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.

Sign up for GitHub

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

Jump to bottom

Feature/14/14 web context (with ADR) #14

Merged
merged 9 commits into from
Jun 30, 2022
Merged

Feature/14/14 web context (with ADR) #14

merged 9 commits into from
Jun 30, 2022

Conversation

algattik
Copy link

@algattik algattik commented Jun 29, 2022 •
edited
Loading

What this PR changes/adds

This is an intermediate PR for Registry Service JWS validation (agera-edc/MinimumViableDataspace#14).

Deploys the Registration Service controller to a custom Jersey context (port mapping), rather than the EDC default context.

Why it does that

In the next PR we want to enforce JWS authentication for the Registration Service controller, using a JAX-RS filter.

However for docker health check (used in docker-compose up --wait in CI to wait until containers have successfully started), we use curl to access the health endpoint, which is deployed in the EDC default context. Therefere, we will want to apply our JWS authentication filter to the default context.

It is anyway good practice not to expose health and management endpoints to public API.

Further notes

Linked Issue(s)

agera-edc/MinimumViableDataspace#14

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • added relevant details to the changelog? (skip with label no-changelog)
  • formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

@github-actions
Copy link

github-actions bot commented Jun 29, 2022 •
edited
Loading

Unit Test Results

  7 files  ±0    7 suites  ±0   1s ⏱️ -1s
19 tests ±0  19 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
27 runs  ±0  27 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 39471c3. ± Comparison against base commit a75c1c8.

♻️ This comment has been updated with latest results.

@algattik algattik changed the title Feature/14/14 web context Feature/14/14 web context (with ADR) Jun 29, 2022
@algattik algattik marked this pull request as ready for review June 29, 2022 06:39
@chrislomonico chrislomonico requested a review from cpeeyush June 30, 2022 08:41
cpeeyush
cpeeyush reviewed Jun 30, 2022
View reviewed changes
docs/developer/decision-records/2022-06-29-http-ports/README.md Outdated

However, for docker health check (used in `docker-compose up --wait` in CI to wait until containers have successfully started), we use `curl` to access the health endpoint, which is deployed in the EDC default context. Therefore, we do not want to apply our authentication filter to the `default` context, and need to introduce an additional context for the API controller.

It is also good practice not to expose health and management endpoints to public API.
Copy link

Choose a reason for hiding this comment

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

nit: not to expose health endpoint to public Do you mean that we need authentication on health endpoint as well ? Because from the doc it seems like we said health endpoint is in default context and there we do not want to apply authentication.

Copy link
Author

Choose a reason for hiding this comment

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

No, by putting it on a different port we can have that port not accessible in deployment

Copy link
Author

Choose a reason for hiding this comment

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

Clarified sentence

cpeeyush
cpeeyush approved these changes Jun 30, 2022
View reviewed changes
Copy link

@cpeeyush cpeeyush left a comment

Choose a reason for hiding this comment

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

Approved with one question to understand ADR doc better.

@algattik algattik merged commit b54edd8 into feature/14-verify-jws Jun 30, 2022
@algattik algattik deleted the feature/14/14-web-context branch June 30, 2022 14:09
@algattik algattik mentioned this pull request Jul 2, 2022
Merged
7 tasks
cpeeyush pushed a commit that referenced this pull request Jul 18, 2022
@algattik @cpeeyush
Feature/14/14 web context (with ADR) (#14)
cb3dee3
Izzzu added a commit that referenced this pull request Jul 19, 2022
@cpeeyush @algattik
@Izzzu @ouphi @marcgs
feat(auth): Registration Service API is authenticated with JWS (#11)
cde6ee3 
* Feature/14/14 upgrade edc (#12)

* Feature/14/14 web context (with ADR) (#14)

* Feature/14/14 participant did (#13)

* Feature/14/14 verify JWS (with ADR) (#11)

* .

* .

* .

* .

* Update ParticipantsCommandTest.java

* Update ParticipantManager.java

* Update ParticipantManager.java

* Update action.yml

* Update verify.yaml

* Update JsonWebSignatureHeaderInterceptor.java

* .

* .

* .

* .

* .

* .

* .

* .

* .

* .

* .

* Update RegistrationApiClientTest.java

* .

* .

* Update Dockerfile

* Update RegistrationServiceCli.java

* Update RegistrationApiClientTest.java

* Update RegistrationServiceExtension.java

* Update RegistrationApiCommandLineClientTest.java

* Create README.md

* Update README.md

* Renamed executeParticipantsAdd to executeParticipantsList

* add comments for headers

* Update RegistrationApiClientTest.java

* Squashed commit of the following:

commit a8cf714
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Wed Jun 29 22:02:52 2022 +0200

    Update RegistrationApiClientTest.java

commit 4decadc
Merge: b537c9b 39471c3
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Wed Jun 29 21:56:59 2022 +0200

    Merge branch 'feature/14/14-web-context' into feature/14/14-verify-jws

commit b537c9b
Merge: 4284f1e 830cc32
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Wed Jun 29 21:54:43 2022 +0200

    Merge branch 'feature/14/14-participant-did' into feature/14/14-verify-jws

commit 4284f1e
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Wed Jun 29 07:42:10 2022 +0200

    .

commit df33c85
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Wed Jun 29 07:36:48 2022 +0200

    .

commit e0a92f1
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Wed Jun 29 07:00:27 2022 +0200

    .

commit 758839b
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 18:28:06 2022 +0200

    .

commit b68e5f4
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 17:49:38 2022 +0200

    .

commit da1682b
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 17:45:30 2022 +0200

    .

commit f486517
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 17:42:17 2022 +0200

    .

commit a0f871e
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 16:06:04 2022 +0200

    .

commit ce92919
Merge: 8644edb a75c1c8
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 16:01:13 2022 +0200

    Merge branch 'feature/14-verify-jws' into feature/14/14-verify-jws

commit 8644edb
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 15:43:41 2022 +0200

    .

commit 357a195
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 15:00:07 2022 +0200

    Update JsonWebSignatureHeaderInterceptor.java

commit 5e80400
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 14:49:06 2022 +0200

    Update verify.yaml

commit 1df6120
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 14:48:44 2022 +0200

    Update action.yml

commit 5a7d4b2
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 14:48:11 2022 +0200

    Update ParticipantManager.java

commit cb2ea94
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 14:48:11 2022 +0200

    Update ParticipantManager.java

commit c2db217
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 14:44:19 2022 +0200

    Update ParticipantsCommandTest.java

commit e6a7b37
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Tue Jun 28 13:48:41 2022 +0200

    .

commit 68fbcd3
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Wed Jun 22 23:07:00 2022 +0200

    .

commit d28d030
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Wed Jun 22 22:57:37 2022 +0200

    .

commit 6049d05
Author: Alexandre Gattiker <algattik@microsoft.com>
Date:   Wed Jun 22 22:57:31 2022 +0200

    .

* .

* .

* .

* .

* Create README.md

* Update README.md

* Update README.md

* Make integration in EDC easier

* Update docs/developer/decision-records/2022-07-01-service-authentication/README.md

Co-authored-by: Izabela Kulakowska <ikulakowska@microsoft.com>

* Update DidJwtAuthenticationFilter.java

* Improve exception messages on auth failure

* Split up method logic

* Update name

* Use double quotes for Boolean var in docker compose yaml to avoid error

* Update docs/developer/decision-records/2022-07-01-service-authentication/README.md

Co-authored-by: Ophélie Le Mentec <17216799+ouphi@users.noreply.github.com>

Co-authored-by: Izabela Kulakowska <ikulakowska@microsoft.com>
Co-authored-by: Peeyush Chandel <555114+cpeeyush@users.noreply.github.com>
Co-authored-by: Ophélie Le Mentec <17216799+ouphi@users.noreply.github.com>

* Move artifact version in config

* Variable renaming for clarity

* Externalize verbose error response config

* PR feedback

Co-authored-by: Alexandre Gattiker <algattik@users.noreply.github.com>
Co-authored-by: Izabela Kulakowska <ikulakowska@microsoft.com>
Co-authored-by: Ophélie Le Mentec <17216799+ouphi@users.noreply.github.com>
Co-authored-by: Marc Gomez <marcgomez@microsoft.com>
@cpeeyush cpeeyush mentioned this pull request Jul 27, 2022
Merged
7 tasks
algattik pushed a commit that referenced this pull request Jul 27, 2022
@paullatzelsperger
feat(build): add signing config (#14)
8144715 
* add signing config

* added snapshot repo

* updated version conditionals
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Izzzu Izzzu Izzzu approved these changes

@cpeeyush cpeeyush cpeeyush approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@algattik @cpeeyush @Izzzu