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.

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

Add optional access control to API #965

Merged
merged 2 commits into from
Jun 25, 2024
Merged

Add optional access control to API #965

merged 2 commits into from
Jun 25, 2024

Conversation

jraddaoui
Copy link
Collaborator

@jraddaoui jraddaoui commented Jun 18, 2024

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.

  • Remove Dex and OpenLDAP from the Kubernetes environment.
  • Add Keycloak to the environment:
    • Use development server.
    • Listen in 7470 to avoid collisions.
    • Import a custom enduro realm with:
      • Clients for Enduro, Temporal and MinIO.
      • Minimal OIDC built-in scopes.
      • Custom client scopes for Enduro and Minio.
      • Custom user attributes and claims mapped to those scopes.
      • Add three default users with different sets of attributes.
    • Without persistence to recreate the custom realm on each start.
    • Update docs to use keycloack instead of dex in /etc/hosts.
    • Update access section to indicate new credentials and service.
  • Set up MinIO and Temporal to SSO with Keycloack.
  • Update default Enduro configuration to work with Keycloack.
  • Remove offline_access from the requested scopes (a following
    commit/PR will make scopes configurable).
  • Update Pulumi project and related Github action.
  • Add TODO note in dashboard to end sessions in the provider.

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.

  • Add OIDC ABAC configuration. Allows to enable or disable access
    control entirely and set a claim to get the attributes. That claim
    could be nested and include values unrelated to Enduro.
  • Extend claims and token verifier to parse ABAC attributes from the
    access token based on configuration.
  • Allow custom scopes in dashboard and configure it to work with the
    Keycloak instance for the dev. env. by default.
  • Add scopes, forbidden error and security to all endpoints in Goa's
    API design.
  • Check scopes against attributes in API requests. As a first access
    control check, the scopes configured on the API design need to be
    included in the user attributes.
    • This check allows attributes with wildcards, so:
      • package:read will only provide access to that action(s).
      • package:* will provide access to all package related actions.
      • * will provide full access to all actions.
  • Add claims with the extracted attributes to the context on each
    request. After that initial check is passed, the claims are included
    in the context to make them available for other possible access
    control checks on the endpoints implementation.
  • 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.

TODO

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

  • Determine the attributes used for each endpoint and user action [done].
  • Consider access control in the dashboard:
    • Add similar configuration and attribute extraction.
    • Add similar checks to hide/show elements/pages.
    • Consider websocket connection and access control (disable it for
      now when access control is enabled) [done].
    • Add landing page for users without the required attributes. Right
      now the landing page is the packages list.
  • Add administration section to the documentation:
    • Add notes to configure OIDC ABAC in the API and the dashboard,
      explaining the current behavior.
    • Add notes to build and serve the dashboard, considering that
      configuration.
    • Document API, including the attributes/scopes needed.

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

@jraddaoui jraddaoui self-assigned this Jun 18, 2024
@jraddaoui jraddaoui force-pushed the dev/issue-957-abac branch from 8e4b23b to 85d8ff1 Compare June 19, 2024 14:23
Copy link

codecov bot commented Jun 19, 2024

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).
Report is 41 commits behind head on main.

Files with missing lines Patch % Lines
internal/api/auth/token_verifier.go 91.11% 3 Missing and 1 partial ⚠️
cmd/enduro-a3m-worker/main.go 0.00% 3 Missing ⚠️
cmd/enduro-am-worker/main.go 0.00% 3 Missing ⚠️
cmd/enduro/main.go 0.00% 3 Missing ⚠️
internal/api/auth/claims.go 91.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jraddaoui jraddaoui force-pushed the dev/issue-957-abac branch from 85d8ff1 to 29a32bb Compare June 19, 2024 14:29
@jraddaoui
Copy link
Collaborator Author

@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!

Copy link
Collaborator

@djjuhasz djjuhasz left a comment

Choose a reason for hiding this comment

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

@jraddaoui this looks great to me, thanks for all the work! 🙇 I like the way ABAC rules are specified and checked, and adding access attributes seems easy now that you've set up the system.

I added a bunch of questions and some minor suggestions, but nothing that I see as a blocker.

.github/workflows/pulumi.yml Show resolved Hide resolved
Tiltfile Show resolved Hide resolved
enduro.toml Outdated Show resolved Hide resolved
enduro.toml Outdated Show resolved Hide resolved
enduro.toml Outdated Show resolved Hide resolved
internal/api/auth/config.go Show resolved Hide resolved
internal/api/auth/token_verifier.go Outdated Show resolved Hide resolved
internal/api/design/package_.go Show resolved Hide resolved
internal/package_/goa.go Outdated Show resolved Hide resolved
internal/upload/service.go Outdated Show resolved Hide resolved
@jraddaoui
Copy link
Collaborator Author

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.

