AGENT-949 AGENT-951: Add new authz handler and swagger security defn. #6784

pawanpinjarkar · 2024-09-19T18:56:05Z

The agent-based installer needs a new security definition to perform the read-only operations for the user persona watcher. The agent-based installer commands such as wait-for and monitor internally use the read-only persona watcher which can only make read requests to the endpoints annotated with watcherAuth security definition.

Other existing authenticators do not support this agent-based installer specific watcherAuth security definition.

Read and set USER_AUTH_TOKEN
Implememnt a new authorization handler agent_local_authz_handler
Implement new security definition AuthWatcherAuth

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
Tested locally with dev-scripts using the installer PR AGENT-950: Implement Separate JWT Tokens for Different User Personas installer#9039
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci · 2024-09-19T18:56:25Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-10-08T23:09:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pawanpinjarkar
Once this PR has been reviewed and has the lgtm label, please assign pastequo for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2024-10-08T23:16:56Z

pawanpinjarkar · 2024-10-08T23:19:07Z

/cc @carbonin

codecov · 2024-10-09T00:05:45Z

Codecov Report

Attention: Patch coverage is 27.86885% with 44 lines in your changes missing coverage. Please review.

Project coverage is 68.64%. Comparing base (26185f1) to head (368eee3).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/auth/agent_local_authz_handler.go	0.00%	34 Missing ⚠️
pkg/auth/authz.go	58.33%	5 Missing ⚠️
pkg/auth/none_authenticator.go	0.00%	3 Missing ⚠️
pkg/auth/rhsso_authenticator.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6784      +/-   ##
==========================================
- Coverage   68.71%   68.64%   -0.08%     
==========================================
  Files         249      250       +1     
  Lines       37440    37494      +54     
==========================================
+ Hits        25727    25737      +10     
- Misses       9414     9458      +44     
  Partials     2299     2299

Files with missing lines	Coverage Δ
pkg/auth/agent_local_authenticator.go	`56.86% <100.00%> (+3.67%)`	⬆️
pkg/auth/authenticator.go	`100.00% <ø> (ø)`
pkg/auth/local_authenticator.go	`50.64% <100.00%> (+1.31%)`	⬆️
pkg/auth/rhsso_authenticator.go	`50.90% <0.00%> (-0.63%)`	⬇️
pkg/auth/none_authenticator.go	`6.89% <0.00%> (-0.80%)`	⬇️
pkg/auth/authz.go	`70.58% <58.33%> (-29.42%)`	⬇️
pkg/auth/agent_local_authz_handler.go	`0.00% <0.00%> (ø)`

... and 2 files with indirect coverage changes

pawanpinjarkar · 2024-10-09T14:18:37Z

/test edge-subsystem-kubeapi-aws

pawanpinjarkar · 2024-10-09T23:35:22Z

/test edge-subsystem-kubeapi-aws

carbonin · 2024-10-16T20:44:20Z

So this looks okay, but is it really what you want to do?

As far as I can tell this just adds another way to provide the same token, so you're not really adding any additional security because someone with that token could provide it using a different header and still use other endpoints.

For this to actually achieve authorization you'd need to issue separate tokens that have some kind of claim that identifies them as read-only. Then in the token verification process somewhere ensure that a read-only token is only valid for the endpoints it's assigned to.

pawanpinjarkar · 2024-10-17T01:08:03Z

So this looks okay, but is it really what you want to do?

As far as I can tell this just adds another way to provide the same token, so you're not really adding any additional security because someone with that token could provide it using a different header and still use other endpoints.

For this to actually achieve authorization you'd need to issue separate tokens that have some kind of claim that identifies them as read-only. Then in the token verification process somewhere ensure that a read-only token is only valid for the endpoints it's assigned to.

yes, so my plan is to have the swagger changes from this PR. Then in a different PR, implement a new authz handler in assisted service for ABI and update the installer to create 3 seperate tokens. Code changes from all the 3 PRs then will do the intended operation.

carbonin · 2024-10-17T13:09:34Z

I think it would be fine to add the authz stuff in this same PR. It gives context that makes these changes worthwhile.

Also I wouldn't want to have to go back and change the swagger again if we run into issues later with the next patch.

openshift-ci-robot · 2024-10-21T15:01:52Z

openshift-ci-robot · 2024-10-21T16:42:18Z

@pawanpinjarkar: This pull request references AGENT-949 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

This pull request references AGENT-951 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

The agent-based installer needs a new security definition to perform the read-only operations for the user persona watcher. The agent-based installer commands such as wait-for and monitor internally uses the read-only persona watcher which can only make read requests to the endpoints annotated with watcherAuth security definition.

