AGENT-869: Implement a new auth type for ABI #6174

pawanpinjarkar · 2024-04-10T16:11:35Z

Introduced a new auth type 'agent-installer-local' for agent based installer
This new auth type is mostly similar to the local auth type
agent based installer generates the necessary keys and JWT tokens and
pass it to assisted service.
Note: When generating an agent ISO, the Agent installer creates ECDSA public/private keys. However, the systemd services of the Agent installer fail to parse multiline keys accurately. To address this, the keys are encoded in base64 format to condense them into a single line before being transmitted to the assisted service. Upon reception, the assisted service decodes these keys back to their original multiline format for subsequent processing.

List all the issues related to this PR

What environments does this code impact?

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

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)
The changes are tested with other relevant changes from agent based installer. The cluster was successfully installed with authentication setup end to end.
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-robot · 2024-04-10T16:11:38Z

@pawanpinjarkar: This pull request references AGENT-869 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.16.0" version, but no target version was set.

In response to this:

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.

pawanpinjarkar · 2024-05-10T20:14:45Z

/hold

codecov · 2024-05-10T20:38:24Z

Codecov Report

Attention: Patch coverage is 55.55556% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 68.26%. Comparing base (fdf233a) to head (ff6deee).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6174      +/-   ##
==========================================
- Coverage   68.27%   68.26%   -0.01%     
==========================================
  Files         241      242       +1     
  Lines       35873    35925      +52     
==========================================
+ Hits        24493    24525      +32     
- Misses       9215     9232      +17     
- Partials     2165     2168       +3

Files	Coverage Δ
internal/bminventory/inventory_v2_handlers.go	`56.60% <100.00%> (ø)`
internal/cluster/auth.go	`78.12% <100.00%> (ø)`
pkg/auth/authenticator.go	`100.00% <100.00%> (ø)`
pkg/auth/agent_local_authenticator.go	`52.00% <52.00%> (ø)`

... and 2 files with indirect coverage changes

pawanpinjarkar · 2024-05-13T14:02:21Z

/hold cancel

openshift-ci-robot · 2024-05-13T14:04:45Z

@pawanpinjarkar: This pull request references AGENT-869 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.16.0" version, but no target version was set.

In response to this:

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 (Agent based installer)

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)
The changes are tested with other relevant changes from agent based installer. The cluster was successfully installed with authentication setup end to end.

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-robot · 2024-05-13T14:05:06Z

@pawanpinjarkar: This pull request references AGENT-869 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.16.0" version, but no target version was set.

In response to this:

Introduced a new auth type 'agent-installer-local' for agent based installer

This new auth type is mostly similar to the local auth type

agent based installer generates the necessary keys and JWT tokens and
pass it to assisted service.

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 (Agent based installer)

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)
The changes are tested with other relevant changes from agent based installer. The cluster was successfully installed with authentication setup end to end.

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.

pawanpinjarkar · 2024-05-13T14:05:09Z

/hold cancel

pawanpinjarkar · 2024-05-13T14:05:18Z

/cc @carbonin

carbonin

Please add tests for the new authenticator.

cmd/agentbasedinstaller/client/main.go

cmd/main.go

internal/controller/controllers/agent_controller.go

internal/host/hostcommands/download_boot_artifacts_cmd.go

pkg/auth/agent_local_authenticator.go

openshift-ci · 2024-05-14T22:28:53Z

[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 ask for approval from carbonin. 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

- Introduced a new auth type 'agent-installer-local' for agent based installer - This new auth type is mostly similar to the local auth type - agent based installer generates the necessary keys and JWT tokens and pass it to assisted service.

openshift-ci-robot · 2024-05-16T12:43:44Z

@pawanpinjarkar: This pull request references AGENT-869 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.16.0" version, but no target version was set.

In response to this:

Introduced a new auth type 'agent-installer-local' for agent based installer

This new auth type is mostly similar to the local auth type

agent based installer generates the necessary keys and JWT tokens and
pass it to assisted service.

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 (Agent based installer)

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)
The changes are tested with other relevant changes from agent based installer. The cluster was successfully installed with authentication setup end to end.

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-robot · 2024-05-16T12:48:25Z

@pawanpinjarkar: This pull request references AGENT-869 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.16.0" version, but no target version was set.

In response to this:

Introduced a new auth type 'agent-installer-local' for agent based installer

This new auth type is mostly similar to the local auth type

agent based installer generates the necessary keys and JWT tokens and
pass it to assisted service.

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 (Agent based installer)

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)
The changes are tested with other relevant changes from agent based installer. The cluster was successfully installed with authentication setup end to end.

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.

pawanpinjarkar · 2024-05-16T22:30:48Z

/retest-required

openshift-ci-robot · 2024-05-16T22:32:19Z

@pawanpinjarkar: This pull request references AGENT-869 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.16.0" version, but no target version was set.

In response to this:

Introduced a new auth type 'agent-installer-local' for agent based installer

This new auth type is mostly similar to the local auth type

agent based installer generates the necessary keys and JWT tokens and
pass it to assisted service.

Note: When generating an agent ISO, the Agent installer creates ECDSA public/private keys. However, the systemd services of the Agent installer fail to parse multiline keys accurately. To address this, the keys are encoded in base64 format to condense them into a single line before being transmitted to the assisted service. Upon reception, the assisted service decodes these keys back to their original multiline format for subsequent processing.

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 (Agent based installer)

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)
The changes are tested with other relevant changes from agent based installer. The cluster was successfully installed with authentication setup end to end.

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-05-17T01:17:51Z

