artefactual-sdps / enduro

Designed to automate the processing of transfers in multiple Archivematica pipelines.
https://enduro.readthedocs.io/
Apache License 2.0
4 stars 3 forks source link

Add optional access control to API #965

Closed jraddaoui closed 6 days ago

jraddaoui commented 1 week ago

Refs #957.

Use Keycloak instead of Dex in dev env

Dex allowed us to implement OIDC authentication in front of an OpenLDAP instance in the development environment. However, it does not provide the IAM tools to manage access control in Enduro.

Add optional access control to API

Optionally enable Attribute Based Access Control (ABAC) in the API. When enabled, the API will look for a custom claim in the access token and extract the attributes relevant to Enduro. If a custom scope needs to be requested to get that claim, it has to be configured in the dashboard.

TODO

Follow-up work that may or may not happen in this pull request.

Note: moving to a BFF auth. flow could simplify some of these changes and documentation, but that won't happen in this PR.

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 85.71429% with 15 lines in your changes missing coverage. Please review.

Project coverage is 53.06%. Comparing base (07ac6f6) to head (673c572).

Files Patch % Lines
internal/api/auth/token_verifier.go 91.11% 3 Missing and 1 partial :warning:
cmd/enduro-a3m-worker/main.go 0.00% 3 Missing :warning:
cmd/enduro-am-worker/main.go 0.00% 3 Missing :warning:
cmd/enduro/main.go 0.00% 3 Missing :warning:
internal/api/auth/claims.go 91.30% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #965 +/- ## ========================================== + Coverage 52.16% 53.06% +0.90% ========================================== Files 100 101 +1 Lines 5590 5661 +71 ========================================== + Hits 2916 3004 +88 + Misses 2428 2408 -20 - Partials 246 249 +3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jraddaoui commented 1 week ago

@djjuhasz @mcantelon @sevein @sbreker

While this is still a work in progress and I'm working in a document explaining the existing OIDC implementation, I think there is enough information in this PR description and in code for you to see where this is going and provide some initial feedback.

I'd appreciate if you can take a look when you have some time. It will be better if you review each commit individually, they have a detailed message (the same as in the PR description), and also check the TODO section. Thanks!

jraddaoui commented 1 week ago

Thanks @djjuhasz! I addressed most of your feedback, I'll ping you for another review after adding all the scopes we discussed yesterday and some of the dashboard TODOs.

sbreker commented 6 days ago

This looks amazing @jraddaoui! :star_struck: I do not have anything to add beyond the others' comments. Thanks for your efforts with this!

I agree that follow-on work might work best as a second PR after this is merged...

jraddaoui commented 6 days ago

Thanks @sevein and @sbreker!

I think I have addressed all feedback and added a few more commits. I'll leave it disabled for now until we add the dashboard side and the documentation, which as you said will happen in another PR.

djjuhasz commented 6 days ago

@jraddaoui I tried testing the authentication out locally, but I'm getting an "NS_ERROR_UNKNOWN_HOST" error from keycloak when I click the Enduro Dashboard "Sign In" button. The only config change I made was to change api.auth.oidc.abac.enabled = true in "enduro.toml".

djjuhasz commented 6 days ago

@jraddaoui nevermind, I forgot to add keycloak to my /etc/hosts. RTFM moment. :facepalm:

djjuhasz commented 6 days ago

After updating my /etc/hosts I logged into the Enduro Dashboard fine with the "readonly" account, and I navigated around the read pages without any issues. I also logged into the Temporal UI and MinIO without any issues.

When I tried to process a SIP, I got an error in the upload activity though:

{
  "message": "[storage create]: invalid value expected *storage.CreatePayload, got &{79dc514f-d42e-40b4-a17b-2c2cc325f88f ZippedBag.zip <nil>}",
  "source": "GoSDK",
  "applicationFailureInfo": {
    "type": "ClientError"
  }
}

It's possible the error is unrelated to your changes, but I've never seen it before.

jraddaoui commented 6 days ago

Thanks @djjuhasz! That's totally related to these changes, I fixed that issue in the tests, but I thought the service was created differently in the main functions. Will fix!

jraddaoui commented 6 days ago

No, it doesn't. That was just a cosmetic commit:

Re-order API design methods in storage service

  • Keep locations and packages methods together.
  • Move submit to indicate update is related to that method and not to create.