Other existing authenticators do not support this agent-based installer specific watcherAuth security definition.

Read 3 seperate auth tokens - AGENT_AUTH_TOKEN, USER_AUTH_TOKEN and WATCHER_AUTH_TOKEN

Implememnt a new authorization handler agent_local_authz_handler

Implement new security definition AuthWatcherAuth

List all the issues related to this PR

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

This change does not require a documentation update (docstring, docs, README, etc)

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

pkg/auth/agent_local_authz_handler.go

carbonin · 2024-10-22T18:57:07Z

pkg/auth/agent_local_authz_handler.go

+		return common.NewApiError(http.StatusInternalServerError, fmt.Errorf("claims error: %s", err))
+	}
+
+	authClaim, ok := claims["sub"].(string)


If this is meant to match the scheme I wouldn't use sub as it doesn't really fit and sub has a documented meaning (ref: https://www.rfc-editor.org/rfc/rfc7519.html#section-4.1.2)

You can review all public claim names at https://www.iana.org/assignments/jwt/jwt.xhtml#claims

I'd say make a claim called auth_scheme to be explicit here.

carbonin · 2024-10-22T18:58:11Z

pkg/auth/agent_local_authz_handler.go

+		return common.NewApiError(http.StatusBadRequest, fmt.Errorf("token missing authClaim or endpoint is missing authScheme"))
+	}
+	if authClaim != authScheme {
+		return common.NewInfraError(http.StatusForbidden, fmt.Errorf("authClaim %s is unauthorized to access. authScheme =%s", authClaim, authScheme))


I think this error is visible to the user so maybe make this a bit more user-friendly, or remove some of this debug-like info.

cmd/agentbasedinstaller/client/main.go

pkg/auth/agent_local_authz_handler.go

carbonin · 2024-10-22T20:13:12Z

swagger.yaml

+  watcherAuth:
+    type: apiKey
+    in: header
+    name: Watcher-Authorization

 security:
  - userAuth: [admin, user]


I also think this might be the slightly more idiomatic way of achieving what we're doing with an additional security definition, but I don't see much real upside to putting this in the roles rather than a separate definition.

If you want to investigate it, then go ahead, but I won't hold the PR up for it.

openshift-ci-robot · 2024-10-25T01:32:07Z

@pawanpinjarkar: This pull request references AGENT-949 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

This pull request references AGENT-951 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

The agent-based installer needs a new security definition to perform the read-only operations for the user persona watcher. The agent-based installer commands such as wait-for and monitor internally use the read-only persona watcher which can only make read requests to the endpoints annotated with watcherAuth security definition.

Other existing authenticators do not support this agent-based installer specific watcherAuth security definition.

Read and set USER_AUTH_TOKEN

Implememnt a new authorization handler agent_local_authz_handler

Implement new security definition AuthWatcherAuth

List all the issues related to this PR

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

This change does not require a documentation update (docstring, docs, README, etc)

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

- Read USER_AUTH_TOKEN - Implememnt a new authorization handler agent_local_authz_handler - Implement new security definition AuthWatcherAuth

…K8sAPI. Simplify claims parsing for authzr

openshift-ci-robot · 2024-10-28T18:59:42Z

@pawanpinjarkar: This pull request references AGENT-949 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

This pull request references AGENT-951 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

The agent-based installer needs a new security definition to perform the read-only operations for the user persona watcher. The agent-based installer commands such as wait-for and monitor internally use the read-only persona watcher which can only make read requests to the endpoints annotated with watcherAuth security definition.

Other existing authenticators do not support this agent-based installer specific watcherAuth security definition.

Read and set USER_AUTH_TOKEN

Implememnt a new authorization handler agent_local_authz_handler

Implement new security definition AuthWatcherAuth

List all the issues related to this PR

New Feature

Enhancement

Bug fix

Tests

Documentation

CI/CD

What environments does this code impact?

Automation (CI, tools, etc)

Cloud

Operator Managed Deployments

None

How was this code tested?

assisted-test-infra environment

dev-scripts environment

Reviewer's test appreciated

Waiting for CI to do a full test run

Manual (Elaborate on how it was tested)
Tested locally with dev-scripts using the installer PR AGENT-950: Implement Separate JWT Tokens for Different User Personas installer#9039

No tests needed

Checklist

Title and description added to both, commit and PR.

Relevant issues have been associated (see CONTRIBUTING guide)

This change does not require a documentation update (docstring, docs, README, etc)

Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?

Is there a bug required (and linked) for this change?

Should this PR be backported?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-10-30T05:20:49Z

@pawanpinjarkar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/edge-e2e-metal-assisted-mtv-4-17	`8dec3f8`	link	true	`/test edge-e2e-metal-assisted-mtv-4-17`
ci/prow/edge-lint	`368eee3`	link	true	`/test edge-lint`
ci/prow/e2e-agent-compact-ipv4	`368eee3`	link	true	`/test e2e-agent-compact-ipv4`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2024

openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. api-review Categorizes an issue or PR as actively needing an API review. labels Sep 19, 2024

pawanpinjarkar closed this Oct 3, 2024

pawanpinjarkar reopened this Oct 8, 2024

pawanpinjarkar changed the title ~~Add new security defn for abi~~ AGENT-951: Add new security defn for ABI Oct 8, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 8, 2024

pawanpinjarkar marked this pull request as ready for review October 8, 2024 23:16

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 8, 2024

openshift-ci bot requested review from ori-amizur and paul-maidment October 8, 2024 23:17

pawanpinjarkar force-pushed the add-new-security-defn-for-ABI branch from 0a96eb3 to 07ae8c0 Compare October 8, 2024 23:18

openshift-ci bot requested a review from carbonin October 8, 2024 23:19

pawanpinjarkar force-pushed the add-new-security-defn-for-ABI branch from 07ae8c0 to ef40248 Compare October 21, 2024 14:55

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 21, 2024

pawanpinjarkar changed the title ~~AGENT-951: Add new security defn for ABI~~ AGENT-949 AGENT-951: Add new authz handler and swagger security defn. Oct 21, 2024

pawanpinjarkar marked this pull request as draft October 21, 2024 15:03

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2024

pawanpinjarkar force-pushed the add-new-security-defn-for-ABI branch from ef40248 to ac696d0 Compare October 21, 2024 16:41

pawanpinjarkar force-pushed the add-new-security-defn-for-ABI branch 2 times, most recently from 6431f47 to 994bf61 Compare October 22, 2024 00:19

pawanpinjarkar marked this pull request as ready for review October 22, 2024 00:19

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 22, 2024

openshift-ci bot requested review from celebdor and linoyaslan October 22, 2024 00:20

pawanpinjarkar force-pushed the add-new-security-defn-for-ABI branch from 994bf61 to 8dec3f8 Compare October 22, 2024 14:53

carbonin reviewed Oct 22, 2024

View reviewed changes

pawanpinjarkar added 3 commits October 28, 2024 14:56

AGENT-949 AGENT-951: Add new authz handler and swagger secuity defn.

82d4021

- Read USER_AUTH_TOKEN - Implememnt a new authorization handler agent_local_authz_handler - Implement new security definition AuthWatcherAuth

review fixes

b932841

Update PayloadFromContext() for ABI to work like AI works in case of …

6404360

…K8sAPI. Simplify claims parsing for authzr

pawanpinjarkar force-pushed the add-new-security-defn-for-ABI branch from 9b5422d to 6404360 Compare October 28, 2024 18:57

pawanpinjarkar requested a review from carbonin October 28, 2024 19:00

pawanpinjarkar mentioned this pull request Oct 28, 2024

AGENT-950: Implement Separate JWT Tokens for Different User Personas openshift/installer#9039

Open

Initialize restAPI handler with newly added AuthWatcherAuth

368eee3

AGENT-949 AGENT-951: Add new authz handler and swagger security defn. #6784

Are you sure you want to change the base?

AGENT-949 AGENT-951: Add new authz handler and swagger security defn. #6784

Conversation

pawanpinjarkar commented Sep 19, 2024 • edited Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Sep 19, 2024

openshift-ci bot commented Oct 8, 2024

openshift-ci-robot commented Oct 8, 2024 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

pawanpinjarkar commented Oct 8, 2024

codecov bot commented Oct 9, 2024 • edited Loading

Codecov Report

pawanpinjarkar commented Oct 9, 2024

pawanpinjarkar commented Oct 9, 2024

carbonin commented Oct 16, 2024

pawanpinjarkar commented Oct 17, 2024

carbonin commented Oct 17, 2024

openshift-ci-robot commented Oct 21, 2024 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Oct 21, 2024 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

carbonin Oct 22, 2024

Choose a reason for hiding this comment

carbonin Oct 22, 2024

Choose a reason for hiding this comment

carbonin Oct 22, 2024

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 25, 2024 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Oct 28, 2024 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Oct 30, 2024

pawanpinjarkar commented Sep 19, 2024 •

edited

Loading

openshift-ci-robot commented Oct 8, 2024 •

edited by openshift-ci bot

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading

openshift-ci-robot commented Oct 21, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Oct 21, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Oct 25, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Oct 28, 2024 •

edited by openshift-ci bot

Loading