@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	`ff6deee`	link	true	`/test edge-e2e-metal-assisted`
ci/prow/e2e-agent-compact-ipv4	`ff6deee`	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.

carbonin · 2024-05-22T14:27:57Z

internal/bminventory/inventory_v2_handlers.go

@@ -756,7 +756,7 @@ func (b *bareMetalInventory) GetInfraEnvDownloadURL(ctx context.Context, params

 func (b *bareMetalInventory) generateShortImageDownloadURL(infraEnvID, imageType, version, arch, imageTokenKey string) (string, *strfmt.DateTime, error) {
 switch b.authHandler.AuthType() {
- case auth.TypeLocal:
+ case auth.TypeLocal, auth.TypeAgentLocal:


Are you ever using the download URL we generate?

carbonin · 2024-05-22T14:28:59Z

internal/cluster/auth.go

@@ -30,7 +30,7 @@ func AgentToken(resource interface{}, authType auth.AuthType) (token string, err
 switch authType {
 case auth.TypeRHSSO:
 token, err = cloudPullSecretToken(pullSecret)
- case auth.TypeLocal:
+ case auth.TypeLocal, auth.TypeAgentLocal:


This is also only ever used when building the discovery ISO. Are you guys using that?

carbonin · 2024-05-22T14:31:38Z

pkg/auth/agent_local_authenticator.go

+ log.WithError(err).Fatal("Error decoding private key:")
+ }
+ cfg.ECPrivateKeyPEM = string(decodedECPrivateKeyPEM)
+ os.Setenv("EC_PRIVATE_KEY_PEM", string(decodedECPrivateKeyPEM))


I really don't think this is a good idea.

It looks like you may not actually need the private key at all since you're creating all of the tokens you need from outside the application.

Maybe just don't bother with the private key?

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

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 10, 2024

pawanpinjarkar force-pushed the agent-installer-new-auth-type branch from afb8392 to 34ad1f1 Compare April 11, 2024 00:55

pawanpinjarkar force-pushed the agent-installer-new-auth-type branch 3 times, most recently from e206f44 to 3940b8e Compare May 10, 2024 19:52

pawanpinjarkar marked this pull request as ready for review May 10, 2024 20:14

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 May 10, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 10, 2024

openshift-ci bot requested review from javipolo and rwsu May 10, 2024 20:15

pawanpinjarkar force-pushed the agent-installer-new-auth-type branch from 3940b8e to 3ba8f9d Compare May 13, 2024 14:01

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2024

openshift-ci bot requested a review from carbonin May 13, 2024 14:05

pawanpinjarkar mentioned this pull request May 13, 2024

AGENT-871: Authenticate wait for openshift/installer#8270

Open

pawanpinjarkar force-pushed the agent-installer-new-auth-type branch 2 times, most recently from a586922 to a69d6a9 Compare May 13, 2024 17:06

carbonin requested changes May 13, 2024

View reviewed changes

openshift-ci bot assigned carbonin May 13, 2024

zaneb mentioned this pull request May 15, 2024

AGENT-875: Authenticate agents openshift/installer#8395

Open

pawanpinjarkar force-pushed the agent-installer-new-auth-type branch from 773090e to 188119a Compare May 16, 2024 12:41

pawanpinjarkar force-pushed the agent-installer-new-auth-type branch 2 times, most recently from 2a0485c to c8be83f Compare May 16, 2024 17:28

Review fixes. Added unit tests.

ff6deee

pawanpinjarkar force-pushed the agent-installer-new-auth-type branch from c8be83f to ff6deee Compare May 16, 2024 19:18

pawanpinjarkar requested a review from carbonin May 16, 2024 19:18

This was referenced May 16, 2024

AGENT-876: Authenticate systemd services and curl requests openshift/installer#8393

Open

AGENT-870: Update AUTH_TYPE openshift/installer#8108

Open

carbonin reviewed May 22, 2024

View reviewed changes

AGENT-869: Implement a new auth type for ABI #6174

Are you sure you want to change the base?

AGENT-869: Implement a new auth type for ABI #6174

Conversation

pawanpinjarkar commented Apr 10, 2024 • edited

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 Apr 10, 2024 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

pawanpinjarkar commented May 10, 2024

codecov bot commented May 10, 2024 • edited

Codecov Report

pawanpinjarkar commented May 13, 2024

openshift-ci-robot commented May 13, 2024 • edited by openshift-ci bot

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 May 13, 2024 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

pawanpinjarkar commented May 13, 2024

pawanpinjarkar commented May 13, 2024

carbonin left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 14, 2024

openshift-ci-robot commented May 16, 2024 • edited by openshift-ci bot

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 May 16, 2024 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

pawanpinjarkar commented May 16, 2024

openshift-ci-robot commented May 16, 2024 • edited by openshift-ci bot

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 May 17, 2024

carbonin May 22, 2024

Choose a reason for hiding this comment

carbonin May 22, 2024

Choose a reason for hiding this comment

carbonin May 22, 2024

Choose a reason for hiding this comment

pawanpinjarkar commented Apr 10, 2024 •

edited

openshift-ci-robot commented Apr 10, 2024 •

edited by openshift-ci bot

codecov bot commented May 10, 2024 •

edited

openshift-ci-robot commented May 13, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 13, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 16, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 16, 2024 •

edited by openshift-ci bot

openshift-ci-robot commented May 16, 2024 •

edited by openshift-ci bot