@jraddaoui jraddaoui force-pushed the dev/issue-957-abac branch from 401b521 to 9197388 Compare June 20, 2024 17:53
Copy link
Contributor

@sevein sevein left a comment

Choose a reason for hiding this comment

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

Looks good! I've added a few minor observations.

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

I'm leaning towards "may not" here. Smaller pull requests are generally better!

internal/api/auth/claims.go Outdated Show resolved Hide resolved
internal/api/auth/config.go Outdated Show resolved Hide resolved
internal/api/auth/token_verifier.go Outdated Show resolved Hide resolved
internal/api/auth/token_verifier.go Outdated Show resolved Hide resolved
internal/api/auth/token_verifier_test.go Outdated Show resolved Hide resolved
internal/package_/goa.go Show resolved Hide resolved
@sbreker
Copy link
Contributor

sbreker commented Jun 24, 2024

This looks amazing @jraddaoui! 🤩 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 jraddaoui force-pushed the dev/issue-957-abac branch 2 times, most recently from 5b0cf03 to 8eb2d4d Compare June 24, 2024 20:27
@jraddaoui
Copy link
Collaborator Author

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.

@jraddaoui jraddaoui requested review from djjuhasz and sevein June 24, 2024 20:31
Copy link
Collaborator

@djjuhasz djjuhasz left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@djjuhasz
Copy link
Collaborator

@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
Copy link
Collaborator

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

@djjuhasz
Copy link
Collaborator

djjuhasz commented Jun 25, 2024

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
Copy link
Collaborator Author

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!

Copy link
Collaborator

@djjuhasz djjuhasz left a comment

Choose a reason for hiding this comment

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

Does the order you add the endpoints to the client effect authentication?

@jraddaoui
Copy link
Collaborator Author

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.

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.

- Remove Dex and OpenLDAP from the Kubernetes environment.
- Add Keycloak to the environment:
  - Use development server.
  - Listen in `7470` to avoid collisions.
  - Import a custom `enduro` realm with:
    - Clients for Enduro, Temporal and MinIO.
    - Minimal OIDC built-in scopes.
    - Custom client scopes for Enduro and Minio.
    - Custom user attributes and claims mapped to those scopes.
    - Add three default users with different sets of attributes.
  - Without persistence to recreate the custom realm on each start.
  - Update docs to use `keycloack` instead of `dex` in `/etc/hosts`.
  - Update access section to indicate new credentials and service.
- Set up MinIO and Temporal to SSO with Keycloack.
- Update default Enduro configuration to work with Keycloack.
- Remove `offline_access` from the requested scopes (a following
  commit/PR will make scopes configurable).
- Update Pulumi project and related Github action.
- Add TODO note in dashboard to end sessions in the provider.
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.

- Add OIDC ABAC configuration. Allows to enable or disable access
  control entirely and set a claim to get the attributes. That claim
  could be nested and include values unrelated to Enduro.
- Extend claims and token verifier to parse ABAC attributes from the
  access token based on configuration.
- Allow custom scopes in dashboard and configure it to work with the
  Keycloak instance for the dev. env. by default.
- Add scopes, forbidden error and security to all endpoints in Goa's
  API design.
- Check scopes against attributes in API requests. As a first access
  control check, the scopes configured on the API design need to be
  included in the user attributes.
  - This check allows attributes with wildcards, so:
    - `package:read` will only provide access to that action(s).
    - `package:*` will provide access to all package related actions.
    - `*` will provide full access to all actions.
- Add claims with the extracted attributes to the context on each
  request. After that initial check is passed, the claims are included
  in the context to make them available for other possible access
  control checks on the endpoints implementation.
- 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`.
@jraddaoui jraddaoui force-pushed the dev/issue-957-abac branch from c7322c3 to 673c572 Compare June 25, 2024 16:27
@jraddaoui jraddaoui marked this pull request as ready for review June 25, 2024 16:28
@jraddaoui jraddaoui merged commit 673c572 into main Jun 25, 2024
15 checks passed
@jraddaoui jraddaoui deleted the dev/issue-957-abac branch June 25, 2024 16:30
@jraddaoui jraddaoui changed the title WIP: Add access control to API and dashboard Add optional access control to API Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants